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 8CF1020F for ; Fri, 6 Feb 2015 12:10:29 +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 1YJgrQ-0003Vn-0c; Fri, 06 Feb 2015 12:14:14 +0100 Message-ID: <54D4A116.1090402@6wind.com> Date: Fri, 06 Feb 2015 12:10:14 +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: rsanford2@gmail.com, dev@dpdk.org References: <1422996127-64370-1-git-send-email-rsanford2@gmail.com> In-Reply-To: <1422996127-64370-1-git-send-email-rsanford2@gmail.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 11:10:29 -0000 Hi Robert, Please see some comments below. On 02/03/2015 09:42 PM, rsanford2@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(&timers[i], delay, SINGLE, rte_get_master_lcore(), > + for (i = 0; i < NB_STRESS2_TIMERS; i++) { > + ret = rte_timer_reset(&timers[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(&collisions, 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(&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) { ... > 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(&collisions, 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 highlight the fact that your patch also fixes rte_timer_reset_sync() that was not working at all. Thanks! Olivier