[dpdk-dev] [PATCH v4 17/17] timer: add support to non-EAL thread

Liang, Cunming cunming.liang at intel.com
Wed Feb 11 07:25:34 CET 2015



> -----Original Message-----
> From: Olivier MATZ [mailto:olivier.matz at 6wind.com]
> Sent: Wednesday, February 11, 2015 1:45 AM
> To: Liang, Cunming; dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v4 17/17] timer: add support to non-EAL thread
> 
> Hi,
> 
> On 02/02/2015 03:02 AM, Cunming Liang wrote:
> > 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 at 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];
> >
> >   /* 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;
> 
> Is this still valid?
> In my understanding, rte_lcore_id() was returning the core id or
> LCORE_ID_ANY if it's a non-EAL thread.
[LCM] It's a nice to have one, in case lcore_id got an invalid number.
We can add a assert to replace these two line.
> 
> >
> >   	/* 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;
> > +
> 
> Are you sure this is required?
> 
> I think prev_status.owner can be LCORE_ID_ANY only in config state,
> as a timer cannot be scheduled on a non-EAL thread. And there is
> already a test that returns -1 if state is CONFIG.
[LCM] Good point, whenever prev_status.owner == LCORE_ID_ANY, the prev_status.state must be RTE_TIMER_CONFIG.
Make sense to me to remove the condition check. 
> 
> 
> >   		/* 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) {
> 
> if (lcore_id != LCORE_ID_ANY) ?
[LCM] Accept.
> 
> 
> > +			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);
> 
> I think the following line:
> tim_lcore = rte_get_next_lcore(LCORE_ID_ANY, 0, 1);
> Will return the first enabled core.
> 
> Maybe using rte_get_master_lcore() is clearer?
[LCM] It doesn't expect must to be a master lcore.
Any available lcore is fine, so I think make sense to just use the first enabled core.
> 
> 
> 
> >   	}
> >
> >   	/* 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) {
> 
> if (lcore_id != LCORE_ID_ANY) ?
> 
> 
> >   		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) {
> 
> if (lcore_id != LCORE_ID_ANY) ?
> 
> 
> >   		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;
> > +
> 
> Maybe an assert is more visible here. Else, if someone calls
> rte_timer_manage() from a non-EAL core, it will just exit
> silently.
> 
> Maybe adding a comment in rte_timer.h saying that this function
> must be called from an EAL core would also help.
[LCM] accept. 
> 
> 
> 
> Regards,
> Olivier



More information about the dev mailing list