Re: [PATCH] mm: fix condition for throttle_direct_reclaim

[Michal Hocko]

On Fri 10-03-17 11:46:20, Shakeel Butt wrote:
> Recently kswapd has been modified to give up after MAX_RECLAIM_RETRIES
> number of unsucessful iterations. Before going to sleep, kswapd thread
> will unconditionally wakeup all threads sleeping on pfmemalloc_wait.
> However the awoken threads will recheck the watermarks and wake the
> kswapd thread and sleep again on pfmemalloc_wait. There is a chance
> of continuous back and forth between kswapd and direct reclaiming
> threads if the kswapd keep failing and thus defeat the purpose of
> adding backoff mechanism to kswapd. So, add kswapd_failures check
> on the throttle_direct_reclaim condition.

I have to say I really do not like this. kswapd_failures shouldn't
really be checked outside of the kswapd context. The
pfmemalloc_watermark_ok/throttle_direct_reclaim is quite complex even
without putting another variable into it. I wish we rather replace this
throttling by something else. Johannes had an idea to throttle by the
number of reclaimers.

Anyway, I am wondering whether we can hit this issue in
practice? Have you seen it happening or is this a result of the code
review? I would assume that that !zone_reclaimable_pages check in
pfmemalloc_watermark_ok should help to some degree.

> Signed-off-by: Shakeel Butt <shakeelb@xxxxxxxxxx>
> ---
>  mm/vmscan.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index bae698484e8e..b2d24cc7a161 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2819,6 +2819,12 @@ static bool pfmemalloc_watermark_ok(pg_data_t *pgdat)
>  	return wmark_ok;
>  }
>  
> +static bool should_throttle_direct_reclaim(pg_data_t *pgdat)
> +{
> +	return (pgdat->kswapd_failures < MAX_RECLAIM_RETRIES &&
> +		!pfmemalloc_watermark_ok(pgdat));
> +}
> +
>  /*
>   * Throttle direct reclaimers if backing storage is backed by the network
>   * and the PFMEMALLOC reserve for the preferred node is getting dangerously
> @@ -2873,7 +2879,7 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
>  
>  		/* Throttle based on the first usable node */
>  		pgdat = zone->zone_pgdat;
> -		if (pfmemalloc_watermark_ok(pgdat))
> +		if (!should_throttle_direct_reclaim(pgdat))
>  			goto out;
>  		break;
>  	}
> @@ -2895,14 +2901,14 @@ static bool throttle_direct_reclaim(gfp_t gfp_mask, struct zonelist *zonelist,
>  	 */
>  	if (!(gfp_mask & __GFP_FS)) {
>  		wait_event_interruptible_timeout(pgdat->pfmemalloc_wait,
> -			pfmemalloc_watermark_ok(pgdat), HZ);
> +			!should_throttle_direct_reclaim(pgdat), HZ);
>  
>  		goto check_pending;
>  	}
>  
>  	/* Throttle until kswapd wakes the process */
>  	wait_event_killable(zone->zone_pgdat->pfmemalloc_wait,
> -		pfmemalloc_watermark_ok(pgdat));
> +		!should_throttle_direct_reclaim(pgdat));
>  
>  check_pending:
>  	if (fatal_signal_pending(current))
> -- 
> 2.12.0.246.ga2ecc84866-goog
> 

-- 
Michal Hocko
SUSE Labs