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@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! :)

Ah right, great suggestion! will do.

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

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