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

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

-- 
Thanks,
Hao