[PATCH slab/for-next-fixes] mm/slab: allow sheaf refill if blocking is not allowed

[PATCH slab/for-next-fixes] mm/slab: allow sheaf refill if blocking is not allowed

[Vlastimil Babka (SUSE)]

Ming Lei reported [1] a regression in the ublk null target benchmark due
to sheaves. The profile shows that the alloc_from_pcs() fastpath fails
and allocations fall back to ___slab_alloc(). It also shows the
allocations happen through mempool_alloc().

The strategy of mempool_alloc() is to call the underlying allocator
(here slab) without __GFP_DIRECT_RECLAIM first. This does not play well
with __pcs_replace_empty_main() checking for gfpflags_allow_blocking()
to decide if it should refill an empty sheaf or fallback to the
slowpath, so we end up falling back.

We could change the mempool strategy but there might be other paths
doing the same ting. So instead allow sheaf refill when blocking is not
allowed, changing the condition to gfpflags_allow_spinning(). The
original condition was unnecessarily restrictive.

Note this doesn't fully resolve the regression [1] as another component
of that are memoryless nodes, which is to be addressed separately.

Reported-by: Ming Lei <ming.lei@redhat.com>
Fixes: e47c897a2949 ("slab: add sheaves to most caches")
Link: https://lore.kernel.org/all/aZ0SbIqaIkwoW2mB@fedora/ [1]
Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
---
 mm/slub.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index b1e9f16ba435..17b200695e9b 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -4567,7 +4567,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
 	struct slab_sheaf *empty = NULL;
 	struct slab_sheaf *full;
 	struct node_barn *barn;
-	bool can_alloc;
+	bool allow_spin;
 
 	lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
 
@@ -4588,8 +4588,9 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
 		return NULL;
 	}
 
