[dpdk-dev,v5,19/19] timer: add support to non-EAL thread
Commit Message
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
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
> -----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
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
@@ -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)
@@ -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.
*