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

[Hao Li <hao.li@linux.dev>]

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