Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Message-ID: <79e83b07045c35527f0b91fa5ba36d9c501cd8af.camel@redhat.com>
Subject: Re: [PATCH] rculist.h: docu: fix wrong function name
From:   Philipp Stanner <pstanner@redhat.com>
To:     paulmck@kernel.org
Cc:     Frederic Weisbecker <frederic@kernel.org>,
        Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Josh Triplett <josh@joshtriplett.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Zqiang <qiang.zhang1211@gmail.com>, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org
Date:   Wed, 20 Sep 2023 14:58:55 +0200
In-Reply-To: <db6bc865-199d-4784-a509-5b0c80c0501c@paulmck-laptop>
References: <20230919194754.20574-2-pstanner@redhat.com>
         <db6bc865-199d-4784-a509-5b0c80c0501c@paulmck-laptop>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-09-20 at 03:53 -0700, Paul E. McKenney wrote:
> On Tue, Sep 19, 2023 at 09:47:55PM +0200, Philipp Stanner wrote:
> > The header contains a comment that details why the functions
> > list_empty_rcu() and list_first_entry_rcu() don't exist. It
> > explains
> > that they don't exist because standard list_empty() can be used
> > just as
> > well, but one can not expect sane results from a subsequent, quote,
> > "list_first_entry_rcu()".
> >=20
> > This function (obviously) does not exist. What the comment's author
> > actually meant was the standard list-function list_first_entry().
> >=20
> > Change the function name in that comment from
> > list_first_entry_rcu() to
> > list_first_entry().
> >=20
> > Additionally, add the parenthesis to list_first_or_null_rcu to be
> > congruent
> > with that entire comment's style.
> >=20
> > Signed-off-by: Philipp Stanner <pstanner@redhat.com>
> > ---
> > Hi!
> > I hope this helps.
> > I wasn't 100.000000% sure if that's correct, but I thought asking
> > is for
> > free 8-)
> >=20
> > Regards,
> > P.
>=20
> Thank you for sending this!=C2=A0 Please see below.
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Thanx, Paul
>=20
> > ---
> > =C2=A0include/linux/rculist.h | 4 ++--
> > =C2=A01 file changed, 2 insertions(+), 2 deletions(-)
> >=20
> > diff --git a/include/linux/rculist.h b/include/linux/rculist.h
> > index d29740be4833..4837d8892691 100644
> > --- a/include/linux/rculist.h
> > +++ b/include/linux/rculist.h
> > @@ -331,9 +331,9 @@ static inline void
> > list_splice_tail_init_rcu(struct list_head *list,
> > =C2=A0 * rcu_dereference() is not needed), which means that list_empty(=
)
> > can be
> > =C2=A0 * used anywhere you would want to use list_empty_rcu().=C2=A0 Ju=
st
> > don't
> > =C2=A0 * expect anything useful to happen if you do a subsequent
> > lockless
> > - * call to list_first_entry_rcu()!!!
> > + * call to list_first_entry()!!!
>=20
> You are quite correct that the original is incorrect, given that it
> does
> not exist, but a better change would be to list_entry_rcu().

Hm, are you sure that's what we want there?
list_entry_rcu() does not take care of actually getting the list_head
to begin with. The caller would have to provide it. The question is how
would he do that? The goal in the purposefully broken example's code
is: Get this list's first entry, if it exists.

Do we agree that the example as is is maybe a bit out of place in the
first place, because it could always cause in a fault when the list got
empty since the check, thus, resulting in us trying to dereference the
list-head?
If that's the case, maybe we should also change the wording "don't
expect anything useful to happen" to something like "don't expect
anything useful to happen (i.e., your code could fault)".
"Not useful" doesn't sound like "can crash", that's my point.


I suppose a working broken example might be:

if (!list_empty(mylist)) {
	first_head =3D READ_ONCE(mylist->next);
	first_member =3D list_entry_rcu(first_head, ...) // <- could fault!
}

?

>  The reason
> being that list_first_entry() does not have READ_ONCE(), allowing the
> compiler to play all sorts of games (see
> https://lwn.net/Articles/793253

Yup, I know that article.
I'm frequently astonished how difficult the situation has become. I was
at times wondering if things were better were C (or a hypothetical
successor language) designed to take parallelism into account in the
language's core.


> for some examples).
>=20
> > =C2=A0 *
> > - * See list_first_or_null_rcu for an alternative.
> > + * See list_first_or_null_rcu() for an alternative.
>=20
> Good catch!
>=20
> Please do feel free to send an update.

Sure, will do once we agreed on the best wording :)

P.

>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=
=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=
=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0Thanx, Paul
>=20
> > =C2=A0 */
> > =C2=A0
> > =C2=A0/**
> > --=20
> > 2.41.0
> >=20
>=20