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

[Harry Yoo <harry.yoo@oracle.com>]

On Thu, Oct 30, 2025 at 04:35:52PM +0100, Vlastimil Babka wrote:
> On 10/30/25 16:27, Alexei Starovoitov wrote:
> > On Thu, Oct 30, 2025 at 6:09 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> >>
> >> On 10/30/25 05:32, Harry Yoo wrote:
> >> > On Thu, Oct 23, 2025 at 03:52:32PM +0200, Vlastimil Babka wrote:
> >> >> diff --git a/mm/slub.c b/mm/slub.c
> >> >> index e2b052657d11..bd67336e7c1f 100644
> >> >> --- a/mm/slub.c
> >> >> +++ b/mm/slub.c
> >> >> @@ -4790,66 +4509,15 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> >> >>
> >> >>      stat(s, ALLOC_SLAB);
> >> >>
> >> >> -    if (IS_ENABLED(CONFIG_SLUB_TINY) || kmem_cache_debug(s)) {
> >> >> -            freelist = alloc_single_from_new_slab(s, slab, orig_size, gfpflags);
> >> >> -
> >> >> -            if (unlikely(!freelist))
> >> >> -                    goto new_objects;
> >> >> -
> >> >> -            if (s->flags & SLAB_STORE_USER)
> >> >> -                    set_track(s, freelist, TRACK_ALLOC, addr,
> >> >> -                              gfpflags & ~(__GFP_DIRECT_RECLAIM));
> >> >> -
> >> >> -            return freelist;
> >> >> -    }
> >> >> -
> >> >> -    /*
> >> >> -     * No other reference to the slab yet so we can
> >> >> -     * muck around with it freely without cmpxchg
> >> >> -     */
> >> >> -    freelist = slab->freelist;
> >> >> -    slab->freelist = NULL;
> >> >> -    slab->inuse = slab->objects;
> >> >> -    slab->frozen = 1;
> >> >> -
> >> >> -    inc_slabs_node(s, slab_nid(slab), slab->objects);
> >> >> +    freelist = alloc_single_from_new_slab(s, slab, orig_size, gfpflags);
> >> >>
> >> >> -    if (unlikely(!pfmemalloc_match(slab, gfpflags) && allow_spin)) {
> >> >> -            /*
> >> >> -             * For !pfmemalloc_match() case we don't load freelist so that
> >> >> -             * we don't make further mismatched allocations easier.
> >> >> -             */
> >> >> -            deactivate_slab(s, slab, get_freepointer(s, freelist));
> >> >> -            return freelist;
> >> >> -    }
> >> >> +    if (unlikely(!freelist))
> >> >> +            goto new_objects;
> >> >
> >> > We may end up in an endless loop in !allow_spin case?
> >> > (e.g., kmalloc_nolock() is called in NMI context and n->list_lock is
> >> > held in the process context on the same CPU)
> >> >
> >> > Allocate a new slab, but somebody is holding n->list_lock, so trylock fails,
> >> > free the slab, goto new_objects, and repeat.
> >>
> >> Ugh, yeah. However, AFAICS this possibility already exists prior to this
> >> patch, only it's limited to SLUB_TINY/kmem_cache_debug(s). But we should fix
> >> it in 6.18 then.

Oops, right ;)

> >> How? Grab the single object and defer deactivation of the slab minus one
> >> object? Would work except for kmem_cache_debug(s) we open again a race for
> >> inconsistency check failure, and we have to undo the simple slab freeing fix
> >>  and handle the accounting issue differently again.

> >> Fail the allocation for the debug case to avoid the consistency check
> >> issues? Would it be acceptable for kmalloc_nolock() users?

I think this should work (and is simple)!

> > You mean something like:
> > diff --git a/mm/slub.c b/mm/slub.c
> > index a8fcc7e6f25a..e9a8b75f31d7 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -4658,8 +4658,11 @@ static void *___slab_alloc(struct kmem_cache
> > *s, gfp_t gfpflags, int node,
> >         if (kmem_cache_debug(s)) {
> >                 freelist = alloc_single_from_new_slab(s, slab,
> > orig_size, gfpflags);
> > 
> > -               if (unlikely(!freelist))
> > +               if (unlikely(!freelist)) {
> > +                       if (!allow_spin)
> > +                               return NULL;
> >                         goto new_objects;
> > +               }
> > 
> > or I misunderstood the issue?
> 
> Yeah that would be the easiest solution, if you can accept the occasional
> allocation failures.

Looks good to me.

-- 
Cheers,
Harry / Hyeonggon