Re: [PATCH v3 5/9] userfaultfd: add minor fault registration mode

[Axel Rasmussen <axelrasmussen@google.com>]

On Tue, Feb 2, 2021 at 9:15 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Feb 01, 2021 at 01:31:59PM -0500, Peter Xu wrote:
> > On Thu, Jan 28, 2021 at 02:48:15PM -0800, Axel Rasmussen wrote:
> > > 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.
>
> >
> > [...]
> >
> > > @@ -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.

> >
> > The rest looks good to me.
> >
> > Thanks,
> >
> > --
> > Peter Xu
>
> --
> Peter Xu
>