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

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

[Hui Zhu]

From: Hui Zhu <zhuhui@kylinos.cn>

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@kernel.org>
Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
---
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);
 	/*
 	 * Lazyfree folios are clean anonymous folios.  They have
 	 * the swapbacked flag cleared, to distinguish them from normal
-- 
2.53.0

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

[Barry Song]

On Mon, Oct 26, 2026 at 4:40 PM Hui Zhu <hui.zhu@linux.dev> wrote:
>
> From: Hui Zhu <zhuhui@kylinos.cn>
>
> 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@kernel.org>
> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
> ---
> 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);

with that, feel free to add:

Reviewed-by: Barry Song <baohua@kernel.org>

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@linux.dev> wrote:
>> From: Hui Zhu <zhuhui@kylinos.cn>
>>
>> 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@kernel.org>
>> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
>> ---
>> 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@kernel.org>

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

[Barry Song]

On Tue, Oct 27, 2026 at 3:25 PM Hui Zhu <hui.zhu@linux.dev> wrote:
[...]
> >> +       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:
>

You’re right. Thanks for the thorough investigation.

Reviewed-by: Barry Song <baohua@kernel.org>

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

[Kairui Song]

On Mon, Oct 26, 2026 at 4:41 PM Hui Zhu <hui.zhu@linux.dev> wrote:
>
> From: Hui Zhu <zhuhui@kylinos.cn>
>
> 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@kernel.org>
> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
> ---
> 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.

And folio_set_lru_refs(.., 0) should be a little bit faster and unify
CLRU / MGLRU. And we can detect lazy freed folios in sort_folios to
avoid PID form touching them?

Something like:
diff --git a/mm/folio.c b/mm/folio.c
index 35e242b48870..416cd4beb182 100644
--- a/mm/folio.c
+++ b/mm/folio.c
@@ -599,10 +599,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);
+       folio_set_lru_refs(folio, 0);
        /*
         * Lazyfree folios are clean anonymous folios.  They have
         * the swapbacked flag cleared, to distinguish them from normal
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6a1af9b92349..e64b7f544d15 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -4835,6 +4835,10 @@ static bool sort_folio(struct lruvec *lruvec,
struct folio *folio, struct scan_c
        if (!folio_evictable(folio))
                return false;

+       /* No need to protect lazyfreed folios */
+       if (folio_test_lazyfree(folio) && !folio_test_dirty(folio))
+               return false;
+
        /* promoted */
        if (gen != lru_gen_from_seq(lrugen->min_seq[type])) {
                list_move(&folio->lru, &lrugen->folios[gen][type][zone]);

---

BTW I also tried to cover this in MGLRU-FG series (not including the
sort_folio part):
https://lore.kernel.org/linux-mm/20261016-mglru-fg-v4-13-2106750c0bfa@tencent.com/

I tested keeping min(refs, LRU_REFS_WORKINGSET) / just set to 0 as in
v4, nothing different from the workload (testing using Android apps as
workload), so I think it's fine either way but the above patch I
posted might help to impove it by a little?

And if CLRU didn't actually expect PG_workingset to be kept but that's
just a leftover or trivial, maybe we can always do both
folio_clear_workingset and folio_set_lru_refs(0) here?