Re: [dpdk-dev] [RFC PATCH] rte_timer: Fix rte_timer_reset return value

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Robert,

Please see some comments below.

On 02/03/2015 09:42 PM, rsanford2@gmail.com wrote:
> From: Robert Sanford <rsanford2@gmail.com>
> 
> - API rte_timer_reset() should return -1 when the timer is in the
> RUNNING or CONFIG state. Instead, it ignores the return value of
> internal function __rte_timer_reset() and always returns 0.
> We change rte_timer_reset() to return the value returned by
> __rte_timer_reset().
> 
> - Change API rte_timer_reset_sync() to invoke rte_pause() while
> spin-waiting for rte_timer_reset() to succeed.
> 
> - Enhance timer stress test 2 to report how many timer reset
> collisions occur, i.e., how many times rte_timer_reset() fails
> due to a timer being in the CONFIG state.
> 
> Signed-off-by: Robert Sanford <rsanford2@gmail.com>
> 
> ---
>  app/test/test_timer.c        |   25 ++++++++++++++++++++++---
>  lib/librte_timer/rte_timer.c |    7 +++----
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/app/test/test_timer.c b/app/test/test_timer.c
> index 4b4800b..2f27f84 100644
> --- a/app/test/test_timer.c
> +++ b/app/test/test_timer.c
> @@ -247,12 +247,15 @@ static int
>  timer_stress2_main_loop(__attribute__((unused)) void *arg)
>  {
>  	static struct rte_timer *timers;
> -	int i;
> +	int i, ret;
>  	static volatile int ready = 0;
>  	uint64_t delay = rte_get_timer_hz() / 4;
>  	unsigned lcore_id = rte_lcore_id();
> +	int32_t my_collisions = 0;
> +	static rte_atomic32_t collisions = RTE_ATOMIC32_INIT(0);
>  
>  	if (lcore_id == rte_get_master_lcore()) {
> +		cb_count = 0;
>  		timers = rte_malloc(NULL, sizeof(*timers) * NB_STRESS2_TIMERS, 0);
>  		if (timers == NULL) {
>  			printf("Test Failed\n");
> @@ -268,15 +271,24 @@ timer_stress2_main_loop(__attribute__((unused)) void *arg)
>  	}
>  
>  	/* have all cores schedule all timers on master lcore */
> -	for (i = 0; i < NB_STRESS2_TIMERS; i++)
> -		rte_timer_reset(&timers[i], delay, SINGLE, rte_get_master_lcore(),
> +	for (i = 0; i < NB_STRESS2_TIMERS; i++) {
> +		ret = rte_timer_reset(&timers[i], delay, SINGLE, rte_get_master_lcore(),
>  				timer_stress2_cb, NULL);
> +		/* there will be collisions when multiple cores simultaneously
> +		 * configure the same timers */
> +		if (ret != 0)
> +			my_collisions++;
> +	}
> +	if (my_collisions != 0)
> +		rte_atomic32_add(&collisions, my_collisions);
>  
>  	ready = 0;
>  	rte_delay_ms(500);
>  
>  	/* now check that we get the right number of callbacks */
>  	if (lcore_id == rte_get_master_lcore()) {
> +		if ((my_collisions = rte_atomic32_read(&collisions)) != 0)
> +			printf("- %d timer reset collisions (OK)\n", my_collisions);

That's not very important, but I think avoiding affectation + comparison
at the same time is clearer:

  my_collisions = rte_atomic32_read(&collisions);
  if (my_collisions != 0) {
  	...

>  		rte_timer_manage();
>  		if (cb_count != NB_STRESS2_TIMERS) {
>  			printf("Test Failed\n");
> @@ -311,6 +323,13 @@ timer_stress2_main_loop(__attribute__((unused)) void *arg)
>  	/* now check that we get the right number of callbacks */
>  	if (lcore_id == rte_get_master_lcore()) {
>  		rte_timer_manage();
> +
> +		/* clean up statics, in case we run again */
> +		rte_free(timers);
> +		timers = 0;

timers = NULL is better than timers = 0 as it's a pointer.

> +		ready = 0;

The lines above should go in another patch as it fixes another problem
(+ a memory leek).
"testpmd: allow to restart timer stress tests"


> +		rte_atomic32_set(&collisions, 0);
> +
>  		if (cb_count != NB_STRESS2_TIMERS) {
>  			printf("Test Failed\n");
>  			printf("- Stress test 2, part 2 failed\n");
> diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c
> index 269a992..d18abf5 100644
> --- a/lib/librte_timer/rte_timer.c
> +++ b/lib/librte_timer/rte_timer.c
> @@ -424,10 +424,8 @@ rte_timer_reset(struct rte_timer *tim, uint64_t ticks,
>  	else
>  		period = 0;
>  
> -	__rte_timer_reset(tim,  cur_time + ticks, period, tim_lcore,
> +	return __rte_timer_reset(tim,  cur_time + ticks, period, tim_lcore,
>  			  fct, arg, 0);
> -
> -	return 0;
>  }
>  
>  /* loop until rte_timer_reset() succeed */
> @@ -437,7 +435,8 @@ rte_timer_reset_sync(struct rte_timer *tim, uint64_t ticks,
>  		     rte_timer_cb_t fct, void *arg)
>  {
>  	while (rte_timer_reset(tim, ticks, type, tim_lcore,
> -			       fct, arg) != 0);
> +			       fct, arg) != 0)
> +		rte_pause();
>  }

Maybe the lines above could go to another patch too.
"timers: relax cpu in rte_timer_reset_sync()"

Also, I think the commit log should highlight the fact that
your patch also fixes rte_timer_reset_sync() that was not
working at all.


Thanks!
Olivier