Re: [PATCH v6 15/15] mm/mglru: Avoid scan anon while swapcache is low for GFP_NOIO allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] mm/mglru: stalled GFP_NOIO aging in should_run_aging() due to
  min_seq clamp
- [Medium] mm/mglru: should_run_aging() calls reclaimable_anon_is_low()
  directly
--

commit bc0f15f68a4ac3f563747aa1e1cc7d4d0f14a37d
Author: Bo Zhang <zhangbo56@xiaomi.com>

mm/mglru: Avoid scan anon while swapcache is low for GFP_NOIO allocation

This commit skips anonymous generation scanning in MGLRU during GFP_NOIO
reclaim when swapcache is low by excluding unreclaimable anonymous pages
from lruvec_evictable_size() and adjusting lru_gen_balance_scan() and
should_run_aging().

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 3793392208781..fa0245a07caa6 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c

[ ... ]

> @@ -4206,12 +4199,14 @@ static void set_initial_priority(struct pglist_data *pgdat, struct scan_control
>  	sc->priority = clamp(priority, DEF_PRIORITY / 2, DEF_PRIORITY);
>  }
>  
> -static void lruvec_evictable_size(struct lruvec *lruvec, int swappiness,
> -				  unsigned long nr[])
> +static void lruvec_evictable_size(struct lruvec *lruvec, struct scan_control *sc,
> +				  int swappiness, unsigned long nr[])
>  {
>  	int type;
>  	unsigned long seq;
>  	struct lru_gen_folio *lrugen = &lruvec->lrugen;
> +	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
> +	int nid = lruvec_pgdat(lruvec)->node_id;
>  	DEFINE_MAX_SEQ(lruvec);
>  	DEFINE_MIN_SEQ(lruvec);
>  
> @@ -4219,6 +4214,8 @@ static void lruvec_evictable_size(struct lruvec *lruvec, int swappiness,
>  	nr[LRU_GEN_FILE] = 0;
>  
>  	for_each_evictable_type(type, swappiness) {
> +		if (type == LRU_GEN_ANON && !can_reclaim_anon_pages(memcg, nid, sc))
> +			continue;
>  		for (seq = min_seq[type]; seq <= max_seq; seq++)
>  			nr[type] += lruvec_gen_size(lrugen, type, seq, sc->reclaim_idx + 1);
>  	}

[ ... ]

> @@ -5113,12 +5106,19 @@ static bool should_run_aging(struct lruvec *lruvec, unsigned long max_seq,
>  			     struct scan_control *sc, int swappiness,
>  			     bool do_balance)
>  {
> +	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
> +	struct mem_cgroup *memcg = lruvec_memcg(lruvec);
>  	DEFINE_MIN_SEQ(lruvec);
>  
>  	/* have to run aging, since eviction is not possible anymore */
>  	if (evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS > max_seq)
>  		return true;
>  
> +	/* run aging when GFP_NOIO limits anon reclaim with few swapcache folios */
> +	if (swappiness && min_seq[LRU_GEN_FILE] + MIN_NR_GENS > max_seq &&
> +	    reclaimable_anon_is_low(memcg, pgdat->node_id, sc))
> +		return true;
> +
>  	/* try to avoid aging, do gentle reclaim at the default priority */
>  	if (sc->priority == DEF_PRIORITY)
>  		return false;

[Severity: Medium]
Can min_seq[LRU_GEN_FILE] + MIN_NR_GENS > max_seq ever become true here
when LRU_GEN_ANON has four generations?

Under GFP_NOIO with low swapcache, lruvec_evictable_size() skips
LRU_GEN_ANON while get_swappiness() still returns a non-zero swappiness in
[1, MAX_SWAPPINESS].  Because LRU_GEN_ANON is not scanned, its oldest
generation is never emptied and min_seq[LRU_GEN_ANON] stays at
max_seq - MAX_NR_GENS + 1 (which is < max_seq - MIN_NR_GENS).  When
evict_folios() empties the older file generations, try_to_inc_min_seq()
clamps min_seq[LRU_GEN_FILE] to max_seq - MIN_NR_GENS:

mm/vmscan.c:try_to_inc_min_seq() {
    ...
	if (swappiness && swappiness <= MAX_SWAPPINESS) {
		unsigned long seq = lrugen->max_seq - MIN_NR_GENS;

		if (min_seq[LRU_GEN_ANON] > seq && min_seq[LRU_GEN_FILE] < seq)
			min_seq[LRU_GEN_ANON] = seq;
		else if (min_seq[LRU_GEN_FILE] > seq && min_seq[LRU_GEN_ANON] < seq)
			min_seq[LRU_GEN_FILE] = seq;
	}
    ...
}

With min_seq[LRU_GEN_FILE] clamped to max_seq - MIN_NR_GENS,
min_seq[LRU_GEN_FILE] + MIN_NR_GENS > max_seq is false.  At DEF_PRIORITY,
should_run_aging() returns false and scan_folios() scans 0 folios from the
already-empty file generations.  At lower priorities, evictable_min_seq()
still uses min_seq[LRU_GEN_ANON] and lru_gen_imbalanced() still counts
unreclaimable anonymous pages in old[LRU_GEN_ANON], stalling aging across
reclaim priorities.

[Severity: Medium]
Should this check !can_reclaim_anon_pages(memcg, pgdat->node_id, sc) and
swappiness <= MAX_SWAPPINESS instead of calling reclaimable_anon_is_low()
directly?

When CONFIG_SWAP is disabled, the reclaimable_anon_is_low() stub
unconditionally returns true even for __GFP_IO reclaimers:

mm/vmscan.c:reclaimable_anon_is_low() {
#else
static inline bool reclaimable_anon_is_low(struct mem_cgroup *memcg,
		int nid, struct scan_control *sc)
{
	return true;
}
#endif /* CONFIG_SWAP */
}

On a !CONFIG_SWAP system with NUMA demotion enabled (or on a CONFIG_SWAP
system during GFP_NOIO reclaim where can_demote() is true),
can_reclaim_anon_pages() returns true and lruvec_evictable_size() includes
LRU_GEN_ANON, yet should_run_aging() treats anonymous generations as
unreclaimable and forces premature aging at DEF_PRIORITY whenever
min_seq[LRU_GEN_FILE] + MIN_NR_GENS > max_seq.  In addition, not bounding
swappiness <= MAX_SWAPPINESS allows SWAPPINESS_ANON_ONLY to trigger this
check even though file pages are not evictable.

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