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

[Olivier MATZ]

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(, 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/-cover-letter.patch

# send it
git send-email --to dev at dpdk.org --cc olivier.matz at 6wind.com \
 --in-reply-to="1422996127-64370-1-git-send-email-rsanford2 at 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 

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

[Robert Sanford]

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()) != 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();
>   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

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

[Olivier MATZ]

Hi Robert,

Please see some comments below.

On 02/03/2015 09:42 PM, rsanford2 at gmail.com wrote:
> From: Robert Sanford 
> 
> - 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 
> 
> ---
>  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([i], delay, SINGLE, 
> rte_get_master_lcore(),
> + for (i = 0; i < NB_STRESS2_TIMERS; i++) {
> + ret = rte_timer_reset([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(, 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()) != 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();
  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(, 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 

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

[rsanfo...@gmail.com]

From: Robert Sanford 

- 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 

---
 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([i], delay, SINGLE, 
rte_get_master_lcore(),
+   for (i = 0; i < NB_STRESS2_TIMERS; i++) {
+   ret = rte_timer_reset([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(, 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()) != 0)
+   printf("- %d timer reset collisions (OK)\n", 
my_collisions);
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;
+   ready = 0;
+   rte_atomic32_set(, 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();
 }

 /* Stop the timer associated with the timer handle tim */
-- 
1.7.1