Re: [PATCH v7 11/16] mm/mglru: Run aging when one type has no populatable folios

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] mm/mglru: skipped aging in try_to_shrink_lruvec() when zero
  folios are scanned
- [Medium] mm/mglru: repeated aging in try_to_shrink_lruvec() when
  no_populatable persists
- [Low] mm/mglru: stale max_seq passed to should_run_aging() in
  try_to_shrink_lruvec()
--

commit b68a220713e0f1b121c4874a48ac05f55108264a
Author: Barry Song (Xiaomi) <baohua@kernel.org>

mm/mglru: Run aging when one type has no populatable folios

This commit moves the aging check in try_to_shrink_lruvec() after
evict_folio_lists() and passes a no_populatable flag to should_run_aging()
so aging runs when one LRU type makes no scan progress despite remaining
scan budget.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 085450819ee8e..41dd77652c82f 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -5105,7 +5105,8 @@ static bool lru_gen_imbalanced(struct lruvec *lruvec, struct scan_control *sc,
>  }
>  
>  static bool should_run_aging(struct lruvec *lruvec, unsigned long max_seq,
> -			     struct scan_control *sc, int swappiness)
> +			     struct scan_control *sc, int swappiness,
> +			     bool no_populatable)
>  {
>  	DEFINE_MIN_SEQ(lruvec);
>  
> @@ -5117,6 +5118,13 @@ static bool should_run_aging(struct lruvec *lruvec, unsigned long max_seq,
>  	if (sc->priority == DEF_PRIORITY)
>  		return false;
>  
> +	/*
> +	 * Run aging if scan budget remains but no folios
> +	 * in populatable generations are available to scan.
> +	 */
> +	if (no_populatable)
> +		return true;
> +
>  	/* better to run aging even though eviction is still possible */
>  	if (evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS == max_seq)
>  		return true;
> @@ -5221,20 +5229,17 @@ static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
>  	lru_gen_prepare_scan(lruvec, sc, memcg, swappiness, nr);
>  
>  	while (nr[LRU_GEN_ANON] > 0 || nr[LRU_GEN_FILE] > 0) {
> +		unsigned long orig_file, orig_anon;
>  		DEFINE_MAX_SEQ(lruvec);
> +		bool no_populatable;
>  
>  		if (mem_cgroup_below_min(sc->target_mem_cgroup, memcg)) {
>  			need_rotate = true;
>  			break;
>  		}
>  
> -		if (should_run_aging(lruvec, max_seq, sc, swappiness)) {
> -			trace_mm_mglru_aging(lruvec, sc, swappiness);
> -			if (try_to_inc_max_seq(lruvec, max_seq, swappiness, false))
> -				need_rotate = true;
> -			should_age = true;
> -		}
> -
> +		orig_anon = nr[LRU_GEN_ANON];
> +		orig_file = nr[LRU_GEN_FILE];
>  		if (!evict_folio_lists(nr, lruvec, sc, swappiness, MIN_LRU_BATCH))
>  			break;
>  

[Severity: High]
Does moving should_run_aging() and try_to_inc_max_seq() after
evict_folio_lists() skip aging whenever evict_folio_lists() scans zero
folios?

mm/vmscan.c:scan_folios() {
	...
	if (sc->priority == DEF_PRIORITY)
		seq = lrugen->max_seq - MIN_NR_GENS;
	else if (sc->priority > DEF_PRIORITY / 2)
		seq = lrugen->max_seq - 1;
	else
		seq = lrugen->max_seq;

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

At DEF_PRIORITY, when only MIN_NR_GENS generations remain
(evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS > max_seq),
scan_folios() caps seq to max_seq - MIN_NR_GENS, which is below min_seq
and already empty.  evict_folio_lists() then returns 0 and breaks out of
the loop before reaching should_run_aging(), making the mandatory
evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS > max_seq aging
check unreachable at DEF_PRIORITY.

Similarly, at DEF_PRIORITY / 2 < sc->priority < DEF_PRIORITY,
scan_folios() caps seq to max_seq - 1.  Whenever all evictable folios for
the budgeted type(s) are in max_seq (for example when both types only have
folios in max_seq, or when only one type has budget left),
evict_folio_lists() returns 0 and breaks before aging can run, leaving
need_rotate false as well so shrink_one() fails to rotate a sizable memcg.

Finally, when sc->priority <= DEF_PRIORITY / 2, running
evict_folio_lists() before should_run_aging() scans directly from max_seq
(where min_gen == max_gen in sort_folio() disables tier protection) before
aging can split the youngest generation, and breaks without aging if the
first batch satisfies should_abort_scan().

> @@ -5248,6 +5253,15 @@ static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
>  		if (root_reclaim(sc) && should_age)
>  			break;
>  
> +		no_populatable = (nr[LRU_GEN_ANON] && nr[LRU_GEN_ANON] == orig_anon) ||
> +				 (nr[LRU_GEN_FILE] && nr[LRU_GEN_FILE] == orig_file);

[Severity: Medium]
Can no_populatable stay true across every MIN_LRU_BATCH iteration and
trigger repeated aging when one type still scans 0 folios after aging?

nr[] is computed once in lru_gen_prepare_scan() and is only decremented by
scanned folios.  If scan_folios() continues to return 0 for one type while
the other type makes progress (for instance when sc->priority <=
DEF_PRIORITY / 2 already scans up to max_seq, when max_seq has no eligible
folios of that type, or when walk_mm() promotes all referenced folios into
the new youngest generation), nr[type] remains equal to orig_anon or
orig_file.  In cgroup reclaim (!root_reclaim(sc)), the loop does not break
on should_age, so should_run_aging() returns true and calls
try_to_inc_max_seq() on every 64-page batch of the other type; in
root_reclaim(sc), it runs unnecessary aging and aborts scanning the
populated type after one extra batch.

> +		if (should_run_aging(lruvec, max_seq, sc, swappiness, no_populatable)) {
> +			trace_mm_mglru_aging(lruvec, sc, swappiness);
> +			if (try_to_inc_max_seq(lruvec, max_seq, swappiness, false))
> +				need_rotate = true;
> +			should_age = true;
> +		}

[Severity: Low]
Can should_run_aging() see a stale max_seq alongside a fresh min_seq here?

DEFINE_MAX_SEQ(lruvec) snapshots max_seq at the start of the loop before
evict_folio_lists(), whereas should_run_aging() reads min_seq after
evict_folio_lists() finishes.  If a concurrent reclaimer advances max_seq
and min_seq during eviction, should_run_aging() compares the new min_seq
against the old max_seq, which can falsely trigger the aging branch, fail
try_to_inc_max_seq() on the stale max_seq, and set should_age = true so
root_reclaim(sc) aborts on the next pass.

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