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

[Vlastimil Babka <vbabka@suse.cz>]

On 7/14/25 13:06, Sebastian Andrzej Siewior wrote:
> On 2025-07-11 19:19:26 [-0700], Alexei Starovoitov wrote:
>> > 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.
> 
> what about this:
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 7e2ffe1d46c6c..3520d1c25c205 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3693,6 +3693,34 @@ static inline void *freeze_slab(struct kmem_cache *s, struct slab *slab)
>  	return freelist;
>  }
>  
> +static void local_lock_cpu_slab(struct kmem_cache *s, const gfp_t gfp_flags,
> +				unsigned long *flags)
> +{
> +	bool allow_spin = gfpflags_allow_spinning(gfp_flags);
> +
> +	/*
> +	 * ___slab_alloc()'s caller is supposed to check if kmem_cache::kmem_cache_cpu::lock
> +	 * can be acquired without a deadlock before invoking the function.
> +	 *
> +	 * On PREEMPT_RT an invocation is not possible from IRQ-off or preempt
> +	 * disabled context. The lock will always be acquired and if needed it
> +	 * block and sleep until the lock is available.
> +	 *
> +	 * On !PREEMPT_RT allocations from any context but NMI are safe. The lock
> +	 * is always acquired with disabled interrupts meaning it is always
> +	 * possible to it.
> +	 * In NMI context it is needed to check if the lock is acquired. If it is not,
> +	 * it is safe to acquire it. The trylock semantic is used to tell lockdep
> +	 * that we don't spin. The BUG_ON() will not trigger if it is safe to acquire
> +	 * the lock.
> +	 *
> +	 */
> +	if (!IS_ENABLED(CONFIG_PREEMPT_RT) && !allow_spin)
> +		BUG_ON(!local_trylock_irqsave(&s->cpu_slab->lock, *flags));
> +	else
> +		local_lock_irqsave(&s->cpu_slab->lock, *flags);

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

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().

> +}
> +
>  /*
>   * Slow path. The lockless freelist is empty or we need to perform
>   * debugging duties.
> @@ -3765,7 +3793,8 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>  		goto deactivate_slab;
>  
>  	/* must check again c->slab in case we got preempted and it changed */
> -	local_lock_irqsave(&s->cpu_slab->lock, flags);
> +	local_lock_cpu_slab(s, gfpflags, &flags);
> +
>  	if (unlikely(slab != c->slab)) {
>  		local_unlock_irqrestore(&s->cpu_slab->lock, flags);
>  		goto reread_slab;
> @@ -3803,7 +3832,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>  
>  deactivate_slab:
>  
> -	local_lock_irqsave(&s->cpu_slab->lock, flags);
> +	local_lock_cpu_slab(s, gfpflags, &flags);
>  	if (slab != c->slab) {
>  		local_unlock_irqrestore(&s->cpu_slab->lock, flags);
>  		goto reread_slab;
> @@ -3819,7 +3848,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>  
>  #ifdef CONFIG_SLUB_CPU_PARTIAL
>  	while (slub_percpu_partial(c)) {
> -		local_lock_irqsave(&s->cpu_slab->lock, flags);
> +		local_lock_cpu_slab(s, gfpflags, &flags);
>  		if (unlikely(c->slab)) {
>  			local_unlock_irqrestore(&s->cpu_slab->lock, flags);
>  			goto reread_slab;
> @@ -3947,7 +3976,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>  
>  retry_load_slab:
>  
> -	local_lock_irqsave(&s->cpu_slab->lock, flags);
> +	local_lock_cpu_slab(s, gfpflags, &flags);
>  	if (unlikely(c->slab)) {
>  		void *flush_freelist = c->freelist;
>  		struct slab *flush_slab = c->slab;
> @@ -4003,12 +4032,8 @@ static void *__slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>  			p = ERR_PTR(-EBUSY);
>  			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);
> -	} else {
> -		p = ___slab_alloc(s, gfpflags, node, addr, c, orig_size);
>  	}
> +	p = ___slab_alloc(s, gfpflags, node, addr, c, orig_size);
>  out:
>  #ifdef CONFIG_PREEMPT_COUNT
>  	slub_put_cpu_ptr(s->cpu_slab);
> 
> 
> Sebastian