Re: [PATCH RFC 10/19] slab: remove cpu (partial) slabs usage from allocation paths

[Chris Mason <clm@meta.com>]

On 1/9/26 3:16 AM, Vlastimil Babka wrote:
> On 10/24/25 16:29, Chris Mason wrote:
>> On Thu, 23 Oct 2025 15:52:32 +0200 Vlastimil Babka <vbabka@suse.cz> wrote:

[ ... ]

> By the way, there was another bug in this patch, causing a severe memory
> leak, which the AI unfortunately didn't flag. Petr reported it during
> performance testing and it took me more than a day to find it. Oh well :)
> 
> Wonder if things got better since then perhaps, and your or Roman's tools
> would find it today? :)

Yes and no.  It didn't find the leak until I changed the prompt to say:
"there is a leak, find it".  I'll see if I can improve things...

> diff --git a/mm/slub.c b/mm/slub.c
> --- a/mm/slub.c
> +++ b/mm/slub.c

[ ... ]

> @@ -3592,54 +3549,53 @@ static void *get_partial_node(struct kmem_cache *s,
>  	else if (!spin_trylock_irqsave(&n->list_lock, flags))
>  		return NULL;
>  	list_for_each_entry_safe(slab, slab2, &n->partial, slab_list) {
> +
> +		struct freelist_counters old, new;
> +
>  		if (!pfmemalloc_match(slab, pc->flags))
>  			continue;
>
>  		if (IS_ENABLED(CONFIG_SLUB_TINY) || kmem_cache_debug(s)) {
> -			void *object = alloc_single_from_partial(s, n, slab,
> +			object = alloc_single_from_partial(s, n, slab,
>  							pc->orig_size);
> -			if (object) {
> -				partial = slab;
> -				pc->object = object;
> +			if (object)
>  				break;
> -			}
>  			continue;
>  		}
>
> +		/*
> +		 * get a single object from the slab. This might race against
> +		 * __slab_free(), which however has to take the list_lock if
> +		 * it's about to make the slab fully free.
> +		 */
> +		do {
> +			old.freelist = slab->freelist;
> +			old.counters = slab->counters;
> +
> +			new.freelist = get_freepointer(s, old.freelist);
> +			new.counters = old.counters;
> +			new.inuse++;
> +
> +		} while (!__slab_update_freelist(s, slab, &old, &new, "get_partial_node"));
> +
> +		object = old.freelist;
> +		if (!new.freelist)
> +			remove_partial(n, slab);

Is there a missing break statement here? The debug path above breaks out
of the loop after successfully allocating an object, but this non-debug
path continues iterating through the partial list. Each iteration overwrites
the object variable, so previously allocated objects would be leaked.

The commit message says "Now we only want to return a single object" which
matches the debug path behavior, but the non-debug path appears to allocate
from every matching slab in the list.

> 	}
>  	spin_unlock_irqrestore(&n->list_lock, flags);
> -	return partial;
> +	return object;
>  }