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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-07-11 11:55:22 [+0200], Vlastimil Babka wrote:
> On 7/11/25 09:50, Sebastian Andrzej Siewior wrote:
> > On 2025-07-08 18:53:00 [-0700], Alexei Starovoitov wrote:
> >> From: Alexei Starovoitov <ast@kernel.org>
> >> 
> >> Introduce local_lock_lockdep_start/end() pair to teach lockdep
> >> about a region of execution where per-cpu local_lock is not taken
> >> and lockdep should consider such local_lock() as "trylock" to
> >> avoid multiple false-positives:
> >> - lockdep doesn't like when the same lock is taken in normal and
> >>   in NMI context
> >> - lockdep cannot recognize that local_locks that protect kmalloc
> >>   buckets are different local_locks and not taken together
> >> 
> >> This pair of lockdep aid is used by slab in the following way:
> >> 
> >> if (local_lock_is_locked(&s->cpu_slab->lock))
> >> 	goto out;
> >> local_lock_lockdep_start(&s->cpu_slab->lock);
> >> p = ___slab_alloc(s, gfpflags, node, addr, c, orig_size);
> >> local_lock_lockdep_end(&s->cpu_slab->lock);
> >> 
> >> Where ___slab_alloc() is calling
> >> local_lock_irqsave(&s->cpu_slab->lock, ...) many times,
> >> and all of them will not deadlock since this lock is not taken.
> > 
> > So you prefer this instead of using a trylock variant in ___slab_alloc()
> > which would simply return in case the trylock fails?
> 
> The code isn't always in a position to "simply return". On !RT I think we
> can at least assume that if we succeeded once, it means we're not a irq/nmi
> interrupting a locked context so we'll succeed the following attempts too.
> On RT IIUC the lock might be taken by someone else, so a trylock might fail
> (even if it should also mean we're in a context that can do a non-try lock).

There is this parent check. If the parent check "allows" the allocation
then on !RT the trylock should always succeed. So the return "empty
handed" would be there but should not happen kind of thing.

On RT this is different so local_lock_is_locked() will return false but
the trylock might fail if the lock is acquired by another task.

But then with this change we do trylock from lockdep's point of view
while in reality we do the full locking including possible context
switch.

That is why I don't like the part where we trick lockdep.

If we the parent check we could trylock for !RT and normal lock for RT
what we actual do.
If there is no parent check then we could do "normal lock" on both
sides.

Sebastian