Re: [PATCH v3 2/6] mm/mglru: introduce helpers for manipulating gen and refs flags

[Kairui Song]

On Sat, Aug 29, 2026 at 12:22 PM Barry Song <baohua@xxxxxxxxxx> wrote:
>
> On Wed, Aug 26, 2026 at 1:53 AM Kairui Song via B4 Relay
> <devnull+kasong.tencent.com@xxxxxxxxxx> wrote:
> >
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > Instead of doing bit ops on folio->flags.f, introduce helpers for
> > adjusting a folio's refs and generation info, making the code easier
> > to debug and understand.
> >
> > No functional change is intended: some combined atomic operations are
> > split into two, which only creates harmless transient states. There is
> > no measurable performance impact, and some paths even look slightly
> > better in the generated assembly.
>
> Hi Kairui,
>
> I like your idea. Overall, it looks good to me. With some cleanup,
> we might have the following:
>
> >
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > ---
> >  include/linux/mm_inline.h | 76 ++++++++++++++++++++++++++++++++++++++++++-----
> >  include/linux/mmzone.h    |  1 +
> >  mm/folio.c                | 19 +++++++-----
> >  mm/vmscan.c               | 61 ++++++++++++++++++++-----------------
> >  4 files changed, 114 insertions(+), 43 deletions(-)
> >
> > diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
> > index 621c8653d8f7..edfaf2661812 100644
> > --- a/include/linux/mm_inline.h
> > +++ b/include/linux/mm_inline.h
> > @@ -142,10 +142,42 @@ static inline int lru_tier_from_refs(int refs, bool workingset)
> >         return workingset ? MAX_NR_TIERS - 1 : order_base_2(refs);
> >  }
> >
> > -static inline int folio_lru_refs(const struct folio *folio)
> > +/**
> > + * lru_gen_from_flags - Return the LRU generation number from folio flags.
> > + * @flags: folio flags
> > + *
> > + * Returns: A number between 0 and (MAX_NR_GENS - 1), inclusive. Returns
> > + * -1 if the flags indicate the folio is off the list (e.g., isolated).
> > + */
> > +static inline int lru_gen_from_flags(unsigned long flags)
> > +{
> > +       int gen = ((flags & LRU_GEN_MASK) >> LRU_GEN_PGOFF);
> > +
> > +       BUILD_BUG_ON(LRU_GEN_MASK & LRU_REFS_MASK);
> > +       gen -= 1;
> > +       VM_WARN_ON_ONCE(gen != -1 && gen >= MAX_NR_GENS);
>
> Since `gen` is an `int`, it seems a bit odd to have
> `gen != -1 && gen >= MAX_NR_GENS` combined here.
>
> Do you actually mean the following instead?
>
> VM_WARN_ON_ONCE(gen < -1 || gen >= MAX_NR_GENS);

Thanks for the review!

Yeah, thats's a better sanity check, will udpate it.

> > +}
> > +
> > +/**
> > + * lru_gen_set_flags - Set the LRU generation number to specified folio flags.
> > + * @flags: pointer to the folio flags
> > + * @gen: generation number, between 0 and (MAX_NR_GENS - 1), inclusive.
> > + */
> > +static inline void lru_gen_set_flags(unsigned long *flags, int gen)
> >  {
> > -       unsigned long flags = READ_ONCE(folio->flags.f);
> > +       VM_WARN_ON_ONCE(gen >= MAX_NR_GENS || gen < 0);
> > +
> > +       *flags &= ~LRU_GEN_MASK;
> > +       *flags |= (gen + 1UL) << LRU_GEN_PGOFF;
> > +}
>
> It seems that we are setting the gen in the flags, rather than
> setting the flags themselves.
>
> The current name makes me think that the function is setting the
> entire flags field, especially since `lru_gen` is used as the
> namespace for various MGLRU functions, e.g.:
>
> void lru_gen_add_mm(struct mm_struct *mm);
> void lru_gen_del_mm(struct mm_struct *mm);
> void lru_gen_migrate_mm(struct mm_struct *mm);
> struct lru_gen_folio;
> lru_gen_update_size(lruvec, folio, old_gen, new_gen);
>
> However, this function only sets the gen bits in the flags.
> Maybe we could use a name that makes this distinction clearer?
>
> Could they be named as below, or is there a better naming option?
>
> set_gen_to_flags(unsigned long *flags, int gen);
> get_gen_from_flags(unsigned long flags);
>
> Or something like flags_to_gen() or gen_from_flags()?

