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

On Thu, Aug 20, 2026 at 9:02 AM Baolin Wang
<[email protected]> 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 <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>>   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.

Hi All,

Actually that's the confusing part, the "new_gen" variable here is
actually the old gen (the gen number before the CAS here) of the
folio, and the "old_gen" here is actually the min_seq gen of the
lruvec, and that's why I'm renaming it.

The current new_gen (which is actually the folio's current gen, the
old gen number) or old_gen (the lruvec's oldest gen) should both never
be < 0 in any case, if it happens, folio_inc_gen will corrupt the gen
counters or page flags.

This change isn't fixing anything, this commit just make old_gen to
hold the folio's current gen number (before the CAS), and if that is <
0 (the folio is off-list), don't touch the folio at all which will
further corrupt a already buggy satuation (this should never happen,
so I added a VM_WARN_ON). And now min_gen will be meaning the lruvec's
oldest gen.

There is no functional change, except one more sanity check and defensive check.