Re: [RFC PATCH 02/21] Add a prelocked wake-up

From: Tim Chen <tim.c.chen@linux.intel.com>
To: Linus Torvalds <torvalds@linux-foundation.org>,
	David Howells <dhowells@redhat.com>,
	Kan Liang <kan.liang@intel.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>,
	Stephen Smalley <sds@tycho.nsa.gov>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Nicolas Dichtel <nicolas.dichtel@6wind.com>,
	raven@themaw.net, Christian Brauner <christian@brauner.io>,
	keyrings@vger.kernel.org, linux-usb@vger.kernel.org,
	linux-block <linux-block@vger.kernel.org>,
	LSM List <linux-security-module@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Linux API <linux-api@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH 02/21] Add a prelocked wake-up
Date: Wed, 16 Oct 2019 10:02:24 -0700	[thread overview]
Message-ID: <533ca3c5-1592-e59b-eafe-bc4a352a442d@linux.intel.com> (raw)
In-Reply-To: <CAHk-=whiz1sHXu8SVZKEC2dup=r5JMrftPtEt6ff9Ea8dyH8yQ@mail.gmail.com>

On 10/15/19 3:14 PM, Linus Torvalds wrote:
> On Tue, Oct 15, 2019 at 2:48 PM David Howells <dhowells@redhat.com> wrote:
>>
>> Add a wakeup call for a case whereby the caller already has the waitqueue
>> spinlock held.
> 
> That naming is crazy.
> 
> We already have helper functions like this, and they are just called
> "wake_up_locked()".
> 
> So the "prelocked" naming is just odd. Make it be
> wake_up_interruptible_sync_poll_locked().
> 
> The helper function should likely be
> 
>   void __wake_up_locked_sync_key(struct wait_queue_head *wq_head,
> unsigned int mode, void *key)
>   {
>         __wake_up_common(wq_head, mode, 1, WF_SYNC, key, NULL);
>   }
>   EXPORT_SYMBOL_GPL(__wake_up_locked_sync_key);
> 
> to match the other naming patterns there.
> 
> [ Unrelated ]
> 
> Looking at that mess of functions, I also wonder if we should try to
> just remove the bookmark code again. It was cute, and it was useful,
> but I think the problem with the page lock list may have been fixed by
> commit 9a1ea439b16b ("mm: put_and_wait_on_page_locked() while page is
> migrated") which avoids the retry condition with
> migrate_page_move_mapping().
> 
> Tim/Kan? Do you have the problematic load still?
> 

Unfortunately, we do not have ready access to that problematic load
which was run by a customer on 8 socket system.  They were not
willing to give the workload to us, and have not responded to my
request to rerun their load with commit 9a1ea439b16b.

The commit greatly reduced migration failures with concurrent page faulting.
And successful migrations could have prevented the big
pile up of waiters faulting and waiting on the page, which was the
problem the bookmark code was trying to solve.

So I also tend to think that the problem should have been resolved.
But unfortunately I don't have a ready workload to confirm.

Tim