From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-la0-f52.google.com (mail-la0-f52.google.com [209.85.215.52]) by dpdk.org (Postfix) with ESMTP id 7D5DD234 for ; Fri, 6 Feb 2015 18:26:30 +0100 (CET) Received: by labgf13 with SMTP id gf13so2555558lab.8 for ; Fri, 06 Feb 2015 09:26:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=SvLHynD8kt6i+UMK3zIaAhJlx5wlrTCkaU0ct/YGhkA=; b=HyQZAppCMZv1YOKmtPjEzBRaj0BtsATvtNKsQPfyY6sUj3/ph/hlQWtUICGCbGQ6Sk mm5PMuGlVfymmKRMUEVuG1Dgv0ZyR3gU3w6n7E9NqZtmwzo0vDRNS/T/g5lQvQ9mYKmq WgaBW9hdTXLreWlR1T+UiqUNfE6WQHQncmBjQpw5QCU7/YHT2ke8t85wAEHzHt1T2z7W 6103xJKhKXnOnPctjQNP4H5AGRho7PX4xB66aR++3s6ezC6HIui4yIZlQjbbd7i9Am4B MVZt26LRV23t8ZI5ojx8sG6hDTPTi4VRENpfbyFZiVGNjKFgVDTSBH0km2kaDOQHD60Z wUYA== MIME-Version: 1.0 X-Received: by 10.152.37.5 with SMTP id u5mr4009974laj.109.1423243590127; Fri, 06 Feb 2015 09:26:30 -0800 (PST) Received: by 10.152.44.161 with HTTP; Fri, 6 Feb 2015 09:26:30 -0800 (PST) In-Reply-To: <54D4A116.1090402@6wind.com> References: <1422996127-64370-1-git-send-email-rsanford2@gmail.com> <54D4A116.1090402@6wind.com> Date: Fri, 6 Feb 2015 12:26:30 -0500 Message-ID: From: Robert Sanford To: Olivier MATZ Content-Type: text/plain; charset=UTF-8 X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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: Fri, 06 Feb 2015 17:26:30 -0000 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