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