Re: [PATCH v3 26/30] mm: vmscan: prepare for reparenting MGLRU folios

[Qi Zheng]

On 1/18/26 11:29 AM, Shakeel Butt wrote:
> I asked AI to review this patch and it find following potential issues:

Thanks.

> Author: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
>
> mm: vmscan: prepare for reparenting MGLRU folios
>
> This commit adds infrastructure for reparenting MGLRU folios from dying
> memcg to parent memcg, including functions to ensure parent has MAX_NR_GENS
> generations before reparenting.
>
> Link: https://lkml.kernel.org/r/92e0728fed3d68855173352416cf8077670610f0.1768389889.git.zhengqi.arch@xxxxxxxxxxxxx
>
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index f1a45c94c2fc..284292653d14 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
>
> [ ... ]
>
> > +static void try_to_inc_max_seq_nowalk(struct mem_cgroup *memcg,
> > +				      struct lruvec *lruvec)
> > +{
> > +	struct lru_gen_mm_list *mm_list = get_mm_list(memcg);
> > +	struct lru_gen_mm_state *mm_state = get_mm_state(lruvec);
> > +	int swappiness = mem_cgroup_swappiness(memcg);
> > +	DEFINE_MAX_SEQ(lruvec);
> > +	bool success = false;
> > +
> > +	/*
> > +	 * We are not iterating the mm_list here, updating mm_state->seq is just
> > +	 * to make mm walkers work properly.
> > +	 */
> > +	if (mm_state) {
> > +		spin_lock(&mm_list->lock);
> > +		VM_WARN_ON_ONCE(mm_state->seq + 1 < max_seq);
> > +		if (max_seq > mm_state->seq) {
>                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> When mm_state->seq equals max_seq (which can happen when mm walkers have
> completed their iteration), this condition is false, success remains false,
> and inc_max_seq() is never called.

I expect that once mm walkers complete their iterations, the number of
generations will have increased, so it won't cause an infinite loop.

> The caller max_lru_gen_memcg() loops with:
>
>      while (get_nr_gens(lruvec, type) < MAX_NR_GENS) {
>          try_to_inc_max_seq_nowalk(memcg, lruvec);
>          cond_resched();
>      }
>
> If try_to_inc_max_seq_nowalk() fails to increment max_seq, get_nr_gens()
> returns the same value, and the while loop continues indefinitely.
>
> Can this cause an infinite loop during memcg offline when mm_state->seq has
> caught up to max_seq?
>
> Suggested fix: Remove the success variable and always call inc_max_seq().
> The mm_state->seq update is only for coordination with mm walkers - it should
> not gate whether we increment the generation. inc_max_seq() has its own
> staleness check (seq < READ_ONCE(lrugen->max_seq)) so calling it unconditionally
> is safe:
>
>      if (mm_state) {
>          spin_lock(&mm_list->lock);
>          VM_WARN_ON_ONCE(mm_state->seq + 1 < max_seq);
>          if (max_seq > mm_state->seq)
>              WRITE_ONCE(mm_state->seq, mm_state->seq + 1);
>          spin_unlock(&mm_list->lock);
>      }
>
>      inc_max_seq(lruvec, max_seq, swappiness);
>
> > +			WRITE_ONCE(mm_state->seq, mm_state->seq + 1);
> > +			success = true;
> > +		}
> > +		spin_unlock(&mm_list->lock);
> > +	} else {
> > +		success = true;
> > +	}
> > +
> > +	if (success)
> > +		inc_max_seq(lruvec, max_seq, swappiness);
> > +}
>
> [ ... ]
>
> > +static void __lru_gen_reparent_memcg(struct lruvec *child_lruvec, struct lruvec *parent_lruvec,
> > +				     int zone, int type)
> > +{
> > +	struct lru_gen_folio *child_lrugen, *parent_lrugen;
> > +	enum lru_list lru = type * LRU_INACTIVE_FILE;
> > +	int i;
>
> [ ... ]
>
> > +		WRITE_ONCE(child_lrugen->nr_pages[gen][type][zone], 0);
> > +		WRITE_ONCE(parent_lrugen->nr_pages[gen][type][zone],
> > +			   parent_lrugen->nr_pages[gen][type][zone] + nr_pages);
> > +
> > +		update_lru_size(parent_lruvec, lru + dst_lru_active, zone, nr_pages);
>                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This update_lru_size() call updates both __update_lru_size() counters and
> mem_cgroup_update_lru_size(). However, lru_gen_reparent_memcg() also calls
> mem_cgroup_update_lru_size() for LRU_UNEVICTABLE. The combination appears
> to cause incorrect accounting.
>
> Note: This issue was fixed in later commit ("mm: mglru: do not call
> update_lru_size() during reparenting").

Right.

> > +	}
> > +}