[dpdk-dev,v4,17/17] 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>
---
lib/librte_timer/rte_timer.c | 40 +++++++++++++++++++++++++++++++---------
lib/librte_timer/rte_timer.h | 2 +-
2 files changed, 32 insertions(+), 10 deletions(-)
Comments
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
> -----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
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
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
@@ -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)
@@ -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).