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

  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(-)
  

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
  

Hi Olivier,

Thanks for reviewing this patch.
Please see my responses to your comments, below.

I also have one request for you. You probably use git almost every day. For
people who only use git maybe once per year, could you please show us the
exact sequence of commands that you run to prepare a patch series? We know
there are man pages and online documents, etc, but it would be an extremely
valuable jumpstart if you just give us a snippet of your shell history
showing the exact commands that you run to prepare and email a patch
series. I would much rather spend time getting the code right, and less
time learning (by trial and error) the nuances of git apply, add, commit,
format-patch, send-email, etc.


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

Yes, I will change this.


> > @@ -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.
>

Yes, I will change this.


> > +             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"
>

Yes, I will split it into two patches: rte_timer and test_timer. But, I
don't see much benefit in splitting test_timer.c changes into separate
patches for each bug discovered.


> > 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()"
>
>
If you mean that we should have one patch for rte_timer_reset() and one for
rte_timer_reset_sync(), my response is: Come on, these are two one-line
fixes in a pair of related and adjacent functions. Let's not go overboard
by splitting them into two patches. :-)

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.
>
>
We said something to that effect: "Change API rte_timer_reset_sync() to
invoke rte_pause() while spin-waiting for rte_timer_reset() to succeed." I
can use different wording if you like.


>
> Thanks!
> Olivier
>

Thank you,
Robert
  

Hi Robert,

On 02/06/2015 06:26 PM, Robert Sanford wrote:
> Hi Olivier,
> 
> Thanks for reviewing this patch.
> Please see my responses to your comments, below.
> 
> I also have one request for you. You probably use git almost every day.
> For people who only use git maybe once per year, could you please show
> us the exact sequence of commands that you run to prepare a patch
> series? We know there are man pages and online documents, etc, but it
> would be an extremely valuable jumpstart if you just give us a snippet
> of your shell history showing the exact commands that you run to prepare
> and email a patch series. I would much rather spend time getting the
> code right, and less time learning (by trial and error) the nuances of
> git apply, add, commit, format-patch, send-email, etc.

The following actions should work fine (please take the time to
understand them before execute them):

# start in your workspace with your patch commit
# "git log -1" shows your timer commit

# remove the commit but keep the files unmodified
git reset HEAD^

# you can see the modified files with "git diff" and "git status"

# Now do the modifications we discussed in the mail in the files

##### first patch: relax cpu

# Once it's done we will add the first commit (relax cpu)
# We will add it in the cache with "git add".
# '-p' means it will ask for each hunk.
git add -p lib/librte_timer/rte_timer.c
# so "n" to the first one, and "y" to the second (with the rte_pause)

# show the cache, it should show the content of the commit
git diff --cached
# add the commit log
git commit

##### second patch: first restarting

# same than above, we just want to commit a part of the diff
git add -p lib/librte_timer/rte_timer.c
# say "n" until you see the lines with "/* clean up statics, in case
# we run again */"
# Then say "e" (edit).

# In the editor, remove the line
#  "+               rte_atomic32_set(&collisions, 0);"
# (we want to have it in the 3rd patch, not this one)
# then save and exit from your editor

# show the cache, it should show the content of the commit
git diff --cached
# add the commit log
git commit

#### 3rd patch, the rest

# easy here
git add app/test/test_timer.c lib/librte_timer/rte_timer.c
git commit

#### check compilation

git rebase -i HEAD~3
# replace all "pick" by "edit", then save and exit

# git stops at your first commit, check compilation, then:
git rebase --contine

# git stops at your 2nd commit, same...
git rebase --contine

# one last time
git rebase --contine

#### send the email

# prepare a directory with your 3 patches
git format-patch -3 --cover-letter \
  --output-directory=timer-patches

# edit the cover letter
${EDITOR} timer-patches/0000-cover-letter.patch

# send it
git send-email --to dev@dpdk.org --cc olivier.matz@6wind.com \
 --in-reply-to="1422996127-64370-1-git-send-email-rsanford2@gmail.com" \
 timer-patches


