diff mbox

[dpdk-dev,v5,19/19] timer: add support to non-EAL thread

Message ID 1423728996-3004-20-git-send-email-cunming.liang@intel.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Cunming Liang Feb. 12, 2015, 8:16 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>
---
 v5 changes:
   add assert in rte_timer_manage
   remove duplicate check in timer_set_config_state

 lib/librte_timer/rte_timer.c | 31 ++++++++++++++++++++++---------
 lib/librte_timer/rte_timer.h |  4 ++--
 2 files changed, 24 insertions(+), 11 deletions(-)

Comments

Ananyev, Konstantin Feb. 12, 2015, 1:54 p.m. UTC | #1
Hi lads,

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

> From: Liang, Cunming

> Sent: Thursday, February 12, 2015 8:17 AM

> To: dev@dpdk.org

> Cc: Ananyev, Konstantin; olivier.matz@6wind.com; Liang, Cunming

> Subject: [PATCH v5 19/19] 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>

> ---

>  v5 changes:

>    add assert in rte_timer_manage

>    remove duplicate check in timer_set_config_state

> 

>  lib/librte_timer/rte_timer.c | 31 ++++++++++++++++++++++---------

>  lib/librte_timer/rte_timer.h |  4 ++--

>  2 files changed, 24 insertions(+), 11 deletions(-)

> 

> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c

> index 269a992..fa43fa9 100644

> --- a/lib/librte_timer/rte_timer.c

> +++ b/lib/librte_timer/rte_timer.c

> @@ -35,6 +35,7 @@

>  #include <stdio.h>

>  #include <stdint.h>

>  #include <inttypes.h>

> +#include <assert.h>

> 

>  #include <rte_atomic.h>

>  #include <rte_common.h>

> @@ -79,9 +80,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)

> @@ -135,7 +137,7 @@ timer_set_config_state(struct rte_timer *tim,

> 

>  		/* 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 +368,15 @@ __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 != 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

> +			/* non-EAL thread do not run rte_timer_manage(),

> +			 * so schedule the timer on the first enabled lcore. */

> +			tim_lcore = rte_get_next_lcore(LCORE_ID_ANY, 0, 1);

>  	}

> 

>  	/* wait that the timer is in correct status before update,

> @@ -378,7 +386,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 != LCORE_ID_ANY) {

>  		priv_timer[lcore_id].updated = 1;

>  	}

> 

> @@ -455,7 +464,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 != LCORE_ID_ANY) {

>  		priv_timer[lcore_id].updated = 1;

>  	}


About ' lcore_id != LCORE_ID_ANY' vs ' lcore_id < RTE_MAX_LCORE'.
I think both ways are valid right now.
Though using ' lcore_id != LCORE_ID_ANY' means, that if user will setup
PER_LCORE(_lcore_id) for dynamically created thread to some value >=  RTE_MAX_LCORE,  
then our code will not work properly.
Konstantin
 
> 

> @@ -499,6 +509,9 @@ void rte_timer_manage(void)

>  	uint64_t cur_time;

>  	int i, ret;

> 

> +	/* timer manager only runs on EAL thread */

> +	assert(lcore_id != LCORE_ID_ANY);

> +

>  	__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..35b8719 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).

> @@ -310,7 +310,7 @@ int rte_timer_pending(struct rte_timer *tim);

>  /**

>   * Manage the timer list and execute callback functions.

>   *

> - * This function must be called periodically from all cores

> + * This function must be called periodically from EAL lcores

>   * main_loop(). It browses the list of pending timers and runs all

>   * timers that are expired.

>   *

> --

> 1.8.1.4
Cunming Liang Feb. 13, 2015, 12:55 a.m. UTC | #2
> -----Original Message-----

> From: Ananyev, Konstantin

> Sent: Thursday, February 12, 2015 9:54 PM

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

> Cc: olivier.matz@6wind.com

> Subject: RE: [PATCH v5 19/19] timer: add support to non-EAL thread

> 

> Hi lads,

> 

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

> > From: Liang, Cunming

> > Sent: Thursday, February 12, 2015 8:17 AM

> > To: dev@dpdk.org

> > Cc: Ananyev, Konstantin; olivier.matz@6wind.com; Liang, Cunming

> > Subject: [PATCH v5 19/19] 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>

> > ---

> >  v5 changes:

> >    add assert in rte_timer_manage

> >    remove duplicate check in timer_set_config_state

> >

> >  lib/librte_timer/rte_timer.c | 31 ++++++++++++++++++++++---------

> >  lib/librte_timer/rte_timer.h |  4 ++--

> >  2 files changed, 24 insertions(+), 11 deletions(-)

> >

> > diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c

> > index 269a992..fa43fa9 100644

> > --- a/lib/librte_timer/rte_timer.c

> > +++ b/lib/librte_timer/rte_timer.c

> > @@ -35,6 +35,7 @@

> >  #include <stdio.h>

> >  #include <stdint.h>

> >  #include <inttypes.h>

> > +#include <assert.h>

> >

> >  #include <rte_atomic.h>

> >  #include <rte_common.h>

> > @@ -79,9 +80,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)

> > @@ -135,7 +137,7 @@ timer_set_config_state(struct rte_timer *tim,

> >

> >  		/* 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 +368,15 @@ __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 != 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

> > +			/* non-EAL thread do not run rte_timer_manage(),

> > +			 * so schedule the timer on the first enabled lcore. */

