On Tue, Feb 2, 2021 at 9:15 AM Peter Xu [off-list ref] wrote:

On Mon, Feb 01, 2021 at 01:31:59PM -0500, Peter Xu wrote:

quoted

On Thu, Jan 28, 2021 at 02:48:15PM -0800, Axel Rasmussen wrote:

quoted

This feature allows userspace to intercept "minor" faults. By "minor"
faults, I mean the following situation:

Let there exist two mappings (i.e., VMAs) to the same page(s) (shared
memory). One of the mappings is registered with userfaultfd (in minor
mode), and the other is not. Via the non-UFFD mapping, the underlying
pages have already been allocated & filled with some contents. The UFFD
mapping has not yet been faulted in; when it is touched for the first
time, this results in what I'm calling a "minor" fault. As a concrete
example, when working with hugetlbfs, we have huge_pte_none(), but
find_lock_page() finds an existing page.

This commit adds the new registration mode, and sets the relevant flag
on the VMAs being registered. In the hugetlb fault path, if we find
that we have huge_pte_none(), but find_lock_page() does indeed find an
existing page, then we have a "minor" fault, and if the VMA has the
userfaultfd registration flag, we call into userfaultfd to handle it.

When re-read, now I'm thinking whether we should restrict the minor fault
scenario with shared mappings always, assuming there's one mapping with uffd
and the other one without, while the non-uffd can modify the data before an
UFFDIO_CONTINUE kicking the uffd process.

To me, it's really more about page cache and that's all..

So I'm wondering whether below would be simpler and actually clearer on
defining minor faults, comparing to the above whole two paragraphs.  For
example, the scemantics do not actually need two mappings:

    For shared memory, userfaultfd missing fault used to only report the event
    if the page cache does not exist for the current fault process.  Here we
    define userfaultfd minor fault as the case where the missing page fault
    does have a backing page cache (so only the pgtable entry is missing).

It should not affect most of your code, but only one below [1].

OK it could be slightly more than that...

E.g. we'd need to make UFFDIO_COPY to not install the write bit if it's
UFFDIO_CONTINUE and if it's private mappings. In hugetlb_mcopy_atomic_pte() now
we apply the write bit unconditionally:

        _dst_pte = make_huge_pte(dst_vma, page, dst_vma->vm_flags & VM_WRITE);

That'll need a touch-up otherwise.

It's just the change seems still very small so I'd slightly prefer to support
it all.  However I don't want to make your series complicated and blocking it,
so please feel free to still make it shared memory if that's your preference.
The worst case is if someone would like to enable this (if with a valid user
scenario) we'd export a new uffd feature flag.

quoted

[...]

quoted

@@ -1302,9 +1301,26 @@ static inline bool vma_can_userfault(struct vm_area_struct *vma,
                                 unsigned long vm_flags)
 {
    /* FIXME: add WP support to hugetlbfs and shmem */
-   return vma_is_anonymous(vma) ||
-           ((is_vm_hugetlb_page(vma) || vma_is_shmem(vma)) &&
-            !(vm_flags & VM_UFFD_WP));
+   if (vm_flags & VM_UFFD_WP) {
+           if (is_vm_hugetlb_page(vma) || vma_is_shmem(vma))
+                   return false;
+   }
+
+   if (vm_flags & VM_UFFD_MINOR) {
+           /*
+            * The use case for minor registration (intercepting minor
+            * faults) is to handle the case where a page is present, but
+            * needs to be modified before it can be used. This requires
+            * two mappings: one with UFFD registration, and one without.
+            * So, it only makes sense to do this with shared memory.
+            */
+           /* FIXME: Add minor fault interception for shmem. */
+           if (!(is_vm_hugetlb_page(vma) && (vma->vm_flags & VM_SHARED)))
+                   return false;

[1]

So here we also restrict the mapping be shared.  My above comment on the commit
message is also another way to ask whether we could also allow it to happen
with non-shared mappings as long as there's a page cache.  If so, we could drop
the VM_SHARED check here.  It won't affect your existing use case for sure, it
just gives more possibility that maybe it could also be used on non-shared
mappings due to some reason in the future.

What do you think?

Agreed, I don't see any reason why it can't work. The only requirement
for it to be useful is, the UFFD-registered area needs to be able to
"see" writes from the non-UFFD-registered area. Whether or not the
UFFD-registered half is shared or not doesn't affect this.

I'll include this change (and the VM_WRITE touchup described above) in a v4.

quoted

The rest looks good to me.

Thanks,

--
Peter Xu

--
Peter Xu