From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 28A37234 for ; Sun, 8 Feb 2015 11:53:30 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:DHE_RSA_AES_128_CBC_SHA1:128) (Exim 4.80) (envelope-from ) id 1YKPY5-0004mG-Do; Sun, 08 Feb 2015 11:57:16 +0100 Message-ID: <54D7401C.4020904@6wind.com> Date: Sun, 08 Feb 2015 11:53:16 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.3.0 MIME-Version: 1.0 To: Robert Sanford References: <1422996127-64370-1-git-send-email-rsanford2@gmail.com> <54D4A116.1090402@6wind.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [RFC PATCH] rte_timer: Fix rte_timer_reset return value X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 08 Feb 2015 10:53:30 -0000 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