From: Nicholas Piggin <npiggin@gmail.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Tim Chen <tim.c.chen@linux.intel.com>,
Mel Gorman <mgorman@techsingularity.net>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@elte.hu>, Andi Kleen <ak@linux.intel.com>,
Kan Liang <kan.liang@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: Mon, 28 Aug 2017 11:29:59 +1000 [thread overview]
Message-ID: <20170828112959.05622961@roar.ozlabs.ibm.com> (raw)
In-Reply-To: <20170828111648.22f81bc5@roar.ozlabs.ibm.com>
On Mon, 28 Aug 2017 11:16:48 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:
> On Sun, 27 Aug 2017 16:12:19 -0700
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
>
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index baba290c276b..0b41c8cbeabc 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -986,10 +986,6 @@ static inline int
> > wait_on_page_bit_common(wait_queue_head_t *q,
> >
> > if (likely(test_bit(bit_nr, &page->flags))) {
> > io_schedule();
> > - if (unlikely(signal_pending_state(state, current))) {
> > - ret = -EINTR;
> > - break;
> > - }
> > }
> >
> > if (lock) {
> > @@ -999,6 +995,11 @@ static inline int
> > wait_on_page_bit_common(wait_queue_head_t *q,
> > if (!test_bit(bit_nr, &page->flags))
> > break;
> > }
> > +
> > + if (unlikely(signal_pending_state(state, current))) {
> > + ret = -EINTR;
> > + break;
> > + }
> > }
> >
> > finish_wait(q, wait);
> >
> > but maybe I'm missing something.
> >
> > Nick, comments?
>
> No I don't think you're missing something. We surely could lose our only
> wakeup in this window. So an exclusive waiter has to always make sure
> they propagate the wakeup (regardless of what they do with the contended
> resources itself).
>
> Seems like your fix should solve it. By the look of how wait_on_bit_lock
> is structured, the author probably did think about this case a little
> better than I did :\
BTW. since you are looking at this stuff, one other small problem I remember
with exclusive waiters is that losing to a concurrent locker puts them to
the back of the queue. I think that could be fixed with some small change to
the wait loops (first add to tail, then retries add to head). Thoughts?
Thanks,
Nick
next prev parent reply other threads:[~2017-08-28 1:30 UTC|newest]
Thread overview: 32+ 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 ` [PATCH 2/2 v2] sched/wait: Introduce lock breaker in wake_up_page_bit Tim Chen
2017-08-25 19:58 ` Linus Torvalds
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 2:54 ` Linus Torvalds
2017-08-26 18:15 ` Linus Torvalds
2017-08-27 21:40 ` Linus Torvalds
2017-08-27 21:42 ` Linus Torvalds
2017-08-27 23:12 ` Linus Torvalds
2017-08-28 1:16 ` Nicholas Piggin
2017-08-28 1:29 ` Nicholas Piggin [this message]
2017-08-28 5:17 ` Linus Torvalds
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-29 12:57 ` Liang, Kan
2017-08-29 16:01 ` Linus Torvalds
2017-08-29 16:13 ` Tim Chen
2017-08-29 16:24 ` Linus Torvalds
2017-08-29 16:57 ` Tim Chen
2017-09-14 2:12 ` Tim Chen
2017-09-14 2:27 ` Linus Torvalds
2017-09-14 16:50 ` Tim Chen
2017-09-14 17:00 ` Linus Torvalds
2017-09-14 16:39 ` Christopher Lameter
2017-08-29 16:17 ` Tim Chen
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
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=20170828112959.05622961@roar.ozlabs.ibm.com \
--to=npiggin@gmail.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=tim.c.chen@linux.intel.com \
--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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).