Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752036AbdH2M5L (ORCPT ); Tue, 29 Aug 2017 08:57:11 -0400 Received: from mga02.intel.com ([134.134.136.20]:49025 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751730AbdH2M5K (ORCPT ); Tue, 29 Aug 2017 08:57:10 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,444,1498546800"; d="scan'208";a="145148383" From: "Liang, Kan" To: Linus Torvalds CC: Tim Chen , Mel Gorman , Peter Zijlstra , "Ingo Molnar" , Andi Kleen , Andrew Morton , Johannes Weiner , Jan Kara , Christopher Lameter , "Eric W . Biederman" , Davidlohr Bueso , linux-mm , Linux Kernel Mailing List Subject: RE: [PATCH 2/2 v2] sched/wait: Introduce lock breaker in wake_up_page_bit Thread-Topic: [PATCH 2/2 v2] sched/wait: Introduce lock breaker in wake_up_page_bit Thread-Index: AQHTHb+Q9zxUNJ5MmUaRufh57Tkd0qKU91KAgAAnnoCAAAjGgIAAA4IAgAAYmQCAACfWgIABATSAgANwC8D//5x3AIAB1jig Date: Tue, 29 Aug 2017 12:57:06 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077537A1C19@SHSMSX103.ccr.corp.intel.com> References: <83f675ad385d67760da4b99cd95ee912ca7c0b44.1503677178.git.tim.c.chen@linux.intel.com> <37D7C6CF3E00A74B8858931C1DB2F077537A07E9@SHSMSX103.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMGE3MDQ5NWMtMjM2NC00OTYxLThkYWQtMThkYjdiNDJhNzgzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IkhNQitLMURYU0tlbno3RTUwVVZMQ3U2dVVoNkIrbVwvckF3cTlRdWR5bkFzPSJ9 x-ctpclassification: CTP_IC dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v7TGNiZV027509 Content-Length: 2539 Lines: 63 > On Mon, Aug 28, 2017 at 7:51 AM, Liang, Kan wrote: > > > > I tried this patch and https://lkml.org/lkml/2017/8/27/222 together. > > But they don't fix the issue. I can still get the similar call stack. > > So the main issue was that I *really* hated Tim's patch #2, and the patch to > clean up the page wait queue should now make his patch series much more > palatable. > > Attached is an ALMOST COMPLETELY UNTESTED forward-port of those two > patches, now without that nasty WQ_FLAG_ARRIVALS logic, because we now > always put the new entries at the end of the waitqueue. > The patches fix the long wait issue. Tested-by: Kan Liang > The attached patches just apply directly on top of plain 4.13-rc7. > > That makes patch #2 much more palatable, since it now doesn't need to play > games and worry about new arrivals. > > But note the lack of testing. I've actually booted this and am running these > two patches right now, but honestly, you should consider them "untested" > simply because I can't trigger the page waiters contention case to begin with. > > But it's really just Tim's patches, modified for the page waitqueue cleanup > which makes patch #2 become much simpler, and now it's > palatable: it's just using the same bookmark thing that the normal wakeup > uses, no extra hacks. > > So Tim should look these over, and they should definitely be tested on that > load-from-hell that you guys have, but if this set works, at least I'm ok with it > now. > > Tim - did I miss anything? I added a "cpu_relax()" in there between the > release lock and irq and re-take it, I'm not convinced it makes any difference, > but I wanted to mark that "take a breather" thing. > > Oh, there's one more case I only realized after the patches: the stupid > add_page_wait_queue() code still adds to the head of the list. > So technically you need this too: > > diff --git a/mm/filemap.c b/mm/filemap.c > index 74123a298f53..598c3be57509 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -1061,7 +1061,7 @@ void add_page_wait_queue(struct page *page, > wait_queue_entry_t *waiter) > unsigned long flags; > > spin_lock_irqsave(&q->lock, flags); > - __add_wait_queue(q, waiter); > + __add_wait_queue_entry_tail(q, waiter); > SetPageWaiters(page); > spin_unlock_irqrestore(&q->lock, flags); > } > > but that only matters if you actually use the cachefiles thing, which I > hope/assume you don't. > > Linus