> > +			tim_lcore = rte_get_next_lcore(LCORE_ID_ANY, 0, 1);

> >  	}

> >

> >  	/* wait that the timer is in correct status before update,

> > @@ -378,7 +386,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 != LCORE_ID_ANY) {

> >  		priv_timer[lcore_id].updated = 1;

> >  	}

> >

> > @@ -455,7 +464,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 != LCORE_ID_ANY) {

> >  		priv_timer[lcore_id].updated = 1;

> >  	}

> 

> About ' lcore_id != LCORE_ID_ANY' vs ' lcore_id < RTE_MAX_LCORE'.

> I think both ways are valid right now.

> Though using ' lcore_id != LCORE_ID_ANY' means, that if user will setup

> PER_LCORE(_lcore_id) for dynamically created thread to some value >=

> RTE_MAX_LCORE,

> then our code will not work properly.

> Konstantin

[LCM] Ok, now we get the agreement as below.
1. only using '< RTE_MAX_LCORE' to check the EAL thread with valid lcore_id.
2. LCORE_ID_ANY is used as lcore_id default value or used for any  unspecified lcore_id assignment.
> 

> >

> > @@ -499,6 +509,9 @@ void rte_timer_manage(void)

> >  	uint64_t cur_time;

> >  	int i, ret;

> >

> > +	/* timer manager only runs on EAL thread */

> > +	assert(lcore_id != LCORE_ID_ANY);

> > +

> >  	__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..35b8719 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).

> > @@ -310,7 +310,7 @@ int rte_timer_pending(struct rte_timer *tim);

> >  /**

> >   * Manage the timer list and execute callback functions.

> >   *

> > - * This function must be called periodically from all cores

> > + * This function must be called periodically from EAL lcores

> >   * main_loop(). It browses the list of pending timers and runs all

> >   * timers that are expired.

> >   *

> > --

> > 1.8.1.4
Olivier Matz Feb. 13, 2015, 9:57 a.m. UTC | #3
Hi,

On 02/13/2015 01:55 AM, Liang, Cunming wrote:
>> About ' lcore_id != LCORE_ID_ANY' vs ' lcore_id < RTE_MAX_LCORE'.
>> I think both ways are valid right now.
>> Though using ' lcore_id != LCORE_ID_ANY' means, that if user will setup
>> PER_LCORE(_lcore_id) for dynamically created thread to some value >=
>> RTE_MAX_LCORE,
>> then our code will not work properly.
>> Konstantin
> [LCM] Ok, now we get the agreement as below.
> 1. only using '< RTE_MAX_LCORE' to check the EAL thread with valid lcore_id.
> 2. LCORE_ID_ANY is used as lcore_id default value or used for any  unspecified lcore_id assignment.

Doing like this is fine for me, although I don't really agree with
the initial argument: it's should not be allowed for an application
to modify an eal variable (_lcore_id).

Regards,
Olivier
diff mbox

Patch

diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
index 269a992..fa43fa9 100644
--- a/lib/librte_timer/rte_timer.c
+++ b/lib/librte_timer/rte_timer.c
@@ -35,6 +35,7 @@ 
 #include <stdio.h>
 #include <stdint.h>
 #include <inttypes.h>
+#include <assert.h>
 
 #include <rte_atomic.h>
 #include <rte_common.h>
@@ -79,9 +80,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)
@@ -135,7 +137,7 @@  timer_set_config_state(struct rte_timer *tim,
 
 		/* 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 +368,15 @@  __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 != 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
+			/* non-EAL thread do not run rte_timer_manage(),
+			 * so schedule the timer on the first enabled lcore. */
+			tim_lcore = rte_get_next_lcore(LCORE_ID_ANY, 0, 1);
 	}
 
 	/* wait that the timer is in correct status before update,
@@ -378,7 +386,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 != LCORE_ID_ANY) {
 		priv_timer[lcore_id].updated = 1;
 	}
 
@@ -455,7 +464,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 != LCORE_ID_ANY) {
 		priv_timer[lcore_id].updated = 1;
 	}
 
@@ -499,6 +509,9 @@  void rte_timer_manage(void)
 	uint64_t cur_time;
 	int i, ret;
 
+	/* timer manager only runs on EAL thread */
+	assert(lcore_id != LCORE_ID_ANY);
+
 	__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..35b8719 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).
@@ -310,7 +310,7 @@  int rte_timer_pending(struct rte_timer *tim);
 /**
  * Manage the timer list and execute callback functions.
  *
- * This function must be called periodically from all cores
+ * This function must be called periodically from EAL lcores
  * main_loop(). It browses the list of pending timers and runs all
  * timers that are expired.
  *