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

[Kairui Song]

On Fri, Nov 6, 2026 at 4:38 PM Hui Zhu <hui.zhu@xxxxxxxxx> wrote:
> 2026/11/6 05:13, Barry Song:
> > On Sat, Oct 31, 2026 at 3:27 PM Kairui Song <ryncsn@xxxxxxxxx> wrote:
> > [...]
> >>>>>>   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.
> > lru_gen_clear_refs() is currently multifunctional. It clears
> > refs/workingset, but also returns true to skip
> > folio_batch_add_and_move() in both deactivate_file_folio()
> > and folio_deactivate().
> >
> > This actually looks a bit weird. However, following the current
> > logic, I've drafted a version that we can use as a starting point
> > for discussion:
> >
> > diff --git a/mm/folio.c b/mm/folio.c
> > index dc2c796a4ea7..bc17cd3b6b6c 100644
> > --- a/mm/folio.c
> > +++ b/mm/folio.c
> > @@ -444,12 +444,12 @@ static bool lru_gen_clear_refs(struct folio *folio)
> >          int type = folio_is_file_lru(folio);
> >          unsigned long seq;
> >
> > -       if (gen < 0)
> > -               return true;
> > -
> >          folio_set_lru_refs(folio, 0);
> >          folio_clear_workingset(folio);
> >
> > +       if (gen < 0)
> > +               return true;
> > +
> >          rcu_read_lock();
> >          seq = READ_ONCE(folio_lruvec(folio)->lrugen.min_seq[type]);
> >          rcu_read_unlock();
> > @@ -465,6 +465,8 @@ static void lru_gen_inc_refs(struct folio *folio)
> >
> >   static bool lru_gen_clear_refs(struct folio *folio)
> >   {
> > +       folio_clear_referenced(folio);
> > +       folio_clear_workingset(folio);
> >          return false;
> >   }
> >
> > The another option is:
> >
> > diff --git a/mm/folio.c b/mm/folio.c
> > index dc2c796a4ea7..8d1fd788188c 100644
> > --- a/mm/folio.c
> > +++ b/mm/folio.c
> > @@ -438,6 +438,12 @@ static void lru_gen_inc_refs(struct folio *folio)
> >          } while (!try_cmpxchg(folio_flags(folio, 0), &old_flags, new_flags));
> >   }
> >
> > +static inline void __lru_gen_clear_refs(struct folio *folio)
> > +{
> > +       folio_set_lru_refs(folio, 0);
> > +       folio_clear_workingset(folio);
> > +}
> > +
> >   static bool lru_gen_clear_refs(struct folio *folio)
> >   {
> >          int gen = folio_lru_gen(folio);
> > @@ -447,8 +453,7 @@ static bool lru_gen_clear_refs(struct folio *folio)
> >          if (gen < 0)
> >                  return true;
> >
> > -       folio_set_lru_refs(folio, 0);
> > -       folio_clear_workingset(folio);
> > +       __lru_gen_clear_refs(folio);
> >
> >          rcu_read_lock();
> >          seq = READ_ONCE(folio_lruvec(folio)->lrugen.min_seq[type]);
> > @@ -463,9 +468,10 @@ static void lru_gen_inc_refs(struct folio *folio)
> >   {
> >   }
> >
> > -static bool lru_gen_clear_refs(struct folio *folio)
> > +static inline void __lru_gen_clear_refs(struct folio *folio)
> >   {
> > -       return false;
> > +       folio_clear_referenced(folio);
> > +       folio_clear_workingset(folio);
> >   }
> >
> >   #endif /* CONFIG_LRU_GEN */
> > @@ -659,10 +665,7 @@ static void lru_lazyfree(struct lruvec *lruvec,
> > struct folio *folio)
> >
> >          lruvec_del_folio(lruvec, folio);
> >          folio_clear_active(folio);
> > -       if (lru_gen_enabled())
> > -               lru_gen_clear_refs(folio);
> > -       else
> > -               folio_clear_referenced(folio);
> > +       __lru_gen_clear_refs(folio);
> >          /*
> >           * Lazyfree folios are clean anonymous folios.  They have
> >           * the swapbacked flag cleared, to distinguish them from normal
>
>
> Hi Barry, Kairui,
>
> >
> > I would personally prefer the latter. What do you think, Hui and Kairui?
>
> I prefer option B too: it keeps the flag clearing independent of the
> list state and leaves the two deactivate callers untouched.
>
> But I'd suggest splitting this in two:
>
> 1. Take v2 as the fix now. If that is acceptable, I will respin it
>     with Cc: stable added.

OK, that makes sense, agree that we can patch that regression first as
a clear backport indeed. Thanks!