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

[Ananyev, Konstantin]

Hi lads,

> -----Original Message-----
> From: Liang, Cunming
> Sent: Thursday, February 12, 2015 8:17 AM
> To: dev at dpdk.org
> Cc: Ananyev, Konstantin; olivier.matz at 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 at 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