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