| Message ID | Zh1GyNht6KKyRMuc@casper.infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] Trylock the mmap_lock in vmf_anon_prepare() | expand |
On Mon, Apr 15, 2024 at 8:24 AM Matthew Wilcox <willy@infradead.org> wrote: > > It seems a shame to fallback if the mmap_lock is readily available. > It doesn't blow up immediately in my testing ... thoughts? I can't think of anything that would break... > > From 70833af858ce41a80b5f0669648ee9f59b149a03 Mon Sep 17 00:00:00 2001 > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > Date: Mon, 15 Apr 2024 11:13:41 -0400 > Subject: [PATCH] mm: Optimise vmf_anon_prepare() for VMAs without an anon_vma > > If the mmap_lock can be taken for read, there's no reason not to take > it so that we can call __anon_vma_prepare(). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/memory.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/mm/memory.c b/mm/memory.c > index b9c23393fa9b..7075fb7f74da 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -3219,11 +3219,15 @@ vm_fault_t vmf_anon_prepare(struct vm_fault *vmf) > if (likely(vma->anon_vma)) > return 0; > if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > - vma_end_read(vma); > - return VM_FAULT_RETRY; > + if (!mmap_read_trylock(vma->vm_mm)) { > + vma_end_read(vma); > + return VM_FAULT_RETRY; > + } > } > if (__anon_vma_prepare(vma)) > return VM_FAULT_OOM; You should drop mmap_lock when returning VM_FAULT_OOM as well. > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) > + mmap_read_unlock(vma->vm_mm); > return 0; > } > > -- > 2.43.0 > >
On Mon, Apr 15, 2024 at 5:24 PM Matthew Wilcox <willy@infradead.org> wrote: > It seems a shame to fallback if the mmap_lock is readily available. > It doesn't blow up immediately in my testing ... thoughts? Seems fine to me, since nothing below the trylocked mmap_lock does vma_start_*(). > >From 70833af858ce41a80b5f0669648ee9f59b149a03 Mon Sep 17 00:00:00 2001 > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > Date: Mon, 15 Apr 2024 11:13:41 -0400 > Subject: [PATCH] mm: Optimise vmf_anon_prepare() for VMAs without an anon_vma > > If the mmap_lock can be taken for read, there's no reason not to take > it so that we can call __anon_vma_prepare(). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Jann Horn <jannh@google.com> > --- > mm/memory.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/mm/memory.c b/mm/memory.c > index b9c23393fa9b..7075fb7f74da 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -3219,11 +3219,15 @@ vm_fault_t vmf_anon_prepare(struct vm_fault *vmf) > if (likely(vma->anon_vma)) > return 0; > if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > - vma_end_read(vma); > - return VM_FAULT_RETRY; > + if (!mmap_read_trylock(vma->vm_mm)) { > + vma_end_read(vma); > + return VM_FAULT_RETRY; > + } > } > if (__anon_vma_prepare(vma)) > return VM_FAULT_OOM; > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) > + mmap_read_unlock(vma->vm_mm); > return 0; > } > > -- > 2.43.0
On Mon, Apr 15, 2024 at 09:14:10AM -0700, Suren Baghdasaryan wrote: > > if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > > - vma_end_read(vma); > > - return VM_FAULT_RETRY; > > + if (!mmap_read_trylock(vma->vm_mm)) { > > + vma_end_read(vma); > > + return VM_FAULT_RETRY; > > + } > > } > > if (__anon_vma_prepare(vma)) > > return VM_FAULT_OOM; > > You should drop mmap_lock when returning VM_FAULT_OOM as well. > > > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) > > + mmap_read_unlock(vma->vm_mm); > > return 0; > > } Thanks. Fixed and pushed to git://git.infradead.org/users/willy/pagecache.git vma-lock +++ b/mm/memory.c @@ -3224,16 +3224,21 @@ static inline vm_fault_t vmf_can_call_fault(const struct vm_fault *vmf) vm_fault_t vmf_anon_prepare(struct vm_fault *vmf) { struct vm_area_struct *vma = vmf->vma; + vm_fault_t ret = 0; if (likely(vma->anon_vma)) return 0; if (vmf->flags & FAULT_FLAG_VMA_LOCK) { - vma_end_read(vma); - return VM_FAULT_RETRY; + if (!mmap_read_trylock(vma->vm_mm)) { + vma_end_read(vma); + return VM_FAULT_RETRY; + } } if (__anon_vma_prepare(vma)) - return VM_FAULT_OOM; - return 0; + ret = VM_FAULT_OOM; + if (vmf->flags & FAULT_FLAG_VMA_LOCK) + mmap_read_unlock(vma->vm_mm); + return ret; } /*
On Mon, Apr 15, 2024 at 11:18 AM Matthew Wilcox <willy@infradead.org> wrote: > > On Mon, Apr 15, 2024 at 09:14:10AM -0700, Suren Baghdasaryan wrote: > > > if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > > > - vma_end_read(vma); > > > - return VM_FAULT_RETRY; > > > + if (!mmap_read_trylock(vma->vm_mm)) { > > > + vma_end_read(vma); > > > + return VM_FAULT_RETRY; > > > + } > > > } > > > if (__anon_vma_prepare(vma)) > > > return VM_FAULT_OOM; > > > > You should drop mmap_lock when returning VM_FAULT_OOM as well. > > > > > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) > > > + mmap_read_unlock(vma->vm_mm); > > > return 0; > > > } > > Thanks. Fixed and pushed to > git://git.infradead.org/users/willy/pagecache.git vma-lock That looks correct now. Reviewed-by: Suren Baghdasaryan <surenb@google.com> > > +++ b/mm/memory.c > @@ -3224,16 +3224,21 @@ static inline vm_fault_t vmf_can_call_fault(const struct vm_fault *vmf) > vm_fault_t vmf_anon_prepare(struct vm_fault *vmf) > { > struct vm_area_struct *vma = vmf->vma; > + vm_fault_t ret = 0; > > if (likely(vma->anon_vma)) > return 0; > if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > - vma_end_read(vma); > - return VM_FAULT_RETRY; > + if (!mmap_read_trylock(vma->vm_mm)) { > + vma_end_read(vma); > + return VM_FAULT_RETRY; > + } > } > if (__anon_vma_prepare(vma)) > - return VM_FAULT_OOM; > - return 0; > + ret = VM_FAULT_OOM; > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) > + mmap_read_unlock(vma->vm_mm); > + return ret; > } > > /* >
diff --git a/mm/memory.c b/mm/memory.c index b9c23393fa9b..7075fb7f74da 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3219,11 +3219,15 @@ vm_fault_t vmf_anon_prepare(struct vm_fault *vmf) if (likely(vma->anon_vma)) return 0; if (vmf->flags & FAULT_FLAG_VMA_LOCK) { - vma_end_read(vma); - return VM_FAULT_RETRY; + if (!mmap_read_trylock(vma->vm_mm)) { + vma_end_read(vma); + return VM_FAULT_RETRY; + } } if (__anon_vma_prepare(vma)) return VM_FAULT_OOM; + if (vmf->flags & FAULT_FLAG_VMA_LOCK) + mmap_read_unlock(vma->vm_mm); return 0; }