Re: [PATCH mm-new v2] mm: mglru: clear the reference counter for rejected folios

[Baoquan He]

On 09/09/26 at 09:57am, Baolin Wang wrote:
> As per the comment on LRU_REFS_FLAGS, when accessed folios are promoted to
> a new generation, LRU_REFS_FLAGS should be cleared so that the reference
> counter can start over.
> 
> For folios rejected by shrink_folio_list(), we clear LRU_REFS_FLAGS and
> set the PG_active flag when lru_gen_folio_seq() would place them in the
> oldest generation. That's fine.
> 
> But for rejected folios where lru_gen_folio_seq() returns a generation
> other than the oldest one (which can be treated as a promotion), we do
> not clear LRU_REFS_FLAGS. This can violate the promotion mechanism. And
> this means the rejected folio enters the new generation with stale, inflated
> tier bits, which can inflate reference counts and distort eviction
> statistics for these rejected folios.
> 
> Fix this by clearing LRU_REFS_FLAGS for rejected folios. Of course, I need
> to evaluate the impact of the changes, which mainly falls into 3 cases:
> 1. When lru_gen_folio_seq() returns the oldest generation for rejected folios,
> there are no logic changes, and they will be put back into the 2nd youngest
> generation.
> 2. For rejected folios with PG_active set by shrink_folio_list(), we only
> clear the LRU_REFS_FLAGS and do not change the generation.
> 3. For rejected folios with PG_referenced set, the original code would put
> them back into the 2nd oldest generation. After this patch, we will put them
> back into the 2nd youngest generation.
> 
> I think case 3 is also reasonable, before commmit 6cbdd9726fb5 ("mm/mglru:
> use folio_mark_accessed to replace folio_set_active"), a rejected referenced
> folio was also put back to the 2nd youngest gen. Meanwhile, I didn't see
> any noticeable performance impact on my 32-core Arm machine when running
> 'make -j32' to build the kernel inside a 3G-limited memcg with either zram
> or NVMe swap.

According to our discussion in v1 thread, the patch log clearly states
the justification, and the code change looks good to me. Thanks for the
effort.

Reviewed-by: Baoquan He <baoquan.he@xxxxxxxxx>

> 
> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> ---
> Changes from v1:
>  - Update the commit message.
>  - Clear LRU_REFS_FLAGS earlier.
> ---
>  mm/vmscan.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 40d3f1b48a74..1557679aadb1 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -5020,11 +5020,12 @@ static int evict_folios(unsigned long nr_to_scan, struct lruvec *lruvec,
>  			continue;
>  		}
>  
> +		/* See the comments on LRU_REFS_FLAGS */
> +		folio_set_lru_refs(folio, 0);
> +
>  		/* don't add rejected folios to the oldest generation */
> -		if (lru_gen_folio_seq(lruvec, folio, false) == min_seq[type]) {
> -			folio_set_lru_refs(folio, 0);
> +		if (lru_gen_folio_seq(lruvec, folio, false) == min_seq[type])
>  			folio_set_active(folio);
> -		}
>  	}
>  
>  	move_folios_to_lru(&list);
> -- 
> 2.47.3
>