[PATCH] mm/slab: extract __free_to_pcs_batch() from free_to_pcs_bulk()

[PATCH] mm/slab: extract __free_to_pcs_batch() from free_to_pcs_bulk()

[Vlastimil Babka (SUSE)]

It has been noted that free_to_pcs_bulk() is difficult to follow, with a
number of goto labels, and this has contributed to two memory leak bugs
in there.

Extract part of the code to __free_to_pcs_batch(), which focuses only on
freeing free-hook-processed local objects to a percpu sheaf, and
returning how many were freed. Zero means a trylock failure or no empty
sheaf available, and thus the caller should fallback to
__kmem_cache_free_bulk().

Make free_to_pcs_bulk() call this in a while loop, removing all goto
labels from the function. __free_to_pcs_batch() retains two rather
straightforward ones.

Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
---
A followup refactoring to Shengming's fix.
---
 mm/slub.c | 116 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 61 insertions(+), 55 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 65febe957886..3f13f497aab4 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -6203,51 +6203,21 @@ static __always_inline bool can_free_to_pcs(struct slab *slab)
 }
 
 /*
- * Bulk free objects to the percpu sheaves.
- * Unlike free_to_pcs() this includes the calls to all necessary hooks
- * and the fallback to freeing to slab pages.
+ * Try to free as many objects (already processed by free hooks) as possible to
+ * a single per-cpu sheaf.
+ *
+ * Returns how many objects were freed. Zero means failure and the caller should
+ * fall back to __kmem_cache_free_bulk().
  */