Here it is. This is one solution, but probably other people do
differently.

The dpdk dev page http://dpdk.org/dev is simple but really useful to
remember the commands. The man pages of git are long, but really well
written, if you have a doubt, you can check there.


>     > +             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"
> 
> 
> Yes, I will split it into two patches: rte_timer and test_timer. But, I
> don't see much benefit in splitting test_timer.c changes into separate
> patches for each bug discovered.

There are several reasons why we want to split patches.

- Small patches are easier to understand (one problem -> one solution),
  it's easier to read for the reviewer, but also for all people that
  will browse the history later

- Smaller patches also reduce the risk to miss something, increasing
  code quality

- Some people may be maintaining their own stable dpdk branch. They can
  pick up only the patches they consider critical.

- When a developer takes time to present its patches properly, it's
  seen by the reviewers as a mark of respect by the reviewers, so they
  are happier to do the review.

>     > 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()"
> 
> 
> If you mean that we should have one patch for rte_timer_reset() and one
> for rte_timer_reset_sync(), my response is: Come on, these are two
> one-line fixes in a pair of related and adjacent functions. Let's not go
> overboard by splitting them into two patches. :-)
> 
>     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.
> 
> 
> We said something to that effect: "Change API rte_timer_reset_sync() to
> invoke rte_pause() while spin-waiting for rte_timer_reset() to succeed."
> I can use different wording if you like.

This does not say that rte_timer_reset_sync() was not working, it says
"change API"... and I don't really see where the API (the interface of
the function) is changed.


Regards,
Olivier
  

Changes in v2:
- split into multiple patches
- minor coding-style changes

Robert Sanford (3):
  timer: fix return value of rte_timer_reset(),
    insert rte_pause() into rte_timer_reset_sync() wait-loop
  app/test: fix timer stress test to succeed on multiple runs,
    display number of times that rte_timer_reset() fails
    (expected) due to races with other cores

app/test/test_timer.c        |   26 +++++++++++++++++++++++---
lib/librte_timer/rte_timer.c |    7 +++----
2 files changed, 26 insertions(+), 7 deletions(-)
  

Hi Robert,

On 02/25/2015 05:09 AM, Robert Sanford wrote:
> Changes in v2:
> - split into multiple patches
> - minor coding-style changes
>
> Robert Sanford (3):
>    timer: fix return value of rte_timer_reset(),
>      insert rte_pause() into rte_timer_reset_sync() wait-loop
>    app/test: fix timer stress test to succeed on multiple runs,
>      display number of times that rte_timer_reset() fails
>      (expected) due to races with other cores
>
> app/test/test_timer.c        |   26 +++++++++++++++++++++++---
> lib/librte_timer/rte_timer.c |    7 +++----
> 2 files changed, 26 insertions(+), 7 deletions(-)
>

Series:
Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thanks!
  

> > Changes in v2:
> > - split into multiple patches
> > - minor coding-style changes
> >
> > Robert Sanford (3):
> >    timer: fix return value of rte_timer_reset(),
> >      insert rte_pause() into rte_timer_reset_sync() wait-loop
> >    app/test: fix timer stress test to succeed on multiple runs,
> >      display number of times that rte_timer_reset() fails
> >      (expected) due to races with other cores
> 
> Series:
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied, thanks

Robert, as you well know rte_timer and you work on it,
maybe you are interested in becoming maintainer?
  

Hi Thomas,

Yes, I'm interested in becoming a maintainer of rte_timer. What are the
responsibilities?


One question about lib rte_timer that's been troubling me for a while: How
are skip lists better than BSD-style timer wheels?

--
Regards,
Robert


On Wed, Feb 25, 2015 at 4:46 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
wrote:

> > > Changes in v2:
> > > - split into multiple patches
> > > - minor coding-style changes
> > >
> > > Robert Sanford (3):
> > >    timer: fix return value of rte_timer_reset(),
> > >      insert rte_pause() into rte_timer_reset_sync() wait-loop
> > >    app/test: fix timer stress test to succeed on multiple runs,
> > >      display number of times that rte_timer_reset() fails
> > >      (expected) due to races with other cores
> >
> > Series:
> > Acked-by: Olivier Matz <olivier.matz@6wind.com>
>
> Applied, thanks
>
> Robert, as you well know rte_timer and you work on it,
> maybe you are interested in becoming maintainer?
>
  

