Re: [PATCH] rcu: mollify sparse with RCU guard

[Boqun Feng <boqun.feng@gmail.com>]

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