RE: [PATCH v2 1/4] eventdev/timer: add periodic event timer support

["Naga Harish K, S V" <s.v.naga.harish.k@intel.com>]

Hi Gabe,

> -----Original Message-----
> From: Carrillo, Erik G <erik.g.carrillo@intel.com>
> Sent: Thursday, August 11, 2022 1:25 AM
> To: Naga Harish K, S V <s.v.naga.harish.k@intel.com>; jerinj@marvell.com
> Cc: pbhagavatula@marvell.com; sthotton@marvell.com; dev@dpdk.org
> Subject: RE: [PATCH v2 1/4] eventdev/timer: add periodic event timer
> support
> 
> Hi Harish,
> 
> > -----Original Message-----
> > From: Naga Harish K, S V <s.v.naga.harish.k@intel.com>
> > Sent: Wednesday, August 10, 2022 2:07 AM
> > To: Carrillo, Erik G <erik.g.carrillo@intel.com>; jerinj@marvell.com
> > Cc: pbhagavatula@marvell.com; sthotton@marvell.com; dev@dpdk.org
> > Subject: [PATCH v2 1/4] eventdev/timer: add periodic event timer
> > support
> >
> > This patch adds support to configure and use periodic event timers in
> > software timer adapter.
> >
> > The structure ``rte_event_timer_adapter_stats`` is extended by adding
> > a new field, ``evtim_drop_count``. This stat represents the number of
> > times an event_timer expiry event is dropped by the event timer adapter.
> >
> > Signed-off-by: Naga Harish K S V <s.v.naga.harish.k@intel.com>
> > ---
> >  lib/eventdev/rte_event_timer_adapter.c | 86 ++++++++++++++++++-----
> --
> > -  lib/eventdev/rte_event_timer_adapter.h |  2 +
> >  lib/eventdev/rte_eventdev.c            |  6 +-
> >  3 files changed, 67 insertions(+), 27 deletions(-)
> >
> > diff --git a/lib/eventdev/rte_event_timer_adapter.c
> > b/lib/eventdev/rte_event_timer_adapter.c
> > index e0d978d641..0de88dfc0f 100644
> > --- a/lib/eventdev/rte_event_timer_adapter.c
> > +++ b/lib/eventdev/rte_event_timer_adapter.c
> > @@ -53,6 +53,14 @@ static const struct event_timer_adapter_ops
> > swtim_ops;  #define EVTIM_SVC_LOG_DBG(...) (void)0  #endif
> >
> > +static inline enum rte_timer_type
> > +get_event_timer_type(const struct rte_event_timer_adapter *adapter) {
> 
> Let's call this function "get_timer_type" since it is selecting a type for an
> rte_timer.
> 
Taken in v3 version of the patch

> > +	return (adapter->data->conf.flags &
> > +			RTE_EVENT_TIMER_ADAPTER_F_PERIODIC) ?
> > +			PERIODICAL : SINGLE;
> > +}
> > +
> >  static int
> >  default_port_conf_cb(uint16_t id, uint8_t event_dev_id, uint8_t
> > *event_port_id,
> >  		     void *conf_arg)
> > @@ -195,10 +203,11 @@ rte_event_timer_adapter_create_ext(
> >  	adapter->data->conf = *conf;  /* copy conf structure */
> >
> >  	/* Query eventdev PMD for timer adapter capabilities and ops */
> > -	ret = dev->dev_ops->timer_adapter_caps_get(dev,
> > +	ret = dev->dev_ops->timer_adapter_caps_get ?
> > +				dev->dev_ops-
> > >timer_adapter_caps_get(dev,
> >  						   adapter->data->conf.flags,
> >  						   &adapter->data->caps,
> > -						   &adapter->ops);
> > +						   &adapter->ops) : 0;
> >  	if (ret < 0) {
> >  		rte_errno = -ret;
> >  		goto free_memzone;
> 
> IMO, this hunk would read better as:
> 
>         if (dev->dev_ops->timer_adapter_caps_get) {
>                 ret = dev->dev_ops->timer_adapter_caps_get(dev,
>                                 adapter->data->conf.flags, &adapter->data->caps,
>                                 &adapter->ops);
>                 if (ret < 0) {
>                         rte_errno = -ret;
>                         goto free_memzone;
>                 }
>         }
> 

Taken in v3 version of the patch

> > @@ -348,10 +357,11 @@ rte_event_timer_adapter_lookup(uint16_t
> > adapter_id)
> >  	dev = &rte_eventdevs[adapter->data->event_dev_id];
> >
> >  	/* Query eventdev PMD for timer adapter capabilities and ops */
> > -	ret = dev->dev_ops->timer_adapter_caps_get(dev,
> > +	ret = dev->dev_ops->timer_adapter_caps_get ?
> > +			dev->dev_ops->timer_adapter_caps_get(dev,
> >  						   adapter->data->conf.flags,
> >  						   &adapter->data->caps,
> > -						   &adapter->ops);
> > +						   &adapter->ops) : 0;
> >  	if (ret < 0) {
> >  		rte_errno = EINVAL;
> >  		return NULL;
> 
> Same comment as above for this hunk...
> 
> Thanks,
> Erik