Re: [PATCH] mm: put_and_wait_on_page_locked() while page is migrated

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Hugh Dickins <hughd@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	bhe@redhat.com, Michal Hocko <mhocko@suse.com>,
	Vlastimil Babka <vbabka@suse.cz>,
	Andrea Arcangeli <aarcange@redhat.com>,
	david@redhat.com, mgorman@techsingularity.net,
	dh.herrmann@gmail.com, Tim Chen <tim.c.chen@linux.intel.com>,
	kan.liang@intel.com, Andi Kleen <ak@linux.intel.com>,
	Davidlohr Bueso <dave@stgolabs.net>,
	Peter Zijlstra <peterz@infradead.org>,
	Christoph Lameter <cl@linux.com>, Nick Piggin <npiggin@gmail.com>,
	pifang@redhat.com,
	Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
	linux-mm@kvack.org
Subject: Re: [PATCH] mm: put_and_wait_on_page_locked() while page is migrated
Date: Sun, 25 Nov 2018 10:30:25 -0800	[thread overview]
Message-ID: <CAHk-=wjeqKYevxGnfCM4UkxX8k8xfArzM6gKkG3BZg1jBYThVQ@mail.gmail.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1811241858540.4415@eggly.anvils>

On Sat, Nov 24, 2018 at 7:21 PM Hugh Dickins <hughd@google.com> wrote:
>
> Linus, I'm addressing this patch to you because I see from Tim Chen's
> thread that it would interest you, and you were disappointed not to
> root cause the issue back then.  I'm not pushing for you to fast-track
> this into 4.20-rc, but I expect Andrew will pick it up for mmotm, and
> thence linux-next.  Or you may spot a terrible defect, but I hope not.

The only terrible defect I spot is that I wish the change to the
'lock' argument in wait_on_page_bit_common() came with a comment
explaining the new semantics.

The old semantics were somewhat obvious (even if not documented): if
'lock' was set,  we'd make the wait exclusive, and we'd lock the page
before returning. That kind of matches the intuitive meaning for the
function prototype, and it's pretty obvious in the callers too.

The new semantics don't have the same kind of really intuitive
meaning, I feel. That "-1" doesn't mean "unlock", it means "drop page
reference", so there is no longer a fairly intuitive and direct
mapping between the argument name and type and the behavior of the
function.

So I don't hate the concept of the patch at all, but I do ask to:

 - better documentation.

   This might not be "documentation" at all, maybe that "lock"
variable should just be renamed (because it's not about just locking
any more), and would be better off as a tristate enum called
"behavior" that has "LOCK, DROP, WAIT" values?

 - while it sounds likely that this is indeed the same issue that
plagues us with the insanely long wait-queues, it would be *really*
nice to have that actually confirmed.

   Does somebody still have access to the customer load that triggered
the horrible scaling issues before?

In particular, on that second issue: the "fixes" that went in for the
wait-queues didn't really fix any real scalability problem, it really
just fixed the excessive irq latency issues due to the long traversal
holding a lock.

If this really fixes the fundamental issue, that should show up as an
actual performance difference, I'd expect..

End result: I like and approve of the patch, but I'd like it a lot
more if the code behavior was clarified a bit, and I'd really like to
close the loop on that old nasty page wait queue issue...

                   Linus