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

Message ID 1422842559-13617-18-git-send-email-cunming.liang@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Cunming Liang Feb. 2, 2015, 2:02 a.m. UTC
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(-)
  

Comments

Olivier Matz Feb. 10, 2015, 5:45 p.m. UTC | #1
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@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.

>
>   	/* 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.


>   		/* 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) ?


> +			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?



>   	}
>
>   	/* 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.



Regards,
Olivier
  
Cunming Liang Feb. 11, 2015, 6:25 a.m. UTC | #2
> -----Original Message-----

> From: Olivier MATZ [mailto:olivier.matz@6wind.com]

> Sent: Wednesday, February 11, 2015 1:45 AM

> To: Liang, Cunming; dev@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@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
  
Olivier Matz Feb. 11, 2015, 5:21 p.m. UTC | #3
Hi,

On 02/11/2015 07:25 AM, Liang, Cunming wrote:
>>> +			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.

Yes I agree it does not need to be the master lcore, but until recently
the definition of the master lcore was "the first enabled core".

I was thinking rte_get_master_lcore() is easier to understand
that rte_get_next_lcore(LCORE_ID_ANY, 0, 1). If you still prefer
to keep the second one, can you add a comment saying something like
"non-EAL thread do not run rte_timer_manage(), so schedule the timer
on the first enabled lcore"?

Thanks,
Olivier
  
Cunming Liang Feb. 12, 2015, 12:29 a.m. UTC | #4
Hi,

> -----Original Message-----

> From: Olivier MATZ [mailto:olivier.matz@6wind.com]

> Sent: Thursday, February 12, 2015 1:22 AM

> To: Liang, Cunming; dev@dpdk.org

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

> 

> Hi,

> 

> On 02/11/2015 07:25 AM, Liang, Cunming wrote:

> >>> +			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.

> 

> Yes I agree it does not need to be the master lcore, but until recently

> the definition of the master lcore was "the first enabled core".

> 

> I was thinking rte_get_master_lcore() is easier to understand

> that rte_get_next_lcore(LCORE_ID_ANY, 0, 1). If you still prefer

> to keep the second one, can you add a comment saying something like

> "non-EAL thread do not run rte_timer_manage(), so schedule the timer

> on the first enabled lcore"?

[LCM] That makes sense, will add it. Thanks.
> 

> Thanks,

> Olivier
  

Patch

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;
 
 	/* 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).