Re: [PATCH v2 5/7] mm: list_lru: introduce caller locking for additions and deletions

David Hildenbrand (Arm)
2026-03-17T10:01:08Z

On 3/12/26 21:51, Johannes Weiner wrote:
> Locking is currently internal to the list_lru API. However, a caller
> might want to keep auxiliary state synchronized with the LRU state.
>
> For example, the THP shrinker uses the lock of its custom LRU to keep
> PG_partially_mapped and vmstats consistent.
>
> To allow the THP shrinker to switch to list_lru, provide normal and
> irqsafe locking primitives as well as caller-locked variants of the
> addition and deletion functions.
>
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
> include/linux/list_lru.h | 34 +++++++++++++
> mm/list_lru.c | 104 +++++++++++++++++++++++++++------------
> 2 files changed, 107 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/list_lru.h b/include/linux/list_lru.h
> index fe739d35a864..4afc02deb44d 100644
> --- a/include/linux/list_lru.h
> +++ b/include/linux/list_lru.h
> @@ -83,6 +83,40 @@ int memcg_list_lru_alloc(struct mem_cgroup *memcg, struct list_lru *lru,
> gfp_t gfp);
> void memcg_reparent_list_lrus(struct mem_cgroup *memcg, struct mem_cgroup *parent);
>

[...]

> 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)
> + bool irq, unsigned long *irq_flags, bool skip_empty)
> {
> struct list_lru_one *l = &lru->node[nid].lru;
>
> - lock_list_lru(l, irq);
> + lock_list_lru(l, irq, irq_flags);
>
> return l;
> }
> #endif /* CONFIG_MEMCG */
>
> -/* The caller must ensure the memcg lifetime. */
> -bool list_lru_add(struct list_lru *lru, struct list_head *item, int nid,
> - struct mem_cgroup *memcg)
> +struct list_lru_one *list_lru_lock(struct list_lru *lru, int nid,
> + struct mem_cgroup *memcg)
> {
> - struct list_lru_node *nlru = &lru->node[nid];
> - struct list_lru_one *l;
> + return lock_list_lru_of_memcg(lru, nid, memcg, false, NULL, false);

The two "bool" parameters really are ugly. Fortunately this is only an
internal function.

The callers are still a bit hard to read; we could add /*skip=empty=*/true).

like

return lock_list_lru_of_memcg(lru, nid, memcg, /* irq= */false, NULL,
/* skip_empty= */false);

Like we do in other code. But I guess we should do it consistently then
(or better add some proper flags).

Anyhow, something that could be cleaned up later.

> +}
> +
> +void list_lru_unlock(struct list_lru_one *l)
> +{
> + unlock_list_lru(l, false, NULL);
> +}
> +
> +struct list_lru_one *list_lru_lock_irqsave(struct list_lru *lru, int nid,
> + struct mem_cgroup *memcg,
> + unsigned long *flags)
> +{
> + return lock_list_lru_of_memcg(lru, nid, memcg, true, flags, false);

And here it gets really confusing. true false false ... am I reading
binary code?

I guess the second "false" should actually be "NULL" :)

> +}
> +
> +void list_lru_unlock_irqrestore(struct list_lru_one *l, unsigned long *flags)
> +{
> + unlock_list_lru(l, true, flags);
> +}
>
> - l = lock_list_lru_of_memcg(lru, nid, memcg, false, false);
> +bool __list_lru_add(struct list_lru *lru, struct list_lru_one *l,
> + struct list_head *item, int nid,
> + struct mem_cgroup *memcg)
> +{
> if (list_empty(item)) {
> list_add_tail(item, &l->list);
> /* Set shrinker bit if the first element was added */
> if (!l->nr_items++)
> set_shrinker_bit(memcg, nid, lru_shrinker_id(lru));
> - unlock_list_lru(l, false);
> - atomic_long_inc(&nlru->nr_items);
> + atomic_long_inc(&lru->node[nid].nr_items);
> + return true;
> + }
> + return false;
> +}
> +
> +bool __list_lru_del(struct list_lru *lru, struct list_lru_one *l,
> + struct list_head *item, int nid)
> +{
> + if (!list_empty(item)) {
> + list_del_init(item);
> + l->nr_items--;
> + atomic_long_dec(&lru->node[nid].nr_items);
> return true;
> }
> - unlock_list_lru(l, false);
> return false;
> }
>
> +/* The caller must ensure the memcg lifetime. */
> +bool list_lru_add(struct list_lru *lru, struct list_head *item, int nid,
> + struct mem_cgroup *memcg)
> +{
> + struct list_lru_one *l;
> + bool ret;
> +
> + l = list_lru_lock(lru, nid, memcg);
> + ret = __list_lru_add(lru, l, item, nid, memcg);
> + list_lru_unlock(l);
> + return ret;
> +}

Nice.

Reviewed-by: David Hildenbrand (Arm) <david@kernel.org>

--
Cheers,

David