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

From: Suren Baghdasaryan <surenb@google.com>
To: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org
Subject: Re: [PATCH 3/6] mm: Handle shared faults under the VMA lock
Date: Thu, 28 Sep 2023 08:02:58 -0700	[thread overview]
Message-ID: <CAJuCfpEq7igxfZ8YERjHHnMvKPxz8DV8fjKKDfTdTxA5iqAxOw@mail.gmail.com> (raw)
In-Reply-To: <ZRUQNmREqtz17S1z@casper.infradead.org>

On Wed, Sep 27, 2023 at 10:33 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> 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.

Ok, I think it's better. At least it does not imply that our intention
is to unlock the VMA.

>