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
Subject: Re: [PATCH] rculist.h: docu: fix wrong function name
Date: Wed, 20 Sep 2023 14:58:55 +0200 [thread overview]
Message-ID: <79e83b07045c35527f0b91fa5ba36d9c501cd8af.camel@redhat.com> (raw)
In-Reply-To: <db6bc865-199d-4784-a509-5b0c80c0501c@paulmck-laptop>
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()".
> >
> > This function (obviously) does not exist. What the comment's author
> > actually meant was the standard list-function list_first_entry().
> >
> > Change the function name in that comment from
> > list_first_entry_rcu() to
> > list_first_entry().
> >
> > Additionally, add the parenthesis to list_first_or_null_rcu to be
> > congruent
> > with that entire comment's style.
> >
> > 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-)
> >
> > Regards,
> > P.
>
> Thank you for sending this! Please see below.
>
> Thanx, Paul
>
> > ---
> > include/linux/rculist.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > 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,
> > * rcu_dereference() is not needed), which means that list_empty()
> > can be
> > * used anywhere you would want to use list_empty_rcu(). Just
> > don't
> > * expect anything useful to happen if you do a subsequent
> > lockless
> > - * call to list_first_entry_rcu()!!!
> > + * call to list_first_entry()!!!
>
> 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 = READ_ONCE(mylist->next);
first_member = 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).
>
> > *
> > - * See list_first_or_null_rcu for an alternative.
> > + * See list_first_or_null_rcu() for an alternative.
>
> Good catch!
>
> Please do feel free to send an update.
Sure, will do once we agreed on the best wording :)
P.
>
> Thanx, Paul
>
> > */
> >
> > /**
> > --
> > 2.41.0
> >
>
prev parent reply other threads:[~2023-09-20 13:00 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-19 19:47 [PATCH] rculist.h: docu: fix wrong function name Philipp Stanner
2023-09-20 10:53 ` Paul E. McKenney
2023-09-20 12:58 ` Philipp Stanner [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=79e83b07045c35527f0b91fa5ba36d9c501cd8af.camel@redhat.com \
--to=pstanner@redhat.com \
--cc=boqun.feng@gmail.com \
--cc=frederic@kernel.org \
--cc=jiangshanlai@gmail.com \
--cc=joel@joelfernandes.org \
--cc=josh@joshtriplett.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=paulmck@kernel.org \
--cc=qiang.zhang1211@gmail.com \
--cc=quic_neeraju@quicinc.com \
--cc=rcu@vger.kernel.org \
--cc=rostedt@goodmis.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).