2015-02-25 06:02, Robert Sanford:
> Hi Thomas,
> 
> Yes, I'm interested in becoming a maintainer of rte_timer. What are the
> responsibilities?

It means we know someone who can answer our questions about rte_timer.
Having you email in the MAINTAINERS file helps to CC you.
And we expect from the maintainer he tried to review patches for its part.
But reviews may be done by someone else.
In general, technical review from the maintainer is more trustable.

If you are still interested, please drop a patch like this one:
	http://dpdk.org/browse/dpdk/commit/?id=a7d7ece480093
  

On Wed, Feb 25, 2015 at 06:02:24AM -0500, Robert Sanford wrote:
> Hi Thomas,
> 
> Yes, I'm interested in becoming a maintainer of rte_timer. What are the
> responsibilities?
> 
> 
> One question about lib rte_timer that's been troubling me for a while: How
> are skip lists better than BSD-style timer wheels?
> 
> --

The skip list may not be any better than a timer wheel - it's just what is used
now, and it does give pretty good performance (insert O(log n) [up to a few 
million timers per core], expiry O(1)).
Originally in DPDK, the timers were maintained in a regular sorted linked list,
but that suffered from scalability issues when starting timers, or stopped before
expiry. The skip-list was therefore a big improvement on that, and gave us
much greater scalability in timers, without any regressions in performance. I
don't know if anyone has tried to implement and benchmark a timer-wheel based
rte_timer library replacement. I'd be interested to see a performance comparison
between the two implementations! :-)

Regards,
/Bruce

> Regards,
> Robert
> 
> 
> On Wed, Feb 25, 2015 at 4:46 AM, Thomas Monjalon <thomas.monjalon@6wind.com>
> wrote:
> 
> > > > Changes in v2:
> > > > - split into multiple patches
> > > > - minor coding-style changes
> > > >
> > > > Robert Sanford (3):
> > > >    timer: fix return value of rte_timer_reset(),
> > > >      insert rte_pause() into rte_timer_reset_sync() wait-loop
> > > >    app/test: fix timer stress test to succeed on multiple runs,
> > > >      display number of times that rte_timer_reset() fails
> > > >      (expected) due to races with other cores
> > >
> > > Series:
> > > Acked-by: Olivier Matz <olivier.matz@6wind.com>
> >
> > Applied, thanks
> >
> > Robert, as you well know rte_timer and you work on it,
> > maybe you are interested in becoming maintainer?
> >
  

On Wed, Feb 25, 2015 at 6:16 AM, Bruce Richardson <
bruce.richardson@intel.com> wrote:

> On Wed, Feb 25, 2015 at 06:02:24AM -0500, Robert Sanford wrote:
>
> >
> > One question about lib rte_timer that's been troubling me for a while:
> How
> > are skip lists better than BSD-style timer wheels?
> >
> > --
>
> The skip list may not be any better than a timer wheel - it's just what is
> used
> now, and it does give pretty good performance (insert O(log n) [up to a few
> million timers per core], expiry O(1)).
> Originally in DPDK, the timers were maintained in a regular sorted linked
> list,
> but that suffered from scalability issues when starting timers, or stopped
> before
> expiry. The skip-list was therefore a big improvement on that, and gave us
> much greater scalability in timers, without any regressions in
> performance. I
> don't know if anyone has tried to implement and benchmark a timer-wheel
> based
> rte_timer library replacement. I'd be interested to see a performance
> comparison
> between the two implementations! :-)
>
> Regards,
> /Bruce
>
>
I've wanted to try out the timer-wheels since before the skip-list version,
but it just hasn't made it to the top of my priority list.
The other thing that concerns me about the skip-list implementation is the
extra cache line that all those pointers consume.

--
Thanks,
Robert