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

[Kairui Song]

On Sat, Aug 29, 2026 at 03:47:42PM +0800, Kairui Song wrote:
> 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.

Hi Barry

After second though, I now remember why I did this in the first place.
And there is already many following checks in upstream:

VM_WARN_ON_ONCE(new_gen != -1 && new_gen >= MAX_NR_GENS);
VM_WARN_ON_ONCE(old_gen != -1 && new_gen >= MAX_NR_GENS);

And gen is signed everything since it has a -1 special value.

The reason is MAX_NR_GENS is unsigned, so a signed -1 will
look larger and trigger false warning on it.

Or we will have to have something like:
VM_WARN_ON_ONCE(gen < -1 || gen >= (int)MAX_NR_GENS)

I prefer to keep it with the existing style, with proper
comment, further sanity check cleanup later. Right now the
usage of MAX_NR_GENS is limited so I think we are fine.

In fact I'm think we can get rid of the -1 by combining with
PG_lru, or use a formal special flag. Which can be done later.