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

[Barry Song]

On Sat, Aug 29, 2026 at 3:48 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
[...]

> > > + * 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.

Yes. The current problem is that `lru_gen` has been used as a
generic prefix for MGLRU functions. Here, however, we mean the
actual generation (`gen`), rather than `lru_gen` as the MGLRU
namespace.

>
> 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.

sounds good to me.

[...]

> > > +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?

sounds good to me.

[...]

> > > --- 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.

Ok.

Best Regards
Barry