Re: [PATCH for-next-fixes v2 2/5] mm/slab: handle allow_spin in slab_free_hook() instead of open coding

[Harry Yoo]

On Wed, Nov 04, 2026 at 02:54:13PM +0100, Vlastimil Babka (SUSE) wrote:
> On 11/3/26 15:47, Harry Yoo (Meta) wrote:
> > kfree_nolock() deliberately skips slab free hooks that should not be
> > called in unknown context. However, there is one more place that
> > needs the same thing: memcg_alloc_abort_single().
> > 
> > Instead of open coding slab_free_hook(), let it handle the allow_spin
> > parameter. This is required to correctly handle allocation failures due
> > to memcg charging.
> > 
> > There are two functional changes from this.
> > 
> > First, memcg_alloc_abort_single() -> slab_free_hook() path no longer
> > calls free hooks that acquire spinlocks if !allow_spin.
> 
> Hm, since this is only changed by patch 4, I don't think it needs mentioning
> here, as it's not any functional change yet.

Good point, will adjust.

> > Second, kfree_nolock() now follows init_on_free which has been ignored.
> > 
> > Besides that, no functional change intended.
> > 
> > Closes: https://lore.kernel.org/asiSUpAYYU8Od4HM@xxxxxxxxx
> > Fixes: af92793e52c3 ("slab: Introduce kmalloc_nolock() and kfree_nolock().")
> > Signed-off-by: Harry Yoo (Meta) <harry@xxxxxxxxxx>
> > ---
> >  mm/slub.c | 104 +++++++++++++++++++++++++++++++++-----------------------------
> >  1 file changed, 56 insertions(+), 48 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 45e077c7526c..36e7bb93418a 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2686,41 +2686,54 @@ struct rcu_delayed_free {
> >   * - memcg_slab_post_alloc_hook()
> >   * - alloc_tagging_slab_alloc_hook()
> >   *
> > + * slab_free_hook() assumes that memcg_slab_free_hook() and
> > + * alloc_tagging_slab_free_hook() are already invoked.
> > + *
> >   * Free hooks that must handle missing corresponding alloc hooks:
> >   * - kmemleak_free_recursive()
> >   * - kfence_free()
> >   *
> > - * Free hooks that have no alloc hook counterpart, and thus safe to call:
> > + * Free hooks that have no alloc hook counterpart:
> >   * - debug_check_no_locks_freed()
> >   * - debug_check_no_obj_freed()
> >   * - __kcsan_check_access()
> >   */
> >  static __always_inline
> >  bool slab_free_hook(struct kmem_cache *s, void *x, bool init,
> > -		    bool after_rcu_delay)
> > +		    bool after_rcu_delay, bool allow_spin)
> 
> Hm 3 bools. Could we bite the bullet and try doing free_flags (extended to
> cover init and after_rcu_delay) here already? :) Given it's not intended for
> a stable backport.

Ack, will try to bite the bullet.

> > @@ -2776,8 +2790,11 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init,
> >  		set_orig_size(s, x, orig_size);
> >  
> >  	}
> > -	/* KASAN might put x into memory quarantine, delaying its reuse. */
> > -	return !kasan_slab_free(s, x, init, still_accessible, false);
> > +	/*
> > +	 * KASAN might put x into memory quarantine, delaying its reuse.
> > +	 * Skip quarantine when spinning is not allowed as it uses a spinlock.
> > +	 */
> > +	return !kasan_slab_free(s, x, init, still_accessible, !allow_spin);
> >  }
> >  
> >  static __fastpath_inline
> > @@ -2789,9 +2806,10 @@ bool slab_free_freelist_hook(struct kmem_cache *s, void **head, void **tail,
> >  	void *next = *head;
> >  	void *old_tail = *tail;
> >  	bool init;
> > +	bool allow_spin = true;
> 
> Then we wouldn't need tricks like this I guess?

Right.

-- 
Cheers,
Harry / Hyeonggon