[PATCH 0/4] slab: cmpxchg cleanups enabled by -fms-extensions

[PATCH 0/4] slab: cmpxchg cleanups enabled by -fms-extensions

[Vlastimil Babka]

After learning about -fms-extensions being enabled for 6.19, I realized
there is some cleanup potential in slub code by extending the definition
and usage of freelist_aba_t, as it can now become an unnamed member of
struct slab. This series performs the cleanup, with no functional
changes intended. Additionally we turn freelist_aba_t to struct
freelist_counters as it doesn't meet any criteria for being a typedef,
per Documentation/process/coding-style.rst

It is based on:
- slab/for-next-fixes
- this cleanup patch https://patch.msgid.link/20251103-fix-nolock-loop-v1-1-6e2b3e82b9da@suse.cz
  (first of another series) to avoid conflicts in __slab_free()
- merge 'kbuild-ms-extensions-6.19' of git://git.kernel.org/pub/scm/linux/kernel/git/kbuild/linux

All of that is provided here:
https://git.kernel.org/pub/scm/linux/kernel/git/vbabka/linux.git/log/?h=b4/slab-fms-cleanup

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
Vlastimil Babka (4):
      slab: separate struct freelist_tid from kmem_cache_cpu
      slab: turn freelist_aba_t to a struct and fully define counters there
      slab: use struct freelist_counters for local variables instead of struct slab
      slab: use struct freelist_counters as parameters in relevant functions

 mm/slab.h |  52 ++++++++++-----------
 mm/slub.c | 155 ++++++++++++++++++++++++++++----------------------------------
 2 files changed, 93 insertions(+), 114 deletions(-)
---
base-commit: c77d09928c398df0556e8ef659b4235559e0a374
change-id: 20251107-slab-fms-cleanup-8e1cc5f5ebd4

Best regards,
-- 
Vlastimil Babka <vbabka@suse.cz>

[PATCH 1/4] slab: separate struct freelist_tid from kmem_cache_cpu

[Vlastimil Babka]

In kmem_cache_cpu we currently have a union of the freelist+tid pair
with freelist_aba_t, relying implicitly on the type compatibility with the
freelist+counters pair used in freelist_aba_t.

To allow further changes to freelist_aba_t, we can instead define a
separate struct freelist_tid (instead of a typedef, per the coding
style) for kmem_cache_cpu, as that affects only a single helper
__update_cpu_freelist_fast().

We can add the resulting struct freelist_tid to kmem_cache_cpu as
unnamed field thanks to -fms-extensions, so that freelist and tid fields
can still be accessed directly.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slub.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 074abe8e79f8..5f6408c9e0fd 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -411,18 +411,22 @@ enum stat_item {
 };
 
 #ifndef CONFIG_SLUB_TINY
