Received-SPF: pass (google.com: domain of linux-kernel+bounces-117609-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Feedback-ID: iad51458e:Fastmail
Date: Mon, 25 Mar 2024 10:33:52 -0700
From: Boqun Feng <boqun.feng@gmail.com>
To: Johannes Berg <johannes@sipsolutions.net>
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
Message-ID: <ZgG1gO9gRJFluTFy@boqun-archlinux>
References: <20240325101626.41584-2-johannes@sipsolutions.net>
 <ZgGnuFJiTX5laS7c@boqun-archlinux>
 <055111792727869a98c1fa693014e0b6f5d256ea.camel@sipsolutions.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <055111792727869a98c1fa693014e0b6f5d256ea.camel@sipsolutions.net>

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