On Fri, Jul 11, 2025 at 8:17 AM Sebastian Andrzej Siewior
<bigeasy@xxxxxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx>
> > >>
> > >> 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.
So you're proposing to replace four local_lock_irqsave() in ___slab_alloc()
with if (!local_trylock_irqsave()) return NULL;
and a nasty comment that it shouldn't happen because we did
local_lock_is_locked() in the caller?
But for RT it will pessimize kmalloc_nolock() chances. More below:
> 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.
Exactly and that's what we need to avoid.
Sleeping in rt_spin_lock() is fine here, since the current task
doesn't hold this per-cpu local_lock.
But there is no such lockdep concept.
Hence the need for local_lock_lockdep_start() which is purely
lockdep-aid and doesn't affect locking logic and checks.
> 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.
correct. In RT it's better to have a full rt_spin_lock.
> That is why I don't like the part where we trick lockdep.
yes. we do trick lockdep. I don't see an alternative.
lockdep doesn't understand this part either:
"inconsistent {INITIAL USE} -> {IN-NMI} usage"
So it has to be tricked regardless.
> If we the parent check we could trylock for !RT and normal lock for RT
> what we actual do.
How would you do a normal rt_spin_lock() ?
lockdep will yell for two reasons in the commit log of this patch.
> If there is no parent check then we could do "normal lock" on both
> sides.
How would ___slab_alloc() know whether there was a parent check or not?
imo keeping local_lock_irqsave() as-is is cleaner,
since if there is no parent check lockdep will rightfully complain.
One can argue that local_lock_is_locked() and local_lock_lockdep_start()
should be paired together and that's what I had in v1, but they're
really different things. local_lock_is_locked() is true run-time
check regardless of lockdep and the other is lockdep specific band-aid.
Keeping them next to each other in __slab_alloc() looks cleaner.
Maybe a bigger comment is necessary.
