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.
