Re: [PATCH 3/6] mm: Handle shared faults under the VMA lock

[Matthew Wilcox <willy@infradead.org>]

On Wed, Sep 27, 2023 at 06:02:47PM -0700, Suren Baghdasaryan wrote:
> On Wed, Sep 27, 2023 at 5:46 PM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Tue, Sep 26, 2023 at 10:25 PM Matthew Wilcox (Oracle)
> > <willy@infradead.org> wrote:
> > >
> > > There are many implementations of ->fault and some of them depend on
> > > mmap_lock being held.  All vm_ops that implement ->map_pages() end
> > > up calling filemap_fault(), which I have audited to be sure it does
> > > not rely on mmap_lock.  So (for now) key off ->map_pages existing
> > > as being a flag to indicate that it's safe to call ->fault while
> > > only holding the vma lock.
> > >
> > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> > > ---
> > >  mm/memory.c | 22 ++++++++++++++++++----
> > >  1 file changed, 18 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index cff78c496728..0f3da4889230 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -3042,6 +3042,21 @@ static inline void wp_page_reuse(struct vm_fault *vmf)
> > >         count_vm_event(PGREUSE);
> > >  }
> > >
> > > +/*
> > > + * We could add a bitflag somewhere, but for now, we know that all
> > > + * vm_ops that have a ->map_pages have been audited and don't need
> > > + * the mmap_lock to be held.
> > > + */
> > > +static inline vm_fault_t vmf_maybe_unlock_vma(const struct vm_fault *vmf)
> > > +{
> > > +       struct vm_area_struct *vma = vmf->vma;
> > > +
> > > +       if (vma->vm_ops->map_pages || !(vmf->flags & FAULT_FLAG_VMA_LOCK))
> > > +               return 0;
> > > +       vma_end_read(vma);
> > > +       return VM_FAULT_RETRY;
> > > +}
> > > +
> > >  static vm_fault_t vmf_anon_prepare(struct vm_fault *vmf)
> > >  {
> > >         struct vm_area_struct *vma = vmf->vma;
> > > @@ -4669,10 +4684,9 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
> > >         vm_fault_t ret, tmp;
> > >         struct folio *folio;
> > >
> > > -       if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
> > > -               vma_end_read(vma);
> > > -               return VM_FAULT_RETRY;
> > > -       }
> > > +       ret = vmf_maybe_unlock_vma(vmf);
> >
> > The name of this new function in this context does not seem
> > appropriate to me. The logic of this check was that we can't rely on
> > VMA lock since it might not be sufficient, so we have to retry with
> > mmap_lock instead. With this change it seems like we intentionally try
> > to unlock the VMA here. IMHO this would be more understandable:
> >
> > static inline bool is_vma_lock_sufficient(struct vm_area_struct *vma) {
> >     return vma->vm_ops->map_pages != NULL;
> > }

Originally I called this function vma_needs_mmap_lock() (with the
opposite polarity).  But I disliked the duplication of code ...

> Same comment for the rest of the patches where vmf_maybe_unlock_vma()
> is being used. It would be great to have this logic coded in one
> function like you do but I could not find an appropriate name that
> would convey that "we want to check if the current lock is sufficient
> and if not then we will drop it and retry". Maybe you or someone else
> can think of a good name for it?

Maybe

+static inline vm_fault_t vmf_can_call_fault(const struct vm_fault *vmf)
+{
+       struct vm_area_struct *vma = vmf->vma;
+
+       if (vma->vm_ops->map_pages || !(vmf->flags & FAULT_FLAG_VMA_LOCK))
+               return 0;
+       vma_end_read(vma);
+       return VM_FAULT_RETRY;
+}

I'm having trouble coming up with a name that doesn't imply it's a bool
predicate.