Re: [PATCH v2 3/6] locking/local_lock: Introduce local_lock_lockdep_start/end()

[Vlastimil Babka <vbabka@suse.cz>]

On 7/14/25 20:46, Alexei Starovoitov wrote:
> On Mon, Jul 14, 2025 at 11:33 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> When in patch 6/6 __slab_alloc() we should have bailed out via
>>
>>         if (unlikely(!gfpflags_allow_spinning(gfpflags))) {
>> +               if (local_lock_is_locked(&s->cpu_slab->lock)) {
>> +                       /*
>> +                        * EBUSY is an internal signal to kmalloc_nolock() to
>> +                        * retry a different bucket. It's not propagated
>> +                        * to the caller.
>> +                        */
>> +                       p = ERR_PTR(-EBUSY);
>> +                       goto out;
>> +               }
>>
>> So it doesn't seem to me as a lack of lockdep tricking, but we reached
>> something we should not have because the avoidance based on
>> local_lock_is_locked() above didn't work properly? At least if I read the
>> splat and backtrace properly, it doesn't seem to suggest a theoretical
>> scenario but that we really tried to lock something we already had locked.
> 
> It's not theoretical. Such slab re-entrance can happen with
> a tracepoint:
> slab -> some tracepoint -> bpf -> slab
> 
> I simulate it with a stress test:
> +extern void (*debug_callback)(void);
> +#define local_unlock_irqrestore(lock, flags)                   \
> +       do {                    \
> +               if (debug_callback) debug_callback(); \
> +               __local_unlock_irqrestore(lock, flags); \
> +       } while (0)
> 
> and debug_callback() calls kmalloc_nolock(random_size) without any bpf
> to simplify testing.
> 
>> > [   39.819857]  my_debug_callback+0x20e/0x390 [bpf_testmod]
>>
>> What exactly did you instrument here?
>>
>> > [   39.819867]  ? page_alloc_kthread+0x320/0x320 [bpf_testmod]
>> > [   39.819875]  ? lock_is_held_type+0x85/0xe0
>> > [   39.819881]  ___slab_alloc+0x256/0xec0
>>
>> And here we took the lock originally?
> 
> yes, but they are truly different local_locks of different
> kmalloc buckets, and local_lock_is_locked() is working.
> 
> See in the splat:
> 
>> > [   39.819646] page_alloc_kthr/2306 is trying to acquire lock:
>> > [   39.819650] ff110001f5cbea88 ((&c->lock)){+.+.}-{3:3}, at:
>> > ___slab_alloc+0xb7/0xec0
>> > [   39.819667]
>> > [   39.819667] but task is already holding lock:
>> > [   39.819668] ff110001f5cbfe88 ((&c->lock)){+.+.}-{3:3}, at:
>> > ___slab_alloc+0xb7/0xec0
> 
> the addresses of the locks are different and they're different
> kmalloc buckets, but lockdep cannot understand this without
> explicit local_lock_lockdep_start().
> The same thing I'm trying to explain in the commit log.

Thanks for the explanation and sorry for being so dense.
Maybe lockdep's lock classes can be used here somehow instead of having to
teach lockdep completely new tricks, but I don't know enough about those to
know for sure.