Re: [PATCH v4 04/17] mm/mglru: frequency guided workingset promotion (MGLRU-FG)

[Barry Song]

On Mon, Oct 19, 2026 at 8:09 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
>
> On Mon, Oct 19, 2026 at 6:10 PM Barry Song <baohua@xxxxxxxxxx> wrote:
> >
[...]
> >
> > So this is the main course, with the earlier ones as appetizers and
> > the later ones as desserts?
> > Overall, this dish looks very delicious to me.
> >
> > I ran kernel build on my machine, the result is quite similar with[1].
> >
> > W/o patch:
> >
> > eal    1m49.094s
> > user    25m31.484s
> > sys     3m40.959s
> > pswpin: 662112
> > pswpout: 2504415
> > pgpgin: 4424336
> > pgpgout: 10355273
> > refault_file: 359626
> > refault_anon: 670724
> >
> > W/ patch:
> >
> > real 1m48.944s
> > user 25m30.987s
> > sys 3m34.605s
> > pswpin: 467952
> > pswpout: 2319080
> > pgpgin: 3481187
> > pgpgout: 9603332
> > swpout_zero: 602042
> > swpin_zero: 6250
> > refault_file: 310538
> > refault_anon: 474154
> >
> > But it behaves in a much more sensible way.
>
> Hi Barry,
>
> Thanks for the review and testing. Glad to see it worked for you.
>
> >
> > I've had a preliminary look, but haven't reviewed it in detail yet.
> > I have a few comments.
> >
> > [1] https://lore.kernel.org/linux-mm/20261016213115.70515-1-baohua@xxxxxxxxxx/
>
> Right there could be some similarities, and some differences, e.g. no
> more lru_gen_in_fault hanlding here, or difference between anon /
> file. BTW did you test the whole series or just this patch? This patch

I tested the whole series rather than this patch in isolation.

[...]

> > Could we use the following instead? Coming from a hardware register background,
> > this looks more like register encoding to me:
> >
> > static inline int lru_get_refs_flags(unsigned long flags)
> > {
> >     int refs;
> >
> >     refs  = !!(flags & BIT(PG_referenced));
> >     refs |= !!(flags & BIT(PG_workingset)) << 1;
> >     refs |= ((flags & LRU_REFS_MASK) >> LRU_REFS_PGOFF) << 2;
> >     return refs;
> > }
>
> No problem; it means the same thing and looks better indeed. I think I
> better change the return type to unsigned too, refs can never be
> negative (gen could be though, as a reserved value).

make sense to me. lru_get_refs_flags() is impossible to be negative.

>
> >
> > >
> > >  static inline int folio_lru_refs(const struct folio *folio)
> > > @@ -236,14 +240,29 @@ static inline void folio_set_lru_refs(struct folio *folio, unsigned int refs)
> > >         } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
> > >  }
> > >
> > > +void folio_inc_lru_refs(struct folio *folio, unsigned int flags);
> > > +
> > >  static inline int folio_lru_gen(const struct folio *folio)
> > >  {
> > >         return lru_get_gen_flags(READ_ONCE(*const_folio_flags(folio, 0)));
> > >  }
> > >
> > > +static inline bool lru_gen_at_tail(struct lruvec *lruvec, int type, int gen)
> > > +{
> > > +       unsigned long min_seq = READ_ONCE(lruvec->lrugen.min_seq[type]);
> > > +
> > > +       /*
> > > +        * Consider a folio is at the tail of its LRU when it is within
> > > +        * MIN_NR_GENS of the oldest generation of its own type
> > > +        */
> > > +       BUILD_BUG_ON(MIN_NR_GENS != 2);
> > > +       return gen == lru_gen_from_seq(min_seq) ||
> > > +              gen == lru_gen_from_seq(min_seq + 1);
> > > +}
> >
> > If only two generations remain, should both be considered hot?
> > If three generations remain, is only the oldest generation old?
> >
> > Also, the name is a bit confusing, as "tail" usually refers
> > to a list. Could we rename it to one of the following?
> >
> > lru_gen_is_old()
> > or
> > lru_gen_is_near_oldest()
>
> Right, I wanted to introduce an idea of something called tailing gens
> (e.g. 8 tailing gens when there are 64 gens in total, folios in
> tailing gens are promoted on each access instead of multiple access,
> this works really well for fine grained hotness detection, and avoids
> over protection too). But for now this helper is just meant to tell
> the caller if the folio is close to be evicted or not, checking
> min_seq only could easily fall into a pitfall where the oldest gen is
> very short.
>
> Maybe lru_gen_at_expiry? Or lru_gen_at_salvage?
>

at_expiry gives the impression that a timer is involved in
triggering a timeout, while at_salvage is not very intuitive
for non-native English speakers.

I'd still prefer lru_gen_is_old(); it's simpler and easier to understand.

