Re: [PATCH 7/7] mm/mglru: improve code readability and harden folio_inc_gen

[Baoquan He]

On 08/18/26 at 01:38pm, Kairui Song via B4 Relay wrote:
> From: Kairui Song <kasong@xxxxxxxxxxx>
> 
> The helper should never be called for an off-list folio, and it always
> expects the folio to be in the oldest generation before doing any
> cmpxchg. Add a sanity check for the off-list case: if it is ever
> violated, bail out and keep the folio flags untouched to minimize the
> damage, instead of silently treating the folio as if it were in the
> oldest generation and promoting it updating the flags to an unexpected
> status.
> 
> Also rename the variables to clearly distinguish the folio's current
> gen from the oldest gen.
> 
> Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> ---
>  mm/vmscan.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 7169cac60869..7e3ae0c6cba3 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3308,18 +3308,22 @@ static int folio_inc_gen(struct lruvec *lruvec, struct folio *folio)
>  {
>  	int type = folio_is_file_lru(folio);
>  	struct lru_gen_folio *lrugen = &lruvec->lrugen;
> -	int new_gen, old_gen = lru_gen_from_seq(lrugen->min_seq[type]);
> +	int new_gen, old_gen, min_gen = lru_gen_from_seq(lrugen->min_seq[type]);
>  	unsigned long new_flags, old_flags = READ_ONCE(*folio_flags(folio, 0));
>  
>  	do {
> -		new_gen = lru_gen_from_flags(old_flags);
> +		old_gen = lru_gen_from_flags(old_flags);
> +		/* This helper should never be called for off-list folios */
> +		VM_WARN_ON_ONCE(old_gen < 0);
> +		if (old_gen < 0)
> +			return min_gen;

As Barry doubted, I think this change is wrong. old_gen < 0 in folio_inc_gen()
could only happen inc_min_seq() call it. While inc_min_seq() call it
because inc_max_seq() need increase max_gen to max_gen + 1 and found
get_nr_gens(lruvec, type) == MAX_NR_GENS, it has to move the oldest gen to
2nd old oldest gen. Here returning min_gen for old_gen < 0 means it will
be put in the lastest max_gen. It may not be expected.

>  
>  		/* folio_update_gen() has promoted this page? */
> -		if (new_gen >= 0 && new_gen != old_gen)
> -			return new_gen;
> +		if (old_gen != min_gen)
> +			return old_gen;
>  
>  		new_flags = old_flags;
> -		new_gen = (old_gen + 1) % MAX_NR_GENS;
> +		new_gen = (min_gen + 1) % MAX_NR_GENS;
>  		lru_gen_set_flags(&new_flags, new_gen);
>  		lru_refs_set_flags(&new_flags, 0);
>  	} while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
> 
> -- 
> 2.55.0
> 
>