On Wed, Sep 27, 2023 at 10:33 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxx> wrote:
> > >
> > > On Tue, Sep 26, 2023 at 10:25 PM Matthew Wilcox (Oracle)
> > > <willy@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > > > ---
> > > > 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.
>
