Message ID | 1468778880-25515-1-git-send-email-h.mikita89@gmail.com (mailing list archive) |
---|---|
State | Accepted, archived |
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 692B92BCF; Sun, 17 Jul 2016 20:08:16 +0200 (CEST) Received: from mail-pf0-f196.google.com (mail-pf0-f196.google.com [209.85.192.196]) by dpdk.org (Postfix) with ESMTP id AC3E22BCE for <dev@dpdk.org>; Sun, 17 Jul 2016 20:08:14 +0200 (CEST) Received: by mail-pf0-f196.google.com with SMTP id g202so10065943pfb.1 for <dev@dpdk.org>; Sun, 17 Jul 2016 11:08:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=db8aHMgBTYOKfu0lNluvAcZ8Uew+68066WJDioj5QAk=; b=FM2lCbyTbAYeIiOG8QnJ97d4QrAYabZ9N6fwE9LBbCC1zMrhImO3yf4vApkIEm3VId 8weQPhGUaedHJ90hwE/zOD/Tbrf+uoNUjFNfKLXIxRbiCqQ0tu4e2/u4ZJeHwx43Abu9 lNT7NeSquanj9mlrfAgNX0w5pqBo4gwZnk36kzbtTt4zSWywG6Ev/Io2rD6qIMLk2Hid 0qQFjak7qLtplAh8szlMddSLB8zloMBc6aMQ3XxmC0i0b1hfSN93yAkkk4vQdiuLEDMT Si4YtOG1fZt5nYA5zmuxVLbieLK/rmiGiQufdExxl7Oc/RyzcGoYoHcFYQ0pdnmAmAff wCcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=db8aHMgBTYOKfu0lNluvAcZ8Uew+68066WJDioj5QAk=; b=JgkYGhRjumRT7MfD2gX7G9Lzk00iiqepIfSCQpxcRAXM/Zd7sKa/sSI1mz6j0Vy7l3 GnsBMPFc6+GAopi6EAvzSB9isjbU+mZ7HbhI7y448Tywve/27h6h6/HDhSfVmTitB35k F8Cjcyy3HTCn0cas42XcBjAWHzN705lRbbMxzJ0UUBrE+6Y7XkW+hMOBNFrbxgqW5Xxs aoIw2YPqeX4dndae/oZC8vEkkarsLQhCwYLueHqdJ2Ynh6C4g0eTiA70mlMcWUVQI5g8 wOID7BLCNHxKM8VJ5YOt1LW/sjHgit80BYbePXLmubdcOQktL0bSXBLxt/XV5J+8XRmV 7FzQ== X-Gm-Message-State: ALyK8tLqdsgBFwuSyoSz/LI7HyAaamlfuBCxXYNONNIOl11xj2Bz/snT3pywNdoEu7vviw== X-Received: by 10.98.35.7 with SMTP id j7mr24754106pfj.39.1468778893852; Sun, 17 Jul 2016 11:08:13 -0700 (PDT) Received: from localhost.localdomain (183.180.67.214.ap.gmobb-fix.jp. [183.180.67.214]) by smtp.gmail.com with ESMTPSA id z10sm3375703pff.95.2016.07.17.11.08.12 (version=TLS1_2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sun, 17 Jul 2016 11:08:13 -0700 (PDT) From: Hiroyuki Mikita <h.mikita89@gmail.com> To: rsanford@akamai.com Cc: dev@dpdk.org Date: Mon, 18 Jul 2016 03:08:00 +0900 Message-Id: <1468778880-25515-1-git-send-email-h.mikita89@gmail.com> X-Mailer: git-send-email 2.7.4 Subject: [dpdk-dev] [PATCH] timer: fix break list when timer_cb reset running timer X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Hiroyuki Mikita
July 17, 2016, 6:08 p.m. UTC
When timer_cb resets another running timer on the same lcore,
the list of expired timers is chained to the pending-list.
This commit prevents a running timer from being reset
by not its own timer_cb.
Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com>
---
lib/librte_timer/rte_timer.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
Comments
Hi Hiroyuki, I am reviewing your 3 timer patches. Can you please explain in more detail your use-case that results in a problem? For example, is it when timer A's callback tries to reset (rte_timer_reset) timer B? If yes, is timer B in PENDING state and likely to expire soon? -- Thanks, Robert On 7/17/16 2:08 PM, "Hiroyuki Mikita" <h.mikita89@gmail.com> wrote: >When timer_cb resets another running timer on the same lcore, >the list of expired timers is chained to the pending-list. >This commit prevents a running timer from being reset >by not its own timer_cb. > >Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com> >--- > lib/librte_timer/rte_timer.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > >diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c >index 3dcdab5..00e80c9 100644 >--- a/lib/librte_timer/rte_timer.c >+++ b/lib/librte_timer/rte_timer.c >@@ -69,6 +69,9 @@ struct priv_timer { > > unsigned prev_lcore; /**< used for lcore round robin */ > >+ /** running timer on this lcore now */ >+ struct rte_timer *running_tim; >+ > #ifdef RTE_LIBRTE_TIMER_DEBUG > /** per-lcore statistics */ > struct rte_timer_debug_stats stats; >@@ -135,9 +138,12 @@ timer_set_config_state(struct rte_timer *tim, > while (success == 0) { > prev_status.u32 = tim->status.u32; > >- /* timer is running on another core, exit */ >+ /* timer is running on another core >+ * or ready to run on local core, exit >+ */ > if (prev_status.state == RTE_TIMER_RUNNING && >- prev_status.owner != (uint16_t)lcore_id) >+ (prev_status.owner != (uint16_t)lcore_id || >+ tim != priv_timer[lcore_id].running_tim)) > return -1; > > /* timer is being configured on another core */ >@@ -580,6 +586,7 @@ void rte_timer_manage(void) > for (tim = run_first_tim; tim != NULL; tim = next_tim) { > next_tim = tim->sl_next[0]; > priv_timer[lcore_id].updated = 0; >+ priv_timer[lcore_id].running_tim = tim; > > /* execute callback function with list unlocked */ > tim->f(tim, tim->arg); >@@ -610,6 +617,7 @@ void rte_timer_manage(void) > rte_spinlock_unlock(&priv_timer[lcore_id].list_lock); > } > } >+ priv_timer[lcore_id].running_tim = NULL; > } > > /* dump statistics about timers */ >-- >2.7.4 >
Hi Robert, Thank you for reviewing. In the following case, the skip list is broken. - Timer A and timer B are configured on the same lcore, in the same pending list. - The expire time of timer A is earlier than that of timer B. - rte_timer_manage() is called on the lcore after the expire time of timer B. - The callback of timer A resets timer B. In rte_timer_manage() process, both timers are expired at the same time, so the running list includes both timers. States of both timers are transited from PENDING to RUNNING, then callbacks are executed sequentially. The callback of timer A resets timer B which is still RUNNING, so timer B is added to the pending-list. In this time, timer B is in both the running list and the pending list. It means that the running list is chained to the pending list. Both lists are broken. Regards, Hiroyuki
On 7/17/16 2:08 PM, "Hiroyuki Mikita" <h.mikita89@gmail.com> wrote: >When timer_cb resets another running timer on the same lcore, >the list of expired timers is chained to the pending-list. >This commit prevents a running timer from being reset >by not its own timer_cb. > >Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com> >--- > lib/librte_timer/rte_timer.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > >diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c >index 3dcdab5..00e80c9 100644 >--- a/lib/librte_timer/rte_timer.c >+++ b/lib/librte_timer/rte_timer.c >@@ -69,6 +69,9 @@ struct priv_timer { > > unsigned prev_lcore; /**< used for lcore round robin */ > >+ /** running timer on this lcore now */ >+ struct rte_timer *running_tim; >+ > #ifdef RTE_LIBRTE_TIMER_DEBUG > /** per-lcore statistics */ > struct rte_timer_debug_stats stats; >@@ -135,9 +138,12 @@ timer_set_config_state(struct rte_timer *tim, > while (success == 0) { > prev_status.u32 = tim->status.u32; > >- /* timer is running on another core, exit */ >+ /* timer is running on another core >+ * or ready to run on local core, exit >+ */ > if (prev_status.state == RTE_TIMER_RUNNING && >- prev_status.owner != (uint16_t)lcore_id) >+ (prev_status.owner != (uint16_t)lcore_id || >+ tim != priv_timer[lcore_id].running_tim)) > return -1; > > /* timer is being configured on another core */ >@@ -580,6 +586,7 @@ void rte_timer_manage(void) > for (tim = run_first_tim; tim != NULL; tim = next_tim) { > next_tim = tim->sl_next[0]; > priv_timer[lcore_id].updated = 0; >+ priv_timer[lcore_id].running_tim = tim; > > /* execute callback function with list unlocked */ > tim->f(tim, tim->arg); >@@ -610,6 +617,7 @@ void rte_timer_manage(void) > rte_spinlock_unlock(&priv_timer[lcore_id].list_lock); > } > } >+ priv_timer[lcore_id].running_tim = NULL; > } > > /* dump statistics about timers */ >-- >2.7.4 > Acked-by: Robert Sanford <rsanford@akamai.com> I tested the three timer patches with app/test timer_autotest and timer_racecond_autotest, and additional private tests.
2016-07-23 0:14 GMT+02:00 Sanford, Robert <rsanford@akamai.com>: > Acked-by: Robert Sanford <rsanford@akamai.com> > > I tested the three timer patches with app/test timer_autotest and > timer_racecond_autotest, and additional private tests. Thanks Robert. Are you confident enough to integrate them in the last days of 16.07? How critical are they? Should we make a RC5 for them?
On 7/23/16 4:49 AM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote: >2016-07-23 0:14 GMT+02:00 Sanford, Robert <rsanford@akamai.com>: >> Acked-by: Robert Sanford <rsanford@akamai.com> >> >> I tested the three timer patches with app/test timer_autotest and >> timer_racecond_autotest, and additional private tests. > >Thanks Robert. >Are you confident enough to integrate them in the last days of 16.07? >How critical are they? >Should we make a RC5 for them? Yes, I'm confident that the patches are safe and correct. However, I'm not sure that we should make a RC just for them. Patch 1 fixes a problem where we incorrectly lower the depth of the skip list, resulting in performance that does not live up to O(log n) that we expect. Summary: performance degradation with large number of timers. Patch 2 fixes a minor inefficiency when timer_manage() races with timer_stop() or timer_reset(). Patch 3 fixes the most serious problem: We may corrupt timer list links if multiple timers expire at roughly the same time, and one of those timers' callback tries to stop/reset other(s) that are scheduled to run in the same call to timer_manage(). Question for Hiroyuki: How did you discover timer linked-list corruption? By code inspection, or do you have a real application that needs that functionality (timers stop/reset each other at roughly the same time)? Regards, Robert
Hi Robert, My application had timers which reset another timer and sometimes did not work. Its profile by 'perf' command showed timer_add occupied 99.9% CPU. It seemed that an infinite loop occurred in rte_timer. I inspected timer codes and found that the main cause was a bug of the patch 3. In this process, I also found the other bugs. Regards, Hiroyuki
Hiroyuki, Robert, I would like to apply these patches quickly. Please could you provide some "Fixes:" line to know the origin of the bugs? Thanks
Fixes: a4b7a5a45cf5 ("timer: fix race condition") 2016-07-25 23:43 GMT+09:00 Thomas Monjalon <thomas.monjalon@6wind.com>: > Hiroyuki, Robert, > I would like to apply these patches quickly. > Please could you provide some "Fixes:" line to know the origin > of the bugs? > Thanks >
2016-07-26 00:14, Hiroyuki Mikita: > Fixes: a4b7a5a45cf5 ("timer: fix race condition") > > 2016-07-25 23:43 GMT+09:00 Thomas Monjalon <thomas.monjalon@6wind.com>: > > Hiroyuki, Robert, > > I would like to apply these patches quickly. > > Please could you provide some "Fixes:" line to know the origin > > of the bugs? > > Thanks Thanks a lot, Hiroyuki
> >When timer_cb resets another running timer on the same lcore, > >the list of expired timers is chained to the pending-list. > >This commit prevents a running timer from being reset > >by not its own timer_cb. > > > >Signed-off-by: Hiroyuki Mikita <h.mikita89@gmail.com> > > Acked-by: Robert Sanford <rsanford@akamai.com> > > I tested the three timer patches with app/test timer_autotest and > timer_racecond_autotest, and additional private tests. Applied, thanks
diff --git a/lib/librte_timer/rte_timer.c b/lib/librte_timer/rte_timer.c index 3dcdab5..00e80c9 100644 --- a/lib/librte_timer/rte_timer.c +++ b/lib/librte_timer/rte_timer.c @@ -69,6 +69,9 @@ struct priv_timer { unsigned prev_lcore; /**< used for lcore round robin */ + /** running timer on this lcore now */ + struct rte_timer *running_tim; + #ifdef RTE_LIBRTE_TIMER_DEBUG /** per-lcore statistics */ struct rte_timer_debug_stats stats; @@ -135,9 +138,12 @@ timer_set_config_state(struct rte_timer *tim, while (success == 0) { prev_status.u32 = tim->status.u32; - /* timer is running on another core, exit */ + /* timer is running on another core + * or ready to run on local core, exit + */ if (prev_status.state == RTE_TIMER_RUNNING && - prev_status.owner != (uint16_t)lcore_id) + (prev_status.owner != (uint16_t)lcore_id || + tim != priv_timer[lcore_id].running_tim)) return -1; /* timer is being configured on another core */ @@ -580,6 +586,7 @@ void rte_timer_manage(void) for (tim = run_first_tim; tim != NULL; tim = next_tim) { next_tim = tim->sl_next[0]; priv_timer[lcore_id].updated = 0; + priv_timer[lcore_id].running_tim = tim; /* execute callback function with list unlocked */ tim->f(tim, tim->arg); @@ -610,6 +617,7 @@ void rte_timer_manage(void) rte_spinlock_unlock(&priv_timer[lcore_id].list_lock); } } + priv_timer[lcore_id].running_tim = NULL; } /* dump statistics about timers */