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

[Hui Zhu]

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.

2. Follow up with option B as a pure cleanup, no Fixes tag. The
   lazyfree site then becomes the off-list __lru_gen_clear_refs()
   call that Kairui wants for later work.

The reason is backporting. Every stable from 6.14 (cc8ec7be78ff) to
7.3 carries the regression, but B is written against
folio_set_lru_refs(), which only landed in this merge window
(64169ceea5b1e). None of the affected trees has that helper, so
backporting B would drag the refs-flags rework along with it. v2
applies to all of them with only the mm/swap.c -> mm/folio.c rename
to adjust.

B also starts clearing PG_workingset on the classic LRU. I agree
with Kairui that this is the right semantics for MADV_FREE, but it
is a change beyond this regression and deserves its own patch - it
fits naturally in the MGLRU-FG flag cleanup, as does the sort_folio()
lazyfree check.

Best,
Hui


> Best,
> Barry