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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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.

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.

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)));