Re: [PATCH v7 07/16] mm/mglru: balance anon and file scan budget by refault rate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mm/mglru: non-real-name handle in Co-developed-by and
  Signed-off-by trailers
- [Medium] mm/mglru: missing DEF_PRIORITY initialization in
  lru_gen_seq_write()
- [Medium] mm/mglru: lru_gen_balance_scan() budgets un-scannable young
  generations
--

commit 730defb97ea747f35c6996e786706ce8e9143280
Author: Kairui Song <kasong@tencent.com>

> mm/mglru: balance anon and file scan budget by refault rate
>
> Replace get_type_to_scan() and isolate_folios(), which selected one
> type per batch, with a proportional split of the scan budget between
> anon and file, and scan both types each round.
>
> lru_gen_balance_scan() splits the total by
>
>         anon : file = swappiness * r_file * w_anon :
>                       (MAX_SWAPPINESS - swappiness) * r_anon * w_file
>
> where r_anon and r_file are the per-type refault rates scaled by
> 4096, clamped to [1, 4096]; a rate at the 4096 ceiling means the type
> refaults at least as often as it is evicted. The w factors bias the
> split toward the LRU sizes at mild swappiness and follow the IO cost
> ratio at the extremes. Both rate accumulators start at MIN_LRU_BATCH,
> so a type without eviction history reads as 100% until evictions
> accumulate, and the floor of 1 keeps a type that never refaults from
> zeroing out the other type's share. Swappiness 0 and
> SWAPPINESS_ANON_ONLY keep their single-type behavior; at swappiness ==
> MAX_SWAPPINESS the file factor is zero, so anon gets the whole budget,
> as in the classical LRU; swappiness 1 no longer forces file-only
> scanning as the old code did.
>
> The proportional protection and priority reduction are applied to the
> total once, before the split, and each budget is capped at its type's
> evictable size. Regular reclaim derives the total from the evictable
> sizes, so the budgets sum to it. The proactive path re-splits the
> remaining target each round, so eviction keeps working while either
> type makes progress, until the target is met or no progress is
> possible. The debugfs scan_control gets .priority = DEF_PRIORITY so
> eviction starts from the oldest gen and reports the default priority
> in the tracepoints.
>
> Signed-off-by: Kairui Song <kasong@tencent.com>
> Co-developed-by: lyugaofei <lyugaofei@xiaomi.com>
> Signed-off-by: lyugaofei <lyugaofei@xiaomi.com>
> Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>

[Severity: High]
Under Documentation/process/submitting-patches.rst, Co-developed-by: and
Signed-off-by: tags must use a real full name rather than an account
handle ("lyugaofei").

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2ddaf6181c5d4..d7b4e36cf7415 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
[ ... ]
> @@ -5052,18 +5084,18 @@ static bool should_run_aging(struct lruvec *lruvec, unsigned long max_seq,
>  	return evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS == max_seq;
>  }
>  
> -static long get_nr_to_scan(struct lruvec *lruvec, struct scan_control *sc,
> -			   struct mem_cgroup *memcg, int swappiness)
> +static void lru_gen_prepare_scan(struct lruvec *lruvec, struct scan_control *sc,
> +		struct mem_cgroup *memcg, int swappiness,
> +		unsigned long nr_to_scan[])
>  {
> -	unsigned long nr_to_scan, evictable, sizes[ANON_AND_FILE];
> -
> -	lruvec_evictable_size(lruvec, swappiness, sizes);
> -	evictable = sizes[LRU_GEN_ANON] + sizes[LRU_GEN_FILE];
> +	unsigned long total;
>  
> -	nr_to_scan = apply_proportional_protection(memcg, sc, evictable);
> -	nr_to_scan >>= sc->priority;
> +	lruvec_evictable_size(lruvec, swappiness, nr_to_scan);
> +	total = nr_to_scan[LRU_GEN_ANON] + nr_to_scan[LRU_GEN_FILE];
>  
> -	return nr_to_scan;
> +	total = apply_proportional_protection(memcg, sc, total);
> +	total >>= sc->priority;
> +	lru_gen_balance_scan(lruvec, swappiness, total, nr_to_scan);
>  }
[ ... ]
> +static long evict_folio_lists(unsigned long nr_to_scan[], struct lruvec *lruvec,
> +			     struct scan_control *sc, int swappiness, unsigned long max_batch)
> +{
> +	int type;
> +	unsigned long batch, scanned, total_scanned = 0;
> +
> +	/*
> +	 * Scan both types with the given budgets; the caller checks if
> +	 * reclaim is satisfied after each pass.
> +	 */
> +	for (type = LRU_GEN_FILE; type >= LRU_GEN_ANON; type--) {
> +		if (!nr_to_scan[type])
> +			continue;
> +
> +		batch = min(nr_to_scan[type], max_batch);
> +		scanned = evict_folios(batch, lruvec, sc, type, swappiness);
> +
> +		nr_to_scan[type] -= min(nr_to_scan[type], scanned);
> +		total_scanned += scanned;
> +	}
> +
> +	return total_scanned;
> +}

