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
