Re: [PATCH] mm/vmscan: clear hopeless kswapd when global direct reclaim makes progress

[Michal Hocko]

On Thu 09-07-26 19:44:29, Altan Hacigumus wrote:
> Direct reclaim clears the hopeless kswapd state only once the node
> becomes balanced. This avoids cgroup memory.high reclaim repeatedly
> reviving a kswapd that cannot balance the node.
> 
> However, it also prevents global direct reclaim from reviving kswapd.
> Under sustained memory pressure, global direct reclaim may continue
> making progress without the node ever reaching the high watermark,
> leaving reclaim to allocating tasks.
> 
> Unlike memcg reclaim, global direct reclaim follows the same node-wide
> reclaim path as kswapd. Clear the hopeless state when global direct
> reclaim makes progress, while continuing to require a balanced node for
> memcg reclaim. kswapd_try_clear_hopeless() becomes static since struct
> scan_control is private to vmscan.c.
> 
> On a workload with most memory mlocked and the remainder under sustained
> churn, with swap enabled, comparing the same 60s window:
>                               base       patched
>   allocstall (all zones)    413441          6532
>   pgsteal_direct          15889552        255619
>   pgsteal_kswapd                 0      26079382
>   PSI memory full avg60     13.10%         9.37%
> 
> Direct reclaim was already reclaiming many pages in the baseline;
> this change lets kswapd resume doing that work asynchronously.
> 
> Signed-off-by: Altan Hacigumus <ahacigu.linux@xxxxxxxxx>
> ---
>  include/linux/mmzone.h |  2 --
>  mm/vmscan.c            | 23 ++++++++++++++---------
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index ca2712187147..1db6f8dac927 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -1630,8 +1630,6 @@ enum kswapd_clear_hopeless_reason {
>  
>  void wakeup_kswapd(struct zone *zone, gfp_t gfp_mask, int order,
>  		   enum zone_type highest_zoneidx);
> -void kswapd_try_clear_hopeless(struct pglist_data *pgdat,
> -			       unsigned int order, int highest_zoneidx);
>  void kswapd_clear_hopeless(pg_data_t *pgdat, enum kswapd_clear_hopeless_reason reason);
>  bool kswapd_test_hopeless(pg_data_t *pgdat);
>  
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 35c3bb15ae96..78fd35a11eee 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -198,6 +198,9 @@ struct scan_control {
>   */
>  int vm_swappiness = 60;
>  
> +static void kswapd_try_clear_hopeless(struct pglist_data *pgdat,
> +				      struct scan_control *sc);
> +
>  #ifdef CONFIG_MEMCG
>  
>  /* Returns true for reclaim through cgroup limits or cgroup interfaces. */
> @@ -5172,7 +5175,7 @@ static void lru_gen_shrink_node(struct pglist_data *pgdat, struct scan_control *
>  	blk_finish_plug(&plug);
>  done:
>  	if (sc->nr_reclaimed > reclaimed)
> -		kswapd_try_clear_hopeless(pgdat, sc->order, sc->reclaim_idx);
> +		kswapd_try_clear_hopeless(pgdat, sc);
>  }
>  
>  /******************************************************************************
> @@ -6251,7 +6254,7 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc)
>  	 * successful direct reclaim run will revive a dormant kswapd.
>  	 */
>  	if (reclaimable)
> -		kswapd_try_clear_hopeless(pgdat, sc->order, sc->reclaim_idx);
> +		kswapd_try_clear_hopeless(pgdat, sc);
>  	else if (sc->cache_trim_mode)
>  		sc->cache_trim_mode_failed = 1;
>  }
> @@ -7530,15 +7533,17 @@ void kswapd_clear_hopeless(pg_data_t *pgdat, enum kswapd_clear_hopeless_reason r
>  }
>  
>  /*
> - * Reset kswapd_failures only when the node is balanced. Without this
> - * check, successful direct reclaim (e.g., from cgroup memory.high
> - * throttling) can keep resetting kswapd_failures even when the node
> - * cannot be balanced, causing kswapd to run endlessly.
> + * Reset kswapd_failures when the node is balanced, or when global
> + * direct reclaim makes progress - then kswapd can make progress too.
> + * Memcg reclaim can succeed where kswapd cannot (memcg protection is
> + * not enforced against the reclaim target), so its progress resets
> + * kswapd_failures only when the node is balanced.
>   */
> -void kswapd_try_clear_hopeless(struct pglist_data *pgdat,
> -			       unsigned int order, int highest_zoneidx)
> +static void kswapd_try_clear_hopeless(struct pglist_data *pgdat,
> +				      struct scan_control *sc)
>  {
> -	if (pgdat_balanced(pgdat, order, highest_zoneidx))
> +	if ((!current_is_kswapd() && !cgroup_reclaim(sc)) ||
> +	    pgdat_balanced(pgdat, sc->order, sc->reclaim_idx))
>  		kswapd_clear_hopeless(pgdat, current_is_kswapd() ?
>  			KSWAPD_CLEAR_HOPELESS_KSWAPD : KSWAPD_CLEAR_HOPELESS_DIRECT);

Is there any particular reason why pgdat_balanced is evaluated for
cgroup_reclaim? Shouldn't this be global reclaim logic only?
I.e.
	if (cgroup_reclaim(sc))
		return;
	if (!current_is_kswapd() || pgdat_balanced(pgdat, sc->order, sc->reclaim_idx))
		kswapd_clear_hopeless(pgdat, current_is_kswapd() ? KSWAPD_CLEAR_HOPELESS_KSWAPD : KSWAPD_CLEAR_HOPELESS_DIRECT);


>  }
> -- 
> 2.55.0

-- 
Michal Hocko
SUSE Labs