Re: [PATCH RFC 09/15] mm/mglru: frequency guided workingset promotion (MGLRU-FG)

On Sat, Aug 15, 2026 at 2:06 AM Kairui Song <[email protected]> wrote:
>
> On Fri, Aug 14, 2026 at 3:19 PM Barry Song <[email protected]> wrote:
> >
> > On Tue, Aug 4, 2026 at 3:47 AM Kairui Song via B4 Relay
> > <[email protected]> wrote:
> > >
> > > From: Kairui Song <[email protected]>
> > >
> > > Complement MGLRU's eviction-time tier-PID protection with access-time
> > > frequency-guided promotion.  Introduce a unified set of helpers built based
> > > on referenced (access) count of a folio.
> > >
> > > Each access increments a folio's referenced count stored in folio flags
> > > (refs), refs still mappes to a logarithmic tier just like before, but with
> > > more formal bit definitions, a few special thresholds are introduced:
> > > LRU_REFS_REFERENCED (1), LRU_REFS_WORKINGSET (2), LRU_REFS_PROTECTED (3),
> > > and LRU_REFS_MAX(7). When it reaches certain threshold, the folio is
> > > promoted proactively instead of wait for the PID controller to kick in.
> > >
> > > Also simplify MGLRU's usage of PG_workingset and PG_referenced, now
> > > these 2 flags are purely used as the lower 2 bit of refs for MGLRU. This
> > > doesn't effect classical LRU in any way. This will actually simplify and
> > > make MGLRU's certain metric reading more accurate, and reduced MGLRU's
> > > original tier / referenced count bit by one since only one extra bit is
> > > now needed to record a max referenced count of 7 (previously 2 extra bits
> > > are needed). This changes make sense because MGLRU doesn't have demotion
> > > so these 2 flags are never separately useful for MGLRU.
> > >
> > > This addresses several shortcomings of the old model:
> > [...]
> >
> > > + * Update the folio's lru refs indicator without taking the folio lock,
> > > + * isolation, or lruvec lock. Used by both page table access (@is_fault=true)
> > > + * and by file access (@is_fault=false).
> > > + */
> > > +int folio_inc_lru_refs(struct folio *folio, bool is_fault, bool is_exec)
> > > +{
> >
> >
> > This is really hard to follow, especially with call sites like:
> >
> > folio_inc_lru_refs(folio, true, false);
> > folio_inc_lru_refs(folio, false, false);
> >
> > The reader needs to remember what the first true and the second
> > false represent, which makes the call sites quite opaque.
> >
> > Could we have something like this instead?
> >
> > #define LRU_REFS_FAULT BIT(0)
> > #define LRU_REFS_EXEC BIT(1)
> >
> > folio_inc_lru_refs(folio, LRU_REFS_FAULT);
> > folio_inc_lru_refs(folio, 0);
> > folio_inc_lru_refs(folio, LRU_REFS_EXEC);
> > folio_inc_lru_refs(folio, LRU_REFS_EXEC | LRU_REFS_FAULT);
>
> Thanks very much for the review and suggestion!
>
> Good idea, but to distinguish from macros like LRU_REFS_PROTECTED and
> LRU_REFS_WORKINGSET, which are special referenced count number, maybe
> LRU_REF_EXEC and LRU_REF_FAULT? BTW we have macros like
> FOLIOREF_RECLAIM_CLEAN, FOLIOREF_KEEP, and folio_referenced, etc.,
> which are commonly used already, that's one main hint behind the name
> choice here.

Yep, LRU_REF_FAULT and LRU_REF_EXEC sound good to me.

BTW, is FAULT really the right term here? It looks like you're
actually using it to indicate the result of PTE scanning.

In the other place, you use "promote" instead:

+static inline int folio_inc_lru_refs(struct folio *folio, bool
promote, bool is_exec)
+{
+       return 0;
+}

So perhaps LRU_REF_MAPPED  would be more appropriate?

Best Regards
Barry