Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753114AbdGXGu6 (ORCPT ); Mon, 24 Jul 2017 02:50:58 -0400 Received: from mx2.suse.de ([195.135.220.15]:49881 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1753014AbdGXGuw (ORCPT ); Mon, 24 Jul 2017 02:50:52 -0400 Date: Mon, 24 Jul 2017 08:50:49 +0200 From: Michal Hocko To: Andrew Morton Cc: Mel Gorman , Tetsuo Handa , Rik van Riel , Johannes Weiner , Vlastimil Babka , linux-mm@kvack.org, LKML Subject: Re: [PATCH] mm, vmscan: do not loop on too_many_isolated for ever Message-ID: <20170724065048.GB25221@dhcp22.suse.cz> References: <20170710074842.23175-1-mhocko@kernel.org> <20170719152014.53a861c57bcb636d6cd9d002@linux-foundation.org> <20170720065625.GB9058@dhcp22.suse.cz> <20170721160104.9f6101b9e8de53638b3b853a@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170721160104.9f6101b9e8de53638b3b853a@linux-foundation.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3490 Lines: 94 On Fri 21-07-17 16:01:04, Andrew Morton wrote: > On Thu, 20 Jul 2017 08:56:26 +0200 Michal Hocko wrote: > > > > > > --- a/mm/vmscan.c > > > > +++ b/mm/vmscan.c > > > > @@ -1713,9 +1713,15 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec, > > > > int file = is_file_lru(lru); > > > > struct pglist_data *pgdat = lruvec_pgdat(lruvec); > > > > struct zone_reclaim_stat *reclaim_stat = &lruvec->reclaim_stat; > > > > + bool stalled = false; > > > > > > > > while (unlikely(too_many_isolated(pgdat, file, sc))) { > > > > - congestion_wait(BLK_RW_ASYNC, HZ/10); > > > > + if (stalled) > > > > + return 0; > > > > + > > > > + /* wait a bit for the reclaimer. */ > > > > + schedule_timeout_interruptible(HZ/10); > > > > > > a) if this task has signal_pending(), this falls straight through > > > and I suspect the code breaks? > > > > It will not break. It will return to the allocation path more quickly > > but no over-reclaim will happen and it will/should get throttled there. > > So nothing critical. > > > > > b) replacing congestion_wait() with schedule_timeout_interruptible() > > > means this task no longer contributes to load average here and it's > > > a (slightly) user-visible change. > > > > you are right. I am not sure it matters but it might be visible. > > > > > c) msleep_interruptible() is nicer > > > > > > d) IOW, methinks we should be using msleep() here? > > > > OK, I do not have objections. Are you going to squash this in or want a > > separate patch explaining all the above? > > I'd prefer to have a comment explaining why interruptible sleep is > being used, because that "what if signal_pending()" case is rather a > red flag. I didn't really consider interruptible vs. uninterruptible sleep so it wasn't really a deliberate decision. Now, that you have brought up the above points I am OK changing that the uninterruptible. Here is a fix up. I am fine with this either folded in or as a separate patch. --- >From 4b295fc1625d499a2e1283b036aea005158b33cc Mon Sep 17 00:00:00 2001 From: Michal Hocko Date: Mon, 24 Jul 2017 08:43:23 +0200 Subject: [PATCH] mm, vmscan: throttle too_many_isolated by uninterruptible sleep "mm, vmscan: do not loop on too_many_isolated for ever" has added an interruptible sleep into shrink_inactive_list when we have isolated too many pages. Andrew has noticed that we used to sleep in uninterruptible sleep previously (in congestion_wait) and that changing that is wrong for at least two reasons - waiting task would not participate in the load average anymore - task with a pending signal will not sleep and bail out immediately While none of those issues are critical in any way but they are unnecessary. The interruptible sleep was more an oversight than a deliberate decision. Fix this by using msleep instead. Spotted-by: Andrew Morton Signed-off-by: Michal Hocko --- mm/vmscan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/vmscan.c b/mm/vmscan.c index 7ba9467b6d58..ed0c29a3db16 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1749,7 +1749,7 @@ shrink_inactive_list(unsigned long nr_to_scan, struct lruvec *lruvec, return 0; /* wait a bit for the reclaimer. */ - schedule_timeout_interruptible(HZ/10); + msleep(100); stalled = true; /* We are about to die and free our memory. Return now. */ -- 2.13.2 -- Michal Hocko SUSE Labs