[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