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

[Johannes Weiner]

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@xxxxxxxxxx>

Thanks for your review!