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
