diff mbox series

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

Message ID 20220810070722.3110873-1-s.v.naga.harish.k@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Jerin Jacob
Headers
Series [v2,1/4] eventdev/timer: add periodic event timer support |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Naga Harish K, S V Aug. 10, 2022, 7:07 a.m. UTC 
  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(-)

Comments

Carrillo, Erik G Aug. 10, 2022, 7:55 p.m. UTC | #1 
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.

> +	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;
                }
        }

> @@ -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
Naga Harish K, S V Aug. 11, 2022, 3:43 p.m. UTC | #2 
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
diff mbox series

Patch

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)
+{
+	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;
@@ -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;
@@ -612,35 +622,44 @@  swtim_callback(struct rte_timer *tim)
 	uint64_t opaque;
 	int ret;
 	int n_lcores;
+	enum rte_timer_type type;
 
 	opaque = evtim->impl_opaque[1];
 	adapter = (struct rte_event_timer_adapter *)(uintptr_t)opaque;
 	sw = swtim_pmd_priv(adapter);
+	type = get_event_timer_type(adapter);
+
+	if (unlikely(sw->in_use[lcore].v == 0)) {
+		sw->in_use[lcore].v = 1;
+		n_lcores = __atomic_fetch_add(&sw->n_poll_lcores, 1,
+					     __ATOMIC_RELAXED);
+		__atomic_store_n(&sw->poll_lcores[n_lcores], lcore,
+				__ATOMIC_RELAXED);
+	}
 
 	ret = event_buffer_add(&sw->buffer, &evtim->ev);
 	if (ret < 0) {
-		/* If event buffer is full, put timer back in list with
-		 * immediate expiry value, so that we process it again on the
-		 * next iteration.
-		 */
-		ret = rte_timer_alt_reset(sw->timer_data_id, tim, 0, SINGLE,
-					  lcore, NULL, evtim);
-		if (ret < 0) {
-			EVTIM_LOG_DBG("event buffer full, failed to reset "
-				      "timer with immediate expiry value");
+		if (type == SINGLE) {
+			/* If event buffer is full, put timer back in list with
+			 * immediate expiry value, so that we process it again
+			 * on the next iteration.
+			 */
+			ret = rte_timer_alt_reset(sw->timer_data_id, tim, 0,
+						SINGLE,	lcore, NULL, evtim);
+			if (ret < 0) {
+				EVTIM_LOG_DBG("event buffer full, failed to "
+						"reset timer with immediate "
+						"expiry value");
+			} else {
+				sw->stats.evtim_retry_count++;
+				EVTIM_LOG_DBG("event buffer full, resetting "
+						"rte_timer with immediate "
+						"expiry value");
+			}
 		} else {
-			sw->stats.evtim_retry_count++;
-			EVTIM_LOG_DBG("event buffer full, resetting rte_timer "
-				      "with immediate expiry value");
+			sw->stats.evtim_drop_count++;
 		}
 
-		if (unlikely(sw->in_use[lcore].v == 0)) {
-			sw->in_use[lcore].v = 1;
-			n_lcores = __atomic_fetch_add(&sw->n_poll_lcores, 1,
-						     __ATOMIC_RELAXED);
-			__atomic_store_n(&sw->poll_lcores[n_lcores], lcore,
-					__ATOMIC_RELAXED);
-		}
 	} else {
 		EVTIM_BUF_LOG_DBG("buffered an event timer expiry event");
 
@@ -654,10 +673,15 @@  swtim_callback(struct rte_timer *tim)
 			sw->n_expired_timers = 0;
 		}
 
-		sw->expired_timers[sw->n_expired_timers++] = tim;
+		/* Don't free rte_timer for a periodic event timer until
+		 * it is cancelled
+		 */
+		if (type == SINGLE)
+			sw->expired_timers[sw->n_expired_timers++] = tim;
 		sw->stats.evtim_exp_count++;
 
-		__atomic_store_n(&evtim->state, RTE_EVENT_TIMER_NOT_ARMED,
+		if (type == SINGLE)
+			__atomic_store_n(&evtim->state, RTE_EVENT_TIMER_NOT_ARMED,
 				__ATOMIC_RELEASE);
 	}
 
@@ -947,6 +971,12 @@  swtim_uninit(struct rte_event_timer_adapter *adapter)
 			   swtim_free_tim,
 			   sw);
 
+	ret = rte_timer_data_dealloc(sw->timer_data_id);
+	if (ret < 0) {
+		EVTIM_LOG_ERR("failed to deallocate timer data instance");
+		return ret;
+	}
+
 	ret = rte_service_component_unregister(sw->service_id);
 	if (ret < 0) {
 		EVTIM_LOG_ERR("failed to unregister service component");
@@ -1053,6 +1083,7 @@  __swtim_arm_burst(const struct rte_event_timer_adapter *adapter,
 	/* Timer list for this lcore is not in use. */
 	uint16_t exp_state = 0;
 	enum rte_event_timer_state n_state;
+	enum rte_timer_type type = SINGLE;
 
 #ifdef RTE_LIBRTE_EVENTDEV_DEBUG
 	/* Check that the service is running. */
@@ -1092,6 +1123,9 @@  __swtim_arm_burst(const struct rte_event_timer_adapter *adapter,
 		return 0;
 	}
 
+	/* update timer type for periodic adapter */
+	type = get_event_timer_type(adapter);
+
 	for (i = 0; i < nb_evtims; i++) {
 		n_state = __atomic_load_n(&evtims[i]->state, __ATOMIC_ACQUIRE);
 		if (n_state == RTE_EVENT_TIMER_ARMED) {
@@ -1135,7 +1169,7 @@  __swtim_arm_burst(const struct rte_event_timer_adapter *adapter,
 
 		cycles = get_timeout_cycles(evtims[i], adapter);
 		ret = rte_timer_alt_reset(sw->timer_data_id, tim, cycles,
-					  SINGLE, lcore_id, NULL, evtims[i]);
+					  type, lcore_id, NULL, evtims[i]);
 		if (ret < 0) {
 			/* tim was in RUNNING or CONFIG state */
 			__atomic_store_n(&evtims[i]->state,
diff --git a/lib/eventdev/rte_event_timer_adapter.h b/lib/eventdev/rte_event_timer_adapter.h
index eab8e59a57..cd10db19e4 100644
--- a/lib/eventdev/rte_event_timer_adapter.h
+++ b/lib/eventdev/rte_event_timer_adapter.h
@@ -193,6 +193,8 @@  struct rte_event_timer_adapter_stats {
 	/**< Event timer retry count */
 	uint64_t adapter_tick_count;
 	/**< Tick count for the adapter, at its resolution */
+	uint64_t evtim_drop_count;
+	/**< event timer expiries dropped */
 };
 
 struct rte_event_timer_adapter;
diff --git a/lib/eventdev/rte_eventdev.c b/lib/eventdev/rte_eventdev.c
index 1dc4f966be..4a2a1178da 100644
--- a/lib/eventdev/rte_eventdev.c
+++ b/lib/eventdev/rte_eventdev.c
@@ -139,7 +139,11 @@  rte_event_timer_adapter_caps_get(uint8_t dev_id, uint32_t *caps)
 
 	if (caps == NULL)
 		return -EINVAL;
-	*caps = 0;
+
+	if (dev->dev_ops->timer_adapter_caps_get == NULL)
+		*caps = RTE_EVENT_TIMER_ADAPTER_CAP_PERIODIC;
+	else
+		*caps = 0;
 
 	return dev->dev_ops->timer_adapter_caps_get ?
 				(*dev->dev_ops->timer_adapter_caps_get)(dev,