Re: [PATCH slab/for-next] mm/slab: simplify freeing remote objects in free_to_pcs_bulk()

[Pedro Falcato]

On Fri, Jul 10, 2026 at 02:04:58PM +0200, Vlastimil Babka (SUSE) wrote:
> We have been moving remote objects to an on-stack array and flushing it
> when full. Instead, we can swap them towards the beginning of the
> supplied array and bulk-free it just once.
> 
> Also add a comment to explain the rationale of freeing remote objects
> last, because now it would appear to be simpler to free them first.
> 
> Signed-off-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>
> ---
> One more cleanup I realized it's possible when processing feedback to
> the previous one [1]
> 
> [1] https://lore.kernel.org/all/20260707-slab-simplify-bulk-pcs-v1-1-4850dbe0d904@xxxxxxxxxx/
> ---
>  mm/slub.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 22045dc919ef..966d2fb114e8 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -6255,7 +6255,7 @@ static unsigned int __free_to_pcs_batch(struct kmem_cache *s, size_t size, void
>  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];
> +	void **remote_objects = p;
>  	unsigned int remote_nr = 0;
>  

Maybe explaining what magic you're pulling off would be useful in the code
as well.

>  	for (unsigned int i = 0; i < size;) {
> @@ -6270,19 +6270,17 @@ static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
>  		}
>  
>  		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;
> +			if (i != remote_nr)
> +				swap(remote_objects[remote_nr], p[i]);
> +			remote_nr++;
>  		}
>  
>  		i++;
>  	}
>  
> +	p += remote_nr;
> +	size -= remote_nr;
> +
>  	while (size) {
>  		unsigned int batch_freed = __free_to_pcs_batch(s, size, p);
>  
> @@ -6296,8 +6294,12 @@ static void free_to_pcs_bulk(struct kmem_cache *s, size_t size, void **p)
>  		size -= batch_freed;
>  	}
>  
> +	/*
> +	 * Processing remote objects last decreases the chances of cpu migration
> +	 * while freeing to sheaves and compromising object locality
> +	 */
>  	if (remote_nr) {
> -		__kmem_cache_free_bulk(s, remote_nr, &remote_objects[0]);
> +		__kmem_cache_free_bulk(s, remote_nr, remote_objects);
>  		stat_add(s, FREE_SLOWPATH, remote_nr);
>  	}
>  }

All-in-all, LGTM

Reviewed-by: Pedro Falcato <pfalcato@xxxxxxx>

> 
> ---
> base-commit: e1fa26489025d6deac76d1dbfe2e0720a3ad84b1
> change-id: 20260710-bulk_free_remote-7fbe6f3abf5d
> 
> Best regards to everyone but Pedro (btw this is not hard-coded,
> there's a config option for this),

I know there's a config option to change your signature but I haven't
been able to simply omit this bit :|

Anyway, I get a stray, you get a stray review. Nice trade.

-- 
Pedro