Re: [PATCH v2 23/26] userfaultfd: wp: don't wake up when doing write protect

[Mike Rapoport]

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@xxxxxxxxxx>
> > > > > ---
> > > > >  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.