[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZgG1gO9gRJFluTFy@boqun-archlinux>
Date: Mon, 25 Mar 2024 10:33:52 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Johannes Berg <johannes@...solutions.net>
Cc: rcu@...r.kernel.org, linux-kernel@...r.kernel.org,
"Paul E. McKenney" <paulmck@...nel.org>,
Frederic Weisbecker <frederic@...nel.org>,
Josh Triplett <josh@...htriplett.org>,
Peter Zijlstra <peterz@...radead.org>,
Dan Carpenter <dan.carpenter@...aro.org>
Subject: Re: [PATCH] rcu: mollify sparse with RCU guard
On Mon, Mar 25, 2024 at 05:41:22PM +0100, Johannes Berg wrote:
> 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.
>
Fair enough.
> I can add a comment in there, I guess, something like
>
> /* sparse doesn't actually "call" cleanup functions */
>
Yeah, that's helpful.
> 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.
>
Indeed.
Regards,
Boqun
> johannes
Powered by blists - more mailing lists