Re: [PATCH v2 6/6] slab: Introduce kmalloc_nolock() and kfree_nolock().

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Thu, Jul 10, 2025 at 2:36 AM Vlastimil Babka <vbabka@suse.cz> wrote:
> > +     if (unlikely(!allow_spin)) {
> > +             folio = (struct folio *)alloc_frozen_pages_nolock(0/* __GFP_COMP is implied */,
> > +                                                               node, order);
> > +     } else if (node == NUMA_NO_NODE)
> >               folio = (struct folio *)alloc_frozen_pages(flags, order);
> >       else
> >               folio = (struct folio *)__alloc_frozen_pages(flags, order, node, NULL);
>
> Nit: should use { } either for everything or nothing (seems your new branch
> would work without them)

leftover from v1. will fix.

> >                       stat(s, ALLOC_NODE_MISMATCH);
> > @@ -3730,7 +3762,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> >        * PFMEMALLOC but right now, we are losing the pfmemalloc
> >        * information when the page leaves the per-cpu allocator
> >        */
> > -     if (unlikely(!pfmemalloc_match(slab, gfpflags)))
> > +     if (unlikely(!pfmemalloc_match(slab, gfpflags) && allow_spin))
> >               goto deactivate_slab;
> >
> >       /* must check again c->slab in case we got preempted and it changed */
> > @@ -3803,7 +3835,12 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> >               slub_set_percpu_partial(c, slab);
> >
> >               if (likely(node_match(slab, node) &&
> > -                        pfmemalloc_match(slab, gfpflags))) {
> > +                        pfmemalloc_match(slab, gfpflags)) ||
> > +                 /*
> > +                  * Reentrant slub cannot take locks necessary
> > +                  * for __put_partials(), hence downgrade to any node
> > +                  */
> > +                 !allow_spin) {
>
> Uh this seems rather ugly, I'd move the comment above everything. Also it's
> not "downgrade" as when you assign NUMA_NO_NODE earlier, I'd say "ignore the
> preference".
> Note that it would be bad to ignore with __GFP_THISNODE but then it's not
> allowed for kmalloc_nolock() so that's fine.

Yes. All correct. Will reword.

> > @@ -3911,6 +3953,12 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> >               void *flush_freelist = c->freelist;
> >               struct slab *flush_slab = c->slab;
> >
> > +             if (unlikely(!allow_spin))
> > +                     /*
> > +                      * Reentrant slub cannot take locks
> > +                      * necessary for deactivate_slab()
> > +                      */
> > +                     return NULL;
>
> Hm but this is leaking the slab we allocated and have in the "slab"
> variable, we need to free it back in that case.
>
> >               c->slab = NULL;
> >               c->freelist = NULL;
> >               c->tid = next_tid(c->tid);
>
> > @@ -4593,10 +4792,31 @@ static __always_inline void do_slab_free(struct kmem_cache *s,
> >       barrier();
> >
> >       if (unlikely(slab != c->slab)) {
> > -             __slab_free(s, slab, head, tail, cnt, addr);
> > +             /* cnt == 0 signals that it's called from kfree_nolock() */
> > +             if (unlikely(!cnt)) {
> > +                     /*
> > +                      * __slab_free() can locklessly cmpxchg16 into a slab,
> > +                      * but then it might need to take spin_lock or local_lock
> > +                      * in put_cpu_partial() for further processing.
> > +                      * Avoid the complexity and simply add to a deferred list.
> > +                      */
> > +                     defer_free(head);
> > +             } else {
> > +                     __slab_free(s, slab, head, tail, cnt, addr);
> > +             }
> >               return;
> >       }
> >
> > +     if (unlikely(!cnt)) {
> > +             if ((in_nmi() || !USE_LOCKLESS_FAST_PATH()) &&
> > +                 local_lock_is_locked(&s->cpu_slab->lock)) {
> > +                     defer_free(head);
> > +                     return;
> > +             }
> > +             cnt = 1;
>
> Hmm we might end up doing a "goto redo" later and then do the wrong thing above?

Great catch. Will fix. That's two serious bugs.
That's my penalty for reviewing other people code 99% of the time
and little time to code myself.