Received-SPF: pass (google.com: domain of linux-kernel+bounces-2917-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231211015717.1067822-1-joel@joelfernandes.org> <ZX4T4E02hbsgnGBY@localhost.localdomain>
In-Reply-To: <ZX4T4E02hbsgnGBY@localhost.localdomain>
From: Joel Fernandes <joel@joelfernandes.org>
Date: Sun, 17 Dec 2023 21:00:15 -0500
Message-ID: <CAEXW_YQK_ct=kMKSO6XnW1NpSjSAVMiMevL-om_i-au9mpUEyw@mail.gmail.com>
Subject: Re: [PATCH v2] srcu: Improve comments about acceleration leak
To: Frederic Weisbecker <frederic@kernel.org>
Cc: 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>, 
	Neeraj Upadhyay <neeraj.iitr10@gmail.com>, rcu@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Dec 16, 2023 at 4:17=E2=80=AFPM Frederic Weisbecker <frederic@kerne=
l.org> wrote:
>
> Le Mon, Dec 11, 2023 at 01:57:16AM +0000, Joel Fernandes (Google) a =C3=
=A9crit :
> > 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 fi=
rst
> 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(st=
ruct 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 ma=
y fail
> > -      * and acceleration may then fail too.
> > +      * It's crucial to capture the snapshot 's' for acceleration befo=
re
> > +      * 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 snapsh=
ot is
> > +      * captured after this sequence of events, the acceleration snaps=
hot 's'
> > +      * could be excessively advanced, leading to acceleration failure=
.
> > +      * In such a scenario, an 'acceleration leak' can occur, where ne=
w
> > +      * callbacks become indefinitely stuck in the RCU_NEXT_TAIL segme=
nt.
> > +      * Also note that encountering advancing failures is a normal
> > +      * occurrence when the grace period for RCU_WAIT_TAIL is in progr=
ess.
> >        *
> > -      * 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 =3D X + 4)=
 and the
> >        *     RCU_NEXT_READY_TAIL also has callbacks (gp_num =3D X + 8).
> > @@ -1264,6 +1273,13 @@ static unsigned long srcu_gp_start_if_needed(str=
uct srcu_struct *ssp,
> >       if (rhp) {
> >               rcu_segcblist_advance(&sdp->srcu_cblist,
> >                                     rcu_seq_current(&ssp->srcu_sup->src=
u_gp_seq));
> > +             /*
> > +              * Acceleration can never fail because the state of gp_se=
q used
> > +              * for advancing is <=3D 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 <=3D 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 adv=
ancing <=3D the gp_seq used for acceleration.

Right, which I understand is the bug.

thanks,

 - Joel