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

[Vlastimil Babka (SUSE)]

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.

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

>> Humm, but if that scenario exist, then is it actually ok to return true on 
>> trylock() in that scenario?
>> 
>> Thanks!
>> Leo
> 
> Sebastian