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

[Harry Yoo]

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

Including the suggested change:

Reviewed-by: Harry Yoo (Oracle) <harry@xxxxxxxxxx>

-- 
Cheers,
Harry / Hyeonggon

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature