Re: [RFC PATCH v5 07/14] mm/mglru: balance anon and file scan budget by refault rate

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mm/mglru: lru_gen_balance_scan() budgets un-scannable gens and zones
--

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

mm/mglru: balance anon and file scan budget by refault rate

This commit replaces get_type_to_scan() and isolate_folios() with
lru_gen_balance_scan() and evict_folio_lists() to split the scan budget
proportionally between anon and file LRUs based on per-type refault rates
and LRU sizes.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index cc20ba8c6cf2c..fed39b0422e01 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -5019,79 +5019,110 @@ static int get_tier_idx(struct lruvec *lruvec, int type)

[ ... ]

> +static void lru_gen_balance_scan(struct lruvec *lruvec, int swappiness,
> +		unsigned long total_scan, unsigned long nr[])
>  {

[ ... ]

> -	/*
> -	 * We are running out of the current reclaim type. Fall back to
> -	 * the other type if allowed.
> +	 * The more extreme the swappiness, the wider the IO cost gap between
> +	 * anon and file, so the split should follow the IO cost ratio rather
> +	 * than the sizes. With a mild swappiness, balance the scan by the
> +	 * actual LRU size more.
>  	 */
> -	if (!scanned && !tried && type_fallback_allowed) {
> -		type = !type;
> -		tried = true;
> -		type_fallback_allowed = false;
> -		goto retry;
> -	}
> +	size_bias = max(1, min(swappiness, MAX_SWAPPINESS - swappiness));
> +	anon_weight = nr[LRU_GEN_ANON] + nr[LRU_GEN_FILE] / size_bias;
> +	file_weight = nr[LRU_GEN_FILE] + nr[LRU_GEN_ANON] / size_bias;
> +
> +	anon_factor = (u64)swappiness * refault_rate[LRU_GEN_FILE] * anon_weight;
> +	file_factor = (u64)(MAX_SWAPPINESS - swappiness) * refault_rate[LRU_GEN_ANON] * file_weight;
> +
> +	/* Cross-multiply the ratio above. */
> +	anon_scan = mul_u64_u64_div_u64(total_scan, anon_factor, anon_factor + file_factor);
> +	file_scan = (u64)total_scan - anon_scan;
> +
>  	/*
> -	 * We scanned some folios but failed to isolate any due to promotions,
> -	 * protections, or races. Retry once to avoid a larger loop.
> +	 * Bound each type by its evictable size. Hand the exceeding part to
> +	 * the other type. Any excess left when total_scan exceeds both sizes
> +	 * is dropped, since it only stresses the scanner with no gain.
>  	 */
> -	if (scanned && !tried) {
> -		tried = true;
> -		goto retry;
> -	}
> -
> -	return total_scanned;
> +	if (anon_scan > nr[LRU_GEN_ANON])
> +		file_scan += anon_scan - nr[LRU_GEN_ANON];
> +	if (file_scan > nr[LRU_GEN_FILE])
> +		anon_scan += file_scan - nr[LRU_GEN_FILE];
> +	nr[LRU_GEN_ANON] = min_t(u64, anon_scan, nr[LRU_GEN_ANON]);
> +	nr[LRU_GEN_FILE] = min_t(u64, file_scan, nr[LRU_GEN_FILE]);
>  }

[ ... ]

> @@ -5212,22 +5244,22 @@ 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];
> +	unsigned long total;
>  
> -	lruvec_evictable_size(lruvec, swappiness, sizes);
> -	evictable = sizes[LRU_GEN_ANON] + sizes[LRU_GEN_FILE];
> +	lruvec_evictable_size(lruvec, swappiness, nr_to_scan);
> +	total = nr_to_scan[LRU_GEN_ANON] + nr_to_scan[LRU_GEN_FILE];
>  
> -	/* try to scrape all its memory if this memcg was deleted */
> +	/* Just scrape all pages calculated above if this memcg was deleted */
>  	if (!mem_cgroup_online(memcg))
> -		return evictable;
> -
> -	nr_to_scan = apply_proportional_protection(memcg, sc, evictable);
> -	nr_to_scan >>= sc->priority;
> +		return;
>  
> -	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 bool should_abort_scan(struct lruvec *lruvec, struct scan_control *sc)
> @@ -5260,6 +5292,30 @@ static bool should_abort_scan(struct lruvec *lruvec, struct scan_control *sc)
>  	return true;
>  }
>  
> +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;
> +}

