On 5/12/25 19:16, Alexei Starovoitov wrote:
> On Mon, May 12, 2025 at 7:04 AM Sebastian Andrzej Siewior
> <bigeasy@xxxxxxxxxxxxx> 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)));
