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>
---
mm/list_lru.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 26463ae29c64..d96fd50fc9af 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -165,8 +165,6 @@ bool list_lru_add(struct list_lru *lru, struct list_head *item, int nid,
struct list_lru_one *l;
l = lock_list_lru_of_memcg(lru, nid, memcg, false, false);
- if (!l)
- return false;
if (list_empty(item)) {
list_add_tail(item, &l->list);
/* Set shrinker bit if the first element was added */
@@ -203,9 +201,8 @@ bool list_lru_del(struct list_lru *lru, struct list_head *item, int nid,
{
struct list_lru_node *nlru = &lru->node[nid];
struct list_lru_one *l;
+
l = lock_list_lru_of_memcg(lru, nid, memcg, false, false);
- if (!l)
- return false;
if (!list_empty(item)) {
list_del_init(item);
l->nr_items--;
--
2.53.0On 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>
---
mm/list_lru.c | 53 ++++++++++++++++++++++++++++-----------------------
1 file changed, 29 insertions(+), 24 deletions(-)
diff --git a/mm/list_lru.c b/mm/list_lru.c
index 26463ae29c64..062394c598d4 100644
--- a/mm/list_lru.c
+++ b/mm/list_lru.c
@@ -77,27 +77,30 @@ static inline bool lock_list_lru(struct list_lru_one *l, bool irq)
}
static inline struct list_lru_one *
-lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
- bool irq, bool skip_empty)
+__lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
+ bool irq)
{
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)) {
- rcu_read_unlock();
+ if (likely(l) && !lock_list_lru(l, irq))
+ l = NULL;
+ rcu_read_unlock();
+
+ return l;
+}
+
+static inline struct list_lru_one *
+lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg)
+{
+ struct list_lru_one *l;
+again:
+ l = __lock_list_lru_of_memcg(lru, nid, memcg, false);
+ if (likely(l))
return l;
- }
- /*
- * 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;
- }
- VM_WARN_ON(!css_is_dying(&memcg->css));
+
+ VM_WARN_ON_ONCE(!css_is_dying(&memcg->css));
memcg = parent_mem_cgroup(memcg);
goto again;
}
@@ -135,8 +138,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, bool skip_empty)
+__lock_list_lru_of_memcg(struct list_lru *lru, int nid, struct mem_cgroup *memcg,
+ bool irq)
{
struct list_lru_one *l = &lru->node[nid].lru;
@@ -148,6 +151,12 @@ 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)
+{
+ return __lock_list_lru_of_memcg(lru, nid, memcg, false);
+}
+
static inline void unlock_list_lru(struct list_lru_one *l, bool irq_off)
{
if (irq_off)
@@ -164,9 +173,7 @@ bool list_lru_add(struct list_lru *lru, struct list_head *item, int nid,
struct list_lru_node *nlru = &lru->node[nid];
struct list_lru_one *l;
- l = lock_list_lru_of_memcg(lru, nid, memcg, false, false);
- if (!l)
- return false;
+ l = lock_list_lru_of_memcg(lru, nid, memcg);
if (list_empty(item)) {
list_add_tail(item, &l->list);
/* Set shrinker bit if the first element was added */
@@ -203,9 +210,7 @@ bool list_lru_del(struct list_lru *lru, struct list_head *item, int nid,
{
struct list_lru_node *nlru = &lru->node[nid];
struct list_lru_one *l;
- l = lock_list_lru_of_memcg(lru, nid, memcg, false, false);
- if (!l)
- return false;
+ l = lock_list_lru_of_memcg(lru, nid, memcg);
if (!list_empty(item)) {
list_del_init(item);
l->nr_items--;
@@ -287,7 +292,7 @@ __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_off, true);
+ l = __lock_list_lru_of_memcg(lru, nid, memcg, irq_off);
if (!l)
return isolated;
list_for_each_safe(item, n, &l->list) {
--
2.52.0
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
On Wed, Mar 18, 2026 at 03:25:29PM -0400, Johannes Weiner wrote: > 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. Yeah, I saw your discussion with David and thought on how can we further reduce the params. > 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? I am fine with whatever route you take. I know you have next version ready to send, I will review the remaining patches for the next version (though I have taken a look on the current series but will add my tags for the next one :P).
On 3/12/26 21:51, 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>
> ---
> mm/list_lru.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/mm/list_lru.c b/mm/list_lru.c
> index 26463ae29c64..d96fd50fc9af 100644
> --- a/mm/list_lru.c
> +++ b/mm/list_lru.c
> @@ -165,8 +165,6 @@ bool list_lru_add(struct list_lru *lru, struct list_head *item, int nid,
> struct list_lru_one *l;
>
> l = lock_list_lru_of_memcg(lru, nid, memcg, false, false);
> - if (!l)
> - return false;
> if (list_empty(item)) {
> list_add_tail(item, &l->list);
> /* Set shrinker bit if the first element was added */
> @@ -203,9 +201,8 @@ bool list_lru_del(struct list_lru *lru, struct list_head *item, int nid,
> {
> struct list_lru_node *nlru = &lru->node[nid];
> struct list_lru_one *l;
> +
> l = lock_list_lru_of_memcg(lru, nid, memcg, false, false);
> - if (!l)
> - return false;
> if (!list_empty(item)) {
> list_del_init(item);
> l->nr_items--;
Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>
--
Cheers,
David
© 2016 - 2026 Red Hat, Inc.