> BTW the whole picture involves three kinds of gens:
> {tailing/expiry/salvage} gens (promote by one gen on each one FD
> access), frequency gens (promote by one gen on multiple FD access),
> times gens (promote to latest gen on PTE access); We now only have
> expiry gens and frequency gens, timed gens are extensions for the
> workingset report or pghot design. e.g we have 64 gens, then 8 tailing
> gens are expiry gens, 16 frequency gens, and 48 newest gens are timed
> gens.
> https://lwn.net/Articles/976985/
>
> I mentioned this in the cover letter that the "climbing of the file
> cache" will stop at 8 gens or 16 gens, this is the whole story, there
> is a lot of context and plan so I trimed them off to focus on the FG
> improvement first..
>
[...]
> > >         if (folio_test_active(folio))
> > > -               gen = MIN_NR_GENS - folio_test_workingset(folio);
> > > +               gen = MIN_NR_GENS - (refs >= LRU_REFS_WORKINGSET);
> >
> > Could we define the "workingset" variant earlier so we can reuse it here?
> >
> > workingset = refs >= LRU_REFS_WORKINGSET;
>
> Looks good, will do.

thanks!


>
> > > +/*
> > > + * Update the folio's lru refs indicator. The caller doesn't need to hold
> > > + * the folio lock, isolate the folio, or hold the lruvec lock. Used by both
> > > + * cache access (flags == 0) and page table access (LRU_REF_MAPPED,
> > > + * optionally with LRU_REF_EXEC).
> > > + */
> > > +void folio_inc_lru_refs(struct folio *folio, unsigned int flags)
> > > +{
> > > +       int max_gen;
> > > +       int type, refs, old_gen, gen;
> > > +       unsigned long new_flags, old_flags, max_seq;
> > > +       struct lru_gen_folio *lrugen;
> > > +       struct lruvec *lruvec = NULL;
> > > +
> > > +       type = folio_is_file_lru(folio);
> > > +       old_flags = READ_ONCE(*folio_flags(folio, 0));
> > > +       do {
> > > +               new_flags = old_flags;
> > > +               old_gen = lru_get_gen_flags(old_flags);
> > > +               refs = lru_get_refs_flags(old_flags) + 1;
> > > +               gen = old_gen;
> > > +               if (old_gen < 0)
> > > +                       goto out;
> > > +               /*
> > > +                * Lock the lruvec if the folio is on-list. We are already
> > > +                * doing lazy promotion so in theory we don't need this,
> > > +                * but for now, concurrent aging would still corrupt the
> > > +                * size counters.  This is a temporary limitation and
> > > +                * will be lifted very soon, so the lock here is not a
> > > +                * performance concern.
> > > +                */
> > > +               if (!lruvec) {
> > > +                       lruvec = lruvec_live_lock_irq(folio_lruvec(folio));
> > > +                       lrugen = &lruvec->lrugen;
> > >                 }
> > > +               max_seq = READ_ONCE(lrugen->max_seq);
> > > +               max_gen = lru_gen_from_seq(max_seq);
> > > +               if (old_gen == max_gen)
> > > +                       goto out;
> > >
> > > -               folio_set_lru_refs(folio, 1);
> > > -               return false;
> > > -       }
> > > +               if (flags & (LRU_REF_MAPPED | LRU_REF_EXEC)) {
> > > +                       /* Promote second page table access or executable */
> > > +                       if (refs > LRU_REFS_REFERENCED || flags & LRU_REF_EXEC)
> > > +                               gen = max_gen;
> >
> > This looks a bit odd to me. Could we handle LRU_REF_EXEC in a
> > separate branch instead of mixing it with refs > LRU_REFS_REFERENCED?
>
> Not sure how? They seem to want the same thing: promote the folio.

I mean we could have a separate branch for LRU_REF_EXEC | LRU_REF_FORCE:

                if (flags & (LRU_REF_EXEC | LRU_REF_FORCE)) {
                        gen = max_gen;
                        refs = min(refs, LRU_REFS_PROTECTED);
                } else if (flags & LRU_REF_MAPPED) {
                        if (refs > LRU_REFS_REFERENCED)
                                gen = max_gen;
                        /* First access only defers eviction of a tail gen */
                        else if (lru_gen_at_tail(lruvec, file, old_gen))
                                gen = (old_gen + 1) % MAX_NR_GENS;
                        refs = min(refs, LRU_REFS_PROTECTED);
                } else if (refs > LRU_REFS_MAX) {


right now, it is:

                if (flags & (LRU_REF_MAPPED | LRU_REF_EXEC | LRU_REF_FORCE)) {
                        if (refs > LRU_REFS_REFERENCED ||
                            flags & (LRU_REF_EXEC | LRU_REF_FORCE))
                                gen = max_gen;
                        /* First access only defers eviction of a tail gen */
                        else if (lru_gen_at_tail(lruvec, file, old_gen))
                                gen = (old_gen + 1) % MAX_NR_GENS;
                        refs = min(refs, LRU_REFS_PROTECTED);
                } else if (refs > LRU_REFS_MAX) {

a bit hard to follow.

>
> > > +                       /* First access only defers eviction of a tail gen */
> > > +                       else if (lru_gen_at_tail(lruvec, type, old_gen))
> > > +                               gen = (old_gen + 1) % MAX_NR_GENS;
> >
> > The logic makes a lot of sense to me. It should be able to
> > differentiate readahead folios from those that are actually mapped
> > and likely to be accessed more than once.
> >
> > If we allow two generations to remain, this could be a problem
> > because old_gen + 1 would overflow. Maybe add a
> > VM_WARN_ON() to guard against this case?
>
> We just checked if (old_gen == max_gen), so this should be fine? I
> mean overflow and the mod MAX_NR_GENS is also part of the sliding
> window logic.

Yep, I missed your check:

                if (old_gen == max_gen)
                        goto out;

[...]
> > Sometimes you use >= LRU_REFS_WORKINGSET, while elsewhere
> > you use refs > LRU_REFS_REFERENCED. Can we unify these
> > checks or define a workingset variant?
>
> No problem; I can use a macro for that, as they mean the same thing.

right, thanks!

>
> > > +/*
> > > + * Update the lru refs indicator of an isolated folio, only used on
> > > + * mapped folios upon the final eviction.
> > > + *
> > > + * Increments the refs count (capped at LRU_REFS_PROTECTED, evict_folios()
> > > + * then caps it at LRU_REFS_WORKINGSET).  Returns true if the caller should
> > > + * activate the folio (second access or executable), false to put it back
> > > + * for a second chance.
> > > + */
> > > +static bool folio_inc_lru_refs_isolated(struct folio *folio, const vma_flags_t *vma_flags)
> > > +{
> > > +       unsigned long new_flags, old_flags = READ_ONCE(*folio_flags(folio, 0));
> > > +       int refs;
> > > +
> > > +       do {
> > > +               new_flags = old_flags;
> > > +               refs = lru_get_refs_flags(old_flags) + 1;
> > > +               lru_set_refs_flags(&new_flags, min(refs, LRU_REFS_PROTECTED));
> >
> > I'm having a hard time understanding why an activated folio
> > still keeps LRU_REFS_PROTECTED, since it will be promoted
> > to the active list anyway :-)
>
> Right the FG idea is to always retain the frequency counts, instead of
> resetting them to 0, so folio_inc_lru_refs can promote FD accessed
> folios much more proactively, when a folio reaches LRU_REFS_MAX it
> will be promoted by one gen.
>

folio_inc_lru_refs_isolated() is only called when
referenced_ptes is true, so these should always be mapped
folios:

if (lru_gen_enabled() && !lru_gen_switching()) {
      if (!referenced_ptes)
             return FOLIOREF_RECLAIM;

      return folio_inc_lru_refs_isolated(folio, &vma_flags) ?
        FOLIOREF_ACTIVATE : FOLIOREF_KEEP;
}

When you say "FD-accessed folios" here, do you mean that
these mapped folios can also be accessed through syscalls,
and that you keep the refs to detect further FD accesses?

> I did a in depth analyze of runtime distribution, with this, we can
> see very hot folio being promoted from max_seq - 1 to max_seq, and
> after aging, the hotness info is still kept and workingset stay stable
> and accurate (it's like the global aging of traditional LFU, which is
> obsoluted in upstream due to overhead despite having better accuracy,
> but now it comes with almost zero cost).
>
> >
> > Later, I see that you set it to LRU_REFS_WORKINGSET in
> > evict_folios():
> >
> > folio_set_lru_refs(folio, min(folio_lru_refs(folio), LRU_REFS_WORKINGSET));
>
> Right that part is meant for aging the older folios and PID controller
> though. LRU_REFS_WORKINGSET could means historical workingset, and
> LRU_REFS_PROTECTED means folios being currently actively protected.
>

folio_inc_lru_refs_isolated() is called from the eviction path.
Folios that are not reclaimed will go through:

__folio_set_lru_refs(folio, min(folio_lru_refs(folio), LRU_REFS_WORKINGSET));

in evict_folios().

So LRU_REFS_PROTECTED eventually becomes
LRU_REFS_WORKINGSET(or 0, 1).

I'm still struggling to understand why we need a transition
from LRU_REFS_PROTECTED to LRU_REFS_WORKINGSET (or 0/1 if
folio_lru_refs() < LRU_REFS_WORKINGSET), since those folios
will always end up in LRU_REFS_WORKINGSET (or 0/1) anyway.

Thanks
Barry