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

[Hui Zhu <hui.zhu@linux.dev>]

2026/11/12 01:21, Liam R. Howlett:

> On 26/11/09 10:16AM, Hui Zhu 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")
>> Cc: stable@vger.kernel.org
>> Suggested-by: Barry Song <baohua@kernel.org>
>> Signed-off-by: Hui Zhu <zhuhui@kylinos.cn>
>> Reviewed-by: Barry Song <baohua@kernel.org>
> One nit..
>
> Reviewed-by: Liam R. Howlett (Google) <liam@infradead.org>
>
>> ---
>> Changelog:
>> v3:
>> Add Cc: stable@vger.kernel.org and pick up Barry's Reviewed-by.
>> No code change compared to v2.
>> Per the thread discussion, the __lru_gen_clear_refs() refactor will be
>> sent as a separate cleanup after this fix lands.
>> 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 35e242b48870b..ea3adb6a89d75 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);
> Since you're not checking the return here, you could use
> folio_set_lru_refs(folio, 0);
> folio_clear_workingset(folio);

Thanks for the review.

That is exactly the shape we agreed on for the follow-up cleanup in
this thread, Barry's __lru_gen_clear_refs() draft. I would still keep
v3 as it is, for two reasons.

folio_set_lru_refs() is only defined under CONFIG_LRU_GEN, so the
open-coded version needs a new stub in mm_inline.h. That turns this
one-spot fix into the header change we planned for the cleanup patch.

The two calls are also unconditional, so on the classic LRU every
lazyfree folio would lose PG_workingset too, which upstream never did
before cc8ec7be78ff. Barry and Kairui just agreed to take this patch
as a clean backport for stable, so that CLRU behaviour change should
go into the cleanup patch with its own justification.

lru_gen_clear_refs() is self-gated on gen >= 0, so the call is a
no-op for folios that are not on a generation list, and the return
value it computes shares the cacheline lruvec_del_folio() touches
right after. I will send the cleanup, with the open-coded clearing
and the stub in one place, once this lands.

Best,
Hui

>
>>   	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.43.0
>>