From: Johannes Berg <[email protected]>
When using "guard(rcu)();" sparse will complain, because even
though it now understands the cleanup attribute, it doesn't
evaluate the calls from it at function exit, and thus doesn't
count the context correctly.
Given that there's a conditional in the resulting code:
static inline void class_rcu_destructor(class_rcu_t *_T)
{
if (_T->lock) {
rcu_read_unlock();
}
}
it seems that even trying to teach sparse to evalulate the
cleanup attribute function it'd still be difficult to really
make it understand the full context here.
Suppress the sparse warning by just releasing the context in
the acquisition part of the function, after all we know it's
safe with the guard, that's the whole point of it.
Signed-off-by: Johannes Berg <[email protected]>
---
include/linux/rcupdate.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 17d7ed5f3ae6..41081ee9c9a7 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -1090,6 +1090,6 @@ rcu_head_after_call_rcu(struct rcu_head *rhp, rcu_callback_t f)
extern int rcu_expedited;
extern int rcu_normal;
-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())
#endif /* __LINUX_RCUPDATE_H */
--
2.44.0
On Mon, Mar 25, 2024 at 05:41:22PM +0100, Johannes Berg wrote:
> 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 ...
>
These are Sparse warnings, not Smatch warning... Smatch doesn't use any
of the Sparse locking annotations. Smatch handles cleanup basically
correctly at this point.
regards,
dan carpenter
On Mon, 2024-03-25 at 21:28 +0300, Dan Carpenter wrote:
> On Mon, Mar 25, 2024 at 05:41:22PM +0100, Johannes Berg wrote:
> > 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 ...
> >
>
> These are Sparse warnings, not Smatch warning... Smatch doesn't use any
> of the Sparse locking annotations.
Sure, of course. I just saw that you added cleanup stuff to sparse to
allow using it in smatch.
> Smatch handles cleanup basically correctly at this point.
Do you "run" / "emit" the cleanup function calls there? I briefly look
at doing that in sparse but it felt ... complicated, and then I saw the
condition in the cleanup function which I thought sparse could probably
not see through anyway.
johannes
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
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
On Mon, Mar 25, 2024 at 11:16:27AM +0100, Johannes Berg wrote:
> From: Johannes Berg <[email protected]>
>
> When using "guard(rcu)();" sparse will complain, because even
> though it now understands the cleanup attribute, it doesn't
> evaluate the calls from it at function exit, and thus doesn't
> count the context correctly.
>
> Given that there's a conditional in the resulting code:
>
> static inline void class_rcu_destructor(class_rcu_t *_T)
> {
> if (_T->lock) {
> rcu_read_unlock();
> }
> }
>
> it seems that even trying to teach sparse to evalulate the
> cleanup attribute function it'd still be difficult to really
> make it understand the full context here.
>
> Suppress the sparse warning by just releasing the context in
> the acquisition part of the function, after all we know it's
> safe with the guard, that's the whole point of it.
>
> Signed-off-by: Johannes Berg <[email protected]>
> ---
> include/linux/rcupdate.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index 17d7ed5f3ae6..41081ee9c9a7 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -1090,6 +1090,6 @@ rcu_head_after_call_rcu(struct rcu_head *rhp, rcu_callback_t f)
> extern int rcu_expedited;
> extern int rcu_normal;
>
> -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)?
But before that how does it looks if we don't fix this entirely? ;-)
Regards,
Boqun
> #endif /* __LINUX_RCUPDATE_H */
> --
> 2.44.0
>
On Mon, Mar 25, 2024 at 07:43:18PM +0100, Johannes Berg wrote:
> On Mon, 2024-03-25 at 21:28 +0300, Dan Carpenter wrote:
> > On Mon, Mar 25, 2024 at 05:41:22PM +0100, Johannes Berg wrote:
> > > 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 ...
> > >
> >
> > These are Sparse warnings, not Smatch warning... Smatch doesn't use any
> > of the Sparse locking annotations.
>
> Sure, of course. I just saw that you added cleanup stuff to sparse to
> allow using it in smatch.
>
> > Smatch handles cleanup basically correctly at this point.
>
> Do you "run" / "emit" the cleanup function calls there?
Yes.
> I briefly look
> at doing that in sparse but it felt ... complicated, and then I saw the
> condition in the cleanup function which I thought sparse could probably
> not see through anyway.
The if (_T->lock) statements are a problem. For those, I have to
manually add them to check_locking.c as an unlock function and to
check_preempt.c as a decrement the preempt count function. The other
place that I have to add them is to smatch_data/db/kernel.return_fixes
because the scoped_guard() macros checks them as well. I had to do
quite a bit of patching things up when the sound subsystem started using
cleanup.h so here is an example of what that looks like:
https://github.com/error27/smatch/commit/a2f68c96f70a0cdc581beff81eb6d412ac8dfc4f
regards,
dan carpenter
On Tue, 2024-03-26 at 10:39 +0300, Dan Carpenter wrote:
> On Mon, Mar 25, 2024 at 07:43:18PM +0100, Johannes Berg wrote:
> > On Mon, 2024-03-25 at 21:28 +0300, Dan Carpenter wrote:
> > > On Mon, Mar 25, 2024 at 05:41:22PM +0100, Johannes Berg wrote:
> > > > 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 ...
> > > >
> > >
> > > These are Sparse warnings, not Smatch warning... Smatch doesn't use any
> > > of the Sparse locking annotations.
> >
> > Sure, of course. I just saw that you added cleanup stuff to sparse to
> > allow using it in smatch.
> >
> > > Smatch handles cleanup basically correctly at this point.
> >
> > Do you "run" / "emit" the cleanup function calls there?
>
> Yes.
I see. I guess that doesn't work for sparse. You write:
This shouldn't really have been needed if I had written the parse.c
code correctly to create new scopes for every __cleanup__.
Would that maybe be a way to handle it in sparse? Though not sure how to
return then.
> > I briefly look
> > at doing that in sparse but it felt ... complicated, and then I saw the
> > condition in the cleanup function which I thought sparse could probably
> > not see through anyway.
>
> The if (_T->lock) statements are a problem. For those, I have to
> manually add them to check_locking.c as an unlock function and to
> check_preempt.c as a decrement the preempt count function.
OK, no fun.
I think overall it's still easier to go with this patch :)
And maybe we should think about replacing what we need sparse for...
johannes
On Tue, Mar 26, 2024 at 08:53:39AM +0100, Johannes Berg wrote:
> On Tue, 2024-03-26 at 10:39 +0300, Dan Carpenter wrote:
> > On Mon, Mar 25, 2024 at 07:43:18PM +0100, Johannes Berg wrote:
> > > On Mon, 2024-03-25 at 21:28 +0300, Dan Carpenter wrote:
> > > > On Mon, Mar 25, 2024 at 05:41:22PM +0100, Johannes Berg wrote:
> > > > > 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 ...
> > > > >
> > > >
> > > > These are Sparse warnings, not Smatch warning... Smatch doesn't use any
> > > > of the Sparse locking annotations.
> > >
> > > Sure, of course. I just saw that you added cleanup stuff to sparse to
> > > allow using it in smatch.
> > >
> > > > Smatch handles cleanup basically correctly at this point.
> > >
> > > Do you "run" / "emit" the cleanup function calls there?
> >
> > Yes.
>
> I see. I guess that doesn't work for sparse. You write:
>
> This shouldn't really have been needed if I had written the parse.c
> code correctly to create new scopes for every __cleanup__.
>
> Would that maybe be a way to handle it in sparse? Though not sure how to
> return then.
I think I was just wrong when I wrote that. But I'm not really sure how
this is normally handled by the compiler.
regards,
dan carpenter