[RFC PATCH 1/2] mm/slub: make the case handling in __slab_free() easier to follow

There are 7 possible transitions in __slab_free():

  a. partial->partial
  b. partial->empty, offlist
  c. partial->empty, onlist, exceeding min_partial
  d. partial->empty, onlist, not exceeding min_partial
  e. full->empty, exceeding min_partial
  f. full->empty, not exceeding min_partial
  g. full->partial

(There is no offlist variant of e, f and g as a full slab is on no
list.)

Clarify which case each branch handles, and replace the goto with a
return at the end of the skipped block so that every branch explicitly
states its coverage.

Case 'a' is the only path that needs neither list_lock nor list
handling. Give it an early continue: handling it upfront is much clearer
than forcing every other case into a nested block.

Also, read SL_partial once after the loop right where it is used, rather
than re-reading it on every iteration.

No functional change.

Signed-off-by: Hao Li <[email protected]>
---
 mm/slub.c | 95 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 49 insertions(+), 46 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index b0cd0572e2f2..e20375307770 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -5748,76 +5748,79 @@ static void __slab_free(struct kmem_cache *s, struct slab *slab,
 		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.
-		 * Unless it's frozen.
+		 * partial->partial: the slab was on the node partial list and
+		 * stays there, so we need no list handling and no list_lock.
+		 *
+		 * Note that continue in a do-while goes on to evaluate the
+		 * condition below, so we do perform the freelist update.
 		 */
-		if (!new.inuse || was_full) {
-
-			n = get_node(s, slab_nid(slab));
-			/*
-			 * Speculatively acquire the list_lock.
-			 * If the cmpxchg does not succeed then we may
-			 * drop the list_lock without any processing.
-			 *
-			 * Otherwise the list_lock will synchronize with
-			 * other processors updating the list of slabs.
-			 */
-			spin_lock_irqsave(&n->list_lock, flags);
+		if (!was_full && new.inuse)
+			continue;
 
-			on_node_partial = slab_test_node_partial(slab);
-		}
+		/*
+		 * The slab might need to be taken off (due to becoming empty)
+		 * or added to (due to not being full anymore) the partial
+		 * list.
+		 *
+		 * Speculatively acquire list_lock before calling cmpxchg(), as
+		 * performing cmpxchg() prior to lock acquisition races with
+		 * concurrent paths, such as the shrinker.
+		 *
+		 * If the cmpxchg does not succeed then we will drop the
+		 * list_lock and retry.
+		 */
+		n = get_node(s, slab_nid(slab));
+		spin_lock_irqsave(&n->list_lock, flags);
 
 	} while (!slab_update_freelist(s, slab, &old, &new, "__slab_free"));
 
 	if (likely(!n)) {
+		/* partial->partial: we didn't take the list_lock */
+		return;
+	}
+
+	on_node_partial = slab_test_node_partial(slab);
+
+	if (!was_full && !on_node_partial) {
 		/*
-		 * We didn't take the list_lock because the slab was already on
-		 * the partial list and will remain there.
+		 * partial->empty, offlist: a bulk refill has taken the slab
+		 * off the partial list and will put it back, so its list
+		 * handling is not ours to do.
 		 */
+		spin_unlock_irqrestore(&n->list_lock, flags);
 		return;
 	}
 
-	/*
-	 * 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 (!was_full && !on_node_partial) {
+	/* full/partial->empty, exceed: we have enough partial slabs already */
+	if (unlikely(!new.inuse && n->nr_partial >= s->min_partial)) {
+		/* partial->empty, onlist, exceed */
+		if (likely(!was_full)) {
+			remove_partial(n, slab);
+			stat(s, FREE_REMOVE_PARTIAL);
+		}
+		/* full->empty, exceed: it is on no list to remove from */
+
 		spin_unlock_irqrestore(&n->list_lock, flags);
+		stat(s, FREE_SLAB);
+		discard_slab(s, slab);
 		return;
 	}
 
 	/*
-	 * If slab became empty, should we add/keep it on the partial list or we
-	 * have enough?
+	 * At this point, only three cases remain:
+	 *   full->partial
+	 *   full->empty, not exceed
+	 *   partial->empty, onlist, not exceed
 	 */
-	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.
-	 */
+	/* full->partial; full->empty, not exceed */
 	if (unlikely(was_full)) {
 		add_partial(n, slab, ADD_TO_TAIL);
 		stat(s, FREE_ADD_PARTIAL);
 	}
-	spin_unlock_irqrestore(&n->list_lock, flags);
-	return;
-
-slab_empty:
-	/*
-	 * 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);
-	}
+	/* partial->empty, onlist, not exceed: it stays where it is */
 
 	spin_unlock_irqrestore(&n->list_lock, flags);
-	stat(s, FREE_SLAB);
-	discard_slab(s, slab);
 }
 
 /*
-- 
2.55.0