On Mon, Mar 13, 2017 at 12:58 PM, Johannes Weiner <han...@cmpxchg.org> wrote: > Hi Shakeel, > > On Fri, Mar 10, 2017 at 11:46:20AM -0800, 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. >> >> Signed-off-by: Shakeel Butt <shake...@google.com> > > You're right, the way it works right now is kind of lame. Did you > observe continued kswapd spinning because of the wakeup ping-pong? >
Yes, I did observe kswapd spinning for more than an hour. >> +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; > > Instead of a second helper function, could you rename > pfmemalloc_watermark_ok() and add the kswapd_failure check at the very > beginning of that function? > Sure, Michal also suggested the same. > Because that check fits nicely with the comment about kswapd having to > be awake, too. We need kswapd operational when throttling reclaimers. > > Thanks