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

[Baoquan He]

On 08/20/26 at 09:02am, Baolin Wang wrote:
> 
> 
> On 8/20/26 8:57 AM, Baoquan He wrote:
> > On 08/20/26 at 08:53am, Baoquan He wrote:
> > > 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
> >    ~~~~~~~~~~~~~~~~
> > Here, I mean it has to move folios from the oldest gen (min_gen) to the 2nd
> > oldest gen (min_gen + 1). The empty min_gen will become the new max_gen.
> > 
> > > be put in the lastest max_gen. It may not be expected.
> 
> But how does old_gen < 0 actually happen? folio_inc_gen() is called under
> the lru lock, so how can a folio listed in MGLRU have a gen counter < 0? If
> this can happen in any case, we should fix this bug first.

That's a good question, and I agree with you that folio_inc_gen() is
called under lru lock, and lru_gen_del_folio() which clears the gen
and take folio off lru is also called under lru lock. So old_gen < 0
here is a "should never happen" defensive branch (plus the
WARN_ON_ONCE), and if it really occurs there is a real bug to fix.
Wondering how Barry trigger his printk debugging and observed it.

However, the defensive branch itself is incorrect. Not only folio of
old_gen < 0 are put in the new max_gen, but what is worse, it doesn't
clear the old_gen <0 in folio->flags even though it's put back in the
min_gen list, next time aging comes to next round of min_gen and sort_folio()
will get a lrugen->folios[-1] out of bound accessing

static bool sort_folio(struct lruvec *lruvec, struct folio *folio, struct scan_control *sc,
                       int tier_idx)
{
	...
        /* promoted */
        if (gen != lru_gen_from_seq(lrugen->min_seq[type])) {
                list_move(&folio->lru, &lrugen->folios[gen][type][zone]);
                return true;
        }
	...
}

So I think we should keep the old code unchanged, fix any warning report 
triggered by VM_WARN_ON_ONCE(old_gen < 0).