On Fri, Feb 21, 2025 at 06:10:02PM +0100, Marco Elver wrote:
> On Thu, Feb 20, 2025 at 05:26PM -0800, Paul E. McKenney wrote:
> [...]
> > > That's what I've tried with this patch (rcu_read_lock_bh() also
> > > acquires "RCU", on top of "RCU_BH"). I need to add a re-entrancy test,
> > > and make sure it doesn't complain about that. At a later stage we
> > > might also want to add more general "BH" and "IRQ" capabilities to
> > > denote they're disabled when held, but that'd overcomplicate the first
> > > version of this series.
> >
> > Fair enough! Then would it work to just do "RCU" now, and ad the "BH"
> > and "IRQ" when those capabilities are added?
>
> I tried if this kind of re-entrant locking works - a test like this:
>
> | --- a/lib/test_capability-analysis.c
> | +++ b/lib/test_capability-analysis.c
> | @@ -370,6 +370,15 @@ static void __used test_rcu_guarded_reader(struct test_rcu_data *d)
> | rcu_read_unlock_sched();
> | }
> |
> | +static void __used test_rcu_reentrancy(struct test_rcu_data *d)
> | +{
> | + rcu_read_lock();
> | + rcu_read_lock_bh();
> | + (void)rcu_dereference(d->data);
> | + rcu_read_unlock_bh();
> | + rcu_read_unlock();
> | +}
>
>
> | $ make lib/test_capability-analysis.o
> | DESCEND objtool
> | CC arch/x86/kernel/asm-offsets.s
> | INSTALL libsubcmd_headers
> | CALL scripts/checksyscalls.sh
> | CC lib/test_capability-analysis.o
> | lib/test_capability-analysis.c:376:2: error: acquiring __capability_RCU 'RCU' that is already held [-Werror,-Wthread-safety-analysis]
> | 376 | rcu_read_lock_bh();
> | | ^
> | lib/test_capability-analysis.c:375:2: note: __capability_RCU acquired here
> | 375 | rcu_read_lock();
> | | ^
> | lib/test_capability-analysis.c:379:2: error: releasing __capability_RCU 'RCU' that was not held [-Werror,-Wthread-safety-analysis]
> | 379 | rcu_read_unlock();
> | | ^
> | lib/test_capability-analysis.c:378:2: note: __capability_RCU released here
> | 378 | rcu_read_unlock_bh();
> | | ^
> | 2 errors generated.
> | make[3]: *** [scripts/Makefile.build:207: lib/test_capability-analysis.o] Error 1
> | make[2]: *** [scripts/Makefile.build:465: lib] Error 2
I was hoping! Ah well... ;-)
> ... unfortunately even for shared locks, the compiler does not like
> re-entrancy yet. It's not yet supported, and to fix that I'd have to go
> and implement that in Clang first before coming back to this.
This would be needed for some types of reader-writer locks, and also for
reference counting, so here is hoping that such support is forthcoming
sooner rather than later.
> I see 2 options for now:
>
> a. Accepting the limitation that doing a rcu_read_lock() (and
> variants) while the RCU read lock is already held in the same function
> will result in a false positive warning (like above). Cases like that
> will need to disable the analysis for that piece of code.
>
> b. Make the compiler not warn about unbalanced rcu_read_lock/unlock(),
> but instead just help enforce a rcu_read_lock() was issued somewhere
> in the function before an RCU-guarded access.
>
> Option (b) is obviously weaker than (a), but avoids the false positives
> while accepting more false negatives.
>
> For all the code that I have already tested this on I observed no false
> positives, so I'd go with (a), but I'm also fine with the weaker
> checking for now until the compiler gains re-entrancy support.
>
> Preferences?
Whichever one provides the best checking without false positives.
Which sounds to me like (a) unless and until false positives crop up,
in which case (b). Which looks to be where you were going anyway. ;-)
Thanx, Paul
