From: Tim Chen <tim.c.chen@linux.intel.com> To: Linus Torvalds <torvalds@linux-foundation.org>, "Liang, Kan" <kan.liang@intel.com> Cc: Mel Gorman <mgorman@techsingularity.net>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@elte.hu>, Andi Kleen <ak@linux.intel.com>, Andrew Morton <akpm@linux-foundation.org>, Johannes Weiner <hannes@cmpxchg.org>, Jan Kara <jack@suse.cz>, Christopher Lameter <cl@linux.com>, "Eric W . Biederman" <ebiederm@xmission.com>, Davidlohr Bueso <dave@stgolabs.net>, linux-mm <linux-mm@kvack.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 2/2 v2] sched/wait: Introduce lock breaker in wake_up_page_bit Date: Tue, 29 Aug 2017 09:17:49 -0700 [thread overview] Message-ID: <dd2f5b2b-cbb3-79ff-6982-94b97ff18986@linux.intel.com> (raw) In-Reply-To: <CA+55aFzotfXc07UoVtxvDpQOP8tEt8pgxeYe+cGs=BDUC_A4pA@mail.gmail.com> On 08/28/2017 09:48 AM, Linus Torvalds wrote: > On Mon, Aug 28, 2017 at 7:51 AM, Liang, Kan <kan.liang@intel.com> 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 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: BTW, are you going to add the chunk below separately as part of your wait queue cleanup patch? Tim > > 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Tim Chen <tim.c.chen@linux.intel.com> To: Linus Torvalds <torvalds@linux-foundation.org>, "Liang, Kan" <kan.liang@intel.com> Cc: Mel Gorman <mgorman@techsingularity.net>, Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@elte.hu>, Andi Kleen <ak@linux.intel.com>, Andrew Morton <akpm@linux-foundation.org>, Johannes Weiner <hannes@cmpxchg.org>, Jan Kara <jack@suse.cz>, Christopher Lameter <cl@linux.com>, "Eric W . Biederman" <ebiederm@xmission.com>, Davidlohr Bueso <dave@stgolabs.net>, linux-mm <linux-mm@kvack.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 2/2 v2] sched/wait: Introduce lock breaker in wake_up_page_bit Date: Tue, 29 Aug 2017 09:17:49 -0700 [thread overview] Message-ID: <dd2f5b2b-cbb3-79ff-6982-94b97ff18986@linux.intel.com> (raw) In-Reply-To: <CA+55aFzotfXc07UoVtxvDpQOP8tEt8pgxeYe+cGs=BDUC_A4pA@mail.gmail.com> On 08/28/2017 09:48 AM, Linus Torvalds wrote: > On Mon, Aug 28, 2017 at 7:51 AM, Liang, Kan <kan.liang@intel.com> 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 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: BTW, are you going to add the chunk below separately as part of your wait queue cleanup patch? Tim > > 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 > -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2017-08-29 16:17 UTC|newest] Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-08-25 16:13 [PATCH 1/2 v2] sched/wait: Break up long wake list walk Tim Chen 2017-08-25 16:13 ` Tim Chen 2017-08-25 16:13 ` [PATCH 2/2 v2] sched/wait: Introduce lock breaker in wake_up_page_bit Tim Chen 2017-08-25 16:13 ` Tim Chen 2017-08-25 19:58 ` Linus Torvalds 2017-08-25 19:58 ` Linus Torvalds 2017-08-25 22:19 ` Tim Chen 2017-08-25 22:19 ` Tim Chen 2017-08-25 22:51 ` Linus Torvalds 2017-08-25 23:03 ` Linus Torvalds 2017-08-26 0:31 ` Linus Torvalds 2017-08-26 0:31 ` Linus Torvalds 2017-08-26 2:54 ` Linus Torvalds 2017-08-26 2:54 ` Linus Torvalds 2017-08-26 18:15 ` Linus Torvalds 2017-08-27 21:40 ` Linus Torvalds 2017-08-27 21:40 ` Linus Torvalds 2017-08-27 21:42 ` Linus Torvalds 2017-08-27 21:42 ` Linus Torvalds 2017-08-27 23:12 ` Linus Torvalds 2017-08-27 23:12 ` Linus Torvalds 2017-08-28 1:16 ` Nicholas Piggin 2017-08-28 1:16 ` Nicholas Piggin 2017-08-28 1:29 ` Nicholas Piggin 2017-08-28 1:29 ` Nicholas Piggin 2017-08-28 5:17 ` Linus Torvalds 2017-08-28 5:17 ` Linus Torvalds 2017-08-28 7:18 ` Nicholas Piggin 2017-08-28 7:18 ` Nicholas Piggin 2017-08-28 14:51 ` Liang, Kan 2017-08-28 16:48 ` Linus Torvalds 2017-08-28 20:01 ` Tim Chen 2017-08-28 20:01 ` Tim Chen 2017-08-29 12:57 ` Liang, Kan 2017-08-29 16:01 ` Linus Torvalds 2017-08-29 16:01 ` Linus Torvalds 2017-08-29 16:13 ` Tim Chen 2017-08-29 16:13 ` Tim Chen 2017-08-29 16:24 ` Linus Torvalds 2017-08-29 16:24 ` Linus Torvalds 2017-08-29 16:57 ` Tim Chen 2017-08-29 16:57 ` Tim Chen 2017-09-14 2:12 ` Tim Chen 2017-09-14 2:12 ` Tim Chen 2017-09-14 2:27 ` Linus Torvalds 2017-09-14 2:27 ` Linus Torvalds 2017-09-14 16:50 ` Tim Chen 2017-09-14 16:50 ` Tim Chen 2017-09-14 17:00 ` Linus Torvalds 2017-09-14 17:00 ` Linus Torvalds 2017-09-14 16:39 ` Christopher Lameter 2017-08-29 16:17 ` Tim Chen [this message] 2017-08-29 16:17 ` Tim Chen 2017-08-29 16:22 ` Linus Torvalds 2017-08-29 16:22 ` Linus Torvalds 2017-08-25 17:46 ` [PATCH 1/2 v2] sched/wait: Break up long wake list walk Christopher Lameter 2017-08-25 17:46 ` Christopher Lameter
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=dd2f5b2b-cbb3-79ff-6982-94b97ff18986@linux.intel.com \ --to=tim.c.chen@linux.intel.com \ --cc=ak@linux.intel.com \ --cc=akpm@linux-foundation.org \ --cc=cl@linux.com \ --cc=dave@stgolabs.net \ --cc=ebiederm@xmission.com \ --cc=hannes@cmpxchg.org \ --cc=jack@suse.cz \ --cc=kan.liang@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=mgorman@techsingularity.net \ --cc=mingo@elte.hu \ --cc=peterz@infradead.org \ --cc=torvalds@linux-foundation.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.