-static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
+static unsigned int __free_to_pcs_batch(struct kmem_cache *s, size_t size, void **p)
 {
 	struct slub_percpu_sheaves *pcs;
 	struct slab_sheaf *main, *empty;
-	bool init = slab_want_init_on_free(s);
-	unsigned int batch, i = 0;
 	struct node_barn *barn;
-	void *remote_objects[PCS_BATCH_MAX];
-	unsigned int remote_nr = 0;
-
-	while (i < size) {
-		struct slab *slab = virt_to_slab(p[i]);
-
-		memcg_slab_free_hook(s, slab, p + i, 1);
-		alloc_tagging_slab_free_hook(s, slab, p + i, 1);
-
-		if (unlikely(!slab_free_hook(s, p[i], init, false))) {
-			p[i] = p[--size];
-			continue;
-		}
-
-		if (unlikely(!can_free_to_pcs(slab))) {
-			remote_objects[remote_nr] = p[i];
-			p[i] = p[--size];
-			if (++remote_nr >= PCS_BATCH_MAX) {
-				__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
-				stat_add(s, FREE_SLOWPATH, remote_nr);
-				remote_nr = 0;
-			}
-			continue;
-		}
-
-		i++;
-	}
-
-	if (!size)
-		goto flush_remote;
+	unsigned int batch;
 
-next_batch:
 	if (!local_trylock(&s->cpu_sheaves->lock))
-		goto fallback;
+		return 0;
 
 	pcs = this_cpu_ptr(s->cpu_sheaves);
 
@@ -6293,29 +6263,65 @@ static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
 
 	stat_add(s, FREE_FASTPATH, batch);
 
-	if (batch < size) {
-		p += batch;
-		size -= batch;
-		goto next_batch;
+	return batch;
+
+no_empty:
+	local_unlock(&s->cpu_sheaves->lock);
+
+	return 0;
+}
+
+/*
+ * Bulk free objects to the percpu sheaves.
+ * Unlike free_to_pcs() this includes the calls to all necessary hooks
+ * and the fallback to freeing to slab pages.
+ */
+static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
+{
+	bool init = slab_want_init_on_free(s);
+	void *remote_objects[PCS_BATCH_MAX];
+	unsigned int remote_nr = 0;
+
+	for (unsigned int i = 0; i < size;) {
+		struct slab *slab = virt_to_slab(p[i]);
+
+		memcg_slab_free_hook(s, slab, p + i, 1);
+		alloc_tagging_slab_free_hook(s, slab, p + i, 1);
+
+		if (unlikely(!slab_free_hook(s, p[i], init, false))) {
+			p[i] = p[--size];
+			continue;
+		}
+
+		if (unlikely(!can_free_to_pcs(slab))) {
+			remote_objects[remote_nr] = p[i];
+			p[i] = p[--size];
+			if (++remote_nr >= PCS_BATCH_MAX) {
+				__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
+				stat_add(s, FREE_SLOWPATH, remote_nr);
+				remote_nr = 0;
+			}
+			continue;
+		}
+
+		i++;
 	}
 
-	if (remote_nr)
-		goto flush_remote;
+	while (size) {
+		unsigned int batch_freed = __free_to_pcs_batch(s, size, p);
 
-	return;
+		if (!batch_freed)
+			break;
 
-no_empty:
-	local_unlock(&s->cpu_sheaves->lock);
+		p += batch_freed;
+		size -= batch_freed;
+	}
 
-	/*
-	 * if we depleted all empty sheaves in the barn or there are too
-	 * many full sheaves, free the rest to slab pages
-	 */
-fallback:
-	__kmem_cache_free_bulk(s, size, p);
-	stat_add(s, FREE_SLOWPATH, size);
+	if (size) {
+		__kmem_cache_free_bulk(s, size, p);
+		stat_add(s, FREE_SLOWPATH, size);
+	}
 
-flush_remote:
 	if (remote_nr) {
 		__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
 		stat_add(s, FREE_SLOWPATH, remote_nr);

---
base-commit: 72bb229f9161a1efcd5df32141b69fcc6ae81a13
change-id: 20260707-slab-simplify-bulk-pcs-a8d0478feef6

Best regards,
--  
Vlastimil Babka (SUSE) <vbabka@kernel.org>

Re: [PATCH] mm/slab: extract __free_to_pcs_batch() from free_to_pcs_bulk()

[hu.shengming]

Vlastimil wrote:
> It has been noted that free_to_pcs_bulk() is difficult to follow, with a
> number of goto labels, and this has contributed to two memory leak bugs
> in there.
> 
> Extract part of the code to __free_to_pcs_batch(), which focuses only on
> freeing free-hook-processed local objects to a percpu sheaf, and
> returning how many were freed. Zero means a trylock failure or no empty
> sheaf available, and thus the caller should fallback to
> __kmem_cache_free_bulk().
> 
> Make free_to_pcs_bulk() call this in a while loop, removing all goto
> labels from the function. __free_to_pcs_batch() retains two rather
> straightforward ones.

Hi Vlastimil,

This looks like a very nice refactoring to me. Splitting out the actual
per-CPU sheaf batching into __free_to_pcs_batch() makes the control flow
in free_to_pcs_bulk() much easier to follow, and also keeps the fallback
handling clearer.

Please feel free to add:
Reviewed-by: Shengming Hu <hu.shengming@zte.com.cn>

--
With Best Regards,
Shengming

> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
> ---
> A followup refactoring to Shengming's fix.
> ---
>  mm/slub.c | 116 +++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 61 insertions(+), 55 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 65febe957886..3f13f497aab4 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -6203,51 +6203,21 @@ static __always_inline bool can_free_to_pcs(struct slab *slab)
>  }
>  
>  /*
> - * Bulk free objects to the percpu sheaves.
> - * Unlike free_to_pcs() this includes the calls to all necessary hooks
> - * and the fallback to freeing to slab pages.
> + * Try to free as many objects (already processed by free hooks) as possible to
> + * a single per-cpu sheaf.
> + *
> + * Returns how many objects were freed. Zero means failure and the caller should
> + * fall back to __kmem_cache_free_bulk().
>   */
> -static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
> +static unsigned int __free_to_pcs_batch(struct kmem_cache *s, size_t size, void **p)
>  {
>  	struct slub_percpu_sheaves *pcs;
>  	struct slab_sheaf *main, *empty;
> -	bool init = slab_want_init_on_free(s);
> -	unsigned int batch, i = 0;
>  	struct node_barn *barn;
> -	void *remote_objects[PCS_BATCH_MAX];
> -	unsigned int remote_nr = 0;
> -
> -	while (i < size) {
> -		struct slab *slab = virt_to_slab(p[i]);
> -
> -		memcg_slab_free_hook(s, slab, p + i, 1);
> -		alloc_tagging_slab_free_hook(s, slab, p + i, 1);
> -
> -		if (unlikely(!slab_free_hook(s, p[i], init, false))) {
> -			p[i] = p[--size];
> -			continue;
> -		}
> -
> -		if (unlikely(!can_free_to_pcs(slab))) {
> -			remote_objects[remote_nr] = p[i];
> -			p[i] = p[--size];
> -			if (++remote_nr >= PCS_BATCH_MAX) {
> -				__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
> -				stat_add(s, FREE_SLOWPATH, remote_nr);
> -				remote_nr = 0;
> -			}
> -			continue;
> -		}
> -
> -		i++;
> -	}
> -
> -	if (!size)
> -		goto flush_remote;
> +	unsigned int batch;
>  
> -next_batch:
>  	if (!local_trylock(&s->cpu_sheaves->lock))
> -		goto fallback;
> +		return 0;
>  
>  	pcs = this_cpu_ptr(s->cpu_sheaves);
>  
> @@ -6293,29 +6263,65 @@ static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
>  
>  	stat_add(s, FREE_FASTPATH, batch);
>  
> -	if (batch < size) {
> -		p += batch;
> -		size -= batch;
> -		goto next_batch;
> +	return batch;
> +
> +no_empty:
> +	local_unlock(&s->cpu_sheaves->lock);
> +
> +	return 0;
> +}
> +
> +/*
> + * Bulk free objects to the percpu sheaves.
> + * Unlike free_to_pcs() this includes the calls to all necessary hooks
> + * and the fallback to freeing to slab pages.
> + */
> +static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
> +{
> +	bool init = slab_want_init_on_free(s);
> +	void *remote_objects[PCS_BATCH_MAX];
> +	unsigned int remote_nr = 0;
> +
> +	for (unsigned int i = 0; i < size;) {
> +		struct slab *slab = virt_to_slab(p[i]);
> +
> +		memcg_slab_free_hook(s, slab, p + i, 1);
> +		alloc_tagging_slab_free_hook(s, slab, p + i, 1);
> +
> +		if (unlikely(!slab_free_hook(s, p[i], init, false))) {
> +			p[i] = p[--size];
> +			continue;
> +		}
> +
> +		if (unlikely(!can_free_to_pcs(slab))) {
> +			remote_objects[remote_nr] = p[i];
> +			p[i] = p[--size];
> +			if (++remote_nr >= PCS_BATCH_MAX) {
> +				__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
> +				stat_add(s, FREE_SLOWPATH, remote_nr);
> +				remote_nr = 0;
> +			}
> +			continue;
> +		}
> +
> +		i++;
>  	}
>  
> -	if (remote_nr)
> -		goto flush_remote;
> +	while (size) {
> +		unsigned int batch_freed = __free_to_pcs_batch(s, size, p);
>  
> -	return;
> +		if (!batch_freed)
> +			break;
>  
> -no_empty:
> -	local_unlock(&s->cpu_sheaves->lock);
> +		p += batch_freed;
> +		size -= batch_freed;
> +	}
>  
> -	/*
> -	 * if we depleted all empty sheaves in the barn or there are too
> -	 * many full sheaves, free the rest to slab pages
> -	 */
> -fallback:
> -	__kmem_cache_free_bulk(s, size, p);
> -	stat_add(s, FREE_SLOWPATH, size);
> +	if (size) {
> +		__kmem_cache_free_bulk(s, size, p);
> +		stat_add(s, FREE_SLOWPATH, size);
> +	}
>  
> -flush_remote:
>  	if (remote_nr) {
>  		__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
>  		stat_add(s, FREE_SLOWPATH, remote_nr);
> 
> ---
> base-commit: 72bb229f9161a1efcd5df32141b69fcc6ae81a13
> change-id: 20260707-slab-simplify-bulk-pcs-a8d0478feef6
> 
> Best regards,
> --  
> Vlastimil Babka (SUSE) <vbabka@kernel.org>

Re: [PATCH] mm/slab: extract __free_to_pcs_batch() from free_to_pcs_bulk()

[Vlastimil Babka (SUSE)]

On 7/7/26 17:34, hu.shengming@zte.com.cn wrote:
> Vlastimil wrote:
>> It has been noted that free_to_pcs_bulk() is difficult to follow, with a
>> number of goto labels, and this has contributed to two memory leak bugs
>> in there.
>> 
>> Extract part of the code to __free_to_pcs_batch(), which focuses only on
>> freeing free-hook-processed local objects to a percpu sheaf, and
>> returning how many were freed. Zero means a trylock failure or no empty
>> sheaf available, and thus the caller should fallback to
>> __kmem_cache_free_bulk().
>> 
>> Make free_to_pcs_bulk() call this in a while loop, removing all goto
>> labels from the function. __free_to_pcs_batch() retains two rather
>> straightforward ones.
> 
> Hi Vlastimil,
> 
> This looks like a very nice refactoring to me. Splitting out the actual
> per-CPU sheaf batching into __free_to_pcs_batch() makes the control flow
> in free_to_pcs_bulk() much easier to follow, and also keeps the fallback
> handling clearer.
> 
> Please feel free to add:
> Reviewed-by: Shengming Hu <hu.shengming@zte.com.cn>

Thanks!

Added to slab/for-next

Re: [PATCH] mm/slab: extract __free_to_pcs_batch() from free_to_pcs_bulk()

[Hao Li]

On Tue, Jul 07, 2026 at 02:16:00PM +0200, Vlastimil Babka (SUSE) wrote:
> It has been noted that free_to_pcs_bulk() is difficult to follow, with a
> number of goto labels, and this has contributed to two memory leak bugs
> in there.
> 
> Extract part of the code to __free_to_pcs_batch(), which focuses only on
> freeing free-hook-processed local objects to a percpu sheaf, and
> returning how many were freed. Zero means a trylock failure or no empty
> sheaf available, and thus the caller should fallback to
> __kmem_cache_free_bulk().
> 
> Make free_to_pcs_bulk() call this in a while loop, removing all goto
> labels from the function. __free_to_pcs_batch() retains two rather
> straightforward ones.
> 
> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>

Nice simplification!

[...]
> +/*
> + * Bulk free objects to the percpu sheaves.
> + * Unlike free_to_pcs() this includes the calls to all necessary hooks
> + * and the fallback to freeing to slab pages.
> + */
> +static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
> +{
> +	bool init = slab_want_init_on_free(s);
> +	void *remote_objects[PCS_BATCH_MAX];
> +	unsigned int remote_nr = 0;
> +
> +	for (unsigned int i = 0; i < size;) {
> +		struct slab *slab = virt_to_slab(p[i]);
> +
> +		memcg_slab_free_hook(s, slab, p + i, 1);
> +		alloc_tagging_slab_free_hook(s, slab, p + i, 1);
> +
> +		if (unlikely(!slab_free_hook(s, p[i], init, false))) {
> +			p[i] = p[--size];
> +			continue;
> +		}
> +
> +		if (unlikely(!can_free_to_pcs(slab))) {
> +			remote_objects[remote_nr] = p[i];
> +			p[i] = p[--size];
> +			if (++remote_nr >= PCS_BATCH_MAX) {
> +				__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
> +				stat_add(s, FREE_SLOWPATH, remote_nr);
> +				remote_nr = 0;
> +			}
> +			continue;
> +		}
> +
> +		i++;
>  	}
>  
> -	if (remote_nr)
> -		goto flush_remote;
> +	while (size) {
> +		unsigned int batch_freed = __free_to_pcs_batch(s, size, p);
>  
> -	return;
> +		if (!batch_freed)
> +			break;
>  
> -no_empty:
> -	local_unlock(&s->cpu_sheaves->lock);
> +		p += batch_freed;
> +		size -= batch_freed;
> +	}
>  
> -	/*
> -	 * if we depleted all empty sheaves in the barn or there are too
> -	 * many full sheaves, free the rest to slab pages
> -	 */
> -fallback:
> -	__kmem_cache_free_bulk(s, size, p);
> -	stat_add(s, FREE_SLOWPATH, size);
> +	if (size) {
> +		__kmem_cache_free_bulk(s, size, p);
> +		stat_add(s, FREE_SLOWPATH, size);
> +	}

By the way, could we move this directly into the loop inside the
`if (!batch_freed)` block to make it a bit more concise? but it's totally fine
to leave it as is! :)

>  
> -flush_remote:
>  	if (remote_nr) {
>  		__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
>  		stat_add(s, FREE_SLOWPATH, remote_nr);
> 
> ---
> base-commit: 72bb229f9161a1efcd5df32141b69fcc6ae81a13
> change-id: 20260707-slab-simplify-bulk-pcs-a8d0478feef6
> 
> Best regards,
> --  
> Vlastimil Babka (SUSE) <vbabka@kernel.org>
> 

-- 
Thanks,
Hao

Re: [PATCH] mm/slab: extract __free_to_pcs_batch() from free_to_pcs_bulk()

[Hao Li]

On Fri, Jul 10, 2026 at 01:18:25PM +0800, Hao Li wrote:
> On Tue, Jul 07, 2026 at 02:16:00PM +0200, Vlastimil Babka (SUSE) wrote:
> > It has been noted that free_to_pcs_bulk() is difficult to follow, with a
> > number of goto labels, and this has contributed to two memory leak bugs
> > in there.
> > 
> > Extract part of the code to __free_to_pcs_batch(), which focuses only on
> > freeing free-hook-processed local objects to a percpu sheaf, and
> > returning how many were freed. Zero means a trylock failure or no empty
> > sheaf available, and thus the caller should fallback to
> > __kmem_cache_free_bulk().
> > 
> > Make free_to_pcs_bulk() call this in a while loop, removing all goto
> > labels from the function. __free_to_pcs_batch() retains two rather
> > straightforward ones.
> > 
> > Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
> 
> Nice simplification!
> 
> [...]
> > +/*
> > + * Bulk free objects to the percpu sheaves.
> > + * Unlike free_to_pcs() this includes the calls to all necessary hooks
> > + * and the fallback to freeing to slab pages.
> > + */
> > +static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
> > +{
> > +	bool init = slab_want_init_on_free(s);
> > +	void *remote_objects[PCS_BATCH_MAX];
> > +	unsigned int remote_nr = 0;
> > +
> > +	for (unsigned int i = 0; i < size;) {
> > +		struct slab *slab = virt_to_slab(p[i]);
> > +
> > +		memcg_slab_free_hook(s, slab, p + i, 1);
> > +		alloc_tagging_slab_free_hook(s, slab, p + i, 1);
> > +
> > +		if (unlikely(!slab_free_hook(s, p[i], init, false))) {
> > +			p[i] = p[--size];
> > +			continue;
> > +		}
> > +
> > +		if (unlikely(!can_free_to_pcs(slab))) {
> > +			remote_objects[remote_nr] = p[i];
> > +			p[i] = p[--size];
> > +			if (++remote_nr >= PCS_BATCH_MAX) {
> > +				__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
> > +				stat_add(s, FREE_SLOWPATH, remote_nr);
> > +				remote_nr = 0;
> > +			}
> > +			continue;
> > +		}
> > +
> > +		i++;
> >  	}
> >  
> > -	if (remote_nr)
> > -		goto flush_remote;
> > +	while (size) {
> > +		unsigned int batch_freed = __free_to_pcs_batch(s, size, p);
> >  
> > -	return;
> > +		if (!batch_freed)
> > +			break;
> >  
> > -no_empty:
> > -	local_unlock(&s->cpu_sheaves->lock);
> > +		p += batch_freed;
> > +		size -= batch_freed;
> > +	}
> >  
> > -	/*
> > -	 * if we depleted all empty sheaves in the barn or there are too
> > -	 * many full sheaves, free the rest to slab pages
> > -	 */
> > -fallback:
> > -	__kmem_cache_free_bulk(s, size, p);
> > -	stat_add(s, FREE_SLOWPATH, size);
> > +	if (size) {
> > +		__kmem_cache_free_bulk(s, size, p);
> > +		stat_add(s, FREE_SLOWPATH, size);
> > +	}
> 
> By the way, could we move this directly into the loop inside the
> `if (!batch_freed)` block to make it a bit more concise? but it's totally fine
> to leave it as is! :)

oops, forgot to add the r-b tag just now :P

Reviewed-by: Hao Li <hao.li@linux.dev>

> 
> >  
> > -flush_remote:
> >  	if (remote_nr) {
> >  		__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
> >  		stat_add(s, FREE_SLOWPATH, remote_nr);
> > 
> > ---
> > base-commit: 72bb229f9161a1efcd5df32141b69fcc6ae81a13
> > change-id: 20260707-slab-simplify-bulk-pcs-a8d0478feef6
> > 
> > Best regards,
> > --  
> > Vlastimil Babka (SUSE) <vbabka@kernel.org>
> > 

-- 
Thanks,
Hao

Re: [PATCH] mm/slab: extract __free_to_pcs_batch() from free_to_pcs_bulk()

[Vlastimil Babka (SUSE)]

On 7/10/26 07:21, Hao Li wrote:
> On Fri, Jul 10, 2026 at 01:18:25PM +0800, Hao Li wrote:
>> On Tue, Jul 07, 2026 at 02:16:00PM +0200, Vlastimil Babka (SUSE) wrote:
>> > It has been noted that free_to_pcs_bulk() is difficult to follow, with a
>> > number of goto labels, and this has contributed to two memory leak bugs
>> > in there.
>> > 
>> > Extract part of the code to __free_to_pcs_batch(), which focuses only on
>> > freeing free-hook-processed local objects to a percpu sheaf, and
>> > returning how many were freed. Zero means a trylock failure or no empty
>> > sheaf available, and thus the caller should fallback to
>> > __kmem_cache_free_bulk().
>> > 
>> > Make free_to_pcs_bulk() call this in a while loop, removing all goto
>> > labels from the function. __free_to_pcs_batch() retains two rather
>> > straightforward ones.
>> > 
>> > Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
>> 
>> Nice simplification!
>> 
>> [...]
>> > +/*
>> > + * Bulk free objects to the percpu sheaves.
>> > + * Unlike free_to_pcs() this includes the calls to all necessary hooks
>> > + * and the fallback to freeing to slab pages.
>> > + */
>> > +static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
>> > +{
>> > +	bool init = slab_want_init_on_free(s);
>> > +	void *remote_objects[PCS_BATCH_MAX];
>> > +	unsigned int remote_nr = 0;
>> > +
>> > +	for (unsigned int i = 0; i < size;) {
>> > +		struct slab *slab = virt_to_slab(p[i]);
>> > +
>> > +		memcg_slab_free_hook(s, slab, p + i, 1);
>> > +		alloc_tagging_slab_free_hook(s, slab, p + i, 1);
>> > +
>> > +		if (unlikely(!slab_free_hook(s, p[i], init, false))) {
>> > +			p[i] = p[--size];
>> > +			continue;
>> > +		}
>> > +
>> > +		if (unlikely(!can_free_to_pcs(slab))) {
>> > +			remote_objects[remote_nr] = p[i];
>> > +			p[i] = p[--size];
>> > +			if (++remote_nr >= PCS_BATCH_MAX) {
>> > +				__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
>> > +				stat_add(s, FREE_SLOWPATH, remote_nr);
>> > +				remote_nr = 0;
>> > +			}
>> > +			continue;
>> > +		}
>> > +
>> > +		i++;
>> >  	}
>> >  
>> > -	if (remote_nr)
>> > -		goto flush_remote;
>> > +	while (size) {
>> > +		unsigned int batch_freed = __free_to_pcs_batch(s, size, p);
>> >  
>> > -	return;
>> > +		if (!batch_freed)
>> > +			break;
>> >  
>> > -no_empty:
>> > -	local_unlock(&s->cpu_sheaves->lock);
>> > +		p += batch_freed;
>> > +		size -= batch_freed;
>> > +	}
>> >  
>> > -	/*
>> > -	 * if we depleted all empty sheaves in the barn or there are too
>> > -	 * many full sheaves, free the rest to slab pages
>> > -	 */
>> > -fallback:
>> > -	__kmem_cache_free_bulk(s, size, p);
>> > -	stat_add(s, FREE_SLOWPATH, size);
>> > +	if (size) {
>> > +		__kmem_cache_free_bulk(s, size, p);
>> > +		stat_add(s, FREE_SLOWPATH, size);
>> > +	}
>> 
>> By the way, could we move this directly into the loop inside the
>> `if (!batch_freed)` block to make it a bit more concise? but it's totally fine
>> to leave it as is! :)

Ah right, great suggestion! will do.

> oops, forgot to add the r-b tag just now :P
> 
> Reviewed-by: Hao Li <hao.li@linux.dev>

Thanks!

>> 
>> >  
>> > -flush_remote:
>> >  	if (remote_nr) {
>> >  		__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
>> >  		stat_add(s, FREE_SLOWPATH, remote_nr);
>> > 
>> > ---
>> > base-commit: 72bb229f9161a1efcd5df32141b69fcc6ae81a13
>> > change-id: 20260707-slab-simplify-bulk-pcs-a8d0478feef6
>> > 
>> > Best regards,
>> > --  
>> > Vlastimil Babka (SUSE) <vbabka@kernel.org>
>> > 
> 

Re: [PATCH] mm/slab: extract __free_to_pcs_batch() from free_to_pcs_bulk()

[Harry Yoo]

[-- Attachment #1.1: Type: text/plain, Size: 3286 bytes --]



On 7/10/26 6:54 PM, Vlastimil Babka (SUSE) wrote:
> On 7/10/26 07:21, Hao Li wrote:
>> On Fri, Jul 10, 2026 at 01:18:25PM +0800, Hao Li wrote:
>>> On Tue, Jul 07, 2026 at 02:16:00PM +0200, Vlastimil Babka (SUSE) wrote:
>>>> It has been noted that free_to_pcs_bulk() is difficult to follow, with a
>>>> number of goto labels, and this has contributed to two memory leak bugs
>>>> in there.
>>>>
>>>> Extract part of the code to __free_to_pcs_batch(), which focuses only on
>>>> freeing free-hook-processed local objects to a percpu sheaf, and
>>>> returning how many were freed. Zero means a trylock failure or no empty
>>>> sheaf available, and thus the caller should fallback to
>>>> __kmem_cache_free_bulk().
>>>>
>>>> Make free_to_pcs_bulk() call this in a while loop, removing all goto
>>>> labels from the function. __free_to_pcs_batch() retains two rather
>>>> straightforward ones.
>>>>
>>>> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@kernel.org>
>>>
>>> Nice simplification!
>>>
>>> [...]
>>>> +/*
>>>> + * Bulk free objects to the percpu sheaves.
>>>> + * Unlike free_to_pcs() this includes the calls to all necessary hooks
>>>> + * and the fallback to freeing to slab pages.
>>>> + */
>>>> +static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
>>>> +{
>>>> +	bool init = slab_want_init_on_free(s);
>>>> +	void *remote_objects[PCS_BATCH_MAX];
>>>> +	unsigned int remote_nr = 0;
>>>> +
>>>> +	for (unsigned int i = 0; i < size;) {
>>>> +		struct slab *slab = virt_to_slab(p[i]);
>>>> +
>>>> +		memcg_slab_free_hook(s, slab, p + i, 1);
>>>> +		alloc_tagging_slab_free_hook(s, slab, p + i, 1);
>>>> +
>>>> +		if (unlikely(!slab_free_hook(s, p[i], init, false))) {
>>>> +			p[i] = p[--size];
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		if (unlikely(!can_free_to_pcs(slab))) {
>>>> +			remote_objects[remote_nr] = p[i];
>>>> +			p[i] = p[--size];
>>>> +			if (++remote_nr >= PCS_BATCH_MAX) {
>>>> +				__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
>>>> +				stat_add(s, FREE_SLOWPATH, remote_nr);
>>>> +				remote_nr = 0;
>>>> +			}
>>>> +			continue;
>>>> +		}
>>>> +
>>>> +		i++;
>>>>  	}
>>>>  
>>>> -	if (remote_nr)
>>>> -		goto flush_remote;
>>>> +	while (size) {
>>>> +		unsigned int batch_freed = __free_to_pcs_batch(s, size, p);
>>>>  
>>>> -	return;
>>>> +		if (!batch_freed)
>>>> +			break;
>>>>  
>>>> -no_empty:
>>>> -	local_unlock(&s->cpu_sheaves->lock);
>>>> +		p += batch_freed;
>>>> +		size -= batch_freed;
>>>> +	}
>>>>  
>>>> -	/*
>>>> -	 * if we depleted all empty sheaves in the barn or there are too
>>>> -	 * many full sheaves, free the rest to slab pages
>>>> -	 */
>>>> -fallback:
>>>> -	__kmem_cache_free_bulk(s, size, p);
>>>> -	stat_add(s, FREE_SLOWPATH, size);
>>>> +	if (size) {
>>>> +		__kmem_cache_free_bulk(s, size, p);
>>>> +		stat_add(s, FREE_SLOWPATH, size);
>>>> +	}
>>>
>>> By the way, could we move this directly into the loop inside the
>>> `if (!batch_freed)` block to make it a bit more concise? but it's totally fine
>>> to leave it as is! :)
> 
> Ah right, great suggestion! will do.

Including the suggested change:

Reviewed-by: Harry Yoo (Oracle) <harry@kernel.org>

-- 
Cheers,
Harry / Hyeonggon

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]