[Severity: Medium]
Can lru_gen_balance_scan() assign scan budget to a type whose scannable
generations are empty, losing that budget instead of handing it to the
other type?

Previously, isolate_folios() fell back to !type whenever scan_folios()
returned 0.  Now lru_gen_prepare_scan() caps each type's budget by
lruvec_evictable_size(), which sums all generations from min_seq[type]
through max_seq:

mm/vmscan.c:lruvec_evictable_size() {
    ...
	for_each_evictable_type(type, swappiness) {
		for (seq = min_seq[type]; seq <= max_seq; seq++)
			nr[type] += lruvec_gen_size(lrugen, type, seq);
	}
}

At sc->priority == DEF_PRIORITY (or sc->priority > DEF_PRIORITY / 2 after
commit "mm/mglru: lift youngest gen protection for higher priority"),
scan_folios() only scans older generations and cannot scan max_seq - 1 or
max_seq.  When one type has drained its scannable generations while still
holding folios in younger protected generations, lruvec_evictable_size()
reports a non-zero nr[type], so lru_gen_balance_scan() does not transfer
its share of total_scan to the other type.

When the other type receives a 0 share (such as at swappiness ==
MAX_SWAPPINESS when anon's scannable generations are empty, or when small
total_scan rounds down to 0), evict_folio_lists() returns 0 and aborts
try_to_shrink_lruvec() immediately even though the other type still has
cold evictable folios.  When both types receive non-zero budgets,
evict_folios() returns 0 on the drained type and leaves its nr_to_scan
entry unchanged, causing repeated no-op evict_folios() calls on every
MIN_LRU_BATCH pass before discarding that unspent budget.

[ ... ]
> @@ -5765,8 +5818,8 @@ static int run_aging(struct lruvec *lruvec, unsigned long seq,
>  static int run_eviction(struct lruvec *lruvec, unsigned long seq, struct scan_control *sc,
>  			int swappiness, unsigned long nr_to_reclaim)
>  {
> -	int nr_batch;
>  	DEFINE_MAX_SEQ(lruvec);
> +	unsigned long nr_to_scan[ANON_AND_FILE];
>  
>  	if (seq + MIN_NR_GENS > max_seq)
>  		return -EINVAL;
> @@ -5782,8 +5835,11 @@ static int run_eviction(struct lruvec *lruvec, unsigned long seq, struct scan_co
>  		if (sc->nr_reclaimed >= nr_to_reclaim)
>  			return 0;
>  
> -		nr_batch = min(nr_to_reclaim - sc->nr_reclaimed, MAX_LRU_BATCH);
> -		if (!evict_folios(nr_batch, lruvec, sc, swappiness))
> +		lruvec_evictable_size(lruvec, swappiness, nr_to_scan);
> +		lru_gen_balance_scan(lruvec, swappiness,
> +				     nr_to_reclaim - sc->nr_reclaimed, nr_to_scan);
> +
> +		if (!evict_folio_lists(nr_to_scan, lruvec, sc, swappiness, MAX_LRU_BATCH))
>  			return 0;

[Severity: Medium]
Was .priority = DEF_PRIORITY accidentally dropped from the scan_control in
lru_gen_seq_write()?

The commit message notes that the debugfs scan_control gets
.priority = DEF_PRIORITY so eviction starts from the oldest generation and
reports the default priority in tracepoints, but lru_gen_seq_write() leaves
sc.priority zero-initialized:

mm/vmscan.c:lru_gen_seq_write() {
    ...
	struct scan_control sc = {
		.may_writepage = true,
		.may_unmap = true,
		.may_swap = true,
		.reclaim_idx = MAX_NR_ZONES - 1,
		.gfp_mask = GFP_KERNEL,
		.proactive = true,
	};
    ...
}

With sc.priority == 0, run_eviction() -> evict_folio_lists() ->
evict_folios() -> scan_folios() takes the sc->priority != DEF_PRIORITY
branch (seq = lrugen->max_seq) and can evict from the youngest generations
whenever older generations of a type are empty, while reporting priority 0
in trace_mm_vmscan_lru_shrink_inactive().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111232603.9934-1-baohua@kernel.org?part=7