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

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

[JaeJoon Jung]

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.
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;
-
-	} 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

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

[Harry Yoo]

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

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

[JaeJoon Jung]

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

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

[David Hildenbrand]

On 10.09.25 04:31, JaeJoon Jung wrote:
> 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.

For the future, please

(1) Don't clean up code you don't fully understand

(2) Test your patches before submission

-- 
Cheers

David / dhildenb

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

[syzbot ci]

syzbot ci has tested the following series

[v1] mm/slub: Removing unnecessary variable accesses in the get_freelist()
https://lore.kernel.org/all/20250910005957.54108-1-rgbi3307@gmail.com
* [PATCH] mm/slub: Removing unnecessary variable accesses in the get_freelist()

and found the following issue:
kernel BUG in __smpboot_create_thread

Full report is available here:
https://ci.syzbot.org/series/832d6128-4556-4bd1-ab18-0676b8e7dbc1

***

kernel BUG in __smpboot_create_thread

tree:      torvalds
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/torvalds/linux
base:      f777d1112ee597d7f7dd3ca232220873a34ad0c8
arch:      amd64
compiler:  Debian clang version 20.1.8 (++20250708063551+0c9f909b7976-1~exp1~20250708183702.136), Debian LLD 20.1.8
config:    https://ci.syzbot.org/builds/7bff7792-4edd-4680-a5e7-3cc9d97ac5ed/config

smpboot: CPU0: Intel QEMU Virtual CPU version 2.5+ (family: 0xf, model: 0x6b, stepping: 0x1)
Performance Events: unsupported Netburst CPU model 107 no PMU driver, software events only.
signal: max sigframe size: 1440
------------[ cut here ]------------
kernel BUG at mm/slub.c:3832!
Oops: invalid opcode: 0000 [#1] SMP KASAN PTI
CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:___slab_alloc+0x13ab/0x1420
Code: ff ff 4c 89 e7 48 c7 c6 3a a1 a0 8d e8 3e 4f 0f ff 90 0f 0b 90 0f 0b 48 89 f7 48 c7 c6 3a a1 a0 8d e8 29 4f 0f ff 90 0f 0b 90 <0f> 0b 90 0f 0b 4c 89 e7 48 c7 c6 3a a1 a0 8d e8 11 4f 0f ff 90 0f
RSP: 0000:ffffc90000047750 EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff888100028000 RCX: 0000000000800001
RDX: 0000000000800001 RSI: ffffffff8dba8965 RDI: ffffffff8be33980
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff82189e55
R10: dffffc0000000000 R11: fffffbfff1f47407 R12: ffff88804b03e480
R13: ffff88801a441640 R14: ffff88804b03e460 R15: ffffffff82189e55
FS:  0000000000000000(0000) GS:ffff8880b8615000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff88813ffff000 CR3: 000000000df36000 CR4: 00000000000006f0
Call Trace:
 <TASK>
 __kmalloc_cache_node_noprof+0x29a/0x3d0
 __smpboot_create_thread+0x11a/0x3b0
 smpboot_register_percpu_thread+0xc6/0x420
 spawn_ksoftirqd+0x2f/0xc0
 do_one_initcall+0x236/0x820
 do_pre_smp_initcalls+0x4c/0x80
 kernel_init_freeable+0x30c/0x4b0
 kernel_init+0x1d/0x1d0
 ret_from_fork+0x3fc/0x770
 ret_from_fork_asm+0x1a/0x30
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:___slab_alloc+0x13ab/0x1420
Code: ff ff 4c 89 e7 48 c7 c6 3a a1 a0 8d e8 3e 4f 0f ff 90 0f 0b 90 0f 0b 48 89 f7 48 c7 c6 3a a1 a0 8d e8 29 4f 0f ff 90 0f 0b 90 <0f> 0b 90 0f 0b 4c 89 e7 48 c7 c6 3a a1 a0 8d e8 11 4f 0f ff 90 0f
RSP: 0000:ffffc90000047750 EFLAGS: 00010046
RAX: 0000000000000000 RBX: ffff888100028000 RCX: 0000000000800001
RDX: 0000000000800001 RSI: ffffffff8dba8965 RDI: ffffffff8be33980
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffffff82189e55
R10: dffffc0000000000 R11: fffffbfff1f47407 R12: ffff88804b03e480
R13: ffff88801a441640 R14: ffff88804b03e460 R15: ffffffff82189e55
FS:  0000000000000000(0000) GS:ffff8880b8615000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff88813ffff000 CR3: 000000000df36000 CR4: 00000000000006f0


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.