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

[Qi Zheng <qi.zheng@linux.dev>]

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@bytedance.com>
> 
> 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@bytedance.com
> 
>> 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.

> 
>> +	}
>> +}
>