Re: [PATCH v9 11/16] mm/mglru: Run aging when one type is drained

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm/mglru: scan_folios() scans seq below min_seq at
  DEF_PRIORITY
- [Low] mm/mglru: missed eviction retry in try_to_shrink_lruvec() on
  concurrent aging
--

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

mm/mglru: Run aging when one type is drained

This commit moves MGLRU aging after eviction in try_to_shrink_lruvec() and
triggers aging once per invocation when an evictable type is drained.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 1450542d709c3..6d942f192e63a 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
[ ... ]
> @@ -5226,22 +5235,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) {
> +		bool drained = false, do_drained_age;
>  		DEFINE_MAX_SEQ(lruvec);
> +		unsigned long new_seq;
> +		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;
> +		scanned = evict_folio_lists(nr, lruvec, sc, swappiness, MIN_LRU_BATCH, &drained);

[Severity: Medium]
Can calling evict_folio_lists() before should_run_aging() cause
scan_folios() to scan a generation below min_seq[type] at DEF_PRIORITY?

Previously, should_run_aging() ran first and incremented max_seq at
DEF_PRIORITY whenever a type had only MIN_NR_GENS generations
(min_seq[type] == max_seq - 1).  With eviction moved ahead of aging,
evict_folio_lists() -> evict_folios() -> scan_folios() now runs while
min_seq[type] == max_seq - 1:

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

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

Because seq is max_seq - 2 (< min_seq[type]), the while (min_seq < max_seq)
loop in lruvec_populated_min_seq() does not execute and returns max_seq - 2.
scan_folios() then scans the out-of-window slot
lrugen->folios[lru_gen_from_seq(max_seq - 2)][type][zone_idx], prematurely
isolating or promoting folios parked in that slot by memcg reparenting to
await future aging, and re-adding rejected folios via move_folios_to_lru()
at scanned_seq + 1 == min_seq[type] instead of promoting them above
min_seq[type].

>  
>  		if (should_abort_scan(lruvec, sc))
>  			break;
> @@ -5253,6 +5257,27 @@ static bool try_to_shrink_lruvec(struct lruvec *lruvec, struct scan_control *sc)
>  		if (root_reclaim(sc) && should_age)
>  			break;
>  
> +		/*
> +		 * Allow drained aging only once per invocation and skip it if
> +		 * max_seq advanced concurrently during eviction
> +		 */
> +		new_seq = READ_ONCE(lruvec->lrugen.max_seq);
> +		do_drained_age = drained && allow_drained_age && max_seq == new_seq;
> +		allow_drained_age &= !do_drained_age;
> +		if (do_drained_age || should_run_aging(lruvec, new_seq, sc, swappiness)) {
> +			trace_mm_mglru_aging(lruvec, sc, swappiness);
> +			if (try_to_inc_max_seq(lruvec, new_seq, swappiness, false))
> +				need_rotate = true;
> +			should_age = true;
> +		}
> +
> +		if (!scanned) {
> +			/* Retry once if aging advanced max_seq */
> +			if (retried || new_seq == READ_ONCE(lruvec->lrugen.max_seq))
> +				break;
> +			retried = true;
> +		}

[Severity: Low]
Should the retry check compare READ_ONCE(lruvec->lrugen.max_seq) against
max_seq instead of new_seq?

CPU0                              CPU1
try_to_shrink_lruvec()
  max_seq = lrugen.max_seq
  evict_folio_lists() -> 0
                                  try_to_inc_max_seq()
                                    inc_max_seq()
                                      lrugen.max_seq++
  new_seq = lrugen.max_seq
  max_seq == new_seq is false
  /* aging skipped */
  new_seq == lrugen.max_seq       <-- breaks without retrying

When a concurrent reclaimer advances max_seq after scan_folios() finds no
folios and before new_seq is read, do_drained_age is skipped because
max_seq != new_seq and should_run_aging() is evaluated against the already
incremented new_seq.  Comparing lrugen.max_seq against new_seq then sees no
change and breaks out of try_to_shrink_lruvec() without retrying eviction
on the newly aged generation.

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