Re: [PATCH v8 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 2 potential
issues to consider:
- [High] mm/mglru: premature loop exit after aging in
  try_to_shrink_lruvec()
- [Medium] mm/mglru: late max_seq read in try_to_shrink_lruvec() causes
  redundant aging
--

commit 9c58557be00bbe7f49dfd8383a560ebff484751b
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 triggers aging once per invocation when a folio
type has remaining scan budget but no populatable folios were scanned.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 59957fab915b7..9071f8889b5c4 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -5109,7 +5109,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);
>  
> @@ -5121,6 +5122,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;
> @@ -5208,6 +5216,13 @@ static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
>  	bool need_rotate = false, should_age = false;
>  	int swappiness = get_swappiness(lruvec, sc);
>  	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
> +	/*
> +	 * Avoid aging the lagging type under extreme swappiness,
> +	 * as reclaim of the unpreferred type is disabled until
> +	 * DEF_PRIORITY / 2.
> +	 */
> +	bool allow_no_populatable_age = (sc->priority <= DEF_PRIORITY / 2) ||
> +		(swappiness > MIN_SWAPPINESS + 1 && swappiness < MAX_SWAPPINESS);
>  
>  	/*
>  	 * Proactive reclaim initiated by userspace for anonymous memory only.
> @@ -5225,22 +5240,19 @@ 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) {
> -		DEFINE_MAX_SEQ(lruvec);
> +		unsigned long orig_file, orig_anon;
> +		unsigned long max_seq;
> +		bool no_populatable;
> +		long scanned;
>  
>  		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;
> -		}
> -
> -		if (!evict_folio_lists(nr, lruvec, sc, swappiness, MIN_LRU_BATCH))
> -			break;
> +		orig_anon = nr[LRU_GEN_ANON];
> +		orig_file = nr[LRU_GEN_FILE];
> +		scanned = evict_folio_lists(nr, lruvec, sc, swappiness, MIN_LRU_BATCH);
>  
>  		if (should_abort_scan(lruvec, sc))
>  			break;
> @@ -5252,6 +5264,21 @@ static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
>  		if (root_reclaim(sc) && should_age)
>  			break;
>  
> +		/* Limit no_populatable aging to once per try_to_shrink_lruvec() */
> +		no_populatable = allow_no_populatable_age &&
> +			((nr[LRU_GEN_ANON] && nr[LRU_GEN_ANON] == orig_anon) ||
> +			(nr[LRU_GEN_FILE] && nr[LRU_GEN_FILE] == orig_file));
> +		allow_no_populatable_age &= !no_populatable;
> +		max_seq = READ_ONCE((lruvec)->lrugen.max_seq);
> +		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: Medium]
Can reading lruvec->lrugen.max_seq after evict_folio_lists() cause
concurrent reclaimers to run redundant back-to-back aging passes on the
same lruvec?

try_to_inc_max_seq() relies on callers passing the max_seq value observed
when the need for aging was detected, deduplicating concurrent callers via
seq <= READ_ONCE(mm_state->seq).  Because max_seq is now read after
evict_folio_lists(), two threads that both observe no_populatable while
max_seq is S can age the lruvec twice:

CPU0                                  CPU1
try_to_shrink_lruvec()                try_to_shrink_lruvec()
  evict_folio_lists()                   evict_folio_lists()
    /* max_seq == S:
     * anon scans 0, file shrinks
     */
                                          /* max_seq == S:
                                           * anon scans 0, file shrinks
                                           */
  no_populatable = true;
  max_seq = S;
  try_to_inc_max_seq(..., S, ...)
    /* advances max_seq to S + 1 */
                                        no_populatable = true;
                                        max_seq = READ_ONCE(lrugen.max_seq);
                                          /* reads S + 1 */
                                        should_run_aging(..., no_populatable)
                                          /* returns true unconditionally */
                                        try_to_inc_max_seq(..., S + 1, ...)
                                          /* ages again to S + 2 */
                                          <-- redundant aging

This triggers unnecessary page-table walks across mm_list, prematurely
advances min_seq once MAX_NR_GENS is reached, and decays PID controller
refault averages in reset_ctrl_pos() on each extra pass.  Reading max_seq
before calling evict_folio_lists() preserves the sequence number at which
no_populatable was observed.

> +		if (!scanned)
> +			break;

[Severity: High]
Does checking !scanned after try_to_inc_max_seq() cause
try_to_shrink_lruvec() to abort without reclaiming any folios right after
aging succeeds?

In scan_folios(), the oldest generation considered for scanning depends on
reclaim priority:

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);
    ...
}

When sc->priority == DEF_PRIORITY and an lruvec has only MIN_NR_GENS (2)
generations (min_seq == max_seq - 1), scan_folios() sets seq to
max_seq - 2 (< min_seq) and evict_folio_lists() returns scanned == 0.
should_run_aging() and try_to_inc_max_seq() then advance max_seq to create
a third generation and set need_rotate = true, but if (!scanned) break
immediately exits the loop using the pre-aging scanned == 0 result,
rotating the memcg with zero folios reclaimed and escalating reclaim
priority prematurely.

The same premature exit happens at DEF_PRIORITY > sc->priority >
DEF_PRIORITY / 2 whenever the remaining type(s) with non-zero nr[] budget
only have folios in max_seq: evict_folio_lists() returns 0, no_populatable
triggers try_to_inc_max_seq() to move those folios into max_seq - 1, and
if (!scanned) break aborts the loop before they can be scanned, undoing
12316f7902f8 ("mm/mglru: don't abort scan immediately right after aging").

In addition, calling evict_folio_lists() and should_abort_scan() before
should_run_aging() causes scan_folios() at sc->priority <=
DEF_PRIORITY / 2 to evict directly from max_seq before
lru_gen_imbalanced() can age page tables to separate hot folios from cold
ones, and skips aging altogether when the first batch satisfies
should_abort_scan().

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