Re: [PATCH v3 2/7] mm/mglru: batch update lrugen->nr_pages in inc_min_seq()

[Baolin Wang]

On 9/2/26 7:24 AM, Barry Song (Xiaomi) wrote:
> Currently, folio_inc_gen() updates lrugen->nr_pages for every folio
> as it advances generations. Instead, accumulate the size changes
> and update lrugen->nr_pages in a batch after scanning the entire
> oldest generation, or when the scan stops because remaining reaches
> zero.
>
> Since we only move folios from the oldest generation to the second
> oldest generation, the active/inactive state cannot change. We can
> therefore skip __lru_update_size().

Make sense to me.

> Signed-off-by: Barry Song (Xiaomi) <baohua@xxxxxxxxxx>
> Tested-by: Xueyuan Chen <xueyuan.chen21@xxxxxxxxx>
> Reviewed-by: Lian Wang <lianux.mm@xxxxxxxxx>
> Reviewed-by: Kunwu Chan <kunwu.chan@xxxxxxxxx>
> ---

LGTM. One nit follows.
Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>

>   mm/vmscan.c | 23 ++++++++++++++++++-----
>   1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 54bce2f608ef..27494505cecc 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -3918,6 +3918,7 @@ static bool inc_min_seq(struct lruvec *lruvec, int type, int swappiness)
>   	struct lru_gen_folio *lrugen = &lruvec->lrugen;
>   	int hist = lru_hist_from_seq(lrugen->min_seq[type]);
>   	int new_gen, old_gen = lru_gen_from_seq(lrugen->min_seq[type]);
> +	int target_gen = (old_gen + 1) % MAX_NR_GENS;
>   
>   	/* For file type, skip the check if swappiness is anon only */
>   	if (type && (swappiness == SWAPPINESS_ANON_ONLY))
> @@ -3927,35 +3928,47 @@ static bool inc_min_seq(struct lruvec *lruvec, int type, int swappiness)
>   	if (!type && !swappiness)
>   		goto done;
>   
> +	VM_WARN_ON_ONCE(get_nr_gens(lruvec, type) != MAX_NR_GENS);
> +	VM_WARN_ON_ONCE(lru_gen_is_active(lruvec, old_gen) !=
> +			lru_gen_is_active(lruvec, target_gen));
>   	/* prevent cold/hot inversion if the type is evictable */
>   	for (zone = 0; zone < MAX_NR_ZONES; zone++) {
>   		struct list_head *head = &lrugen->folios[old_gen][type][zone];
> +		long delta = 0;
>   
>   		while (!list_empty(head)) {
>   			struct folio *folio = lru_to_folio(head);
> +			long nr_pages = folio_nr_pages(folio);
>   			int refs = folio_lru_refs(folio);
>   			bool workingset = folio_test_workingset(folio);
> +			bool gen_increased;
>   
>   			VM_WARN_ON_ONCE_FOLIO(folio_test_unevictable(folio), folio);
>   			VM_WARN_ON_ONCE_FOLIO(folio_test_active(folio), folio);
>   			VM_WARN_ON_ONCE_FOLIO(folio_is_file_lru(folio) != type, folio);
>   			VM_WARN_ON_ONCE_FOLIO(folio_zonenum(folio) != zone, folio);
>   
> -			new_gen = folio_inc_gen(lruvec, folio);
> +			new_gen = __folio_inc_gen(folio, old_gen, &gen_increased);

IMO, it's better to add some comments to describe why we don't need to 
call __lru_update_size(), in case someone thinks this needs to be fixed 
in the future. :)