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

[Hui Zhu]

2026/10/26 17:47, Barry Song:
> On Mon, Oct 26, 2026 at 4:40 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);
> The only thing I'm not sure about is whether we need
> if (lru_gen_enabled()) here to keep the semantics consistent
> with other places where we use it.
>
> ...
>     2    676  mm/folio.c <<deactivate_file_folio>>
>               if (lru_gen_enabled() && lru_gen_clear_refs(folio))
>     3    695  mm/folio.c <<folio_deactivate>>
>               if (lru_gen_enabled() ? lru_gen_clear_refs(folio) :
> !folio_test_active(folio))
>
> So maybe we still need
>
> + if (lru_gen_enabled())
>          lru_gen_clear_refs(folio);

I think we maybe don't need it here.  The other two sites consume the
return value, and that is what the check disambiguates:

    if (gen < 0)
        return true;

An off-list folio has gen == -1, so without lru_gen_enabled() both
deactivate_file_folio() and folio_deactivate() would take that "true"
and return early, skipping the deactivation.  lru_lazyfree() ignores
the return value, since it re-adds the folio a few lines below anyway.

The side effects gate themselves, and on something more precise than
the global key: a folio on a classic list always has gen == -1, which
fill_evictable() even asserts,

    VM_WARN_ON_ONCE_FOLIO(folio_lru_gen(folio) != -1, folio);

and drain_evictable() clears LRU_GEN_MASK on the way out.  So an
unguarded call either clears the refs of a folio that is on a
generation list, or does nothing at all - no flag writes and no lruvec
access.  The classic LRU cannot be affected, including with
CONFIG_LRU_GEN=n, where the stub inlines away.

Guarding on the key would reopen a narrower version of this bug.
lru_gen_change_state() flips it before migrating any folio:

    if (enabled)
static_branch_enable_cpuslocked(&lru_gen_caps[LRU_GEN_CORE]);
    else
static_branch_disable_cpuslocked(&lru_gen_caps[LRU_GEN_CORE]);

    memcg = mem_cgroup_iter(NULL, NULL, NULL);

so while MGLRU is being turned off the key is already false, but every
lruvec that drain_evictable() has not reached yet still has its folios
on generation lists.  A MADV_FREE batch drained on another CPU in that
window would keep PG_workingset.

What about add a comment instead of the check:

    /* no-op unless the folio is on a generation list */
    lru_gen_clear_refs(folio);

Best,
Hui

> with that, feel free to add:
>
> Reviewed-by: Barry Song <baohua@xxxxxxxxxx>