-/*
- * When changing the layout, make sure freelist and tid are still compatible
- * with this_cpu_cmpxchg_double() alignment requirements.
- */
-struct kmem_cache_cpu {
+struct freelist_tid {
 	union {
 		struct {
-			void **freelist;	/* Pointer to next available object */
+			void *freelist;		/* Pointer to next available object */
 			unsigned long tid;	/* Globally unique transaction id */
 		};
-		freelist_aba_t freelist_tid;
+		freelist_full_t freelist_tid;
 	};
+};
+
+/*
+ * When changing the layout, make sure freelist and tid are still compatible
+ * with this_cpu_cmpxchg_double() alignment requirements.
+ */
+struct kmem_cache_cpu {
+	struct freelist_tid;
 	struct slab *slab;	/* The slab from which we are allocating */
 #ifdef CONFIG_SLUB_CPU_PARTIAL
 	struct slab *partial;	/* Partially allocated slabs */
@@ -4367,11 +4371,11 @@ __update_cpu_freelist_fast(struct kmem_cache *s,
 			   void *freelist_old, void *freelist_new,
 			   unsigned long tid)
 {
-	freelist_aba_t old = { .freelist = freelist_old, .counter = tid };
-	freelist_aba_t new = { .freelist = freelist_new, .counter = next_tid(tid) };
+	struct freelist_tid old = { .freelist = freelist_old, .tid = tid };
+	struct freelist_tid new = { .freelist = freelist_new, .tid = next_tid(tid) };
 
-	return this_cpu_try_cmpxchg_freelist(s->cpu_slab->freelist_tid.full,
-					     &old.full, new.full);
+	return this_cpu_try_cmpxchg_freelist(s->cpu_slab->freelist_tid,
+					     &old.freelist_tid, new.freelist_tid);
 }
 
 /*

-- 
2.51.1

[PATCH 2/4] slab: turn freelist_aba_t to a struct and fully define counters there

[Vlastimil Babka]

In struct slab we currently have freelist and counters pair, where
counters itself is a union of unsigned long with a sub-struct of
several smaller fields. Then for the usage with double cmpxchg we have
freelist_aba_t that duplicates the definition of the freelist+counters
with implicitly the same layout as the full definition in struct slab.

Thanks to -fms-extension we can now move the full counters definition to
freelist_aba_t (while changing it to struct freelist_counters as a
typedef is unnecessary and discouraged) and replace the relevant part in
struct slab to an unnamed reference to it.

The immediate benefit is the removal of duplication and no longer
relying on the same layout implicitly. It also allows further cleanups
thanks to having the full definition of counters in struct
freelist_counters.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slab.h | 52 ++++++++++++++++++++++++----------------------------
 mm/slub.c |  8 +++++---
 2 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/mm/slab.h b/mm/slab.h
index 078daecc7cf5..42627b87d50c 100644
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -40,13 +40,29 @@ typedef u64 freelist_full_t;
  * Freelist pointer and counter to cmpxchg together, avoids the typical ABA
  * problems with cmpxchg of just a pointer.
  */
-typedef union {
-	struct {
-		void *freelist;
-		unsigned long counter;
+struct freelist_counters {
+	union {
+		struct {
+			void *freelist;
+			union {
+				unsigned long counters;
+				struct {
+					unsigned inuse:16;
+					unsigned objects:15;
+					/*
+					 * If slab debugging is enabled then the
+					 * frozen bit can be reused to indicate
+					 * that the slab was corrupted
+					 */
+					unsigned frozen:1;
+				};
+			};
+		};
+#ifdef system_has_freelist_aba
+		freelist_full_t freelist_counters;
+#endif
 	};
-	freelist_full_t full;
-} freelist_aba_t;
+};
 
 /* Reuses the bits in struct page */
 struct slab {
@@ -69,27 +85,7 @@ struct slab {
 #endif
 			};
 			/* Double-word boundary */
-			union {
-				struct {
-					void *freelist;		/* first free object */
-					union {
-						unsigned long counters;
-						struct {
-							unsigned inuse:16;
-							unsigned objects:15;
-							/*
-							 * If slab debugging is enabled then the
-							 * frozen bit can be reused to indicate
-							 * that the slab was corrupted
-							 */
-							unsigned frozen:1;
-						};
-					};
-				};
-#ifdef system_has_freelist_aba
-				freelist_aba_t freelist_counter;
-#endif
-			};
+			struct freelist_counters;
 		};
 		struct rcu_head rcu_head;
 	};
@@ -114,7 +110,7 @@ SLAB_MATCH(_unused_slab_obj_exts, obj_exts);
 #undef SLAB_MATCH
 static_assert(sizeof(struct slab) <= sizeof(struct page));
 #if defined(system_has_freelist_aba)
-static_assert(IS_ALIGNED(offsetof(struct slab, freelist), sizeof(freelist_aba_t)));
+static_assert(IS_ALIGNED(offsetof(struct slab, freelist), sizeof(struct freelist_counters)));
 #endif
 
 /**
diff --git a/mm/slub.c b/mm/slub.c
index 5f6408c9e0fd..8330e4f8b3b2 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -764,10 +764,12 @@ __update_freelist_fast(struct slab *slab,
 		      void *freelist_new, unsigned long counters_new)
 {
 #ifdef system_has_freelist_aba
-	freelist_aba_t old = { .freelist = freelist_old, .counter = counters_old };
-	freelist_aba_t new = { .freelist = freelist_new, .counter = counters_new };
+	struct freelist_counters old = { .freelist = freelist_old, .counters = counters_old };
+	struct freelist_counters new = { .freelist = freelist_new, .counters = counters_new };
 
-	return try_cmpxchg_freelist(&slab->freelist_counter.full, &old.full, new.full);
+	return try_cmpxchg_freelist(&slab->freelist_counters,
+				    &old.freelist_counters,
+				    new.freelist_counters);
 #else
 	return false;
 #endif

-- 
2.51.1

[PATCH 3/4] slab: use struct freelist_counters for local variables instead of struct slab

[Vlastimil Babka]

In several functions we declare local struct slab variables so we can
work with the freelist and counters fields (including the sub-counters
that are in the union) comfortably.

With struct freelist_counters containing the full counters definition,
we can now reduce the local variables to that type as we don't need the
other fields in struct slab.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slub.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 8330e4f8b3b2..a55e0af26ec7 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3725,8 +3725,7 @@ static void deactivate_slab(struct kmem_cache *s, struct slab *slab,
 	void *nextfree, *freelist_iter, *freelist_tail;
 	int tail = DEACTIVATE_TO_HEAD;
 	unsigned long flags = 0;
-	struct slab new;
-	struct slab old;
+	struct freelist_counters old, new;
 
 	if (READ_ONCE(slab->freelist)) {
 		stat(s, DEACTIVATE_REMOTE_FREES);
@@ -4390,7 +4389,7 @@ __update_cpu_freelist_fast(struct kmem_cache *s,
  */
 static inline void *get_freelist(struct kmem_cache *s, struct slab *slab)
 {
-	struct slab new;
+	struct freelist_counters new;
 	unsigned long counters;
 	void *freelist;
 
@@ -4418,7 +4417,7 @@ static inline void *get_freelist(struct kmem_cache *s, struct slab *slab)
  */
 static inline void *freeze_slab(struct kmem_cache *s, struct slab *slab)
 {
-	struct slab new;
+	struct freelist_counters new;
 	unsigned long counters;
 	void *freelist;
 
@@ -5867,7 +5866,7 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
 {
 	void *old_head;
 	bool was_frozen, was_full;
-	struct slab new;
+	struct freelist_counters new;
 	unsigned long counters;
 	struct kmem_cache_node *n = NULL;
 	unsigned long flags;

-- 
2.51.1

[PATCH 4/4] slab: use struct freelist_counters as parameters in relevant functions

[Vlastimil Babka]

In functions such as [__]slab_update_freelist() and
__slab_update_freelist_fast/slow() we pass old and new freelist and
counters as 4 separate parameters. The underlying
__update_freelist_fast() then constructs struct freelist_counters
variables for passing the full freelist+counter combinations to cmpxchg
double.

In most cases we actually start with struct freelist_counters variables,
but then pass the individual fields, only to construct new struct
freelist_counters variables. While it's all inlined and thus should be
efficient, we can simplify this code.

Thus replace the 4 parameters for individual fields with two
freelist_aba_t pointers wherever applicable. __update_freelist_fast()
can then pass them directly to try_cmpxchg_freelist().

The code is also more obvious as the pattern becomes unified such that
we set up "old" and "new" struct freelist_counters variables upfront as
we fully need them to be, and simply call [__]slab_update_freelist() on
them.  Previously some of the "new" values would be hidden as one of the
many parameters and thus make it harder to figure out what the code
does.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/slub.c | 126 ++++++++++++++++++++++++++------------------------------------
 1 file changed, 52 insertions(+), 74 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index a55e0af26ec7..ddd71f4937fa 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -759,34 +759,29 @@ static __always_inline void slab_unlock(struct slab *slab)
 }
 
 static inline bool
-__update_freelist_fast(struct slab *slab,
-		      void *freelist_old, unsigned long counters_old,
-		      void *freelist_new, unsigned long counters_new)
+__update_freelist_fast(struct slab *slab, struct freelist_counters *old,
+		       struct freelist_counters *new)
 {
 #ifdef system_has_freelist_aba
-	struct freelist_counters old = { .freelist = freelist_old, .counters = counters_old };
-	struct freelist_counters new = { .freelist = freelist_new, .counters = counters_new };
-
 	return try_cmpxchg_freelist(&slab->freelist_counters,
-				    &old.freelist_counters,
-				    new.freelist_counters);
+				    &old->freelist_counters,
+				    new->freelist_counters);
 #else
 	return false;
 #endif
 }
 
 static inline bool
-__update_freelist_slow(struct slab *slab,
-		      void *freelist_old, unsigned long counters_old,
-		      void *freelist_new, unsigned long counters_new)
+__update_freelist_slow(struct slab *slab, struct freelist_counters *old,
+		       struct freelist_counters *new)
 {
 	bool ret = false;
 
 	slab_lock(slab);
-	if (slab->freelist == freelist_old &&
-	    slab->counters == counters_old) {
-		slab->freelist = freelist_new;
-		slab->counters = counters_new;
+	if (slab->freelist == old->freelist &&
+	    slab->counters == old->counters) {
+		slab->freelist = new->freelist;
+		slab->counters = new->counters;
 		ret = true;
 	}
 	slab_unlock(slab);
@@ -802,22 +797,18 @@ __update_freelist_slow(struct slab *slab,
  * interrupt the operation.
  */
 static inline bool __slab_update_freelist(struct kmem_cache *s, struct slab *slab,
-		void *freelist_old, unsigned long counters_old,
-		void *freelist_new, unsigned long counters_new,
-		const char *n)
+		struct freelist_counters *old, struct freelist_counters *new, const char *n)
 {
 	bool ret;
 
 	if (USE_LOCKLESS_FAST_PATH())
 		lockdep_assert_irqs_disabled();
 
-	if (s->flags & __CMPXCHG_DOUBLE) {
-		ret = __update_freelist_fast(slab, freelist_old, counters_old,
-				            freelist_new, counters_new);
-	} else {
-		ret = __update_freelist_slow(slab, freelist_old, counters_old,
-				            freelist_new, counters_new);
-	}
+	if (s->flags & __CMPXCHG_DOUBLE)
+		ret = __update_freelist_fast(slab, old, new);
+	else
+		ret = __update_freelist_slow(slab, old, new);
+
 	if (likely(ret))
 		return true;
 
@@ -832,21 +823,17 @@ static inline bool __slab_update_freelist(struct kmem_cache *s, struct slab *sla
 }
 
 static inline bool slab_update_freelist(struct kmem_cache *s, struct slab *slab,
-		void *freelist_old, unsigned long counters_old,
-		void *freelist_new, unsigned long counters_new,
-		const char *n)
+		struct freelist_counters *old, struct freelist_counters *new, const char *n)
 {
 	bool ret;
 
 	if (s->flags & __CMPXCHG_DOUBLE) {
-		ret = __update_freelist_fast(slab, freelist_old, counters_old,
-				            freelist_new, counters_new);
+		ret = __update_freelist_fast(slab, old, new);
 	} else {
 		unsigned long flags;
 
 		local_irq_save(flags);
-		ret = __update_freelist_slow(slab, freelist_old, counters_old,
-				            freelist_new, counters_new);
+		ret = __update_freelist_slow(slab, old, new);
 		local_irq_restore(flags);
 	}
 	if (likely(ret))
@@ -3774,10 +3761,7 @@ static void deactivate_slab(struct kmem_cache *s, struct slab *slab,
 		} else {
 			new.freelist = old.freelist;
 		}
-	} while (!slab_update_freelist(s, slab,
-		old.freelist, old.counters,
-		new.freelist, new.counters,
-		"unfreezing slab"));
+	} while (!slab_update_freelist(s, slab, &old, &new, "unfreezing slab"));
 
 	/*
 	 * Stage three: Manipulate the slab list based on the updated state.
@@ -4389,27 +4373,24 @@ __update_cpu_freelist_fast(struct kmem_cache *s,
  */
 static inline void *get_freelist(struct kmem_cache *s, struct slab *slab)
 {
-	struct freelist_counters new;
-	unsigned long counters;
-	void *freelist;
+	struct freelist_counters old, new;
 
 	lockdep_assert_held(this_cpu_ptr(&s->cpu_slab->lock));
 
 	do {
-		freelist = slab->freelist;
-		counters = slab->counters;
+		old.freelist = slab->freelist;
+		old.counters = slab->counters;
 
-		new.counters = counters;
+		new.freelist = NULL;
+		new.counters = old.counters;
 
-		new.inuse = slab->objects;
-		new.frozen = freelist != NULL;
+		new.inuse = old.objects;
+		new.frozen = old.freelist != NULL;
 
-	} while (!__slab_update_freelist(s, slab,
-		freelist, counters,
-		NULL, new.counters,
-		"get_freelist"));
 
-	return freelist;
+	} while (!__slab_update_freelist(s, slab, &old, &new, "get_freelist"));
+
+	return old.freelist;
 }
 
 /*
@@ -4417,26 +4398,22 @@ static inline void *get_freelist(struct kmem_cache *s, struct slab *slab)
  */
 static inline void *freeze_slab(struct kmem_cache *s, struct slab *slab)
 {
-	struct freelist_counters new;
-	unsigned long counters;
-	void *freelist;
+	struct freelist_counters old, new;
 
 	do {
-		freelist = slab->freelist;
-		counters = slab->counters;
+		old.freelist = slab->freelist;
+		old.counters = slab->counters;
 
-		new.counters = counters;
+		new.freelist = NULL;
+		new.counters = old.counters;
 		VM_BUG_ON(new.frozen);
 
-		new.inuse = slab->objects;
+		new.inuse = old.objects;
 		new.frozen = 1;
 
-	} while (!slab_update_freelist(s, slab,
-		freelist, counters,
-		NULL, new.counters,
-		"freeze_slab"));
+	} while (!slab_update_freelist(s, slab, &old, &new, "freeze_slab"));
 
-	return freelist;
+	return old.freelist;
 }
 
 /*
@@ -5864,10 +5841,8 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
 			unsigned long addr)
 
 {
-	void *old_head;
 	bool was_frozen, was_full;
-	struct freelist_counters new;
-	unsigned long counters;
+	struct freelist_counters old, new;
 	struct kmem_cache_node *n = NULL;
 	unsigned long flags;
 	bool on_node_partial;
@@ -5891,13 +5866,19 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
 			spin_unlock_irqrestore(&n->list_lock, flags);
 			n = NULL;
 		}
-		old_head = slab->freelist;
-		counters = slab->counters;
-		set_freepointer(s, tail, old_head);
-		new.counters = counters;
-		was_frozen = !!new.frozen;
-		was_full = (old_head == NULL);
+
+		old.freelist = slab->freelist;
+		old.counters = slab->counters;
+
+		was_full = (old.freelist == NULL);
+		was_frozen = old.frozen;
+
+		set_freepointer(s, tail, old.freelist);
+
+		new.freelist = head;
+		new.counters = old.counters;
 		new.inuse -= cnt;
+
 		/*
 		 * Might need to be taken off (due to becoming empty) or added
 		 * to (due to not being full anymore) the partial list.
@@ -5926,10 +5907,7 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
 			}
 		}
 
-	} while (!slab_update_freelist(s, slab,
-		old_head, counters,
-		head, new.counters,
-		"__slab_free"));
+	} while (!slab_update_freelist(s, slab, &old, &new, "__slab_free"));
 
 	if (likely(!n)) {
 

-- 
2.51.1

Re: [PATCH 1/4] slab: separate struct freelist_tid from kmem_cache_cpu

[Harry Yoo]

On Fri, Nov 07, 2025 at 02:51:23PM +0100, Vlastimil Babka wrote:
> In kmem_cache_cpu we currently have a union of the freelist+tid pair
> with freelist_aba_t, relying implicitly on the type compatibility with the
> freelist+counters pair used in freelist_aba_t.
> 
> To allow further changes to freelist_aba_t, we can instead define a
> separate struct freelist_tid (instead of a typedef, per the coding
> style) for kmem_cache_cpu, as that affects only a single helper
> __update_cpu_freelist_fast().
> 
> We can add the resulting struct freelist_tid to kmem_cache_cpu as
> unnamed field thanks to -fms-extensions, so that freelist and tid fields
> can still be accessed directly.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---

Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH 2/4] slab: turn freelist_aba_t to a struct and fully define counters there

[Harry Yoo]

On Fri, Nov 07, 2025 at 02:51:24PM +0100, Vlastimil Babka wrote:
> In struct slab we currently have freelist and counters pair, where
> counters itself is a union of unsigned long with a sub-struct of
> several smaller fields. Then for the usage with double cmpxchg we have
> freelist_aba_t that duplicates the definition of the freelist+counters
> with implicitly the same layout as the full definition in struct slab.
> 
> Thanks to -fms-extension we can now move the full counters definition to
> freelist_aba_t (while changing it to struct freelist_counters as a
> typedef is unnecessary and discouraged) and replace the relevant part in
> struct slab to an unnamed reference to it.
> 
> The immediate benefit is the removal of duplication and no longer
> relying on the same layout implicitly. It also allows further cleanups
> thanks to having the full definition of counters in struct
> freelist_counters.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---

Nice cleanup!

Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH 3/4] slab: use struct freelist_counters for local variables instead of struct slab

[Harry Yoo]

On Fri, Nov 07, 2025 at 02:51:25PM +0100, Vlastimil Babka wrote:
> In several functions we declare local struct slab variables so we can
> work with the freelist and counters fields (including the sub-counters
> that are in the union) comfortably.
> 
> With struct freelist_counters containing the full counters definition,
> we can now reduce the local variables to that type as we don't need the
> other fields in struct slab.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---

Yeah I've also been thinking there should be a type for the counters
instead of declaring struct slab variables like this.

Cheers to -fms-extensions! :)

Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH 4/4] slab: use struct freelist_counters as parameters in relevant functions

[Harry Yoo]

On Fri, Nov 07, 2025 at 02:51:26PM +0100, Vlastimil Babka wrote:
> In functions such as [__]slab_update_freelist() and
> __slab_update_freelist_fast/slow() we pass old and new freelist and
> counters as 4 separate parameters. The underlying
> __update_freelist_fast() then constructs struct freelist_counters
> variables for passing the full freelist+counter combinations to cmpxchg
> double.
> 
> In most cases we actually start with struct freelist_counters variables,
> but then pass the individual fields, only to construct new struct
> freelist_counters variables. While it's all inlined and thus should be
> efficient, we can simplify this code.
> 
> Thus replace the 4 parameters for individual fields with two
> freelist_aba_t pointers wherever applicable. __update_freelist_fast()

nit: I guess you meant struct freelist_counters pointers, because
freelist_aba_t is gone.

> can then pass them directly to try_cmpxchg_freelist().
> 
> The code is also more obvious as the pattern becomes unified such that
> we set up "old" and "new" struct freelist_counters variables upfront as
> we fully need them to be, and simply call [__]slab_update_freelist() on
> them.  Previously some of the "new" values would be hidden as one of the
> many parameters and thus make it harder to figure out what the code
> does.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---

Nice!

Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH 4/4] slab: use struct freelist_counters as parameters in relevant functions

[Vlastimil Babka]

On 11/13/25 09:32, Harry Yoo wrote:
> On Fri, Nov 07, 2025 at 02:51:26PM +0100, Vlastimil Babka wrote:
>> In functions such as [__]slab_update_freelist() and
>> __slab_update_freelist_fast/slow() we pass old and new freelist and
>> counters as 4 separate parameters. The underlying
>> __update_freelist_fast() then constructs struct freelist_counters
>> variables for passing the full freelist+counter combinations to cmpxchg
>> double.
>> 
>> In most cases we actually start with struct freelist_counters variables,
>> but then pass the individual fields, only to construct new struct
>> freelist_counters variables. While it's all inlined and thus should be
>> efficient, we can simplify this code.
>> 
>> Thus replace the 4 parameters for individual fields with two
>> freelist_aba_t pointers wherever applicable. __update_freelist_fast()
> 
> nit: I guess you meant struct freelist_counters pointers, because
> freelist_aba_t is gone.

Yeah, stale text from before removing freelist_aba_t, will update, thanks.

>> can then pass them directly to try_cmpxchg_freelist().
>> 
>> The code is also more obvious as the pattern becomes unified such that
>> we set up "old" and "new" struct freelist_counters variables upfront as
>> we fully need them to be, and simply call [__]slab_update_freelist() on
>> them.  Previously some of the "new" values would be hidden as one of the
>> many parameters and thus make it harder to figure out what the code
>> does.
>> 
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> ---
> 
> Nice!
> 
> Looks good to me,
> Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

Thanks a lot for all your reviews!