Re: [RFC PATCH v5 14/14] 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 issue(s) to consider:
- [High] mm/mglru: unclamped scan budget on early return in lru_gen_balance_scan()
- [Medium] mm/mglru: inconsistent swappiness state when anon reclaim is skipped for GFP_NOIO
--

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

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

This commit enables the low-swapcache check in reclaimable_anon_is_low() for
MGLRU under GFP_NOIO and zeroes nr_to_scan[LRU_GEN_ANON] in
lru_gen_prepare_scan() when can_reclaim_anon_pages() returns false. It also
updates lru_gen_balance_scan() to return early when either nr[LRU_GEN_ANON]
or nr[LRU_GEN_FILE] is zero.

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index c47fe07f7290a..71bc7fedf7a9d 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c

[ ... ]

> @@ -5062,26 +5055,23 @@ static void lru_gen_balance_scan(struct lruvec *lruvec, struct scan_control *sc,
>  {
>  	struct lru_gen_folio *lrugen = &lruvec->lrugen;
>  	u64 refault_rate[ANON_AND_FILE], anon_factor, file_factor;
> -	u64 anon_scan, file_scan, total;
> +	u64 anon_scan, file_scan;
>  	u64 anon_weight, file_weight;
>  	int size_bias;
>  
> -	total = nr[LRU_GEN_ANON] + nr[LRU_GEN_FILE];
> -	if (!total)
> -		return;
> -
>  	if (swappiness <= MIN_SWAPPINESS) {
>  		nr[LRU_GEN_ANON] = 0;
>  		nr[LRU_GEN_FILE] = min_t(u64, total_scan, nr[LRU_GEN_FILE]);
> -		return;
>  	}
>  
>  	if (swappiness == SWAPPINESS_ANON_ONLY) {
>  		nr[LRU_GEN_FILE] = 0;
>  		nr[LRU_GEN_ANON] = min_t(u64, total_scan, nr[LRU_GEN_ANON]);
> -		return;
>  	}
>  
> +	if (!nr[LRU_GEN_ANON] || !nr[LRU_GEN_FILE])
> +		return;

[Severity: High]
When 1 <= swappiness <= MAX_SWAPPINESS and either nr[LRU_GEN_ANON] or
nr[LRU_GEN_FILE] is 0, does this early return leave the remaining non-zero
nr[] entry unclamped by total_scan?

Both callers of lru_gen_balance_scan() first populate nr_to_scan[] with the
full unscaled evictable LRU sizes via lruvec_evictable_size(). In
lru_gen_prepare_scan():

    lruvec_evictable_size(lruvec, swappiness, nr_to_scan);
    if (!can_reclaim_anon_pages(memcg, lruvec_pgdat(lruvec)->node_id, sc))
            nr_to_scan[LRU_GEN_ANON] = 0;
    total = nr_to_scan[LRU_GEN_ANON] + nr_to_scan[LRU_GEN_FILE];

    /* Just scrape all pages calculated above if this memcg was deleted */
    if (!mem_cgroup_online(memcg))
            return;

    total = apply_proportional_protection(memcg, sc, total);
    total >>= sc->priority;
    lru_gen_balance_scan(lruvec, sc, swappiness, total, nr_to_scan);

And in run_eviction():

    lruvec_evictable_size(lruvec, swappiness, nr_to_scan);
    lru_gen_balance_scan(lruvec, sc, swappiness,
                         nr_to_reclaim - sc->nr_reclaimed, nr_to_scan);

Previously, when one type was 0 and swappiness was between 1 and
MAX_SWAPPINESS, execution continued to the end of lru_gen_balance_scan()
where nr[LRU_GEN_ANON] and nr[LRU_GEN_FILE] were bounded to total_scan.

With this change, nr[] is only clamped to total_scan before the early return
when swappiness <= MIN_SWAPPINESS or swappiness == SWAPPINESS_ANON_ONLY. When
swappiness is in the normal range (for example, the default 60) and one LRU
type is 0 (either because !can_reclaim_anon_pages() zeroed
nr_to_scan[LRU_GEN_ANON] under GFP_NOIO, or because an lruvec only has
evictable pages of one type), lru_gen_balance_scan() returns early with the
non-zero nr[type] still set to the entire unscaled evictable size of that
LRU, bypassing sc->priority scaling, memory.low proportional protection in
apply_proportional_protection(), and the nr_to_reclaim target in
run_eviction().

>  	for (int type = LRU_GEN_ANON; type <= LRU_GEN_FILE; type++) {
>  		int hist = lru_hist_from_seq(READ_ONCE(lrugen->min_seq[type]));
>  		/* start at MIN_LRU_BATCH to smooth the rate on gen start */
> @@ -5307,6 +5297,8 @@ static void lru_gen_prepare_scan(struct lruvec *lruvec, struct scan_control *sc,
>  	unsigned long total;
>  
>  	lruvec_evictable_size(lruvec, swappiness, nr_to_scan);
> +	if (!can_reclaim_anon_pages(memcg, lruvec_pgdat(lruvec)->node_id, sc))
> +		nr_to_scan[LRU_GEN_ANON] = 0;
>  	total = nr_to_scan[LRU_GEN_ANON] + nr_to_scan[LRU_GEN_FILE];
>  
>  	/* Just scrape all pages calculated above if this memcg was deleted */

[Severity: Medium]
Could zeroing nr_to_scan[LRU_GEN_ANON] in lru_gen_prepare_scan() while
get_swappiness() still returns a non-zero swappiness cause state
inconsistencies across the rest of the MGLRU reclaim and aging paths under
GFP_NOIO when reclaimable_anon_is_low() is true?

In try_to_shrink_lruvec() and shrink_one(), get_swappiness() does not check
can_reclaim_anon_pages() or reclaimable_anon_is_low():

    static int get_swappiness(struct lruvec *lruvec, struct scan_control *sc)
    {
            ...
            if (!sc->may_swap)
                    return 0;

            if (!can_demote(pgdat->node_id, sc, memcg) &&
                mem_cgroup_get_nr_swap_pages(memcg) < MIN_LRU_BATCH)
                    return 0;

            return swappiness;
    }

When swap space is available during GFP_NOIO reclaim with low swapcache,
swappiness remains non-zero while anon scanning is skipped:

1. In try_to_inc_min_seq() and should_run_aging(), a non-zero swappiness
   causes MGLRU to treat LRU_GEN_ANON as evictable:

       if (evictable_min_seq(min_seq, swappiness) + MIN_NR_GENS > max_seq)
               return true;

   Because LRU_GEN_ANON is not scanned, min_seq[LRU_GEN_ANON] does not
   advance. When min_seq[LRU_GEN_ANON] lags at max_seq - 3,
   try_to_inc_min_seq() clamps min_seq[LRU_GEN_FILE] to max_seq - MIN_NR_GENS
   and evictable_min_seq() stays below max_seq - MIN_NR_GENS, preventing
   should_run_aging() from aging even after all evictable file generations
   are exhausted.

2. In shrink_one() -> lruvec_is_sizable(), a non-zero swappiness causes
   lruvec_evictable_size() to count unreclaimable anon pages in total:

       lruvec_evictable_size(lruvec, swappiness, sizes);
       total = sizes[LRU_GEN_ANON] + sizes[LRU_GEN_FILE];

   For an anon-heavy memcg with little or no file cache under GFP_NOIO,
   lruvec_is_sizable() still returns true even though nothing can be scanned,
   preventing shrink_one() from rotating the memcg.

Would checking can_reclaim_anon_pages() in get_swappiness() (returning 0 when
anon pages cannot be reclaimed) avoid both this mismatch and the unclamped
nr[LRU_GEN_FILE] budget in lru_gen_balance_scan() for this path?

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