On Thu, May 8, 2025 at 6:03 PM Harry Yoo <harry.yoo@xxxxxxxxxx> wrote:
> > + s = kmalloc_slab(size, NULL, alloc_gfp, _RET_IP_);
> > +
> > + if (!(s->flags & __CMPXCHG_DOUBLE))
> > + /*
> > + * kmalloc_nolock() is not supported on architectures that
> > + * don't implement cmpxchg16b.
> > + */
> > + return NULL;
>
> Hmm when someone uses slab debugging flags (e.g., passing boot
> parameter slab_debug=FPZ as a hardening option on production [1], or
> just for debugging), __CMPXCHG_DOUBLE is not set even when the arch
> supports it.
>
> Is it okay to fail all kmalloc_nolock() calls in such cases?
I studied the code and the git history.
Looks like slub doesn't have to disable cmpxchg mode when slab_debug is on.
The commit 41bec7c33f37 ("mm/slub: remove slab_lock() usage for debug
operations")
removed slab_lock from debug validation checks.
So right now slab_lock() only serializes slab->freelist/counter update.
It's still necessary on arch-s that don't have cmpxchg, but that's it.
Only __update_freelist_slow() is using it.
folio_lock() is the same lock, but PG_locked is there for different
reasons. 10+ years ago PG_locked bit was reused for slab serialization.
Today it's unnecessary to do so. Abusing the same bit for
freelist/counter updates doesn't provide any better slab debugging.
The comment next to SLAB_NO_CMPXCHG is obsolete as well.
It's been there since days that slab_lock() was taken during
consistency checks. Removing misuse of PG_locked is a good thing on
its own.
I think the following diff is appropriate:
diff --git a/mm/slub.c b/mm/slub.c
index 044e43ee3373..9d615cfd1b6f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -286,14 +286,6 @@ static inline bool
kmem_cache_has_cpu_partial(struct kmem_cache *s)
#define DEBUG_DEFAULT_FLAGS (SLAB_CONSISTENCY_CHECKS | SLAB_RED_ZONE | \
SLAB_POISON | SLAB_STORE_USER)
-/*
- * These debug flags cannot use CMPXCHG because there might be consistency
- * issues when checking or reading debug information
- */
-#define SLAB_NO_CMPXCHG (SLAB_CONSISTENCY_CHECKS | SLAB_STORE_USER | \
- SLAB_TRACE)
-
-
/*
* Debugging flags that require metadata to be stored in the slab. These get
* disabled when slab_debug=O is used and a cache's min order increases with
@@ -6654,7 +6646,7 @@ int do_kmem_cache_create(struct kmem_cache *s,
const char *name,
}
#ifdef system_has_freelist_aba
- if (system_has_freelist_aba() && !(s->flags & SLAB_NO_CMPXCHG)) {
+ if (system_has_freelist_aba()) {
/* Enable fast mode */
s->flags |= __CMPXCHG_DOUBLE;
}
It survived my stress tests.
Thoughts?
