| Message ID | 20230627042321.1763765-9-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Per-VMA lock support for swap and userfaults | expand |
On Mon, Jun 26, 2023 at 09:23:21PM -0700, Suren Baghdasaryan wrote: > Enable handle_userfault to operate under VMA lock by releasing VMA lock > instead of mmap_lock and retrying. This mostly good to me (besides the new DROP flag.. of course), thanks. Still some nitpicks below. > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > fs/userfaultfd.c | 42 ++++++++++++++++++++++-------------------- > mm/memory.c | 9 --------- > 2 files changed, 22 insertions(+), 29 deletions(-) > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > index 4e800bb7d2ab..b88632c404b6 100644 > --- a/fs/userfaultfd.c > +++ b/fs/userfaultfd.c > @@ -277,17 +277,17 @@ static inline struct uffd_msg userfault_msg(unsigned long address, > * hugepmd ranges. > */ > static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, > - struct vm_area_struct *vma, > - unsigned long address, > - unsigned long flags, > - unsigned long reason) > + struct vm_fault *vmf, > + unsigned long reason) > { > + struct vm_area_struct *vma = vmf->vma; > pte_t *ptep, pte; > bool ret = true; > > - mmap_assert_locked(ctx->mm); > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) > + mmap_assert_locked(ctx->mm); Maybe we can have a helper asserting proper vma protector locks (mmap for !VMA_LOCK and vma read lock for VMA_LOCK)? It basically tells the context the vma is still safe to access. > > - ptep = hugetlb_walk(vma, address, vma_mmu_pagesize(vma)); > + ptep = hugetlb_walk(vma, vmf->address, vma_mmu_pagesize(vma)); > if (!ptep) > goto out; > > @@ -308,10 +308,8 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, > } > #else > static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, > - struct vm_area_struct *vma, > - unsigned long address, > - unsigned long flags, > - unsigned long reason) > + struct vm_fault *vmf, > + unsigned long reason) > { > return false; /* should never get here */ > } > @@ -325,11 +323,11 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, > * threads. > */ > static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx, > - unsigned long address, > - unsigned long flags, > + struct vm_fault *vmf, > unsigned long reason) > { > struct mm_struct *mm = ctx->mm; > + unsigned long address = vmf->address; > pgd_t *pgd; > p4d_t *p4d; > pud_t *pud; > @@ -337,7 +335,8 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx, > pte_t *pte; > bool ret = true; > > - mmap_assert_locked(mm); > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) > + mmap_assert_locked(mm); (the assert helper can also be used here) > > pgd = pgd_offset(mm, address); > if (!pgd_present(*pgd)) > @@ -445,7 +444,8 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) > * Coredumping runs without mmap_lock so we can only check that > * the mmap_lock is held, if PF_DUMPCORE was not set. > */ > - mmap_assert_locked(mm); > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) > + mmap_assert_locked(mm); > > ctx = vma->vm_userfaultfd_ctx.ctx; > if (!ctx) > @@ -561,15 +561,17 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) > spin_unlock_irq(&ctx->fault_pending_wqh.lock); > > if (!is_vm_hugetlb_page(vma)) > - must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags, > - reason); > + must_wait = userfaultfd_must_wait(ctx, vmf, reason); > else > - must_wait = userfaultfd_huge_must_wait(ctx, vma, > - vmf->address, > - vmf->flags, reason); > + must_wait = userfaultfd_huge_must_wait(ctx, vmf, reason); > if (is_vm_hugetlb_page(vma)) > hugetlb_vma_unlock_read(vma); > - mmap_read_unlock(mm); > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > + /* WARNING: VMA can't be used after this */ > + vma_end_read(vma); > + } else > + mmap_read_unlock(mm); I also think maybe we should have a helper mm_release_fault_lock() just release different locks for with/without VMA_LOCK. It can also be used in the other patch of folio_lock_or_retry(). > + vmf->flags |= FAULT_FLAG_LOCK_DROPPED; > > if (likely(must_wait && !READ_ONCE(ctx->released))) { > wake_up_poll(&ctx->fd_wqh, EPOLLIN); > diff --git a/mm/memory.c b/mm/memory.c > index bdf46fdc58d6..923c1576bd14 100644 > --- a/mm/memory.c > +++ b/mm/memory.c > @@ -5316,15 +5316,6 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, > if (!vma_start_read(vma)) > goto inval; > > - /* > - * Due to the possibility of userfault handler dropping mmap_lock, avoid > - * it for now and fall back to page fault handling under mmap_lock. > - */ > - if (userfaultfd_armed(vma)) { > - vma_end_read(vma); > - goto inval; > - } > - > /* Check since vm_start/vm_end might change before we lock the VMA */ > if (unlikely(address < vma->vm_start || address >= vma->vm_end)) { > vma_end_read(vma); > -- > 2.41.0.178.g377b9f9a00-goog >
On Tue, Jun 27, 2023 at 8:54 AM Peter Xu <peterx@redhat.com> wrote: > > On Mon, Jun 26, 2023 at 09:23:21PM -0700, Suren Baghdasaryan wrote: > > Enable handle_userfault to operate under VMA lock by releasing VMA lock > > instead of mmap_lock and retrying. > > This mostly good to me (besides the new DROP flag.. of course), thanks. > Still some nitpicks below. > > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > fs/userfaultfd.c | 42 ++++++++++++++++++++++-------------------- > > mm/memory.c | 9 --------- > > 2 files changed, 22 insertions(+), 29 deletions(-) > > > > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > > index 4e800bb7d2ab..b88632c404b6 100644 > > --- a/fs/userfaultfd.c > > +++ b/fs/userfaultfd.c > > @@ -277,17 +277,17 @@ static inline struct uffd_msg userfault_msg(unsigned long address, > > * hugepmd ranges. > > */ > > static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, > > - struct vm_area_struct *vma, > > - unsigned long address, > > - unsigned long flags, > > - unsigned long reason) > > + struct vm_fault *vmf, > > + unsigned long reason) > > { > > + struct vm_area_struct *vma = vmf->vma; > > pte_t *ptep, pte; > > bool ret = true; > > > > - mmap_assert_locked(ctx->mm); > > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) > > + mmap_assert_locked(ctx->mm); > > Maybe we can have a helper asserting proper vma protector locks (mmap for > !VMA_LOCK and vma read lock for VMA_LOCK)? It basically tells the context > the vma is still safe to access. > > > > > - ptep = hugetlb_walk(vma, address, vma_mmu_pagesize(vma)); > > + ptep = hugetlb_walk(vma, vmf->address, vma_mmu_pagesize(vma)); > > if (!ptep) > > goto out; > > > > @@ -308,10 +308,8 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, > > } > > #else > > static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, > > - struct vm_area_struct *vma, > > - unsigned long address, > > - unsigned long flags, > > - unsigned long reason) > > + struct vm_fault *vmf, > > + unsigned long reason) > > { > > return false; /* should never get here */ > > } > > @@ -325,11 +323,11 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, > > * threads. > > */ > > static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx, > > - unsigned long address, > > - unsigned long flags, > > + struct vm_fault *vmf, > > unsigned long reason) > > { > > struct mm_struct *mm = ctx->mm; > > + unsigned long address = vmf->address; > > pgd_t *pgd; > > p4d_t *p4d; > > pud_t *pud; > > @@ -337,7 +335,8 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx, > > pte_t *pte; > > bool ret = true; > > > > - mmap_assert_locked(mm); > > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) > > + mmap_assert_locked(mm); > > (the assert helper can also be used here) > > > > > pgd = pgd_offset(mm, address); > > if (!pgd_present(*pgd)) > > @@ -445,7 +444,8 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) > > * Coredumping runs without mmap_lock so we can only check that > > * the mmap_lock is held, if PF_DUMPCORE was not set. > > */ > > - mmap_assert_locked(mm); > > + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) > > + mmap_assert_locked(mm); > > > > ctx = vma->vm_userfaultfd_ctx.ctx; > > if (!ctx) > > @@ -561,15 +561,17 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) > > spin_unlock_irq(&ctx->fault_pending_wqh.lock); > > > > if (!is_vm_hugetlb_page(vma)) > > - must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags, > > - reason); > > + must_wait = userfaultfd_must_wait(ctx, vmf, reason); > > else > > - must_wait = userfaultfd_huge_must_wait(ctx, vma, > > - vmf->address, > > - vmf->flags, reason); > > + must_wait = userfaultfd_huge_must_wait(ctx, vmf, reason); > > if (is_vm_hugetlb_page(vma)) > > hugetlb_vma_unlock_read(vma); > > - mmap_read_unlock(mm); > > + if (vmf->flags & FAULT_FLAG_VMA_LOCK) { > > + /* WARNING: VMA can't be used after this */ > > + vma_end_read(vma); > > + } else > > + mmap_read_unlock(mm); > > I also think maybe we should have a helper mm_release_fault_lock() just > release different locks for with/without VMA_LOCK. It can also be used in > the other patch of folio_lock_or_retry(). All seem to be good suggestions. I'll try implementing them in the next version. Thanks! > > > + vmf->flags |= FAULT_FLAG_LOCK_DROPPED; > > > > if (likely(must_wait && !READ_ONCE(ctx->released))) { > > wake_up_poll(&ctx->fd_wqh, EPOLLIN); > > diff --git a/mm/memory.c b/mm/memory.c > > index bdf46fdc58d6..923c1576bd14 100644 > > --- a/mm/memory.c > > +++ b/mm/memory.c > > @@ -5316,15 +5316,6 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, > > if (!vma_start_read(vma)) > > goto inval; > > > > - /* > > - * Due to the possibility of userfault handler dropping mmap_lock, avoid > > - * it for now and fall back to page fault handling under mmap_lock. > > - */ > > - if (userfaultfd_armed(vma)) { > > - vma_end_read(vma); > > - goto inval; > > - } > > - > > /* Check since vm_start/vm_end might change before we lock the VMA */ > > if (unlikely(address < vma->vm_start || address >= vma->vm_end)) { > > vma_end_read(vma); > > -- > > 2.41.0.178.g377b9f9a00-goog > > > > -- > Peter Xu > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c index 4e800bb7d2ab..b88632c404b6 100644 --- a/fs/userfaultfd.c +++ b/fs/userfaultfd.c @@ -277,17 +277,17 @@ static inline struct uffd_msg userfault_msg(unsigned long address, * hugepmd ranges. */ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, - struct vm_area_struct *vma, - unsigned long address, - unsigned long flags, - unsigned long reason) + struct vm_fault *vmf, + unsigned long reason) { + struct vm_area_struct *vma = vmf->vma; pte_t *ptep, pte; bool ret = true; - mmap_assert_locked(ctx->mm); + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) + mmap_assert_locked(ctx->mm); - ptep = hugetlb_walk(vma, address, vma_mmu_pagesize(vma)); + ptep = hugetlb_walk(vma, vmf->address, vma_mmu_pagesize(vma)); if (!ptep) goto out; @@ -308,10 +308,8 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, } #else static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, - struct vm_area_struct *vma, - unsigned long address, - unsigned long flags, - unsigned long reason) + struct vm_fault *vmf, + unsigned long reason) { return false; /* should never get here */ } @@ -325,11 +323,11 @@ static inline bool userfaultfd_huge_must_wait(struct userfaultfd_ctx *ctx, * threads. */ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx, - unsigned long address, - unsigned long flags, + struct vm_fault *vmf, unsigned long reason) { struct mm_struct *mm = ctx->mm; + unsigned long address = vmf->address; pgd_t *pgd; p4d_t *p4d; pud_t *pud; @@ -337,7 +335,8 @@ static inline bool userfaultfd_must_wait(struct userfaultfd_ctx *ctx, pte_t *pte; bool ret = true; - mmap_assert_locked(mm); + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) + mmap_assert_locked(mm); pgd = pgd_offset(mm, address); if (!pgd_present(*pgd)) @@ -445,7 +444,8 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) * Coredumping runs without mmap_lock so we can only check that * the mmap_lock is held, if PF_DUMPCORE was not set. */ - mmap_assert_locked(mm); + if (!(vmf->flags & FAULT_FLAG_VMA_LOCK)) + mmap_assert_locked(mm); ctx = vma->vm_userfaultfd_ctx.ctx; if (!ctx) @@ -561,15 +561,17 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason) spin_unlock_irq(&ctx->fault_pending_wqh.lock); if (!is_vm_hugetlb_page(vma)) - must_wait = userfaultfd_must_wait(ctx, vmf->address, vmf->flags, - reason); + must_wait = userfaultfd_must_wait(ctx, vmf, reason); else - must_wait = userfaultfd_huge_must_wait(ctx, vma, - vmf->address, - vmf->flags, reason); + must_wait = userfaultfd_huge_must_wait(ctx, vmf, reason); if (is_vm_hugetlb_page(vma)) hugetlb_vma_unlock_read(vma); - mmap_read_unlock(mm); + if (vmf->flags & FAULT_FLAG_VMA_LOCK) { + /* WARNING: VMA can't be used after this */ + vma_end_read(vma); + } else + mmap_read_unlock(mm); + vmf->flags |= FAULT_FLAG_LOCK_DROPPED; if (likely(must_wait && !READ_ONCE(ctx->released))) { wake_up_poll(&ctx->fd_wqh, EPOLLIN); diff --git a/mm/memory.c b/mm/memory.c index bdf46fdc58d6..923c1576bd14 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -5316,15 +5316,6 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm, if (!vma_start_read(vma)) goto inval; - /* - * Due to the possibility of userfault handler dropping mmap_lock, avoid - * it for now and fall back to page fault handling under mmap_lock. - */ - if (userfaultfd_armed(vma)) { - vma_end_read(vma); - goto inval; - } - /* Check since vm_start/vm_end might change before we lock the VMA */ if (unlikely(address < vma->vm_start || address >= vma->vm_end)) { vma_end_read(vma);
Enable handle_userfault to operate under VMA lock by releasing VMA lock instead of mmap_lock and retrying. Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- fs/userfaultfd.c | 42 ++++++++++++++++++++++-------------------- mm/memory.c | 9 --------- 2 files changed, 22 insertions(+), 29 deletions(-)