Re: [PATCH for-next-fixes v2 4/5] mm/slab: fix a deadlock in memcg_alloc_abort_single()

[Harry Yoo]

On Tue, Nov 03, 2026 at 06:40:09PM +0000, Alexei Starovoitov wrote:
> On Tue, Nov 03, 2026 at 06:47 AM Harry Yoo (Meta) <harry@xxxxxxxxxx> wrote:
> >  	if (likely(size == 1)) {
> > -		memcg_alloc_abort_single(s, *p);
> > +		bool allow_spin = alloc_flags_allow_spinning(ac->alloc_flags);
> > +
> > +		memcg_alloc_abort_single(s, *p, allow_spin);
> >  		*p = NULL;
> >  	} else {
> >  		kmem_cache_free_bulk(s, size, p);
> 
> can we call kfree_nolock(*p) here when spinning is not allowed ?
> Karl's patch did that. You replied that memcg_slab_free_hook() should
> not see an object that is not charged:
> https://lore.kernel.org/all/asaAwg1KeZoEYd4K@thinkpad/
> It's fine. __memcg_slab_free_hook() skips an object with objcg == NULL.
> The else branch relies on that. kmem_cache_free_bulk() also gets
> the objects that are not charged yet.

While this is technically true, this relies too much on the current
implementation details and makes the code hard to follow.

> > +	if (likely(allow_spin))
> >  		__slab_free(s, slab, object, object, 1, _RET_IP_);
> > +	else
> > +		defer_free(s, object);
> 
> That's an irq_work interrupt per failed charge.

irq_work overhead should not matter by the time memcg fails charging.
the kernel can't reclaim memory in unknown context and allocations
won't succeed anyway once we hit the limit (unless somebody else
reclaims memory for us). We have a bigger problem there.

> kfree_nolock() tries free_to_pcs() first. It calls defer_free() only
> when that fails.
> 
> Then no need for allow_spin in memcg_alloc_abort_single(),
> and the fix doesn't depend on patch 2.

Patch 2 is an independent bugfix as-is already. We should really stop
open-coding slab_free_hook(). That ended up missing init_on_free handing.
 
> Something like this (compile tested only):
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 45e077c..fb92b48 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2551,7 +2551,10 @@ bool memcg_slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags,
>  		return true;
> 
>  	if (likely(size == 1)) {
> -		memcg_alloc_abort_single(s, *p);
> +		if (likely(alloc_flags_allow_spinning(ac->alloc_flags)))
> +			memcg_alloc_abort_single(s, *p);
> +		else
> +			kfree_nolock(*p);
>  		*p = NULL;
>  	} else {
>  		kmem_cache_free_bulk(s, size, p);

-- 
Cheers,
Harry / Hyeonggon