I think having lru_gen as the prefix is a good idea actually since the
header here is a very generic one, the word "gen" could be used my
many other subsystem or drivers, gen_to_flags sounds too generic..
While lru_gen is a very specific MGLRU thing.

How about just rename it lru_set_gen_flags / lru_get_gen_flags? And we
will have lru_set_refs_flags / lru_get_refs_flags below.

>
> >
> > +/**
> > + * lru_refs_from_flags - Return LRU referenced / access count from folio flags.
> > + * @flags: folio flags
> > + */
> > +static inline int lru_refs_from_flags(unsigned long flags)
> > +{
> >         if (!(flags & BIT(PG_referenced)))
> >                 return 0;
> >         /*
> > @@ -155,11 +187,40 @@ static inline int folio_lru_refs(const struct folio *folio)
> >         return ((flags & LRU_REFS_MASK) >> LRU_REFS_PGOFF) + 1;
> >  }
> >
> > -static inline int folio_lru_gen(const struct folio *folio)
> > +/**
> > + * lru_refs_set_flags - Set the LRU referenced / access count to specified folio flags.
> > + * @flags: pointer to the folio flags
> > + * @refs: referenced / access count number, between 0 and LRU_REFS_MAX, inclusive.
> > + */
> > +static inline void lru_refs_set_flags(unsigned long *flags, unsigned int refs)
> > +{
> > +       VM_WARN_ON_ONCE(refs > LRU_REFS_MAX);
> > +       BUILD_BUG_ON(LRU_REFS_MAX != (LRU_REFS_MASK >> LRU_REFS_PGOFF) + 1);
> > +
> > +       *flags &= ~LRU_REFS_FLAGS;
> > +       if (!refs)
> > +               return;
> > +       *flags |= (BIT(PG_referenced) | ((refs - 1UL) << LRU_REFS_PGOFF));
> > +}
>
> Similar to the above, maybe we could use more descriptive names?

And here we have lru_set_refs_flags?
>
> > +
> > +static inline int folio_lru_refs(const struct folio *folio)
> >  {
> > -       unsigned long flags = READ_ONCE(folio->flags.f);
> > +       return lru_refs_from_flags(READ_ONCE(*const_folio_flags(folio, 0)));
> > +}
> > +
> > +static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
> > +{
> > +       unsigned long new_flags, old_flags = READ_ONCE(*folio_flags(folio, 0));
> > +
> > +       do {
> > +               new_flags = old_flags;
> > +               lru_refs_set_flags(&new_flags, refs);
> > +       } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
> > +}
> >
> > -       return ((flags & LRU_GEN_MASK) >> LRU_GEN_PGOFF) - 1;
>
> [...]
>
> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 73a81b4a3e16..9ee9f8dc6805 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -843,19 +843,22 @@ static bool lru_gen_set_refs(struct folio *folio, const vma_flags_t *vma_flags)
> >         if (!folio_test_referenced(folio) && !folio_test_workingset(folio)) {
> >                 /* Activate file-backed executable folios after first usage. */
> >                 if (is_exec_file_folio(folio, vma_flags)) {
> > -                       set_mask_bits(&folio->flags.f, LRU_REFS_FLAGS, BIT(PG_workingset));
> > +                       folio_set_workingset(folio);
> > +                       folio_set_lru_refs(folio, 0);
> >                         return true;
> >                 }
> >
> > -               set_mask_bits(&folio->flags.f, LRU_REFS_MASK, BIT(PG_referenced));
> > +               folio_set_lru_refs(folio, 1);
>
> We have quite a few instances of folio_set_lru_refs(folio, 1) and
> folio_set_lru_refs(folio, 0). Do these values have any special
> meaning? Could we add wrapper helpers to make the code more readable?
>
> `1` just means `PG_referenced`, right? Could we add a comment or
> changelog description to clarify that `1` actually means `refs = 0`,
> but `PG_referenced` is set?

Right, I introduced a LRU_REFS_REFERENCED in a later series. Now a
comment could be good enough, as in fact PG_referenced is already kind
of a unstable page flags under both MGLRU and CLRU. For MGLRU it only
mean the refs count is larger than 1 so using a strict 1 here seems
even cleaner, but it still keep getting cleared in many places. As for
CLRU, the flag keeps getting reset when a folio is activated leading
to an under-accounted value from smap and other components. So
sanitizing its usage is a long-term goal.