Re: [dpdk-dev] [PATCH v1 15/15] timer: add support to non-EAL thread

["Liang, Cunming" <cunming.liang@intel.com>]

> -----Original Message-----
> From: Walukiewicz, Miroslaw
> Sent: Thursday, January 22, 2015 5:58 PM
> To: Liang, Cunming; dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH v1 15/15] timer: add support to non-EAL thread
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Cunming Liang
> > Sent: Thursday, January 22, 2015 9:17 AM
> > To: dev@dpdk.org
> > Subject: [dpdk-dev] [PATCH v1 15/15] timer: add support to non-EAL thread
> >
> > Allow to setup timers only for EAL (lcore) threads (__lcore_id <
> > MAX_LCORE_ID).
> > E.g. – dynamically created thread will be able to reset/stop timer for lcore
> > thread,
> > but it will be not allowed to setup timer for itself or another non-lcore
> > thread.
> > rte_timer_manage() for non-lcore thread would simply do nothing and
> > return straightway.
> >
> > Signed-off-by: Cunming Liang <cunming.liang@intel.com>
> > ---
> >  lib/librte_timer/rte_timer.c | 40 +++++++++++++++++++++++++++++++----
> > -----
> >  lib/librte_timer/rte_timer.h |  2 +-
> >  2 files changed, 32 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> > index 269a992..601c159 100644
> > --- a/lib/librte_timer/rte_timer.c
> > +++ b/lib/librte_timer/rte_timer.c
> > @@ -79,9 +79,10 @@ static struct priv_timer priv_timer[RTE_MAX_LCORE];
> >
> 
> Why not extend the priv_timer size to value being in range returned by
> rte_lcore_id().
> 
> All timer stuff will work automatically after such change without any change in
> timer logic including stats.
[Liang, Cunming] The same reason as mempool does.
It won't expect to involve dynamic unique id allocation for user thread on the first step.
The failure secondary won't release the reserved id which cause potential unexpected leak.
So will look for other approach to improve the libraries in the next step.
> 
> >  /* when debug is enabled, store some statistics */
> >  #ifdef RTE_LIBRTE_TIMER_DEBUG
> > -#define __TIMER_STAT_ADD(name, n) do {				\
> > -		unsigned __lcore_id = rte_lcore_id();		\
> > -		priv_timer[__lcore_id].stats.name += (n);	\
> > +#define __TIMER_STAT_ADD(name, n) do {
> > 	\
> > +		unsigned __lcore_id = rte_lcore_id();			\
> > +		if (__lcore_id < RTE_MAX_LCORE)
> > 	\
> > +			priv_timer[__lcore_id].stats.name += (n);	\
> >  	} while(0)
> >  #else
> >  #define __TIMER_STAT_ADD(name, n) do {} while(0)
> > @@ -127,15 +128,26 @@ timer_set_config_state(struct rte_timer *tim,
> >  	unsigned lcore_id;
> >
> >  	lcore_id = rte_lcore_id();
> > +	if (lcore_id >= RTE_MAX_LCORE)
> > +		lcore_id = LCORE_ID_ANY;
> >
> >  	/* wait that the timer is in correct status before update,
> >  	 * and mark it as being configured */
> >  	while (success == 0) {
> >  		prev_status.u32 = tim->status.u32;
> >
> > +		/*
> > +		 * prevent race condition of non-EAL threads
> > +		 * to update the timer. When 'owner == LCORE_ID_ANY',
> > +		 * it means updated by a non-EAL thread.
> > +		 */
> > +		if (lcore_id == (unsigned)LCORE_ID_ANY &&
> > +		    (uint16_t)lcore_id == prev_status.owner)
> > +			return -1;
> > +
> >  		/* timer is running on another core, exit */
> >  		if (prev_status.state == RTE_TIMER_RUNNING &&
> > -		    (unsigned)prev_status.owner != lcore_id)
> > +		    prev_status.owner != (uint16_t)lcore_id)
> >  			return -1;
> >
> >  		/* timer is being configured on another core */
> > @@ -366,9 +378,13 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t
> > expire,
> >
> >  	/* round robin for tim_lcore */
> >  	if (tim_lcore == (unsigned)LCORE_ID_ANY) {
> > -		tim_lcore =
> > rte_get_next_lcore(priv_timer[lcore_id].prev_lcore,
> > -					       0, 1);
> > -		priv_timer[lcore_id].prev_lcore = tim_lcore;
> > +		if (lcore_id < RTE_MAX_LCORE) {
> > +			tim_lcore = rte_get_next_lcore(
> > +				priv_timer[lcore_id].prev_lcore,
> > +				0, 1);
> > +			priv_timer[lcore_id].prev_lcore = tim_lcore;
> > +		} else
> > +			tim_lcore = rte_get_next_lcore(LCORE_ID_ANY, 0,
> > 1);
> >  	}
> >
> >  	/* wait that the timer is in correct status before update,
> > @@ -378,7 +394,8 @@ __rte_timer_reset(struct rte_timer *tim, uint64_t
> > expire,
> >  		return -1;
> >
> >  	__TIMER_STAT_ADD(reset, 1);
> > -	if (prev_status.state == RTE_TIMER_RUNNING) {
> > +	if (prev_status.state == RTE_TIMER_RUNNING &&
> > +	    lcore_id < RTE_MAX_LCORE) {
> >  		priv_timer[lcore_id].updated = 1;
> >  	}
> >
> > @@ -455,7 +472,8 @@ rte_timer_stop(struct rte_timer *tim)
> >  		return -1;
> >
> >  	__TIMER_STAT_ADD(stop, 1);
> > -	if (prev_status.state == RTE_TIMER_RUNNING) {
> > +	if (prev_status.state == RTE_TIMER_RUNNING &&
> > +	    lcore_id < RTE_MAX_LCORE) {
> >  		priv_timer[lcore_id].updated = 1;
> >  	}
> >
> > @@ -499,6 +517,10 @@ void rte_timer_manage(void)
> >  	uint64_t cur_time;
> >  	int i, ret;
> >
> > +	/* timer manager only runs on EAL thread */
> > +	if (lcore_id >= RTE_MAX_LCORE)
> > +		return;
> > +
> >  	__TIMER_STAT_ADD(manage, 1);
> >  	/* optimize for the case where per-cpu list is empty */
> >  	if (priv_timer[lcore_id].pending_head.sl_next[0] == NULL)
> > diff --git a/lib/librte_timer/rte_timer.h b/lib/librte_timer/rte_timer.h
> > index 4907cf5..5c5df91 100644
> > --- a/lib/librte_timer/rte_timer.h
> > +++ b/lib/librte_timer/rte_timer.h
> > @@ -76,7 +76,7 @@ extern "C" {
> >  #define RTE_TIMER_RUNNING 2 /**< State: timer function is running. */
> >  #define RTE_TIMER_CONFIG  3 /**< State: timer is being configured. */
> >
> > -#define RTE_TIMER_NO_OWNER -1 /**< Timer has no owner. */
> > +#define RTE_TIMER_NO_OWNER -2 /**< Timer has no owner. */
> >
> >  /**
> >   * Timer type: Periodic or single (one-shot).
> > --
> > 1.8.1.4