Re: [PATCH v7 14/16] mm/mglru: Avoid prolonged anon reclaim with swappiness=200

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] mm/mglru: commit Author and Signed-off-by use handle instead of
  real name
- [Medium] mm/mglru: zero sc.priority in run_eviction() skews
  lru_gen_balance_scan()
--

commit 8a29a76d461ffd04769caa1f7ed98ca18efe1fb3
Author: lyugaofei <lyugaofei@xiaomi.com>

> mm/mglru: Avoid prolonged anon reclaim with swappiness=200
>
> It has been observed that swappiness=200 can significantly slow down
> reclaim on some machines, with some workloads running 3x slower
> under MGLRU than under CLRU.
>
> This is because swappiness=200 currently results in file_factor=0.
> Give file reclaim a small factor at lower priorities, for example,
> when sc->priority < DEF_PRIORITY / 2.
>
> Signed-off-by: lyugaofei <lyugaofei@xiaomi.com>
> Co-developed-by: Barry Song (Xiaomi) <baohua@kernel.org>
> Signed-off-by: Barry Song (Xiaomi) <baohua@kernel.org>

[Severity: High]
Per Documentation/process/submitting-patches.rst, both the commit Author
and the Signed-off-by trailer must use a known real full human name rather
than an account handle ("lyugaofei").

> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 2030c15c06674..a92b14190d733 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -4889,8 +4889,8 @@ static int get_tier_idx(struct lruvec *lruvec, int type)
>   * We don't calculate the scan cost, MGLRU uses lazy promotion, which will
>   * cancel the scan budget natually.
>   */
> -static void lru_gen_balance_scan(struct lruvec *lruvec, int swappiness,
> -		unsigned long total_scan, unsigned long nr[])
> +static void lru_gen_balance_scan(struct lruvec *lruvec, struct scan_control *sc,
> +		int swappiness, unsigned long total_scan, unsigned long nr[])
>  {
>  	struct lru_gen_folio *lrugen = &lruvec->lrugen;
>  	u64 refault_rate[ANON_AND_FILE], anon_factor, file_factor;
> @@ -4947,7 +4947,12 @@ static void lru_gen_balance_scan(struct lruvec *lruvec, int swappiness,
>  	file_weight = nr[LRU_GEN_FILE] + nr[LRU_GEN_ANON] / size_bias;
>  
>  	anon_factor = (u64)swappiness * refault_rate[LRU_GEN_FILE] * anon_weight;
> -	file_factor = (u64)(MAX_SWAPPINESS - swappiness) * refault_rate[LRU_GEN_ANON] * file_weight;
> +	if (swappiness == MAX_SWAPPINESS)
> +		file_factor = sc->priority < DEF_PRIORITY / 2 ?
> +			      refault_rate[LRU_GEN_ANON] * file_weight : 0;
> +	else
> +		file_factor = (u64)(MAX_SWAPPINESS - swappiness) *
> +			      refault_rate[LRU_GEN_ANON] * file_weight;

[ ... ]

> @@ -5891,7 +5896,7 @@ static int run_eviction(struct lruvec *lruvec, unsigned long seq, struct scan_co
>  			return 0;
>  
>  		lruvec_evictable_size(lruvec, sc, swappiness, nr_to_scan);
> -		lru_gen_balance_scan(lruvec, swappiness,
> +		lru_gen_balance_scan(lruvec, sc, swappiness,
>  				     nr_to_reclaim - sc->nr_reclaimed, nr_to_scan);
>  
>  		if (!evict_folio_lists(nr_to_scan, lruvec, sc, swappiness, MAX_LRU_BATCH))

[Severity: Medium]
Does passing sc from run_eviction() into lru_gen_balance_scan() cause
swappiness=200 via debugfs proactive reclaim to always calculate a non-zero
file_factor?

lru_gen_seq_write() initializes sc on the stack without setting .priority
to DEF_PRIORITY, leaving sc.priority zero-initialized when passed to
run_cmd() -> run_eviction().  In lru_gen_balance_scan(), sc->priority <
DEF_PRIORITY / 2 (0 < 6) is then unconditionally true, so proactive
eviction with swappiness=200 always reclaims file pages instead of
preferring anonymous memory.

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