[dpdk-dev,v6,08/23] eventtimer: add adapter start/stop definitions

  Add definitions to the default software implementation for the functions
that start and stop adapter instances.

Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
---
 lib/librte_eventdev/rte_event_timer_adapter.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)
  

On Wed, Jan 10, 2018 at 06:20:59PM -0600, Erik Gabriel Carrillo wrote:
> Add definitions to the default software implementation for the functions
> that start and stop adapter instances.
>
> Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> ---
>  lib/librte_eventdev/rte_event_timer_adapter.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/lib/librte_eventdev/rte_event_timer_adapter.c b/lib/librte_eventdev/rte_event_timer_adapter.c
> index 38f4dcf..27e6226 100644
> --- a/lib/librte_eventdev/rte_event_timer_adapter.c
> +++ b/lib/librte_eventdev/rte_event_timer_adapter.c
> @@ -565,7 +565,18 @@ sw_event_timer_adapter_uninit(struct rte_event_timer_adapter *adapter)
>  static int
>  sw_event_timer_adapter_start(const struct rte_event_timer_adapter *adapter)
>  {
> -	RTE_SET_USED(adapter);
> +	int ret;
> +	struct rte_event_timer_adapter_sw_data *sw_data;
> +
> +	sw_data = adapter->data->adapter_priv;
> +
> +	ret = rte_service_component_runstate_set(sw_data->service_id, 1);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* If no service core is mapped to the service, fail */
> +	if (!rte_service_runstate_get(sw_data->service_id))
> +		return -ENOENT;

If a service is mapped to more than one service core then the service is
executed in parallel if it is multi thread safe else every core takes a lock
and executes the service callback.

Now in case of timer adapter the following piece of code arms the timer on
the service core that currently runs the service.
	rte_timer_reset_sync(tim, cycles, SINGLE,
				     rte_lcore_id(),
				     sw_event_timer_cb,
				     msg->evtim);
This might lead to delay in timer expiry being called as rte_timer_manage() has
to be scheduled on the same service core again.

The immediate solution that comes to my mind is to limit the number of service
cores mapped to 1.

Thoughts?

Cheers,
Pavan.

>
>  	return 0;
>  }
> @@ -573,9 +584,9 @@ sw_event_timer_adapter_start(const struct rte_event_timer_adapter *adapter)
>  static int
>  sw_event_timer_adapter_stop(const struct rte_event_timer_adapter *adapter)
>  {
> -	RTE_SET_USED(adapter);
> -
> -	return 0;
> +	struct rte_event_timer_adapter_sw_data *sw_data;
> +	sw_data = adapter->data->adapter_priv;
> +	return rte_service_component_runstate_set(sw_data->service_id, 0);
>  }
>
>  static void
> --
> 2.6.4
>
  

> -----Original Message-----
> From: Pavan Nikhilesh [mailto:pbhagavatula@caviumnetworks.com]
> Sent: Thursday, January 11, 2018 11:29 AM
> To: Carrillo, Erik G <erik.g.carrillo@intel.com>;
> jerin.jacob@caviumnetworks.com; nipun.gupta@nxp.com;
> hemant.agrawal@nxp.com
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v6 08/23] eventtimer: add adapter start/stop definitions
> 
> On Wed, Jan 10, 2018 at 06:20:59PM -0600, Erik Gabriel Carrillo wrote:
> > Add definitions to the default software implementation for the
> > functions that start and stop adapter instances.
> >
> > Signed-off-by: Erik Gabriel Carrillo <erik.g.carrillo@intel.com>
> > ---
> >  lib/librte_eventdev/rte_event_timer_adapter.c | 19
> > +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/librte_eventdev/rte_event_timer_adapter.c
> > b/lib/librte_eventdev/rte_event_timer_adapter.c
> > index 38f4dcf..27e6226 100644
> > --- a/lib/librte_eventdev/rte_event_timer_adapter.c
> > +++ b/lib/librte_eventdev/rte_event_timer_adapter.c
> > @@ -565,7 +565,18 @@ sw_event_timer_adapter_uninit(struct
> > rte_event_timer_adapter *adapter)  static int
> > sw_event_timer_adapter_start(const struct rte_event_timer_adapter
> > *adapter)  {
> > -	RTE_SET_USED(adapter);
> > +	int ret;
> > +	struct rte_event_timer_adapter_sw_data *sw_data;
> > +
> > +	sw_data = adapter->data->adapter_priv;
> > +
> > +	ret = rte_service_component_runstate_set(sw_data->service_id, 1);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* If no service core is mapped to the service, fail */
> > +	if (!rte_service_runstate_get(sw_data->service_id))
> > +		return -ENOENT;
> 
> If a service is mapped to more than one service core then the service is
> executed in parallel if it is multi thread safe else every core takes a lock and
> executes the service callback.
> 
> Now in case of timer adapter the following piece of code arms the timer on
> the service core that currently runs the service.
> 	rte_timer_reset_sync(tim, cycles, SINGLE,
> 				     rte_lcore_id(),
> 				     sw_event_timer_cb,
> 				     msg->evtim);
> This might lead to delay in timer expiry being called as rte_timer_manage()
> has to be scheduled on the same service core again.
> 
> The immediate solution that comes to my mind is to limit the number of
> service cores mapped to 1.
> 
> Thoughts?

Yes, good catch.  This service is not MT-safe and if it gets mapped to multiple lcores,  each such lcore will be unable to run the service function while other cores are running it, which would introduce delays unnecessarily since rte_timer_manage gets called less frequently.   I agree that the number of mapped service cores should be limited to 1 while the service is MT-unsafe.  If we do that, we can also put the message ring into single-consumer mode.

In a future change, I can also look at making the service MT-safe.

> 
> Cheers,
> Pavan.
> 
> >
> >  	return 0;
> >  }
> > @@ -573,9 +584,9 @@ sw_event_timer_adapter_start(const struct
> > rte_event_timer_adapter *adapter)  static int
> > sw_event_timer_adapter_stop(const struct rte_event_timer_adapter
> > *adapter)  {
> > -	RTE_SET_USED(adapter);
> > -
> > -	return 0;
> > +	struct rte_event_timer_adapter_sw_data *sw_data;
> > +	sw_data = adapter->data->adapter_priv;
> > +	return rte_service_component_runstate_set(sw_data->service_id,
> 0);
> >  }
> >
> >  static void
> > --
> > 2.6.4
> >