Re: [PATCH v2 1/7] mm: list_lru: lock_list_lru_of_memcg() cannot return NULL if !skip_empty

[Johannes Weiner]

On Wed, Mar 18, 2026 at 10:56:55AM -0700, Shakeel Butt wrote:
> On Thu, Mar 12, 2026 at 04:51:49PM -0400, Johannes Weiner wrote:
> > skip_empty is only for the shrinker to abort and skip a list that's
> > empty or whose cgroup is being deleted.
> > 
> > For list additions and deletions, the cgroup hierarchy is walked
> > upwards until a valid list_lru head is found, or it will fall back to
> > the node list. Acquiring the lock won't fail. Remove the NULL checks
> > in those callers.
> > 
> > Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> > ---
> 
> What do you think about squashing the following into this patch?
> 
> From bd56ea4505f792e00079b1a8dd98cb6f7a5e7215 Mon Sep 17 00:00:00 2001
> From: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> Date: Wed, 18 Mar 2026 10:43:53 -0700
> Subject: [PATCH] list_lru: cleanup
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>

Thanks for taking a look!

There is some overlap and conflict between your delta and what later
patches in the series do.

AFAICS, the main thing left over would be: to have
__lock_list_lru_of_memcg() for the reclaimer (which does not walk the
parents during a cgroup deletion race) and lock_list_lru_of_memcg()
which does. Thereby eliminating the @skip_empty bool. The downside
would be to have another level in the lock function stack which is
duplicated for CONFIG_MEMCG and !CONFIG_MEMCG, and the !CONFIG_MEMCG
versions are identical.

I'm not sure that's worth it?

---
 mm/list_lru.c | 50 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/mm/list_lru.c b/mm/list_lru.c
index 1ccdd45b1d14..cab716d94ac5 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -83,13 +83,12 @@ list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
 }
 
 static inline struct list_lru_one *
-lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
-		       bool irq, unsigned long *irq_flags, bool skip_empty)
+__lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
+			 bool irq, unsigned long *irq_flags)
 {
 	struct list_lru_one *l;
 
 	rcu_read_lock();
-again:
 	l = list_lru_from_memcg_idx(lru, nid, memcg_kmem_id(memcg));
 	if (likely(l)) {
 		lock_list_lru(l, irq, irq_flags);
@@ -99,18 +98,24 @@ lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
 		}
 		unlock_list_lru(l, irq, irq_flags);
 	}
-	/*
-	 * Caller may simply bail out if raced with reparenting or
-	 * may iterate through the list_lru and expect empty slots.
-	 */
-	if (skip_empty) {
-		rcu_read_unlock();
-		return NULL;
+	return NULL;
+}
+
+static inline struct list_lru_one *
+lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
+		       bool irq, unsigned long *irq_flags)
+{
+	struct list_lru_one *l;
+
+	for (;;) {
+		l = __lock_list_lru_of_memcg(lru, nid, memcg, irq, irq_flags);
+		if (likely(l))
+			return l;
+		VM_WARN_ON(!css_is_dying(&memcg->css));
+		memcg = parent_mem_cgroup(memcg);
 	}
-	VM_WARN_ON(!css_is_dying(&memcg->css));
-	memcg = parent_mem_cgroup(memcg);
-	goto again;
 }
+
 #else
 static void list_lru_register(struct list_lru *lru)
 {
@@ -137,8 +142,8 @@ list_lru_from_memcg_idx(struct list_lru *lru, int nid, int idx)
 }
 
 static inline struct list_lru_one *
-lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
-		       bool irq, unsigned long *irq_flags, bool skip_empty)
+__lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
+			 bool irq, unsigned long *irq_flags)
 {
 	struct list_lru_one *l = &lru->node[nid].lru;
 
@@ -146,13 +151,20 @@ lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
 
 	return l;
 }
+
+static inline struct list_lru_one *
+lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
+		       bool irq, unsigned long *irq_flags)
+{
+	return __lock_list_lru_of_memcg(lru, nid, memcg, irq, irq_flags);
+}
 #endif /* CONFIG_MEMCG */
 
 struct list_lru_one *list_lru_lock(struct list_lru *lru, int nid,
 				   struct mem_cgroup *memcg)
 {
 	return lock_list_lru_of_memcg(lru, nid, memcg, /*irq=*/false,
-				      /*irq_flags=*/NULL, /*skip_empty=*/false);
+				      /*irq_flags=*/NULL);
 }
 
 void list_lru_unlock(struct list_lru_one *l)
@@ -165,7 +177,7 @@ struct list_lru_one *list_lru_lock_irqsave(struct list_lru *lru, int nid,
 					   unsigned long *flags)
 {
 	return lock_list_lru_of_memcg(lru, nid, memcg, /*irq=*/true,
-				      /*irq_flags=*/flags, /*skip_empty=*/false);
+				      /*irq_flags=*/flags);
 }
 
 void list_lru_unlock_irqrestore(struct list_lru_one *l, unsigned long *flags)
@@ -313,8 +325,8 @@ __list_lru_walk_one(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
 	unsigned long isolated = 0;
 
 restart:
-	l = lock_list_lru_of_memcg(lru, nid, memcg, /*irq=*/irq_off,
-				   /*irq_flags=*/NULL, /*skip_empty=*/true);
+	l = __lock_list_lru_of_memcg(lru, nid, memcg, /*irq=*/irq_off,
+				     /*irq_flags=*/NULL);
 	if (!l)
 		return isolated;
 	list_for_each_safe(item, n, &l->list) {
-- 
2.53.0