mm/slub.c | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-)
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.0syzbot 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.
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
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
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
© 2016 - 2025 Red Hat, Inc.