Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754475AbdCMVs0 (ORCPT ); Mon, 13 Mar 2017 17:48:26 -0400 Received: from mail-pg0-f42.google.com ([74.125.83.42]:32920 "EHLO mail-pg0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752024AbdCMVsS (ORCPT ); Mon, 13 Mar 2017 17:48:18 -0400 MIME-Version: 1.0 In-Reply-To: <20170313195833.GA25454@cmpxchg.org> References: <20170310194620.5021-1-shakeelb@google.com> <20170313195833.GA25454@cmpxchg.org> From: Shakeel Butt Date: Mon, 13 Mar 2017 14:48:16 -0700 Message-ID: Subject: Re: [PATCH] mm: fix condition for throttle_direct_reclaim To: Johannes Weiner Cc: Mel Gorman , Michal Hocko , Vlastimil Babka , Andrew Morton , Jia He , Hillf Danton , Linux MM , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2113 Lines: 50 On Mon, Mar 13, 2017 at 12:58 PM, Johannes Weiner 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 > > 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