From: Johannes Berg <johannes@sipsolutions.net>
To: Boqun Feng <boqun.feng@gmail.com>
Cc: rcu@vger.kernel.org, linux-kernel@vger.kernel.org,
"Paul E. McKenney" <paulmck@kernel.org>,
Frederic Weisbecker <frederic@kernel.org>,
Josh Triplett <josh@joshtriplett.org>,
Peter Zijlstra <peterz@infradead.org>,
Dan Carpenter <dan.carpenter@linaro.org>
Subject: Re: [PATCH] rcu: mollify sparse with RCU guard
Date: Mon, 25 Mar 2024 17:41:22 +0100 [thread overview]
Message-ID: <055111792727869a98c1fa693014e0b6f5d256ea.camel@sipsolutions.net> (raw)
In-Reply-To: <ZgGnuFJiTX5laS7c@boqun-archlinux>
On Mon, 2024-03-25 at 09:35 -0700, Boqun Feng wrote:
> > -DEFINE_LOCK_GUARD_0(rcu, rcu_read_lock(), rcu_read_unlock())
> > +DEFINE_LOCK_GUARD_0(rcu, do { rcu_read_lock(); __release(RCU); } while(0), rcu_read_unlock())
> >
>
> Hmm.. not a big fan of this. __release(RCU) following a rcu_read_lock()
> is really confusing. Maybe we can introduce a _rcu_read_lock():
>
> void _rcu_read_lock(bool guard) {
> __rcu_read_lock();
> // Skip sparse annotation in "guard(rcu)()" to work
> // around sparse's lack of support of cleanup.
> if (!guard)
> __acquire(RCU);
> rcu_lock_acquire(...);
> ...
> }
>
> and normal rcu_read_lock() is just a _rcu_read_lock(false), RCU guard is
> a _rcu_read_lock(true)?
Not sure I see any value in that, that's pretty much equivalent but
seems IMHO less specific, where here we know we really want this only in
this case. I don't see any other case where we'd want to ever "call"
_rcu_read_lock(true).
Also __acquire()/__release() are just empty macros without __CHECKER__.
So not sure the indirection really is warranted for this special case.
I can add a comment in there, I guess, something like
/* sparse doesn't actually "call" cleanup functions */
perhaps. That reminds me I forgot to CC Dan ...
> But before that how does it looks if we don't fix this entirely? ;-)
Well basically every time you write
void myfunc(void)
{
guard(rcu)();
...
}
sparse will complain about mismatched locks, which is _really_ annoying
for e.g. networking where there's (a) a kind of "no new warnings" rule,
and (b) sparse is actually important for all the endian annotations etc.
Which right now means that we can't use all this new machinery, which is
a shame.
johannes
next prev parent reply other threads:[~2024-03-25 16:41 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-25 10:16 [PATCH] rcu: mollify sparse with RCU guard Johannes Berg
2024-03-25 16:35 ` Boqun Feng
2024-03-25 16:41 ` Johannes Berg [this message]
2024-03-25 17:33 ` Boqun Feng
2024-03-25 18:28 ` Dan Carpenter
2024-03-25 18:43 ` Johannes Berg
2024-03-26 7:39 ` Dan Carpenter
2024-03-26 7:53 ` Johannes Berg
2024-03-26 8:20 ` Dan Carpenter
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=055111792727869a98c1fa693014e0b6f5d256ea.camel@sipsolutions.net \
--to=johannes@sipsolutions.net \
--cc=boqun.feng@gmail.com \
--cc=dan.carpenter@linaro.org \
--cc=frederic@kernel.org \
--cc=josh@joshtriplett.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rcu@vger.kernel.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).