On Tue, Sep 27, 2016 at 7:34 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> Right, I never really liked that patch. In any case, the below seems to
> boot, although the lock_page_wait() thing did get my brain in a bit of a
> twist. Doing explicit loops with PG_contended inside wq->lock would be
> more obvious but results in much more code.
>
> We could muck about with PG_contended naming/placement if any of this
> shows benefit.
>
> It does boot on my x86_64 and builds a kernel, so it must be perfect ;-)
This patch looks very much like what I was thinking of. Except you
made that bit clearing more complicated than I would have done.
I see why you did it (it's hard to clear the bit when the wait-queue
that is associated with it can be associated with multiple pages), but
I think it would be perfectly fine to just not even try to make the
"contended" bit be an exact bit. You can literally leave it set
(giving us the existing behavior), but then when you hit the
__unlock_page() case, and you look up the page_waitqueue(), and find
that the waitqueue is empty, *then* you clear it.
So you'd end up going through the slow path one too many times per
page, but considering that right now we *always* go through that
slow-path, and the "one too many times" is "two times per IO rather
than just once", it really is not a performance issue. I'd rather go
for simple and robust.
I get a bit nervous when I see you being so careful in counting the
number of waiters that match the page key - if any of that code ever
gets it wrong (because two different pages that shared a waitqueue
happen to race at just the right time), and the bit gets cleared too
early, you will get some *very* hard-to-debug problems.
So I actually think your patch is a bit too clever.
But maybe there's a reason for that that I just don't see. My gut feel
is that your patch is good.
.. and hey, it booted and compiled the kernel, so as you say, it must
be perfect.
Linus
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>