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

[Shakeel Butt]

I asked AI to review this patch and it find following potential issues:

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.

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

> +	}
> +}