Re: [PATCH v2] mm/mglru: fix a no-op lru_gen_clear_refs() call in lru_lazyfree()

[Kairui Song]

On Sat, Oct 31, 2026 at 3:25 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
>
> On Sat, Oct 31, 2026 at 10:42 AM Barry Song <baohua@xxxxxxxxxx> wrote:
> >
> > On Tue, Oct 27, 2026 at 7:40 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> > >
> > > On Mon, Oct 26, 2026 at 4:41 PM Hui Zhu <hui.zhu@xxxxxxxxx> wrote:
> > > >
> > > > From: Hui Zhu <zhuhui@xxxxxxxxxx>
> > > >
> > > > lru_lazyfree() calls lru_gen_clear_refs() after lruvec_del_folio(), but
> > > > lru_gen_del_folio() has already cleared LRU_GEN_MASK, so folio_lru_gen()
> > > > returns -1 and lru_gen_clear_refs() exits at its "gen < 0" check without
> > > > clearing the refs or PG_workingset.
> > > >
> > > > A MADV_FREE folio therefore keeps the state it had before madvise(2),
> > > > even though madvise_free_pte_range() has already cleared the accessed and
> > > > dirty bits of its PTEs.  lru_gen_folio_seq() places it one generation
> > > > younger, and PG_workingset puts it in the last tier, so sort_folio()
> > > > promotes and protects it instead of evicting it and charges it to
> > > > lrugen->protected[], which read_ctrl_pos() uses to balance anon against
> > > > file reclaim.  __folio_inc_gen() clears the refs but not PG_workingset,
> > > > so this repeats every time the folio reaches the oldest generation.  If
> > > > the folio is redirtied first, ttu_anon_lazyfree_folio() keeps it and
> > > > lru_gen_eviction() packs the stale state into the shadow entry, so a
> > > > later refault is promoted to the youngest generation.
> > > >
> > > > Call lru_gen_clear_refs() while the folio is still on the generation
> > > > list, as deactivate_file_folio() and folio_deactivate() already do.  Its
> > > > return value only says whether the folio is in the oldest generation,
> > > > which lru_lazyfree() does not need because it re-adds the folio below.
> > > > Keep folio_clear_referenced() unconditional: the classic LRU needs it,
> > > > and it also covers the folios that lru_gen_clear_refs() skips.
> > > >
> > > > Fixes: cc8ec7be78ff ("mm/mglru: optimize deactivation")
> > > > Suggested-by: Barry Song <baohua@xxxxxxxxxx>
> > > > Signed-off-by: Hui Zhu <zhuhui@xxxxxxxxxx>
> > > > ---
> > > > Changelog:
> > > > v2:
> > > > Call lru_gen_clear_refs() before lruvec_del_folio() instead of
> > > > open-coding the flag clears; drop the lru_gen_enabled() branch and
> > > > the comment (Barry Song).
> > > >
> > > >  mm/folio.c | 6 ++----
> > > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/mm/folio.c b/mm/folio.c
> > > > index 35e242b48870..ea3adb6a89d7 100644
> > > > --- a/mm/folio.c
> > > > +++ b/mm/folio.c
> > > > @@ -597,12 +597,10 @@ static void lru_lazyfree(struct lruvec *lruvec, struct folio *folio)
> > > >             folio_test_swapcache(folio) || folio_test_unevictable(folio))
> > > >                 return;
> > > >
> > > > +       lru_gen_clear_refs(folio);
> > > >         lruvec_del_folio(lruvec, folio);
> > > >         folio_clear_active(folio);
> > > > -       if (lru_gen_enabled())
> > > > -               lru_gen_clear_refs(folio);
> > > > -       else
> > > > -               folio_clear_referenced(folio);
> > > > +       folio_clear_referenced(folio);
> > >
> > > Why can't we just call folio_set_lru_refs(.., 0), and should we clear
> > > PG_workingset? Keeping it helps PSI on refualt / redirty tracking (not
> > > sure if meaningful but CLRU does this), clearing it makes eviction
> > > better, but the eviction issue is MGLRU-only, CLRU keeps it.
> >
> > I recall that in v1 [1], Hui mentioned:
> >
> > "Use folio_clear_referenced() rather than
> > folio_set_lru_refs(), which is not defined without
> > CONFIG_LRU_GEN."
> >
>
> Or prehaps we can have a folio_clear_lru_refs() do in a unified way
> for For both CLRU / MGLRU? (Just like folio_migrate_lru_refs).

And it will be better if we can keep this operation off-list just like
right now :), some later works could benefit from it.