Re: [PATCH mm-new] mm/mglru: clarify the generation placement in lru_gen_folio_seq()

[Kairui Song]

On Tue, Oct 20, 2026 at 5:27 PM Ridong Chen <ridong.chen@xxxxxxxxx> wrote:
> On 10/20/2026 4:29 PM, Barry Song wrote:
> > On Tue, Oct 20, 2026 at 4:20 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> >>
> >> On Tue, Oct 20, 2026 at 10:36 AM Ridong Chen <ridong.chen@xxxxxxxxx> wrote:
> >>>
> >>> From: Ridong Chen <chenridong@xxxxxxxxxx>
> >>>
> >>> lru_gen_folio_seq() computes how many generations below max_seq a
> >>> folio should be added to, but stores it in a variable named "gen".
> >>> Elsewhere in MGLRU "gen" means an index into lrugen->folios[], i.e.
> >>> seq % MAX_NR_GENS, so reusing the name for a relative offset is
> >>> confusing.
> >>>
> >>> The comment table above the code also no longer matches it. It only
> >>> covers the PG_active and the final branch, leaving out the reclaiming
> >>> and "can't evict yet" cases, and it lists LRU_REFS_FLAGS as an input
> >>> although lru_gen_folio_seq() never looks at it.
> >>>
> >>> Rename the local variable to "age", redraw the table so that each
> >>> branch maps to the age and seq it produces, and note that the result
> >>> is clamped to min_seq[type].
> >>>
> >>> No functional change.
> >>>
> >>> Assisted-by: LLM
> >>> Signed-off-by: Ridong Chen <chenridong@xxxxxxxxxx>
> >>> ---
> >>>   include/linux/mm_inline.h | 48 ++++++++++++++++++++++++++-------------
> >>>   1 file changed, 32 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> >>> index ab69b9930893..a933d8c14558 100644
> >>> --- a/include/linux/mm_inline.h
> >>> +++ b/include/linux/mm_inline.h
> >>> @@ -302,34 +302,50 @@ static inline unsigned long lru_gen_folio_seq(const struct lruvec *lruvec,
> >>>                                                const struct folio *folio,
> >>>                                                bool reclaiming)
> >>>   {
> >>> -       int gen;
> >>> +       int age;
> >>>          int type = folio_is_file_lru(folio);
> >>>          const struct lru_gen_folio *lrugen = &lruvec->lrugen;
> >>>
> >>>          /*
> >>> -        * +-----------------------------------+-----------------------------------+
> >>> -        * | Accessed through page tables and  | Accessed through file descriptors |
> >>> -        * | promoted by folio_update_gen()    | and protected by folio_inc_gen()  |
> >>> -        * +-----------------------------------+-----------------------------------+
> >>> -        * | PG_active (set while isolated)    |                                   |
> >>> -        * +-----------------+-----------------+-----------------+-----------------+
> >>> -        * |  PG_workingset  |  PG_referenced  |  PG_workingset  |  LRU_REFS_FLAGS |
> >>> -        * +-----------------------------------+-----------------------------------+
> >>> -        * |<---------- MIN_NR_GENS ---------->|                                   |
> >>> -        * |<---------------------------- MAX_NR_GENS ---------------------------->|
> >>> +        *            +-----------------+-----------------+-----------------+-----------------+
> >>> +        * age        |        1        |        2        |        3        |        4        |
> >>> +        *            +-----------------+-----------------+-----------------+-----------------+
> >>> +        * seq        |     max_seq     |    max_seq-1    |    max_seq-2    |    max_seq-3    |
> >>
> >> I think this comment update is not beneficial, we might have more than
> >> 4 gens in the furture, so hardcoding a number is not a good idea.
> >>
> >>> +        *            +-----------------------------------+-----------------------------------+
> >>> +        *            |<---------- MIN_NR_GENS ---------->|                                   |
> >>> +        *            |<---------------------------- MAX_NR_GENS ---------------------------->|
> >>> +        *            +-----------------------------------+-----------------------------------+
> >>> +        *            | Accessed through page tables and  | Accessed through file descriptors |
> >>> +        *            | promoted by folio_update_gen()    | and protected by folio_inc_gen()  |
> >>> +        *            +-----------------+-----------------+-----------------+-----------------+
> >>> +        * PG_active  |  PG_workingset  | !PG_workingset  |                 |                 |
> >>> +        *            +-----------------+-----------------+-----------------+-----------------+
> >>> +        * reclaiming |                 |                 |                 |     always      |
> >>> +        *            +-----------------+-----------------+-----------------+-----------------+
> >>> +        * can't      |                 |  anon && not in |                 |                 |
> >>> +        * evict yet  |                 |  swapcache, or  |                 |                 |
> >>> +        *            |                 |  PG_reclaim &&  |                 |                 |
> >>> +        *            |                 | dirty/writeback |                 |                 |
> >>
> >> I think it's better not to describe too many details in the comment as
> >> it actually easier to just look at the code.
> >>
>
> Fair points.
>
> What confused me about the current table is that it is not clear
> which dimension it is drawn along. The columns are generations, but
> the rows mix access paths (page tables vs. file descriptors) with
> the flags that protect each of them, and some of those, like
> LRU_REFS_FLAGS, are not inputs to lru_gen_folio_seq() at all. So
> when reading it next to the code, it is hard to tell which branch
> puts a folio where.
>
> How about keeping the existing table for the design overview, and
> just adding a short note above it on what "age" means, without
> hardcoding the number of generations, e.g.:
>
>          /*
>           * The folio is added to seq = max_seq + 1 - age, so age 1 is
>           * the youngest generation. Ages up to MIN_NR_GENS are active,
>           * and the result is clamped to min_seq[type].
>           */
>
> >>> +        *            +-----------------+-----------------+-----------------+-----------------+
> >>> +        * else       |                 |                 |  PG_workingset  | !PG_workingset  |
> >>> +        *            |                 |                 |       or        |       &&        |
> >>> +        *            |                 |                 |  PG_referenced  | !PG_referenced  |
> >>> +        *            +-----------------+-----------------+-----------------+-----------------+
> >>> +        *
> >>> +        * The resulting seq is clamped to min_seq[type].
> >>>           */
> >>>          if (folio_test_active(folio))
> >>> -               gen = MIN_NR_GENS - folio_test_workingset(folio);
> >>> +               age = MIN_NR_GENS - folio_test_workingset(folio);
> >>
> >> That's a very nice rename, the `gen` variable is so confusing to me
> >> for a long time, I thought about renaming it to `gap`, `age` looks
> >> much better. Thanks!
> >
> > Agreed with the rename part. Maybe this doesn't conflict much
> > with MGLRU-FG either. It could be cherry-picked before
> > MGLRU-FG or included as part of the MGLRU-FG series.
> > I'd vote for the latter.
>
> Sounds good to me. Kairui, would you like to pick up the rename, and
> the comment change above if you agree, as part of MGLRU-FG?

Good idea, will do!