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;
> }
>
> static vm_fault_t vmf_anon_prepare(struct vm_fault *vmf)
> {
> ...
> if ((vmf->flags & FAULT_FLAG_VMA_LOCK) && !is_vma_lock_sufficient(vma) {
> vma_end_read(vma);
> return VM_FAULT_RETRY;
> }
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?
>
> > + if (ret)
> > + return ret;
> >
> > ret = __do_fault(vmf);
> > if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY)))
> > --
> > 2.40.1
> >
