Re: [PATCH 4/6] locking/local_lock: Introduce local_lock_irqsave_check()

[Vlastimil Babka <vbabka@suse.cz>]

On 5/12/25 19:16, Alexei Starovoitov wrote:
> On Mon, May 12, 2025 at 7:04 AM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
>>
>> On 2025-04-30 20:27:16 [-0700], Alexei Starovoitov wrote:
>> > --- a/include/linux/local_lock_internal.h
>> > +++ b/include/linux/local_lock_internal.h
>> > @@ -168,6 +168,15 @@ do {                                                             \
>> >  /* preemption or migration must be disabled before calling __local_lock_is_locked */
>> >  #define __local_lock_is_locked(lock) READ_ONCE(this_cpu_ptr(lock)->acquired)
>> >
>> > +#define __local_lock_irqsave_check(lock, flags)                                      \
>> > +     do {                                                                    \
>> > +             if (IS_ENABLED(CONFIG_DEBUG_LOCK_ALLOC) &&                      \
>> > +                 (!__local_lock_is_locked(lock) || in_nmi()))                \
>> > +                     WARN_ON_ONCE(!__local_trylock_irqsave(lock, flags));    \
>> > +             else                                                            \
>> > +                     __local_lock_irqsave(lock, flags);                      \
>> > +     } while (0)
>> > +
>>
>> Hmm. If I see this right in SLUB then this is called from preemptible
>> context. Therefore the this_cpu_ptr() from __local_lock_is_locked()
>> should trigger a warning here.
> 
> When preemptible the migration is disabled. So no warning.
> 
>> This check variant provides only additional debugging and otherwise
>> behaves as local_lock_irqsave(). Therefore the in_nmi() should return
>> immediately with a WARN_ON() regardless if the lock is available or not
>> because the non-try variant should never be invoked from an NMI.
> 
> non-try variant can be invoked from NMI, because the earlier
> __local_lock_is_locked() check tells us that the lock is not locked.
> And it's safe to do.
> And that's the main challenge here.
> local_lock_irqsave_check() macro fights lockdep here.
> 
>> This looks like additional debug infrastructure that should be part of
>> local_lock_irqsave() itself,
> 
> The pattern of
> 
> if (!__local_lock_is_locked(lock)) {
>    .. lots of code..
>    local_lock_irqsave(lock);
> 
> is foreign to lockdep.
> 
> Since it can be called from NMI the lockdep just hates it:
> 
> [ 1021.956825] inconsistent {INITIAL USE} -> {IN-NMI} usage.
> ...
> [ 1021.956888]   lock(per_cpu_ptr(&lock));
> [ 1021.956890]   <Interrupt>
> [ 1021.956891]     lock(per_cpu_ptr(&lock));
> ..
> 
> and technically lockdep is correct.
> For any normal lock it's a deadlock waiting to happen,
> but not here.
> 
> Even without NMI the lockdep doesn't like it:
> [   14.627331] page_alloc_kthr/1965 is trying to acquire lock:
> [   14.627331] ffff8881f6ebe0f0 ((local_lock_t
> *)&c->lock){-.-.}-{3:3}, at: ___slab_alloc+0x9a9/0x1ab0
> [   14.627331]
> [   14.627331] but task is already holding lock:
> [   14.627331] ffff8881f6ebd490 ((local_lock_t
> *)&c->lock){-.-.}-{3:3}, at: ___slab_alloc+0xc7/0x1ab0
> [   14.627331]
> [   14.627331] other info that might help us debug this:
> [   14.627331]  Possible unsafe locking scenario:
> [   14.627331]
> [   14.627331]        CPU0
> [   14.627331]        ----
> [   14.627331]   lock((local_lock_t *)&c->lock);
> [   14.627331]   lock((local_lock_t *)&c->lock);
> 
> When slub is holding lock ...bd490 we detect it with
> __local_lock_is_locked(),
> then we check that lock ..be0f0 is not locked,
> and proceed to acquire it, but
> lockdep will show the above splat.
> 
> So local_lock_irqsave_check() is a workaround to avoid
> these two false positives from lockdep.
> 
> Yours and Vlastimil's observation is correct, that ideally
> local_lock_irqsave() should just handle it,
> but I don't see how to do it.
> How can lockdep understand the if (!locked()) lock() pattern ?
> Such usage is correct only for per-cpu local lock when migration
> is disabled from check to acquire.

Thanks, I think I finally understand the issue and why a _check variant is
necessary. As a general note as this is so tricky, having more details in
comments and commit messages can't hurt so we can understand it sooner :)

Again this would be all simpler if we could just use trylock instead of
_check(), but then we need to handle the fallbacks. And AFAIU on RT trylock
can fail "spuriously", i.e. when we don't really preempt ourselves, as we
discussed in that memcg thread.

> Hence the macro is doing:
> if (IS_ENABLED(CONFIG_DEBUG_LOCK_ALLOC) &&
>    (!__local_lock_is_locked(lock) || in_nmi()))
>          WARN_ON_ONCE(!__local_trylock_irqsave(lock, flags));
> 
> in_nmi() part is a workaround for the first lockdep splat
> and __local_lock_is_locked() is a workaround for 2nd lockdep splat,
> though the code did __local_lock_is_locked() check already.

So here's where this would be useful to have that info in a comment.
However, I wonder about it, as the code uses __local_trylock_irqsave(), so
lockdep should see it as an opportunistic attempt and not splat as that
trylock alone should be avoiding deadlock - if not we might have a bug in
the lockdep bits of trylock.

> In your other email you wonder whether
> rt_mutex_base_is_locked() should be enough.
> It's not.
> We need to check:
> __local_lock_is_locked(__lock) \
> rt_mutex_owner(&this_cpu_ptr(__lock)->lock) == current
> 
> Because the following sequence is normal in PREEMP_RT:
> kmalloc
>   local_lock_irqsave(lock_A)
>      preemption
>         kmalloc_nolock
>            if (is_locked(lock_A) == true)
>                retry:  is_locked(lock_B) == false
>                          local_lock_irqsave_check(lock_B)
> 
> while lock_B could be locked on another CPU by a different task.
> So we cannot trylock(lock_B) here.
> Hence in PREEMPT_RT
> __local_lock_irqsave_check() is doing:
> WARN_ON_ONCE(__local_lock_is_locked(lock));
> spin_lock(this_cpu_ptr((lock)));