Re: [RFC PATCH] rte_timer: Fix rte_timer_reset return value

[Olivier MATZ <olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org>]

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-VfR2kkLFssw@public.gmane.org --cc olivier.matz-pdR9zngts4EAvxtiuMwx3w@public.gmane.org \
 --in-reply-to="1422996127-64370-1-git-send-email-rsanford2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org" \
 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