Hi Jerin, Thanks for reviewing. I've responded in-line:
> -----Original Message----- > From: Jerin Jacob [mailto:jerin.ja...@caviumnetworks.com] > Sent: Monday, March 12, 2018 2:54 AM > To: Carrillo, Erik G <erik.g.carri...@intel.com> > Cc: pbhagavat...@caviumnetworks.com; dev@dpdk.org; > nipun.gu...@nxp.com; hemant.agra...@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.carri...@intel.com> > > To: pbhagavat...@caviumnetworks.com > > CC: dev@dpdk.org, jerin.ja...@caviumnetworks.com, > nipun.gu...@nxp.com, > > hemant.agra...@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.ja...@caviumnetworks.com> > > Signed-off-by: Pavan Nikhilesh <pbhagavat...@caviumnetworks.com> > > Signed-off-by: Erik Gabriel Carrillo <erik.g.carri...@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
