[dpdk-dev] [PATCH v7 1/7] eventtimer: add event timer adapter API

Carrillo, Erik G erik.g.carrillo at intel.com
Mon Mar 12 17:22:05 CET 2018


Hi Jerin,

Thanks for reviewing.  I've responded in-line:

> -----Original Message-----
> From: Jerin Jacob [mailto:jerin.jacob at caviumnetworks.com]
> Sent: Monday, March 12, 2018 2:54 AM
> To: Carrillo, Erik G <erik.g.carrillo at intel.com>
> Cc: pbhagavatula at caviumnetworks.com; dev at dpdk.org;
> nipun.gupta at nxp.com; hemant.agrawal at nxp.com
> Subject: Re: [PATCH v7 1/7] eventtimer: add event timer adapter API
> 
> -----Original Message-----
> > Date: Thu, 8 Mar 2018 15:54:00 -0600
> > From: Erik Gabriel Carrillo <erik.g.carrillo at intel.com>
> > To: pbhagavatula at caviumnetworks.com
> > CC: dev at dpdk.org, jerin.jacob at caviumnetworks.com,
> nipun.gupta at nxp.com,
> > hemant.agrawal at nxp.com
> > Subject: [PATCH v7 1/7] eventtimer: add event timer adapter API
> > X-Mailer: git-send-email 1.7.10
> 
> IMO, you can add some git commit description here.

Will do.

> 
> >
> > Signed-off-by: Jerin Jacob <jerin.jacob at caviumnetworks.com>
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula at caviumnetworks.com>
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo at intel.com>
> > ---
> >  lib/librte_eventdev/Makefile                  |   1 +
> >  lib/librte_eventdev/rte_event_timer_adapter.h | 645
> ++++++++++++++++++++++++++
> >  lib/librte_eventdev/rte_eventdev.h            |  41 +-
> >  3 files changed, 653 insertions(+), 34 deletions(-)  create mode
> > 100644 lib/librte_eventdev/rte_event_timer_adapter.h
> >
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this structure may change without prior notice
> > + *
> > + * Timer adapter configuration structure  */ struct
> > +rte_event_timer_adapter_conf {
> > +	uint8_t event_dev_id;
> > +	/**< Event device identifier */
> > +	uint16_t timer_adapter_id;
> > +	/**< Event timer adapter identifier */
> > +	uint32_t socket_id;
> > +	/**< Identifer of socket from which to allocate memory for adapter
> > +*/
> 
> s/Identifer/Identifier
>

Will fix.
 
> > +	enum rte_event_timer_adapter_clk_src clk_src;
> > +	/**< Clock source for timer adapter */
> > +	uint64_t timer_tick_ns;
> > +	/**< Timer adapter resolution in ns */
> > +	uint64_t max_tmo_ns;
> > +	/**< Maximum timer timeout(expiry) in ns */
> > +	uint64_t nb_timers;
> > +	/**< Total number of timers per adapter */
> > +	uint64_t flags;
> > +	/**< Timer adapter config flags (RTE_EVENT_TIMER_ADAPTER_F_*)
> */ };
> > +/**
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Set an event timer's initial state and initialize the event it carries.
> > + *
> > + * @param evtim
> > + *   A pointer to an event timer structure.
> > + */
> > +void __rte_experimental
> > +rte_event_timer_init(struct rte_event_timer *evtim);
> 
> Since it can be used in fastpath, How about making it as "static inline"
> function?
> Any it is just setting some variables.
> 

Will do.

> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Arm a burst of event timers with separate expiration timeout tick
> > +for each
> > + * event timer.
> > + *
> > + * Before calling this function, the application allocates
> > + * ``struct rte_event_timer`` objects from mempool or huge page
> > +backed
> > + * application buffers of desired size. On successful allocation,
> > + * application updates the `struct rte_event_timer`` attributes such
> > +as
> > + * expiry event attributes, timeout ticks from now.
> > + * This function submits the event timer arm requests to the event
> > +timer adapter
> > + * and on expiry, the events will be injected to designated event queue.
> > + *
> > + * @param adapter
> > + *   A pointer to an event timer adapter structure.
> > + * @param evtims
> > + *   Pointer to an array of objects of type *rte_event_timer* structure.
> > + * @param nb_evtims
> > + *   Number of event timers in the supplied array.
> > + *
> > + * @return
> > + *   The number of successfully armed event timers. The return value can
> be less
> > + *   than the value of the *nb_evtims* parameter. If the return value is
> less
> > + *   than *nb_evtims*, the remaining event timers at the end of *evtims*
> > + *   are not consumed, and the caller has to take care of them, and
> rte_errno
> > + *   is set accordingly. Possible errno values include:
> > + *   - EINVAL Invalid timer adapter, expiry event queue ID is invalid, or an
> > + *   expiry event's sched type doesn't match the capabilities of the
> > + *   destination event queue.
> > + *   - EAGAIN Specified timer adapter is not running
> > + *   - EALREADY A timer was encountered that was already armed
> > + */
> > +int __rte_experimental
> 
> To maintain the consistency across eventdev and other subsystems in DPDK.
> We could return uint16_t for all the fast path functions. ie. exiting "int" error
> return can be changed to
> 
> rte_errno = -EINVAL;
> return 0;
> 
> This would avoid some series typecasting issues as well for the _retry_ case.

Sounds good.  I'll change the return type to uint16_t, but the errno behavior seems
to already be what you describe so I think we should be good there.

<... snipped ...>

> > - *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> CONTRIBUTORS
> > - *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT
> NOT
> > - *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND
> FITNESS FOR
> > - *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> COPYRIGHT
> > - *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
> INCIDENTAL,
> > - *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
> BUT NOT
> > - *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> LOSS OF USE,
> > - *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED
> AND ON ANY
> > - *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR
> TORT
> > - *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> OF THE USE
> > - *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
> DAMAGE.
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2016 Cavium, Inc.
> > + * Copyright(c) 2016-2018 Intel Corporation.
> > + * Copyright(c) 2016 NXP.
> 
> Please send existing license changes as separate patch as we need to get
> ACK from all vendors.
> 

Will do.

> > + * All rights reserved.
> >   */
> >
> >  #ifndef _RTE_EVENTDEV_H_
> > @@ -923,8 +896,8 @@ rte_event_dev_close(uint8_t dev_id);  /**< The
> > event generated from ethdev subsystem */
> >  #define RTE_EVENT_TYPE_CRYPTODEV        0x1
> >  /**< The event generated from crypodev subsystem */
> > -#define RTE_EVENT_TYPE_TIMERDEV         0x2
> > -/**< The event generated from timerdev subsystem */
> > +#define RTE_EVENT_TYPE_TIMER		0x2
> > +/**< The event generated from event timer adapter */
> >  #define RTE_EVENT_TYPE_CPU              0x3
> >  /**< The event generated from cpu for pipelining.
> >   * Application may use *sub_event_type* to further classify the event
> > --
> > 2.6.4
> 
> Other than above minor changes, It looks really good to me.

Thanks,
Gabriel


More information about the dev mailing list