From: Mike Rapoport <rppt@linux.ibm.com>
To: Peter Xu <peterx@redhat.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
David Hildenbrand <david@redhat.com>,
Hugh Dickins <hughd@google.com>, Maya Gokhale <gokhale2@llnl.gov>,
Jerome Glisse <jglisse@redhat.com>,
Pavel Emelyanov <xemul@virtuozzo.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Martin Cracauer <cracauer@cons.org>, Shaohua Li <shli@fb.com>,
Marty McFadden <mcfadden8@llnl.gov>,
Andrea Arcangeli <aarcange@redhat.com>,
Mike Kravetz <mike.kravetz@oracle.com>,
Denis Plotnikov <dplotnikov@virtuozzo.com>,
Mike Rapoport <rppt@linux.vnet.ibm.com>,
Mel Gorman <mgorman@suse.de>,
"Kirill A . Shutemov" <kirill@shutemov.name>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v2 23/26] userfaultfd: wp: don't wake up when doing write protect
Date: Tue, 26 Feb 2019 10:00:29 +0200 [thread overview]
Message-ID: <20190226080029.GH5873@rapoport-lnx> (raw)
In-Reply-To: <20190226074117.GL13653@xz-x1>
On Tue, Feb 26, 2019 at 03:41:17PM +0800, Peter Xu wrote:
> On Tue, Feb 26, 2019 at 09:29:33AM +0200, Mike Rapoport wrote:
> > On Tue, Feb 26, 2019 at 02:24:52PM +0800, Peter Xu wrote:
> > > On Mon, Feb 25, 2019 at 11:09:35PM +0200, Mike Rapoport wrote:
> > > > On Tue, Feb 12, 2019 at 10:56:29AM +0800, Peter Xu wrote:
> > > > > It does not make sense to try to wake up any waiting thread when we're
> > > > > write-protecting a memory region. Only wake up when resolving a write
> > > > > protected page fault.
> > > > >
> > > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > > ---
> > > > > fs/userfaultfd.c | 13 ++++++++-----
> > > > > 1 file changed, 8 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > > > > index 81962d62520c..f1f61a0278c2 100644
> > > > > --- a/fs/userfaultfd.c
> > > > > +++ b/fs/userfaultfd.c
> > > > > @@ -1771,6 +1771,7 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
> > > > > struct uffdio_writeprotect uffdio_wp;
> > > > > struct uffdio_writeprotect __user *user_uffdio_wp;
> > > > > struct userfaultfd_wake_range range;
> > > > > + bool mode_wp, mode_dontwake;
> > > > >
> > > > > if (READ_ONCE(ctx->mmap_changing))
> > > > > return -EAGAIN;
> > > > > @@ -1789,18 +1790,20 @@ static int userfaultfd_writeprotect(struct userfaultfd_ctx *ctx,
> > > > > if (uffdio_wp.mode & ~(UFFDIO_WRITEPROTECT_MODE_DONTWAKE |
> > > > > UFFDIO_WRITEPROTECT_MODE_WP))
> > > > > return -EINVAL;
> > > > > - if ((uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP) &&
> > > > > - (uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE))
> > > > > +
> > > > > + mode_wp = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_WP;
> > > > > + mode_dontwake = uffdio_wp.mode & UFFDIO_WRITEPROTECT_MODE_DONTWAKE;
> > > > > +
> > > > > + if (mode_wp && mode_dontwake)
> > > > > return -EINVAL;
> > > >
> > > > This actually means the opposite of the commit message text ;-)
> > > >
> > > > Is any dependency of _WP and _DONTWAKE needed at all?
> > >
> > > So this is indeed confusing at least, because both you and Jerome have
> > > asked the same question... :)
> > >
> > > My understanding is that we don't have any reason to wake up any
> > > thread when we are write-protecting a range, in that sense the flag
> > > UFFDIO_WRITEPROTECT_MODE_DONTWAKE is already meaningless in the
> > > UFFDIO_WRITEPROTECT ioctl context. So before everything here's how
> > > these flags are defined:
> > >
> > > struct uffdio_writeprotect {
> > > struct uffdio_range range;
> > > /* !WP means undo writeprotect. DONTWAKE is valid only with !WP */
> > > #define UFFDIO_WRITEPROTECT_MODE_WP ((__u64)1<<0)
> > > #define UFFDIO_WRITEPROTECT_MODE_DONTWAKE ((__u64)1<<1)
> > > __u64 mode;
> > > };
> > >
> > > To make it clear, we simply define it as "DONTWAKE is valid only with
> > > !WP". When with that, "mode_wp && mode_dontwake" is indeed a
> > > meaningless flag combination. Though please note that it does not
> > > mean that the operation ("don't wake up the thread") is meaningless -
> > > that's what we'll do no matter what when WP==1. IMHO it's only about
> > > the interface not the behavior.
> > >
> > > I don't have a good way to make this clearer because firstly we'll
> > > need the WP flag to mark whether we're protecting or unprotecting the
> > > pages. Later on, we need DONTWAKE for page fault handling case to
> > > mark that we don't want to wake up the waiting thread now. So both
> > > the flags have their reason to stay so far. Then with all these in
> > > mind what I can think of is only to forbid using DONTWAKE in WP case,
> > > and that's how above definition comes (I believe, because it was
> > > defined that way even before I started to work on it and I think it
> > > makes sense).
> >
> > There's no argument how DONTWAKE can be used with !WP. The
> > userfaultfd_writeprotect() is called in response of the uffd monitor to WP
> > page fault, it asks to clear write protection to some range, but it does
> > not want to wake the faulting thread yet but rather it will use uffd_wake()
> > later.
> >
> > Still, I can't grok the usage of DONTWAKE with WP=1. In my understanding,
> > in this case userfaultfd_writeprotect() is called unrelated to page faults,
> > and the monitored thread runs freely, so why it should be waked at all?
>
> Exactly this is how I understand it. And that's why I wrote this
> patch to remove the extra wakeup() since I think it's unecessary.
>
> >
> > And what happens, if the thread is waiting on a missing page fault and we
> > do userfaultfd_writeprotect(WP=1) at the same time?
>
> Then IMHO the userfaultfd_writeprotect() will be a noop simply because
> the page is still missing. Here if with the old code (before this
> patch) we'll probably even try to wake up this thread but this thread
> should just fault again on the same address due to the fact that the
> page is missing. After this patch the monitored thread should
> continue to wait on the missing page.
So, my understanding of what we have is:
userfaultfd_writeprotect() can be used either to mark a region as write
protected or to resolve WP page fault.
In the first case DONTWAKE does not make sense and we forbid setting it
with WP=1.
In the second case it's the uffd monitor decision whether to wake up the
faulting thread immediately after #PF is resolved or later, so with WP=0 we
allow DONTWAKE.
I suggest to extend the comment in the definition of
'struct uffdio_writeprotect' to something like
/*
* Write protecting a region (WP=1) is unrelated to page faults, therefore
* DONTWAKE flag is meaningless with WP=1.
* Removing write protection (WP=0) in response to a page fault wakes the
* faulting task unless DONTWAKE is set.
*/
And a documentation update along these lines would be appreciated :)
> Thanks,
>
> --
> Peter Xu
>
--
Sincerely yours,
Mike.
next prev parent reply other threads:[~2019-02-26 8:00 UTC|newest]
Thread overview: 113+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-12 2:56 [PATCH v2 00/26] userfaultfd: write protection support Peter Xu
2019-02-12 2:56 ` [PATCH v2 01/26] mm: gup: rename "nonblocking" to "locked" where proper Peter Xu
2019-02-21 15:17 ` Jerome Glisse
2019-02-22 3:42 ` Peter Xu
2019-02-12 2:56 ` [PATCH v2 02/26] mm: userfault: return VM_FAULT_RETRY on signals Peter Xu
2019-02-21 15:29 ` Jerome Glisse
2019-02-22 3:51 ` Peter Xu
2019-02-12 2:56 ` [PATCH v2 03/26] userfaultfd: don't retake mmap_sem to emulate NOPAGE Peter Xu
2019-02-21 15:34 ` Jerome Glisse
2019-02-12 2:56 ` [PATCH v2 04/26] mm: allow VM_FAULT_RETRY for multiple times Peter Xu
2019-02-13 3:34 ` Peter Xu
2019-02-20 11:48 ` Peter Xu
2019-02-21 8:56 ` [PATCH v2.1 " Peter Xu
2019-02-21 15:53 ` Jerome Glisse
2019-02-22 4:25 ` Peter Xu
2019-02-22 15:11 ` Jerome Glisse
2019-02-25 6:19 ` Peter Xu
2019-02-12 2:56 ` [PATCH v2 05/26] mm: gup: " Peter Xu
2019-02-21 16:06 ` Jerome Glisse
2019-02-22 4:41 ` Peter Xu
2019-02-22 15:13 ` Jerome Glisse
2019-02-12 2:56 ` [PATCH v2 06/26] userfaultfd: wp: add helper for writeprotect check Peter Xu
2019-02-21 16:07 ` Jerome Glisse
2019-02-25 15:41 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 07/26] userfaultfd: wp: hook userfault handler to write protection fault Peter Xu
2019-02-21 16:25 ` Jerome Glisse
2019-02-25 15:43 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 08/26] userfaultfd: wp: add WP pagetable tracking to x86 Peter Xu
2019-02-21 17:20 ` Jerome Glisse
2019-02-25 15:48 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 09/26] userfaultfd: wp: userfaultfd_pte/huge_pmd_wp() helpers Peter Xu
2019-02-21 17:21 ` Jerome Glisse
2019-02-25 17:12 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 10/26] userfaultfd: wp: add UFFDIO_COPY_MODE_WP Peter Xu
2019-02-21 17:29 ` Jerome Glisse
2019-02-22 7:11 ` Peter Xu
2019-02-22 15:15 ` Jerome Glisse
2019-02-25 6:45 ` Peter Xu
2019-02-25 15:58 ` Mike Rapoport
2019-02-26 5:09 ` Peter Xu
2019-02-26 8:28 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 11/26] mm: merge parameters for change_protection() Peter Xu
2019-02-21 17:32 ` Jerome Glisse
2019-02-12 2:56 ` [PATCH v2 12/26] userfaultfd: wp: apply _PAGE_UFFD_WP bit Peter Xu
2019-02-21 17:44 ` Jerome Glisse
2019-02-22 7:31 ` Peter Xu
2019-02-22 15:17 ` Jerome Glisse
2019-02-25 18:00 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 13/26] mm: export wp_page_copy() Peter Xu
2019-02-21 17:44 ` Jerome Glisse
2019-02-12 2:56 ` [PATCH v2 14/26] userfaultfd: wp: handle COW properly for uffd-wp Peter Xu
2019-02-21 18:04 ` Jerome Glisse
2019-02-22 8:46 ` Peter Xu
2019-02-22 15:35 ` Jerome Glisse
2019-02-25 7:13 ` Peter Xu
2019-02-25 15:32 ` Jerome Glisse
2019-02-12 2:56 ` [PATCH v2 15/26] userfaultfd: wp: drop _PAGE_UFFD_WP properly when fork Peter Xu
2019-02-21 18:06 ` Jerome Glisse
2019-02-22 9:09 ` Peter Xu
2019-02-22 15:36 ` Jerome Glisse
2019-02-25 18:19 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 16/26] userfaultfd: wp: add pmd_swp_*uffd_wp() helpers Peter Xu
2019-02-21 18:07 ` Jerome Glisse
2019-02-25 18:20 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 17/26] userfaultfd: wp: support swap and page migration Peter Xu
2019-02-21 18:16 ` Jerome Glisse
2019-02-25 7:48 ` Peter Xu
2019-02-25 18:28 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 18/26] khugepaged: skip collapse if uffd-wp detected Peter Xu
2019-02-21 18:17 ` Jerome Glisse
2019-02-25 18:50 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 19/26] userfaultfd: introduce helper vma_find_uffd Peter Xu
2019-02-21 18:19 ` Jerome Glisse
2019-02-25 20:48 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 20/26] userfaultfd: wp: support write protection for userfault vma range Peter Xu
2019-02-21 18:23 ` Jerome Glisse
2019-02-25 8:16 ` Peter Xu
2019-02-25 20:52 ` Mike Rapoport
2019-02-26 6:06 ` Peter Xu
2019-02-26 6:43 ` Mike Rapoport
2019-02-26 7:20 ` Peter Xu
2019-02-26 7:46 ` Mike Rapoport
2019-02-26 7:54 ` Peter Xu
2019-02-12 2:56 ` [PATCH v2 21/26] userfaultfd: wp: add the writeprotect API to userfaultfd ioctl Peter Xu
2019-02-21 18:28 ` Jerome Glisse
2019-02-25 8:31 ` Peter Xu
2019-02-25 21:03 ` Mike Rapoport
2019-02-26 6:30 ` Peter Xu
2019-02-12 2:56 ` [PATCH v2 22/26] userfaultfd: wp: enabled write protection in userfaultfd API Peter Xu
2019-02-21 18:29 ` Jerome Glisse
2019-02-25 8:34 ` Peter Xu
2019-02-12 2:56 ` [PATCH v2 23/26] userfaultfd: wp: don't wake up when doing write protect Peter Xu
2019-02-21 18:36 ` Jerome Glisse
2019-02-25 8:58 ` Peter Xu
2019-02-25 21:15 ` Mike Rapoport
2019-02-25 21:09 ` Mike Rapoport
2019-02-26 6:24 ` Peter Xu
2019-02-26 7:29 ` Mike Rapoport
2019-02-26 7:41 ` Peter Xu
2019-02-26 8:00 ` Mike Rapoport [this message]
2019-02-28 2:47 ` Peter Xu
2019-02-26 8:00 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 24/26] userfaultfd: wp: UFFDIO_REGISTER_MODE_WP documentation update Peter Xu
2019-02-21 18:38 ` Jerome Glisse
2019-02-25 21:19 ` Mike Rapoport
2019-02-26 6:53 ` Peter Xu
2019-02-26 7:04 ` Mike Rapoport
2019-02-26 7:42 ` Peter Xu
2019-02-12 2:56 ` [PATCH v2 25/26] userfaultfd: selftests: refactor statistics Peter Xu
2019-02-26 6:50 ` Mike Rapoport
2019-02-12 2:56 ` [PATCH v2 26/26] userfaultfd: selftests: add write-protect test Peter Xu
2019-02-26 6:58 ` Mike Rapoport
2019-02-26 7:52 ` Peter Xu
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=20190226080029.GH5873@rapoport-lnx \
--to=rppt@linux.ibm.com \
--cc=aarcange@redhat.com \
--cc=cracauer@cons.org \
--cc=david@redhat.com \
--cc=dgilbert@redhat.com \
--cc=dplotnikov@virtuozzo.com \
--cc=gokhale2@llnl.gov \
--cc=hannes@cmpxchg.org \
--cc=hughd@google.com \
--cc=jglisse@redhat.com \
--cc=kirill@shutemov.name \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mcfadden8@llnl.gov \
--cc=mgorman@suse.de \
--cc=mike.kravetz@oracle.com \
--cc=peterx@redhat.com \
--cc=rppt@linux.vnet.ibm.com \
--cc=shli@fb.com \
--cc=xemul@virtuozzo.com \
/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).