Re: [PATCH v4 4/4] slub: apply new pw_queue_on() interface

[Leonardo Bras <leobras.c@gmail.com>]

On Mon, Jul 13, 2026 at 12:55:36PM +0200, Vlastimil Babka (SUSE) wrote:
> On 7/13/26 09:36, Sebastian Andrzej Siewior wrote:
> > On 2026-07-12 19:35:28 [-0300], Leonardo Bras wrote:
> >> On Wed, May 20, 2026 at 04:53:08PM +0200, Sebastian Andrzej Siewior wrote:
> >> > On 2026-05-18 22:27:50 [-0300], Leonardo Bras wrote:
> >> > > @@ -4733,121 +4735,121 @@ void *alloc_from_pcs(struct kmem_cache *s, gfp_t gfp, int node)
> >> > >  
> >> > >  	/*
> >> > >  	 * We assume the percpu sheaves contain only local objects although it's
> >> > >  	 * not completely guaranteed, so we verify later.
> >> > >  	 */
> >> > >  	if (unlikely(node_requested && node != numa_mem_id())) {
> >> > >  		stat(s, ALLOC_NODE_MISMATCH);
> >> > >  		return NULL;
> >> > >  	}
> >> > >  
> >> > > -	if (!local_trylock(&s->cpu_sheaves->lock))
> >> > > +	if (!pw_trylock_local(&s->cpu_sheaves->lock))
> >> > >  		return NULL;
> >> > 
> >> > alloc_from_pcs() can be called from kmalloc_nolock()/ NMI context.
> >> > I don't remember why exactly local_trylock_t was introduced here instead
> >> > of a per-CPU spinlock_t. 
> >> 
> >> Probably to save the cost of using atomic operations on locking, and having 
> 
> Yes. Also function call overhead as spinlocks are often not inlined.

Oh, right, I remember getting numbers on that. Inlined per-cpu spinlocks 
take much less time.

> 
> >> about the same restrictions that would allow using local_locks
> 
> Indeed.
> 
> >> > But there should be nothing wrong with a
> >> > trylock on it from NMI as you do here.
> >> 
> >> Awesome!
> > 
> > The problem is always the unlock which requires full locking and is
> > usually the problem from NMI.
> > 
> >> > 
> >> > One thing worth noting, on !PREEMPT_RT, spin_trylock() always succeeds
> >> > on UP. kmalloc_nolock() checks for it, not sure about other callers.
> >> 
> >> 
> >> Sorry, I did not sure I understand that part. 
> >> You mean we have since it always returns true, we may be in NMI context, 
> >> after it was interrupted holding this lock, and it will return true which 
> >> will use the protected area even though the lock should avoid it?
> > 
> > from include/linux/spinlock_api_up.h:
> > | static __always_inline int _raw_spin_trylock(raw_spinlock_t *lock)
> > |         __cond_acquires(true, lock)
> > | {
> > |         __LOCK(lock);
> > |         return 1;
> > | }
> > 
> > on UP a spin_trylock() always succeeds.
> 
> Indeed. This was a problem in the page allocator, because there we don't
> even disable irqs, so it wasn't just nmi, but an irq that could get a
> false-positive spin_trylock(). See 038a102535eb ("mm/page_alloc: prevent pcp
> corruption with SMP=n") which was a hot fix.
> 
> This was later cleaned up with 3 commits starting with a373f371166d
> ("mm/page_alloc: effectively disable pcp with CONFIG_SMP=n").
> 
> Since here you're also replacing local_trylock() (with no _irqsave) with
> effectively spin_trylock(), the problem also won't be limited to NMIs and
> thus the checks in kmalloc_nolock() won't help.
> 
> But I see in Patch 1:
> 
> +config PWLOCKS
> +	bool "Per-CPU Work locks"
> +	depends on SMP || COMPILE_TEST
> 
> So that's basically avoiding the problem in the same way as the page
> allocator after the clean up.
> Maybe just remove the COMPILE_TEST part? It's not clear to me what it
> achieves. Just make it require SMP as there's no point for this on !SMP.
> 

I don't mind removing it, but IIRC that was added by Marcelo, I have to 
ask him the reason he added it.

Thanks!
Leo