Message ID | 20231211015717.1067822-1-joel@joelfernandes.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:bcd1:0:b0:403:3b70:6f57 with SMTP id r17csp6788039vqy; Sun, 10 Dec 2023 18:03:32 -0800 (PST) X-Google-Smtp-Source: AGHT+IHOvQPSqSlA9gXmjqaaw42cC5SJwBAKWIxuNGU2iQCUq4O69U6zHXDen7/ANYBvnT5Q7wv1 X-Received: by 2002:a92:c267:0:b0:35d:65fd:184b with SMTP id h7-20020a92c267000000b0035d65fd184bmr6943821ild.4.1702260212123; Sun, 10 Dec 2023 18:03:32 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1702260212; cv=none; d=google.com; s=arc-20160816; b=M1anbjvDbYM2FNmRJHTt7PhGTYmYJmvhh3fYhL+0vdqwAD9ekuKaaw4KTPb6S7cUT1 KgO7IlvJQhVlOyvsZOtOfPLGwL9bD+bLl66nzH8heDiJ32odY2QeQTsWvCuRTt8pxe1F 8XTzeHEm77PJ6iTgeyM7c1IglfFQufbGYKeOqzJmVTp2fI6LmavKB1W3GM7nRI1YEd3C vxDl4DDqatlodEY+3q23whIiU/3Y91lCijxflA5iMwYzRLrJvKiKgvtR3s+5JbVnlJR9 j9Rg6NuG1uouenwmRDE/8hqq7xtJHpMfryQWz5ZZLjWayRnWTTBUhxyFrZP6o7NN8Tzg w2nw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=/Vr+YzVHJ/mPJrnzMDjXJsm4sTrJoVDHcdAw1WNnfhs=; fh=BnbAwVfaF3tWfO2YnZVpfq4cR+VTwjdVCnJMMHddqn4=; b=eHhM6lIqQJl8jrhO0oK5EGrxoAvPZmBRMgFDXdUrZOLzgYlNQ25fKTQaYHuSZ6mwnB EMMkGHwnnKzvcvWtwTApAy/rE8DN+HlNoJWdnmHC5YobOtFItbAi2WXrFvslUIiQ+xRb biZX6ay58ZEFVBXkY1+NFupSwJVaIiwbkRRjnywgXoc9eIq+iSAZIXpabIQgKVWNS9Ju u48ieCPrP7zkel6dQub2pGuboCwP9/k4ADH55vU0JlKSHXnfucicb38JPwn8wxrTJ50F KvBBr2mqNy5YWF/ueGfbC5vF7Dd17br1E/j8s/QYzJoCCwFsRovAwN2LtOV7fh0pH9AL nUrg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@joelfernandes.org header.s=google header.b=o92bPhVg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id ba8-20020a170902720800b001d033061b35si5329773plb.49.2023.12.10.18.03.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 10 Dec 2023 18:03:32 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@joelfernandes.org header.s=google header.b=o92bPhVg; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 3C9E18075912; Sun, 10 Dec 2023 18:03:31 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232299AbjLKB5Z (ORCPT <rfc822;hjfbswb@gmail.com> + 99 others); Sun, 10 Dec 2023 20:57:25 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49320 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229483AbjLKB5Y (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sun, 10 Dec 2023 20:57:24 -0500 Received: from mail-io1-xd2c.google.com (mail-io1-xd2c.google.com [IPv6:2607:f8b0:4864:20::d2c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AA82EE5 for <linux-kernel@vger.kernel.org>; Sun, 10 Dec 2023 17:57:30 -0800 (PST) Received: by mail-io1-xd2c.google.com with SMTP id ca18e2360f4ac-7b6fc474d16so168517639f.1 for <linux-kernel@vger.kernel.org>; Sun, 10 Dec 2023 17:57:30 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=joelfernandes.org; s=google; t=1702259849; x=1702864649; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=/Vr+YzVHJ/mPJrnzMDjXJsm4sTrJoVDHcdAw1WNnfhs=; b=o92bPhVgbk4+YC5frbcvgO0CTMoUE0sUyyXnbwH+wCFtUN6ib9YMCx0tp8EOYws4C1 i7pN14jEvQUfKBdfvVJ3GhT7GzpAqoOF6sEGmVU40vHAt4pTuJYP2bNG4tkKcwbbZupN Nuom+jKi1EpuYFxmOY6M2q6xjvv+chr+00EE8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1702259849; x=1702864649; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=/Vr+YzVHJ/mPJrnzMDjXJsm4sTrJoVDHcdAw1WNnfhs=; b=FzQdHbde93OAislZu9S3XLOSmRH8oOlgayasCM+PgM4lpN61dXui9gWAFVtH/tuMOd QmvjMVQn57Nyns1k+KfnjBFqLube39O84SPdD105yw7zTtTdm09sAOFxCc2Qf/nbyqFD NQu88iUbyrAEAIgEr3aaQoj4Icu0wby9QoyhJwF8JX0vh0srH42rpzBA8td2cknFuv6q h9wkiQw4oODXpA5b0PO7jtgLY3gYOC+TJ2Ozdnat3Nki7UBaxs3AZHgEVQ/Hl2+eSP/D G1bbjlimxIWu9ka2m+53igKtrQcGcfmIIsVTmHq7m1J3kYQzLdL6Bum9PnqVoro2ox2z wIoA== X-Gm-Message-State: AOJu0Yy5xOoLqmKkJ1ihuIjvp5n5ExD1sAyuQgc8uQD0vl1LAX360Iwa zrG0HRaNdN4L4MVCJjZu6yhk+BiD+f6q/o7gWz0= X-Received: by 2002:a6b:4115:0:b0:7b4:28f8:d0 with SMTP id n21-20020a6b4115000000b007b428f800d0mr5524811ioa.25.1702259849093; Sun, 10 Dec 2023 17:57:29 -0800 (PST) Received: from joelboxx5.c.googlers.com.com (74.120.171.34.bc.googleusercontent.com. [34.171.120.74]) by smtp.gmail.com with ESMTPSA id h11-20020a056602130b00b007b74680132esm474248iov.38.2023.12.10.17.57.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 10 Dec 2023 17:57:28 -0800 (PST) From: "Joel Fernandes (Google)" <joel@joelfernandes.org> To: linux-kernel@vger.kernel.org, Lai Jiangshan <jiangshanlai@gmail.com>, "Paul E. McKenney" <paulmck@kernel.org>, Josh Triplett <josh@joshtriplett.org>, Steven Rostedt <rostedt@goodmis.org>, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: "Joel Fernandes (Google)" <joel@joelfernandes.org>, Frederic Weisbecker <frederic@kernel.org>, Neeraj Upadhyay <neeraj.iitr10@gmail.com>, rcu@vger.kernel.org Subject: [PATCH v2] srcu: Improve comments about acceleration leak Date: Mon, 11 Dec 2023 01:57:16 +0000 Message-ID: <20231211015717.1067822-1-joel@joelfernandes.org> X-Mailer: git-send-email 2.43.0.472.g3155946c3a-goog MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Sun, 10 Dec 2023 18:03:31 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1784949203869901740 X-GMAIL-MSGID: 1784949203869901740 |
Series |
[v2] srcu: Improve comments about acceleration leak
|
|
Commit Message
Joel Fernandes
Dec. 11, 2023, 1:57 a.m. UTC
The comments added in commit 1ef990c4b36b ("srcu: No need to
advance/accelerate if no callback enqueued") are a bit confusing to me.
The comments are describing a scenario for code that was moved and is
no longer the way it was (snapshot after advancing). Improve the code
comments to reflect this and also document by acceleration can never
fail.
Cc: Frederic Weisbecker <frederic@kernel.org>
Cc: Neeraj Upadhyay <neeraj.iitr10@gmail.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
v1->v2: Fix typo in change log.
kernel/rcu/srcutree.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
Comments
> On Dec 10, 2023, at 8:57 PM, Joel Fernandes (Google) <joel@joelfernandes.org> wrote: > > The comments added in commit 1ef990c4b36b ("srcu: No need to > advance/accelerate if no callback enqueued") are a bit confusing to me. > The comments are describing a scenario for code that was moved and is > no longer the way it was (snapshot after advancing). Improve the code > comments to reflect this and also document by acceleration can never > fail. > > Cc: Frederic Weisbecker <frederic@kernel.org> > Cc: Neeraj Upadhyay <neeraj.iitr10@gmail.com> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> Do we want to quick review and put it in Neeraj PR? Or next merge window ok with me. Just that then I have to keep track of it ;-) Thanks, - Joel > --- > v1->v2: Fix typo in change log. > > kernel/rcu/srcutree.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 0351a4e83529..051e149490d1 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1234,11 +1234,20 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, > if (rhp) > rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp); > /* > - * The snapshot for acceleration must be taken _before_ the read of the > - * current gp sequence used for advancing, otherwise advancing may fail > - * and acceleration may then fail too. > + * It's crucial to capture the snapshot 's' for acceleration before > + * reading the current gp_seq that is used for advancing. This is > + * essential because if the acceleration snapshot is taken after a > + * failed advancement attempt, there's a risk that a grace period may > + * conclude and a new one may start in the interim. If the snapshot is > + * captured after this sequence of events, the acceleration snapshot 's' > + * could be excessively advanced, leading to acceleration failure. > + * In such a scenario, an 'acceleration leak' can occur, where new > + * callbacks become indefinitely stuck in the RCU_NEXT_TAIL segment. > + * Also note that encountering advancing failures is a normal > + * occurrence when the grace period for RCU_WAIT_TAIL is in progress. > * > - * This could happen if: > + * To see this, consider the following events which occur if > + * rcu_seq_snap() were to be called after advance: > * > * 1) The RCU_WAIT_TAIL segment has callbacks (gp_num = X + 4) and the > * RCU_NEXT_READY_TAIL also has callbacks (gp_num = X + 8). > @@ -1264,6 +1273,13 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, > if (rhp) { > rcu_segcblist_advance(&sdp->srcu_cblist, > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq)); > + /* > + * Acceleration can never fail because the state of gp_seq used > + * for advancing is <= the state of gp_seq used for > + * acceleration. This means that RCU_NEXT_TAIL segment will > + * always be able to be emptied by the acceleration into the > + * RCU_NEXT_READY_TAIL or RCU_WAIT_TAIL segments. > + */ > WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s)); > } > if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) { > -- > 2.43.0.472.g3155946c3a-goog >
On Tue, Dec 12, 2023 at 03:11:30PM -0500, Joel Fernandes wrote: > > > > On Dec 10, 2023, at 8:57 PM, Joel Fernandes (Google) <joel@joelfernandes.org> wrote: > > > > The comments added in commit 1ef990c4b36b ("srcu: No need to > > advance/accelerate if no callback enqueued") are a bit confusing to me. > > The comments are describing a scenario for code that was moved and is > > no longer the way it was (snapshot after advancing). Improve the code > > comments to reflect this and also document by acceleration can never > > fail. > > > > Cc: Frederic Weisbecker <frederic@kernel.org> > > Cc: Neeraj Upadhyay <neeraj.iitr10@gmail.com> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > Do we want to quick review and put it in Neeraj PR? > > Or next merge window ok with me. Just that then I have to keep track of it ;-) Or it could get an ack and I could pull it into -rcu. ;-) Thanx, Paul > Thanks, > > - Joel > > > > > --- > > v1->v2: Fix typo in change log. > > > > kernel/rcu/srcutree.c | 24 ++++++++++++++++++++---- > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 0351a4e83529..051e149490d1 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1234,11 +1234,20 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, > > if (rhp) > > rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp); > > /* > > - * The snapshot for acceleration must be taken _before_ the read of the > > - * current gp sequence used for advancing, otherwise advancing may fail > > - * and acceleration may then fail too. > > + * It's crucial to capture the snapshot 's' for acceleration before > > + * reading the current gp_seq that is used for advancing. This is > > + * essential because if the acceleration snapshot is taken after a > > + * failed advancement attempt, there's a risk that a grace period may > > + * conclude and a new one may start in the interim. If the snapshot is > > + * captured after this sequence of events, the acceleration snapshot 's' > > + * could be excessively advanced, leading to acceleration failure. > > + * In such a scenario, an 'acceleration leak' can occur, where new > > + * callbacks become indefinitely stuck in the RCU_NEXT_TAIL segment. > > + * Also note that encountering advancing failures is a normal > > + * occurrence when the grace period for RCU_WAIT_TAIL is in progress. > > * > > - * This could happen if: > > + * To see this, consider the following events which occur if > > + * rcu_seq_snap() were to be called after advance: > > * > > * 1) The RCU_WAIT_TAIL segment has callbacks (gp_num = X + 4) and the > > * RCU_NEXT_READY_TAIL also has callbacks (gp_num = X + 8). > > @@ -1264,6 +1273,13 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, > > if (rhp) { > > rcu_segcblist_advance(&sdp->srcu_cblist, > > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq)); > > + /* > > + * Acceleration can never fail because the state of gp_seq used > > + * for advancing is <= the state of gp_seq used for > > + * acceleration. This means that RCU_NEXT_TAIL segment will > > + * always be able to be emptied by the acceleration into the > > + * RCU_NEXT_READY_TAIL or RCU_WAIT_TAIL segments. > > + */ > > WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s)); > > } > > if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) { > > -- > > 2.43.0.472.g3155946c3a-goog > >
Le Mon, Dec 11, 2023 at 01:57:16AM +0000, Joel Fernandes (Google) a écrit : > The comments added in commit 1ef990c4b36b ("srcu: No need to > advance/accelerate if no callback enqueued") are a bit confusing to me. I know some maintainers who may argue that in the changelog world, the first person doesn't exist :-) > The comments are describing a scenario for code that was moved and is > no longer the way it was (snapshot after advancing). Improve the code > comments to reflect this and also document by acceleration can never s/by/why > fail. > > Cc: Frederic Weisbecker <frederic@kernel.org> > Cc: Neeraj Upadhyay <neeraj.iitr10@gmail.com> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > --- > v1->v2: Fix typo in change log. > > kernel/rcu/srcutree.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index 0351a4e83529..051e149490d1 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1234,11 +1234,20 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, > if (rhp) > rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp); > /* > - * The snapshot for acceleration must be taken _before_ the read of the > - * current gp sequence used for advancing, otherwise advancing may fail > - * and acceleration may then fail too. > + * It's crucial to capture the snapshot 's' for acceleration before > + * reading the current gp_seq that is used for advancing. This is > + * essential because if the acceleration snapshot is taken after a > + * failed advancement attempt, there's a risk that a grace period may > + * conclude and a new one may start in the interim. If the snapshot is > + * captured after this sequence of events, the acceleration snapshot 's' > + * could be excessively advanced, leading to acceleration failure. > + * In such a scenario, an 'acceleration leak' can occur, where new > + * callbacks become indefinitely stuck in the RCU_NEXT_TAIL segment. > + * Also note that encountering advancing failures is a normal > + * occurrence when the grace period for RCU_WAIT_TAIL is in progress. > * > - * This could happen if: > + * To see this, consider the following events which occur if > + * rcu_seq_snap() were to be called after advance: > * > * 1) The RCU_WAIT_TAIL segment has callbacks (gp_num = X + 4) and the > * RCU_NEXT_READY_TAIL also has callbacks (gp_num = X + 8). > @@ -1264,6 +1273,13 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, > if (rhp) { > rcu_segcblist_advance(&sdp->srcu_cblist, > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq)); > + /* > + * Acceleration can never fail because the state of gp_seq used > + * for advancing is <= the state of gp_seq used for > + * acceleration. What do you mean by "state" here? If it's the gp_seq number, that doesn't look right. The situation raising the initial bug also involved a gp_seq used for advancing <= the gp_seq used for acceleration. Thanks. > + This means that RCU_NEXT_TAIL segment will > + * always be able to be emptied by the acceleration into the > + * RCU_NEXT_READY_TAIL or RCU_WAIT_TAIL segments. > + */ > WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s)); > } > if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) { > -- > 2.43.0.472.g3155946c3a-goog >
On Sat, Dec 16, 2023 at 4:17 PM Frederic Weisbecker <frederic@kernel.org> wrote: > > Le Mon, Dec 11, 2023 at 01:57:16AM +0000, Joel Fernandes (Google) a écrit : > > The comments added in commit 1ef990c4b36b ("srcu: No need to > > advance/accelerate if no callback enqueued") are a bit confusing to me. > > I know some maintainers who may argue that in the changelog world, the first > person doesn't exist :-) Heh, that's fair. Ok I can drop the 'to me'. ;-) > > > The comments are describing a scenario for code that was moved and is > > no longer the way it was (snapshot after advancing). Improve the code > > comments to reflect this and also document by acceleration can never > > s/by/why Ok. > > fail. > > > > Cc: Frederic Weisbecker <frederic@kernel.org> > > Cc: Neeraj Upadhyay <neeraj.iitr10@gmail.com> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org> > > --- > > v1->v2: Fix typo in change log. > > > > kernel/rcu/srcutree.c | 24 ++++++++++++++++++++---- > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index 0351a4e83529..051e149490d1 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1234,11 +1234,20 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, > > if (rhp) > > rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp); > > /* > > - * The snapshot for acceleration must be taken _before_ the read of the > > - * current gp sequence used for advancing, otherwise advancing may fail > > - * and acceleration may then fail too. > > + * It's crucial to capture the snapshot 's' for acceleration before > > + * reading the current gp_seq that is used for advancing. This is > > + * essential because if the acceleration snapshot is taken after a > > + * failed advancement attempt, there's a risk that a grace period may > > + * conclude and a new one may start in the interim. If the snapshot is > > + * captured after this sequence of events, the acceleration snapshot 's' > > + * could be excessively advanced, leading to acceleration failure. > > + * In such a scenario, an 'acceleration leak' can occur, where new > > + * callbacks become indefinitely stuck in the RCU_NEXT_TAIL segment. > > + * Also note that encountering advancing failures is a normal > > + * occurrence when the grace period for RCU_WAIT_TAIL is in progress. > > * > > - * This could happen if: > > + * To see this, consider the following events which occur if > > + * rcu_seq_snap() were to be called after advance: > > * > > * 1) The RCU_WAIT_TAIL segment has callbacks (gp_num = X + 4) and the > > * RCU_NEXT_READY_TAIL also has callbacks (gp_num = X + 8). > > @@ -1264,6 +1273,13 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, > > if (rhp) { > > rcu_segcblist_advance(&sdp->srcu_cblist, > > rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq)); > > + /* > > + * Acceleration can never fail because the state of gp_seq used > > + * for advancing is <= the state of gp_seq used for > > + * acceleration. > > What do you mean by "state" here? State means "value at a certain point in time" here. > If it's the gp_seq number, that doesn't look right. Uff, I screwed up the comment. I swapped "acceleration" and "advancing". I should say: "Acceleration can never fail because the state of gp_seq value used for acceleration is <= the state of gp_seq used for advancing." Does that sound correct now? > The situation raising the initial bug also involved a gp_seq used for advancing <= the gp_seq used for acceleration. Right, which I understand is the bug. thanks, - Joel
Le Sun, Dec 17, 2023 at 09:00:15PM -0500, Joel Fernandes a écrit : > "Acceleration can never fail because the state of gp_seq value used > for acceleration is <= the state of gp_seq used for advancing." > > Does that sound correct now? That can be confusing since acceleration relies on rcu_seq_snap() while advance relies on rcu_seq_current(). And rcu_seq_snap() returns a snapshot that may be above the subsequent rcu_seq_current() return value. So it should rather be something like: "The base current gp_seq value used to produce the snapshot has to be <= the gp_seq used for advancing." Thanks.
On Mon, Dec 18, 2023 at 7:13 AM Frederic Weisbecker <frederic@kernel.org> wrote: > > Le Sun, Dec 17, 2023 at 09:00:15PM -0500, Joel Fernandes a écrit : > > "Acceleration can never fail because the state of gp_seq value used > > for acceleration is <= the state of gp_seq used for advancing." > > > > Does that sound correct now? > > That can be confusing since acceleration relies on rcu_seq_snap() while > advance relies on rcu_seq_current(). And rcu_seq_snap() returns a snapshot > that may be above the subsequent rcu_seq_current() return value. > > So it should rather be something like: > > "The base current gp_seq value used to produce the snapshot has to > be <= the gp_seq used for advancing." Yeah "base current gp_seq" though probably equally confusing sounds a bit better, so I'll just use that instead of "state of gp_seq". With that can I add your Review tag? - Joel
Le Mon, Dec 18, 2023 at 10:27:56AM -0500, Joel Fernandes a écrit : > On Mon, Dec 18, 2023 at 7:13 AM Frederic Weisbecker <frederic@kernel.org> wrote: > > > > Le Sun, Dec 17, 2023 at 09:00:15PM -0500, Joel Fernandes a écrit : > > > "Acceleration can never fail because the state of gp_seq value used > > > for acceleration is <= the state of gp_seq used for advancing." > > > > > > Does that sound correct now? > > > > That can be confusing since acceleration relies on rcu_seq_snap() while > > advance relies on rcu_seq_current(). And rcu_seq_snap() returns a snapshot > > that may be above the subsequent rcu_seq_current() return value. > > > > So it should rather be something like: > > > > "The base current gp_seq value used to produce the snapshot has to > > be <= the gp_seq used for advancing." > > Yeah "base current gp_seq" though probably equally confusing sounds a > bit better, so I'll just use that instead of "state of gp_seq". > > With that can I add your Review tag? Sure, sounds good! Thanks. > > - Joel >
diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index 0351a4e83529..051e149490d1 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -1234,11 +1234,20 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, if (rhp) rcu_segcblist_enqueue(&sdp->srcu_cblist, rhp); /* - * The snapshot for acceleration must be taken _before_ the read of the - * current gp sequence used for advancing, otherwise advancing may fail - * and acceleration may then fail too. + * It's crucial to capture the snapshot 's' for acceleration before + * reading the current gp_seq that is used for advancing. This is + * essential because if the acceleration snapshot is taken after a + * failed advancement attempt, there's a risk that a grace period may + * conclude and a new one may start in the interim. If the snapshot is + * captured after this sequence of events, the acceleration snapshot 's' + * could be excessively advanced, leading to acceleration failure. + * In such a scenario, an 'acceleration leak' can occur, where new + * callbacks become indefinitely stuck in the RCU_NEXT_TAIL segment. + * Also note that encountering advancing failures is a normal + * occurrence when the grace period for RCU_WAIT_TAIL is in progress. * - * This could happen if: + * To see this, consider the following events which occur if + * rcu_seq_snap() were to be called after advance: * * 1) The RCU_WAIT_TAIL segment has callbacks (gp_num = X + 4) and the * RCU_NEXT_READY_TAIL also has callbacks (gp_num = X + 8). @@ -1264,6 +1273,13 @@ static unsigned long srcu_gp_start_if_needed(struct srcu_struct *ssp, if (rhp) { rcu_segcblist_advance(&sdp->srcu_cblist, rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq)); + /* + * Acceleration can never fail because the state of gp_seq used + * for advancing is <= the state of gp_seq used for + * acceleration. This means that RCU_NEXT_TAIL segment will + * always be able to be emptied by the acceleration into the + * RCU_NEXT_READY_TAIL or RCU_WAIT_TAIL segments. + */ WARN_ON_ONCE(!rcu_segcblist_accelerate(&sdp->srcu_cblist, s)); } if (ULONG_CMP_LT(sdp->srcu_gp_seq_needed, s)) {