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

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

On 2025-07-14 17:35:52 [+0200], Vlastimil Babka wrote:
> If we go with this, then I think the better approach would be simply:
> 
> if (unlikely(!local_trylock_irqsave(&s->cpu_slab->lock, *flags))
> 	local_lock_irqsave(&s->cpu_slab->lock, *flags);
> 
> - no branches before the likely to succeed local_trylock_irqsave()
> - the unlikely local_lock_irqsave() fallback exists to handle the PREEMPT_RT
> case / provide lockdep checks in case of screwing up
> - we don't really need to evaluate allow_spin or add BUG_ON() (which is
> actively disallowed to add these days anyway) - if we screw up, either
> lockdep will splat, or we deadlock

Some people added BUG_ON() in cases were a warning would be more
applicable and recovery would be still be possible. I don't see how to
recover from this (unless you want return NULL) plus it should not
happen.
The only downside would that you don't evaluate the spinning part but
this only matters on RT since !RT should always succeed. So why not.

> Also I'm thinking on !PREEMPT_RT && !LOCKDEP we don't even need the fallback
> local_lock_irqsave part? The trylock is supposed to always succeed, right?
> Either we allow spinning and that means we're not under kmalloc_nolock() and
> should not be interrupting the locked section (as before this series). Or
> it's the opposite and then the earlier local_lock_is_locked() check should
> have prevented us from going here. So I guess we could just trylock without
> checking the return value - any screw up should blow up quickly even without
> the BUG_ON().

As explained above, under normal circumstances the trylock will always
succeed on !RT. But ignoring the return value here does not feel right
and the API might get extended to warn if the return error is ignored.

Sebastian