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@xxxxxxxxxx>

--
With Best Regards,
Shengming

> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>