[ ... ]

> @@ -5948,8 +6001,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;
>  
>  		cond_resched();

[Severity: Medium]
Can budgeting with lruvec_evictable_size() in lru_gen_balance_scan() cause
lost scan budget and premature reclaim termination in try_to_shrink_lruvec()
and run_eviction() when one type has no folios in scannable generations or
eligible zones?

Previously, isolate_folios() in mm/vmscan.c fell back to scanning !type
whenever scan_folios() returned scanned == 0. With isolate_folios() removed,
lru_gen_prepare_scan() and run_eviction() rely on lruvec_evictable_size()
and lru_gen_balance_scan() to cap each type's budget by nr[type] and hand
any excess to the other type.

However, lruvec_evictable_size() sums folios across all generations up to
max_seq (including the protected youngest generations max_seq - 1 and
max_seq) and across all zones:

mm/vmscan.c:lruvec_evictable_size() {
	for_each_evictable_type(type, swappiness) {
		for (seq = min_seq[type]; seq <= max_seq; seq++) {
			gen = lru_gen_from_seq(seq);
			for (zone = 0; zone < MAX_NR_ZONES; zone++)
				nr[type] += max(READ_ONCE(lrugen->nr_pages[gen][type][zone]), 0L);
		}
	}
}

In contrast, scan_folios() only scans zones 0..sc->reclaim_idx, and at
sc->priority == DEF_PRIORITY (or after the later commit "mm/mglru: lift
youngest gen protection for higher priority" in this series, which caps
scanning at max_seq - MIN_NR_GENS) it does not scan the youngest generations:

mm/vmscan.c:scan_folios() {
	if (sc->priority == DEF_PRIORITY)
		seq = lrugen->min_seq[type];
	else
		seq = lrugen->max_seq;

	seq = lruvec_populated_min_seq(lruvec, type, sc->reclaim_idx + 1, seq);
}

When one type has no scannable folios in eligible generations or zones (for
example, after its cold generations <= max_seq - MIN_NR_GENS are drained or
when its folios reside in zones > sc->reclaim_idx, such as under GFP_DMA32),
lruvec_evictable_size() still reports a large non-zero nr[type] from the
active generations or higher zones. As a result, lru_gen_balance_scan() does
not cap that type's budget or transfer it to the scannable type.

In the proactive reclaim path (lru_gen_seq_write() -> run_cmd() ->
run_eviction()), sc.priority is fixed at DEF_PRIORITY. Once one type drains
its cold generations while retaining folios in max_seq - 1 or max_seq, any
round where the other type's share rounds down to 0 in:

	anon_scan = mul_u64_u64_div_u64(total_scan, anon_factor, anon_factor + file_factor);
	file_scan = (u64)total_scan - anon_scan;

(for instance, whenever nr_to_reclaim - sc->nr_reclaimed == 1 for swappiness
between 1 and 199, or for larger remaining targets when one factor dominates,
or at swappiness == MAX_SWAPPINESS when anon's cold generations are empty)
assigns the entire remaining budget to the drained type and 0 to the type
that still has cold folios. evict_folio_lists() then scans 0 from the drained
type, skips the scannable type, and returns 0, causing run_eviction() to
abort before meeting nr_to_reclaim.

Similarly, in regular reclaim (try_to_shrink_lruvec() ->
lru_gen_prepare_scan() -> lru_gen_balance_scan() -> evict_folio_lists()),
when evict_folios() returns scanned == 0 for the un-scannable type:

	nr_to_scan[type] -= min(nr_to_scan[type], scanned);

leaves nr_to_scan[type] unchanged. While the scannable type still has budget,
evict_folio_lists() repeatedly calls evict_folios() (acquiring
lruvec->lru_lock) on the empty type on every MIN_LRU_BATCH iteration, and
once the scannable type's share is consumed, the empty type's remaining
budget is lost instead of being transferred to the scannable type.

Should lru_gen_balance_scan() bound each type by the size of its scannable
generations and eligible zones, or should evict_folio_lists() transfer the
remaining budget to the other type when evict_folios() returns 0?

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