-	full = barn_replace_empty_sheaf(barn, pcs->main,
-					gfpflags_allow_spinning(gfp));
+	allow_spin = gfpflags_allow_spinning(gfp);
+
+	full = barn_replace_empty_sheaf(barn, pcs->main, allow_spin);
 
 	if (full) {
 		stat(s, BARN_GET);
@@ -4599,9 +4600,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
 
 	stat(s, BARN_GET_FAIL);
 
-	can_alloc = gfpflags_allow_blocking(gfp);
-
-	if (can_alloc) {
+	if (allow_spin) {
 		if (pcs->spare) {
 			empty = pcs->spare;
 			pcs->spare = NULL;
@@ -4612,7 +4611,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
 
 	local_unlock(&s->cpu_sheaves->lock);
 
-	if (!can_alloc)
+	if (!allow_spin)
 		return NULL;
 
 	if (empty) {
@@ -4632,11 +4631,8 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
 	if (!full)
 		return NULL;
 
-	/*
-	 * we can reach here only when gfpflags_allow_blocking
-	 * so this must not be an irq
-	 */
-	local_lock(&s->cpu_sheaves->lock);
+	if (!local_trylock(&s->cpu_sheaves->lock))
+		goto barn_put;
 	pcs = this_cpu_ptr(s->cpu_sheaves);
 
 	/*
@@ -4667,6 +4663,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
 		return pcs;
 	}
 
+barn_put:
 	barn_put_full_sheaf(barn, full);
 	stat(s, BARN_PUT);
 
-- 
2.53.0

Re: [PATCH slab/for-next-fixes] mm/slab: allow sheaf refill if blocking is not allowed

[Harry Yoo]

On Mon, Mar 02, 2026 at 10:55:37AM +0100, Vlastimil Babka (SUSE) wrote:
> Ming Lei reported [1] a regression in the ublk null target benchmark due
> to sheaves. The profile shows that the alloc_from_pcs() fastpath fails
> and allocations fall back to ___slab_alloc(). It also shows the
> allocations happen through mempool_alloc().
> 
> The strategy of mempool_alloc() is to call the underlying allocator
> (here slab) without __GFP_DIRECT_RECLAIM first. This does not play well
> with __pcs_replace_empty_main() checking for gfpflags_allow_blocking()
> to decide if it should refill an empty sheaf or fallback to the
> slowpath, so we end up falling back.
> 
> We could change the mempool strategy but there might be other paths
> doing the same ting. So instead allow sheaf refill when blocking is not
> allowed, changing the condition to gfpflags_allow_spinning(). The
> original condition was unnecessarily restrictive.
> 
> Note this doesn't fully resolve the regression [1] as another component
> of that are memoryless nodes, which is to be addressed separately.
> 
> Reported-by: Ming Lei <ming.lei@redhat.com>
> Fixes: e47c897a2949 ("slab: add sheaves to most caches")
> Link: https://lore.kernel.org/all/aZ0SbIqaIkwoW2mB@fedora/
> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
> ---
>  mm/slub.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index b1e9f16ba435..17b200695e9b 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4632,11 +4631,8 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>  	if (!full)
>  		return NULL;
>  
> -	/*
> -	 * we can reach here only when gfpflags_allow_blocking
> -	 * so this must not be an irq
> -	 */
> -	local_lock(&s->cpu_sheaves->lock);
> +	if (!local_trylock(&s->cpu_sheaves->lock))
> +		goto barn_put;

My AI buddy says (don't worry, I filtered it):
| When local_trylock() fails above, the function jumps to barn_put and returns
| pcs without holding the lock. This appears to violate the function's contract
| documented in the comment at the beginning of __pcs_replace_empty_main():
| 
|     "If not successful, returns NULL and the local lock unlocked."
| 
| The caller in alloc_from_pcs() checks for NULL to detect failure:
| 
|     if (unlikely(pcs->main->size == 0)) {
|         pcs = __pcs_replace_empty_main(s, pcs, gfp);
|         if (unlikely(!pcs))
|             return NULL;
|     }
| 
| If the trylock fails and pcs (non-NULL) is returned, the caller proceeds
| without realizing the lock was never re-acquired. This leads to accessing
| pcs->main without the lock and later trying to unlock a lock that isn't held.

And the analysis sounds correct to me.

perhaps it should be:

if (!local_trylock(&s->cpu_sheaves->lock)) {
	pcs = NULL;
	goto barn_put;
}

>  	pcs = this_cpu_ptr(s->cpu_sheaves);
>  
>  	/*
> @@ -4667,6 +4663,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>  		return pcs;
>  	}
>  
> +barn_put:
>  	barn_put_full_sheaf(barn, full);
>  	stat(s, BARN_PUT);

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH slab/for-next-fixes] mm/slab: allow sheaf refill if blocking is not allowed

[Vlastimil Babka]

On 3/4/26 4:05 AM, Harry Yoo wrote:
> On Mon, Mar 02, 2026 at 10:55:37AM +0100, Vlastimil Babka (SUSE) wrote:
>> Ming Lei reported [1] a regression in the ublk null target benchmark due
>> to sheaves. The profile shows that the alloc_from_pcs() fastpath fails
>> and allocations fall back to ___slab_alloc(). It also shows the
>> allocations happen through mempool_alloc().
>>
>> The strategy of mempool_alloc() is to call the underlying allocator
>> (here slab) without __GFP_DIRECT_RECLAIM first. This does not play well
>> with __pcs_replace_empty_main() checking for gfpflags_allow_blocking()
>> to decide if it should refill an empty sheaf or fallback to the
>> slowpath, so we end up falling back.
>>
>> We could change the mempool strategy but there might be other paths
>> doing the same ting. So instead allow sheaf refill when blocking is not
>> allowed, changing the condition to gfpflags_allow_spinning(). The
>> original condition was unnecessarily restrictive.
>>
>> Note this doesn't fully resolve the regression [1] as another component
>> of that are memoryless nodes, which is to be addressed separately.
>>
>> Reported-by: Ming Lei <ming.lei@redhat.com>
>> Fixes: e47c897a2949 ("slab: add sheaves to most caches")
>> Link: https://lore.kernel.org/all/aZ0SbIqaIkwoW2mB@fedora/
>> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
>> ---
>>  mm/slub.c | 21 +++++++++------------
>>  1 file changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index b1e9f16ba435..17b200695e9b 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -4632,11 +4631,8 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>>  	if (!full)
>>  		return NULL;
>>  
>> -	/*
>> -	 * we can reach here only when gfpflags_allow_blocking
>> -	 * so this must not be an irq
>> -	 */
>> -	local_lock(&s->cpu_sheaves->lock);
>> +	if (!local_trylock(&s->cpu_sheaves->lock))
>> +		goto barn_put;
> 
> My AI buddy says (don't worry, I filtered it):
> | When local_trylock() fails above, the function jumps to barn_put and returns
> | pcs without holding the lock. This appears to violate the function's contract
> | documented in the comment at the beginning of __pcs_replace_empty_main():
> | 
> |     "If not successful, returns NULL and the local lock unlocked."
> | 
> | The caller in alloc_from_pcs() checks for NULL to detect failure:
> | 
> |     if (unlikely(pcs->main->size == 0)) {
> |         pcs = __pcs_replace_empty_main(s, pcs, gfp);
> |         if (unlikely(!pcs))
> |             return NULL;
> |     }
> | 
> | If the trylock fails and pcs (non-NULL) is returned, the caller proceeds
> | without realizing the lock was never re-acquired. This leads to accessing
> | pcs->main without the lock and later trying to unlock a lock that isn't held.
> 
> And the analysis sounds correct to me.
> 
> perhaps it should be:
> 
> if (!local_trylock(&s->cpu_sheaves->lock)) {
> 	pcs = NULL;
> 	goto barn_put;
> }

Thanks a lot Harry. In fact I realized this mistake after initially
sending the patch to Ming in a reply, and fixed it locally (same as you
suggest).
Or so I thought, because the fix got apparently lost.
So I'll do that now in slab/for-next-fixes

Or actually I think a more robust way is to set pcs = NULL after the
unlock, unconditionally, so I'll do that.

>>  	pcs = this_cpu_ptr(s->cpu_sheaves);
>>  
>>  	/*
>> @@ -4667,6 +4663,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>>  		return pcs;
>>  	}
>>  
>> +barn_put:
>>  	barn_put_full_sheaf(barn, full);
>>  	stat(s, BARN_PUT);
> 

Re: [PATCH slab/for-next-fixes] mm/slab: allow sheaf refill if blocking is not allowed

[Harry Yoo]

On Wed, Mar 04, 2026 at 10:58:58AM +0100, Vlastimil Babka wrote:
> On 3/4/26 4:05 AM, Harry Yoo wrote:
> > On Mon, Mar 02, 2026 at 10:55:37AM +0100, Vlastimil Babka (SUSE) wrote:
> >> Ming Lei reported [1] a regression in the ublk null target benchmark due
> >> to sheaves. The profile shows that the alloc_from_pcs() fastpath fails
> >> and allocations fall back to ___slab_alloc(). It also shows the
> >> allocations happen through mempool_alloc().
> >>
> >> The strategy of mempool_alloc() is to call the underlying allocator
> >> (here slab) without __GFP_DIRECT_RECLAIM first. This does not play well
> >> with __pcs_replace_empty_main() checking for gfpflags_allow_blocking()
> >> to decide if it should refill an empty sheaf or fallback to the
> >> slowpath, so we end up falling back.
> >>
> >> We could change the mempool strategy but there might be other paths
> >> doing the same ting. So instead allow sheaf refill when blocking is not
> >> allowed, changing the condition to gfpflags_allow_spinning(). The
> >> original condition was unnecessarily restrictive.
> >>
> >> Note this doesn't fully resolve the regression [1] as another component
> >> of that are memoryless nodes, which is to be addressed separately.
> >>
> >> Reported-by: Ming Lei <ming.lei@redhat.com>
> >> Fixes: e47c897a2949 ("slab: add sheaves to most caches")
> >> Link: https://lore.kernel.org/all/aZ0SbIqaIkwoW2mB@fedora/
> >> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
> >> ---
> >>  mm/slub.c | 21 +++++++++------------
> >>  1 file changed, 9 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/mm/slub.c b/mm/slub.c
> >> index b1e9f16ba435..17b200695e9b 100644
> >> --- a/mm/slub.c
> >> +++ b/mm/slub.c
> >> @@ -4632,11 +4631,8 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
> >>  	if (!full)
> >>  		return NULL;
> >>  
> >> -	/*
> >> -	 * we can reach here only when gfpflags_allow_blocking
> >> -	 * so this must not be an irq
> >> -	 */
> >> -	local_lock(&s->cpu_sheaves->lock);
> >> +	if (!local_trylock(&s->cpu_sheaves->lock))
> >> +		goto barn_put;
> > 
> > My AI buddy says (don't worry, I filtered it):
> > | When local_trylock() fails above, the function jumps to barn_put and returns
> > | pcs without holding the lock. This appears to violate the function's contract
> > | documented in the comment at the beginning of __pcs_replace_empty_main():
> > | 
> > |     "If not successful, returns NULL and the local lock unlocked."
> > | 
> > | The caller in alloc_from_pcs() checks for NULL to detect failure:
> > | 
> > |     if (unlikely(pcs->main->size == 0)) {
> > |         pcs = __pcs_replace_empty_main(s, pcs, gfp);
> > |         if (unlikely(!pcs))
> > |             return NULL;
> > |     }
> > | 
> > | If the trylock fails and pcs (non-NULL) is returned, the caller proceeds
> > | without realizing the lock was never re-acquired. This leads to accessing
> > | pcs->main without the lock and later trying to unlock a lock that isn't held.
> > 
> > And the analysis sounds correct to me.
> > 
> > perhaps it should be:
> > 
> > if (!local_trylock(&s->cpu_sheaves->lock)) {
> > 	pcs = NULL;
> > 	goto barn_put;
> > }
> 
> Thanks a lot Harry. In fact I realized this mistake after initially
> sending the patch to Ming in a reply, and fixed it locally (same as you
> suggest).
> Or so I thought, because the fix got apparently lost.

That happens sometimes, yeah :)

> So I'll do that now in slab/for-next-fixes

Thanks.

> Or actually I think a more robust way is to set pcs = NULL after the
> unlock, unconditionally, so I'll do that.

Oh, that sounds better!

> >>  	pcs = this_cpu_ptr(s->cpu_sheaves);
> >>  
> >>  	/*
> >> @@ -4667,6 +4663,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
> >>  		return pcs;
> >>  	}
> >>  
> >> +barn_put:
> >>  	barn_put_full_sheaf(barn, full);
> >>  	stat(s, BARN_PUT);

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH slab/for-next-fixes] mm/slab: allow sheaf refill if blocking is not allowed

[Hao Li]

On Mon, Mar 02, 2026 at 10:55:37AM +0100, Vlastimil Babka (SUSE) wrote:
> Ming Lei reported [1] a regression in the ublk null target benchmark due
> to sheaves. The profile shows that the alloc_from_pcs() fastpath fails
> and allocations fall back to ___slab_alloc(). It also shows the
> allocations happen through mempool_alloc().
> 
> The strategy of mempool_alloc() is to call the underlying allocator
> (here slab) without __GFP_DIRECT_RECLAIM first. This does not play well
> with __pcs_replace_empty_main() checking for gfpflags_allow_blocking()
> to decide if it should refill an empty sheaf or fallback to the
> slowpath, so we end up falling back.
> 
> We could change the mempool strategy but there might be other paths
> doing the same ting. So instead allow sheaf refill when blocking is not
> allowed, changing the condition to gfpflags_allow_spinning(). The
> original condition was unnecessarily restrictive.
> 
> Note this doesn't fully resolve the regression [1] as another component
> of that are memoryless nodes, which is to be addressed separately.
> 
> Reported-by: Ming Lei <ming.lei@redhat.com>
> Fixes: e47c897a2949 ("slab: add sheaves to most caches")
> Link: https://lore.kernel.org/all/aZ0SbIqaIkwoW2mB@fedora/ [1]
> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
> ---
>  mm/slub.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index b1e9f16ba435..17b200695e9b 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4567,7 +4567,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>  	struct slab_sheaf *empty = NULL;
>  	struct slab_sheaf *full;
>  	struct node_barn *barn;
> -	bool can_alloc;
> +	bool allow_spin;
>  
>  	lockdep_assert_held(this_cpu_ptr(&s->cpu_sheaves->lock));
>  
> @@ -4588,8 +4588,9 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>  		return NULL;
>  	}
>  
> -	full = barn_replace_empty_sheaf(barn, pcs->main,
> -					gfpflags_allow_spinning(gfp));
> +	allow_spin = gfpflags_allow_spinning(gfp);
> +
> +	full = barn_replace_empty_sheaf(barn, pcs->main, allow_spin);
>  
>  	if (full) {
>  		stat(s, BARN_GET);
> @@ -4599,9 +4600,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>  
>  	stat(s, BARN_GET_FAIL);
>  
> -	can_alloc = gfpflags_allow_blocking(gfp);
> -
> -	if (can_alloc) {
> +	if (allow_spin) {
>  		if (pcs->spare) {
>  			empty = pcs->spare;
>  			pcs->spare = NULL;
> @@ -4612,7 +4611,7 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>  
>  	local_unlock(&s->cpu_sheaves->lock);
>  
> -	if (!can_alloc)
> +	if (!allow_spin)
>  		return NULL;
>  
>  	if (empty) {
> @@ -4632,11 +4631,8 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>  	if (!full)
>  		return NULL;
>  
> -	/*
> -	 * we can reach here only when gfpflags_allow_blocking
> -	 * so this must not be an irq
> -	 */
> -	local_lock(&s->cpu_sheaves->lock);
> +	if (!local_trylock(&s->cpu_sheaves->lock))
> +		goto barn_put;

A quick question to make sure I understand this correctly.

My understanding is that after this patch, there is now a new case where
allocations with __GFP_KSWAPD_RECLAIM set (e.g GFP_ATOMIC) can also reach this
lock-reacquire path.

If we were to keep using local_lock here:

1. On non-RT kernels it seems fine, since alloc_from_pcs() already does a
   local_trylock(&s->cpu_sheaves->lock) check.

2. But on PREEMPT_RT, local_lock could potentially schedule away, which may add
   latency. So the idea of using local_trylock here is to fail fast and return
   without incurring that latency - is that the intent behind this change?

-- 
Thanks,
Hao

Re: [PATCH slab/for-next-fixes] mm/slab: allow sheaf refill if blocking is not allowed

[Vlastimil Babka (SUSE)]

On 3/4/26 8:44 AM, Hao Li wrote:
> On Mon, Mar 02, 2026 at 10:55:37AM +0100, Vlastimil Babka (SUSE) wrote:
>> @@ -4632,11 +4631,8 @@ __pcs_replace_empty_main(struct kmem_cache *s, struct slub_percpu_sheaves *pcs,
>>  	if (!full)
>>  		return NULL;
>>  
>> -	/*
>> -	 * we can reach here only when gfpflags_allow_blocking
>> -	 * so this must not be an irq
>> -	 */
>> -	local_lock(&s->cpu_sheaves->lock);
>> +	if (!local_trylock(&s->cpu_sheaves->lock))
>> +		goto barn_put;
> 
> A quick question to make sure I understand this correctly.
> 
> My understanding is that after this patch, there is now a new case where
> allocations with __GFP_KSWAPD_RECLAIM set (e.g GFP_ATOMIC) can also reach this
> lock-reacquire path.
> 
> If we were to keep using local_lock here:
> 
> 1. On non-RT kernels it seems fine, since alloc_from_pcs() already does a
>    local_trylock(&s->cpu_sheaves->lock) check.
> 
> 2. But on PREEMPT_RT, local_lock could potentially schedule away, which may add
>    latency. So the idea of using local_trylock here is to fail fast and return
>    without incurring that latency - is that the intent behind this change?

Great question, thanks!

So the main intent is that lockdep would complain if it saw this
local_lock() happening in e.g. an irq handler. It doesn't know that it's
safe from deadlocks because we already succeeded a trylock before and
thus the irq handler didn't interrupt anyone holding the lock.

Trying to teach lockdep such things leads to the complicated initial
design of kmalloc_nolock() before it could be simplified by sheaves.

On !RT it makes no difference as the trylock will succeed always. On RT
it may not, but indeed they may prefer avoiding the latency as you say.