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

Johannes Weiner
2026-03-17T14:03:13Z

On Tue, Mar 17, 2026 at 11:00:59AM +0100, David Hildenbrand (Arm) wrote:
> On 3/12/26 21:51, Johannes Weiner wrote:
> > -/* 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.

Yeah, I absolutely hate this too. I only didn't look further because
it's internal, but...

> 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.

This is a great idea.

I have to send a v3 for the fix in __folio_freeze_and_split_unmapped()
and the lockdep key, so I'll make this change along with it.

> > +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" :)

Good catch, I'll fix that.

> > +/* 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>

Thanks for your review!