The function is tricky and many of its tests are hard to understand. Try
to improve that by using more descriptively named variables and added
comments.
- rename 'prior' to 'old_head' to match the head and tail parameters
- introduce a 'bool was_full' to make it more obvious what we are
testing instead of the !prior and prior tests
- add or improve comments in various places to explain what we're doing
Also replace kmem_cache_has_cpu_partial() tests with
IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) which are compile-time constants.
We can do that because the kmem_cache_debug(s) case is handled upfront
via free_to_partial_list().
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/slub.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 45 insertions(+), 17 deletions(-)
diff --git a/mm/slub.c b/mm/slub.c
index f1a5373eee7b..074abe8e79f8 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5859,8 +5859,8 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
unsigned long addr)
{
- void *prior;
- int was_frozen;
+ void *old_head;
+ bool was_frozen, was_full;
struct slab new;
unsigned long counters;
struct kmem_cache_node *n = NULL;
@@ -5874,20 +5874,37 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
return;
}
+ /*
+ * It is enough to test IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) below
+ * instead of kmem_cache_has_cpu_partial(s), because kmem_cache_debug(s)
+ * is the only other reason it can be false, and it is already handled
+ * above.
+ */
+
do {
if (unlikely(n)) {
spin_unlock_irqrestore(&n->list_lock, flags);
n = NULL;
}
- prior = slab->freelist;
+ old_head = slab->freelist;
counters = slab->counters;
- set_freepointer(s, tail, prior);
+ set_freepointer(s, tail, old_head);
new.counters = counters;
- was_frozen = new.frozen;
+ was_frozen = !!new.frozen;
+ was_full = (old_head == NULL);
new.inuse -= cnt;
- if ((!new.inuse || !prior) && !was_frozen) {
- /* Needs to be taken off a list */
- if (!kmem_cache_has_cpu_partial(s) || prior) {
+ /*
+ * Might need to be taken off (due to becoming empty) or added
+ * to (due to not being full anymore) the partial list.
+ * Unless it's frozen.
+ */
+ if ((!new.inuse || was_full) && !was_frozen) {
+ /*
+ * If slab becomes non-full and we have cpu partial
+ * lists, we put it there unconditionally to avoid
+ * taking the list_lock. Otherwise we need it.
+ */
+ if (!(IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) && was_full)) {
n = get_node(s, slab_nid(slab));
/*
@@ -5905,7 +5922,7 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
}
} while (!slab_update_freelist(s, slab,
- prior, counters,
+ old_head, counters,
head, new.counters,
"__slab_free"));
@@ -5917,7 +5934,7 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
* activity can be necessary.
*/
stat(s, FREE_FROZEN);
- } else if (kmem_cache_has_cpu_partial(s) && !prior) {
+ } else if (IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) && was_full) {
/*
* If we started with a full slab then put it onto the
* per cpu partial list.
@@ -5926,6 +5943,11 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
stat(s, CPU_PARTIAL_FREE);
}
+ /*
+ * In other cases we didn't take the list_lock because the slab
+ * was already on the partial list and will remain there.
+ */
+
return;
}
@@ -5933,19 +5955,24 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
* This slab was partially empty but not on the per-node partial list,
* in which case we shouldn't manipulate its list, just return.
*/
- if (prior && !on_node_partial) {
+ if (!was_full && !on_node_partial) {
spin_unlock_irqrestore(&n->list_lock, flags);
return;
}
+ /*
+ * If slab became empty, should we add/keep it on the partial list or we
+ * have enough?
+ */
if (unlikely(!new.inuse && n->nr_partial >= s->min_partial))
goto slab_empty;
/*
* Objects left in the slab. If it was not on the partial list before
- * then add it.
+ * then add it. This can only happen when cache has no per cpu partial
+ * list otherwise we would have put it there.
*/
- if (!kmem_cache_has_cpu_partial(s) && unlikely(!prior)) {
+ if (!IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) && unlikely(was_full)) {
add_partial(n, slab, DEACTIVATE_TO_TAIL);
stat(s, FREE_ADD_PARTIAL);
}
@@ -5953,10 +5980,11 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
return;
slab_empty:
- if (prior) {
- /*
- * Slab on the partial list.
- */
+ /*
+ * The slab could have a single object and thus go from full to empty in
+ * a single free, but more likely it was on the partial list. Remove it.
+ */
+ if (likely(!was_full)) {
remove_partial(n, slab);
stat(s, FREE_REMOVE_PARTIAL);
}
--
2.51.1On Wed, Nov 05, 2025 at 10:05:29AM +0100, Vlastimil Babka wrote: > The function is tricky and many of its tests are hard to understand. Try > to improve that by using more descriptively named variables and added > comments. > > - rename 'prior' to 'old_head' to match the head and tail parameters > - introduce a 'bool was_full' to make it more obvious what we are > testing instead of the !prior and prior tests Yeah I recall these were cryptic when I was analyzing slab few years ago :) > - add or improve comments in various places to explain what we're doing > > Also replace kmem_cache_has_cpu_partial() tests with > IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) which are compile-time constants. > > We can do that because the kmem_cache_debug(s) case is handled upfront > via free_to_partial_list(). This makes sense. By the way, should we also check IS_ENABLED(CONFIG_SLUB_TINY) in kmem_cache_has_cpu_partial()? > Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > --- The code is much cleaner! Reviewed-by: Harry Yoo <harry.yoo@oracle.com> -- Cheers, Harry / Hyeonggon
On 11/6/25 09:26, Harry Yoo wrote: > On Wed, Nov 05, 2025 at 10:05:29AM +0100, Vlastimil Babka wrote: >> The function is tricky and many of its tests are hard to understand. Try >> to improve that by using more descriptively named variables and added >> comments. >> >> - rename 'prior' to 'old_head' to match the head and tail parameters >> - introduce a 'bool was_full' to make it more obvious what we are >> testing instead of the !prior and prior tests > > Yeah I recall these were cryptic when I was analyzing slab few years > ago :) > >> - add or improve comments in various places to explain what we're doing >> >> Also replace kmem_cache_has_cpu_partial() tests with >> IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) which are compile-time constants. >> >> We can do that because the kmem_cache_debug(s) case is handled upfront >> via free_to_partial_list(). > > This makes sense. By the way, should we also check IS_ENABLED(CONFIG_SLUB_TINY) > in kmem_cache_has_cpu_partial()? If you really mean testing CONFIG_SLUB_TINY then it's not necessary because CONFIG_SLUB_CPU_PARTIAL depends on !TINY. If you mean using IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) instead of the #ifdef, that could be possible, just out of scope here. And hopefully will be gone fully, so no point in polishing at this point. Unlike __slab_free() which stays. >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> >> --- > > The code is much cleaner! > > Reviewed-by: Harry Yoo <harry.yoo@oracle.com> >
On Thu, Nov 06, 2025 at 09:43:24AM +0100, Vlastimil Babka wrote: > On 11/6/25 09:26, Harry Yoo wrote: > > On Wed, Nov 05, 2025 at 10:05:29AM +0100, Vlastimil Babka wrote: > >> The function is tricky and many of its tests are hard to understand. Try > >> to improve that by using more descriptively named variables and added > >> comments. > >> > >> - rename 'prior' to 'old_head' to match the head and tail parameters > >> - introduce a 'bool was_full' to make it more obvious what we are > >> testing instead of the !prior and prior tests > > > > Yeah I recall these were cryptic when I was analyzing slab few years > > ago :) > > > >> - add or improve comments in various places to explain what we're doing > >> > >> Also replace kmem_cache_has_cpu_partial() tests with > >> IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) which are compile-time constants. > >> > >> We can do that because the kmem_cache_debug(s) case is handled upfront > >> via free_to_partial_list(). > > > > This makes sense. By the way, should we also check IS_ENABLED(CONFIG_SLUB_TINY) > > in kmem_cache_has_cpu_partial()? > > If you really mean testing CONFIG_SLUB_TINY then it's not necessary because > CONFIG_SLUB_CPU_PARTIAL depends on !TINY. I really meant this and yeah I missed that! > If you mean using IS_ENABLED(CONFIG_SLUB_CPU_PARTIAL) instead of the #ifdef, > that could be possible, just out of scope here. And hopefully will be gone > fully, so no point in polishing at this point. Unlike __slab_free() which stays. Agreed. > >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz> > >> --- > > > > The code is much cleaner! > > > > Reviewed-by: Harry Yoo <harry.yoo@oracle.com> -- Cheers, Harry / Hyeonggon
© 2016 - 2025 Red Hat, Inc.