Re: [PATCH v2 3/5] userfaultfd: introduce write-likely mode for uffd operations

From: Peter Xu <peterx@redhat.com>
To: Nadav Amit <namit@vmware.com>
Cc: Linux MM <linux-mm@kvack.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Hugh Dickins <hughd@google.com>,
	Axel Rasmussen <axelrasmussen@google.com>,
	David Hildenbrand <david@redhat.com>,
	Mike Rapoport <rppt@linux.ibm.com>
Subject: Re: [PATCH v2 3/5] userfaultfd: introduce write-likely mode for uffd operations
Date: Mon, 18 Jul 2022 17:27:15 -0400	[thread overview]
Message-ID: <YtXQM6GhK+qbAa2R@xz-m1.local> (raw)
In-Reply-To: <9498662D-BE49-4EC7-8E7D-C78653EF671C@vmware.com>

On Mon, Jul 18, 2022 at 08:25:46PM +0000, Nadav Amit wrote:
> >> @@ -311,6 +321,7 @@ struct uffdio_continue {
> >> struct uffdio_range range;
> >> #define UFFDIO_CONTINUE_MODE_DONTWAKE ((__u64)1<<0)
> >> #define UFFDIO_CONTINUE_MODE_ACCESS_LIKELY ((__u64)1<<1)
> >> +#define UFFDIO_CONTINUE_MODE_WRITE_LIKELY ((__u64)1<<2)
> >> __u64 mode;
> > 
> > I thought you would have some reasoning on having the flag for unprotect
> > (since our last discussion you mentioned it) but it seems not there..
> > 
> > Then, could we only keep the zeropage write hint but drop the rest?
> > They're never used in this whole series besides the zeropage one, meanwhile
> > I think we're still not reaching consensus on whether they'll be helpful?
> 
> I think that I didn’t communicate clearly enough two things. First, the
> access flags are used here.
> 
> Now, you are correct that although the unprotect flag is defined here, it is
> not used in this patch-set. There is a reason for that.
> 
> It turns out that using David’s work to map a writable page can cause
> undesired behaviors - the clean PTE, which we discussed, and additional TLB
> shootdowns. Since it required a lot of changes to get rid of these
> additional shootdowns, I put the unprotect changes in a different patch-set.
> 
> https://lore.kernel.org/all/20220718120212.3180-1-namit@vmware.com/
> 
> Let me know if that answers your question.

Okay, I'll read it tomorrow, thanks.  Though note that IMHO we should have
the fix without depending on WRITE_HINT at all.  I hope that's what'll
happen in the other patchset, or I can also comment there.

Btw, if there's direct dependency on flags I'd rather squash the two
patchsets.  The thing is by sololy reading this patch the reader will have
no idea why you wanted to have WRITE_HINT outside ZEROPAGE, at least to me.
We could have introduced WRITE_HINT for ZEROPAGE in this patch (then IMO
you can squash that part with patch 4) then leave the rest for the other
patchset.

-- 
Peter Xu