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

Johannes Weiner
2026-03-18T19:25:38Z

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@cmpxchg.org>
> > ---
>
> 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@linux.dev>
> Date: Wed, 18 Mar 2026 10:43:53 -0700
> Subject: [PATCH] list_lru: cleanup
>
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>

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