Re: [PATCH] mm/slub: Removing unnecessary variable accesses in the get_freelist()

[JaeJoon Jung <rgbi3307@gmail.com>]

Hi, Harry Hyeonggon Yoo,

Thank you for your kind and detailed reply.

I hadn't thought of the union data type.

Since the counters, inuse, and frozen members are unioned within the
slab structure,
there is the problem that the above values are not applied to
__slab_update_freelist() and try_cmpxchg_freelist() if the existing
code is not followed.

I canceled my patch request.
I apologized for my inconvenience.

On Wed, 10 Sept 2025 at 10:39, Harry Yoo <harry.yoo@oracle.com> wrote:
>
> Hi Jaejoon,
>
> I updated my email from 42.hyeyoo@gmail.com to harry.yoo@oracle.com
> a while ago. Please check up-to-date MAINTAINERS file when sending a patch.
>
> On Wed, Sep 10, 2025 at 09:59:56AM +0900, JaeJoon Jung wrote:
> > It pass a NULL pointer to the freelist_new variable
> > in the __slab_update_freelist() function so that it don't have to re-fetch
> > the variable values inside the while loop.
>
> No, it needs to re-fetch values when cmpxchg fails.
> Otherwise it would fall into an infinite loop, no?
>
> at a high level overview, cmpxchg works like this (atomically, of course):
>
> retry:
>     old = var;
>     // modify some bits in 'old' and store it to 'new'
>     new = old + something;
>     if (var == old) { // compare
>          var = new; // exchange if the value is expected
>     } else {
>         // if var != old, someone else updated the variable. retry
>         goto retry;
>     }
>
> and this retry will certainly fail if you don't you re-fetch the value,
> modify it, and try cmpxchg again. The 'old' value fetched before failing
> cmpxchg will not match anymore because other CPUs already updated that
> variable.
>
> > Removing unnecessary variable accesses as shown below
> > will reduce the code size of the get_freelist() function and make it faster.
> >
> > Signed-off-by: JaeJoon Jung <rgbi3307@gmail.com>
> > ---
> >  mm/slub.c | 21 ++++-----------------
> >  1 file changed, 4 insertions(+), 17 deletions(-)
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index d257141896c9..2e305a17a9d7 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -3654,27 +3654,14 @@ __update_cpu_freelist_fast(struct kmem_cache *s,
> >   */
> >  static inline void *get_freelist(struct kmem_cache *s, struct slab *slab)
> >  {
> > -     struct slab new;
> > -     unsigned long counters;
> > -     void *freelist;
> > -
> >       lockdep_assert_held(this_cpu_ptr(&s->cpu_slab->lock));
> >
> > -     do {
> > -             freelist = slab->freelist;
> > -             counters = slab->counters;
> > -
> > -             new.counters = counters;
> > -
> > -             new.inuse = slab->objects;
> > -             new.frozen = freelist != NULL;
>
> ...and the frozen and inuse bits are part of counters field,
> so they are not updated anymore?
>
> > -
> > -     } while (!__slab_update_freelist(s, slab,
> > -             freelist, counters,
> > -             NULL, new.counters,
> > +     while (!__slab_update_freelist(s, slab,
> > +             slab->freelist, slab->counters,
> > +             NULL, slab->counters,
> >               "get_freelist"));
> >
> > -     return freelist;
> > +     return slab->freelist;
> >  }
> >
> >  /*
> > --
> > 2.43.0
>
> --
> Cheers,
> Harry / Hyeonggon