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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [Medium] mm/mglru: lru_gen_balance_scan() budgets un-scannable
  generations
- [Low] mm/mglru: lru_gen_prepare_scan() re-adds obsolete offline memcg
  check
- [Low] mm/mglru: u64 overflow in lru_gen_balance_scan() excess budget
  handoff
- [Low] mm/mglru: unzeroed budget on empty scan in evict_folio_lists()
--

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

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

This commit replaces MGLRU's single-type-per-batch selection with
lru_gen_balance_scan() and evict_folio_lists() to split the scan budget
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 e2807faf26deb..6b325fe73436a 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4858,79 +4858,110 @@ static int get_tier_idx(struct lruvec *lruvec, int type)

[ ... ]

> +	/* 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]);
>  }

[Severity: Low]
Can anon_scan overflow u64 when total_scan exceeds total and both if
branches run?

In lru_gen_seq_write() -> run_cmd() -> run_eviction(), the optional
nr_to_reclaim argument defaults to ULONG_MAX, so total_scan passed to
lru_gen_balance_scan() can be near U64_MAX.  When anon_scan exceeds
nr[LRU_GEN_ANON], the first branch sets file_scan to
total_scan - nr[LRU_GEN_ANON] without clamping anon_scan, and the second
branch then adds total_scan - total to the unclamped anon_scan.  If
anon_scan + total_scan - total wraps modulo 2^64 to a value smaller than
nr[LRU_GEN_ANON], nr[LRU_GEN_ANON] is truncated below the available
anonymous size.  Using else if (or clamping total_scan to total upfront)
avoids the overflow.

[ ... ]

> @@ -5051,18 +5083,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];
>  
> -	nr_to_scan = apply_proportional_protection(memcg, sc, evictable);
> -	nr_to_scan >>= sc->priority;
> +	/* Just scrape all pages calculated above if this memcg was deleted */
> +	if (!mem_cgroup_online(memcg))
> +		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);
>  }

[Severity: Low]
Does this re-introduce the !mem_cgroup_online(memcg) early return that
2e203d692b0cb ("mm/vmscan: remove redundant !mem_cgroup_online() checks")
removed from get_nr_to_scan()?

Offlined memcgs already have all LRU folios reparented and
lruvec_evictable_size() equal to 0, so returning early here only skips
apply_proportional_protection() and misses setting sc->memcg_low_skipped
when an offlined memcg still has stale effective memory.low protection.

[ ... ]

> @@ -5095,6 +5131,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;
> +}

[Severity: Low]
When evict_folios() returns 0 for one type because that type has no folios
in scannable generations, shouldn't nr_to_scan[type] be cleared to 0?

Leaving nr_to_scan[type] unchanged causes try_to_shrink_lruvec() to call
evict_folios() on the empty type on every subsequent MIN_LRU_BATCH pass
while the other type still has budget, and then run one extra iteration
(including should_run_aging() and try_to_inc_max_seq()) after the
scannable type's budget reaches 0.

[ ... ]

> @@ -5781,8 +5838,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 lru_gen_balance_scan() assign scan budget to a type that has no folios
in scannable generations instead of handing that budget to the other type?

Previously, isolate_folios() fell back to !type whenever scan_folios()
returned 0.  Now, lru_gen_balance_scan() only transfers excess budget when
a type's share exceeds nr[type] from lruvec_evictable_size(), which sums
folios across all generations min_seq[type]..max_seq including protected
younger generations (such as max_seq - 1 and max_seq at DEF_PRIORITY).

When one type has drained all folios in its scannable cold generations
while still holding folios in younger generations, lruvec_evictable_size()
remains non-zero and lru_gen_balance_scan() keeps assigning budget to that
type.  In try_to_shrink_lruvec(), that share of the scan budget is lost;
in run_eviction() (where priority is fixed at DEF_PRIORITY), if the
remaining scannable type's share rounds down to 0 (for example when
nr_to_reclaim - sc->nr_reclaimed is small or at swappiness ==
MAX_SWAPPINESS), evict_folio_lists() scans nothing and aborts proactive
reclaim early while the other type still has cold folios.

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