diff mbox series

[v2,4/9] mm: Move FAULT_FLAG_VMA_LOCK check into handle_pte_fault()

Message ID 20230711202047.3818697-5-willy@infradead.org (mailing list archive)
State New, archived
Headers show
Series Avoid the mmap lock for fault-around | expand

Commit Message

Matthew Wilcox July 11, 2023, 8:20 p.m. UTC
Push the check down from __handle_mm_fault().  There's a mild upside to
this patch in that we'll allocate the page tables while under the VMA
lock rather than the mmap lock, reducing the hold time on the mmap lock,
since the retry will find the page tables already populated.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 mm/memory.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Suren Baghdasaryan July 14, 2023, 3:17 a.m. UTC | #1
On Tue, Jul 11, 2023 at 1:20 PM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
>
> Push the check down from __handle_mm_fault().  There's a mild upside to
> this patch in that we'll allocate the page tables while under the VMA
> lock rather than the mmap lock, reducing the hold time on the mmap lock,
> since the retry will find the page tables already populated.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Took some time but seems safe to me...

Reviewed-by: Suren Baghdasaryan <surenb@google.com>

> ---
>  mm/memory.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 6eda5c5f2069..52f7fdd78380 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -4924,6 +4924,11 @@ static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
>  {
>         pte_t entry;
>
> +       if ((vmf->flags & FAULT_FLAG_VMA_LOCK) && !vma_is_anonymous(vmf->vma)) {
> +               vma_end_read(vmf->vma);
> +               return VM_FAULT_RETRY;
> +       }
> +
>         if (unlikely(pmd_none(*vmf->pmd))) {
>                 /*
>                  * Leave __pte_alloc() until later: because vm_ops->fault may
> @@ -5020,11 +5025,6 @@ static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
>         p4d_t *p4d;
>         vm_fault_t ret;
>
> -       if ((flags & FAULT_FLAG_VMA_LOCK) && !vma_is_anonymous(vma)) {
> -               vma_end_read(vma);
> -               return VM_FAULT_RETRY;
> -       }
> -
>         pgd = pgd_offset(mm, address);
>         p4d = p4d_alloc(mm, pgd, address);
>         if (!p4d)
> --
> 2.39.2
>
Jann Horn July 24, 2023, 3:46 p.m. UTC | #2
On Tue, Jul 11, 2023 at 10:20 PM Matthew Wilcox (Oracle)
<willy@infradead.org> wrote:
> Push the check down from __handle_mm_fault().  There's a mild upside to
> this patch in that we'll allocate the page tables while under the VMA
> lock rather than the mmap lock, reducing the hold time on the mmap lock,
> since the retry will find the page tables already populated.

This commit, by moving the check from __handle_mm_fault() to
handle_pte_fault(), also makes the non-anonymous THP paths (including
the DAX huge fault handling) reachable for VMA-locked faults, right?
Is that intentional?
Matthew Wilcox July 24, 2023, 4:37 p.m. UTC | #3
On Mon, Jul 24, 2023 at 05:46:57PM +0200, Jann Horn wrote:
> On Tue, Jul 11, 2023 at 10:20 PM Matthew Wilcox (Oracle)
> <willy@infradead.org> wrote:
> > Push the check down from __handle_mm_fault().  There's a mild upside to
> > this patch in that we'll allocate the page tables while under the VMA
> > lock rather than the mmap lock, reducing the hold time on the mmap lock,
> > since the retry will find the page tables already populated.
> 
> This commit, by moving the check from __handle_mm_fault() to
> handle_pte_fault(), also makes the non-anonymous THP paths (including
> the DAX huge fault handling) reachable for VMA-locked faults, right?
> Is that intentional?

Oof, this patch is all kinds of buggy.  Will split this into several
pieces.  Thanks!
diff mbox series

Patch

diff --git a/mm/memory.c b/mm/memory.c
index 6eda5c5f2069..52f7fdd78380 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4924,6 +4924,11 @@  static vm_fault_t handle_pte_fault(struct vm_fault *vmf)
 {
 	pte_t entry;
 
+	if ((vmf->flags & FAULT_FLAG_VMA_LOCK) && !vma_is_anonymous(vmf->vma)) {
+		vma_end_read(vmf->vma);
+		return VM_FAULT_RETRY;
+	}
+
 	if (unlikely(pmd_none(*vmf->pmd))) {
 		/*
 		 * Leave __pte_alloc() until later: because vm_ops->fault may
@@ -5020,11 +5025,6 @@  static vm_fault_t __handle_mm_fault(struct vm_area_struct *vma,
 	p4d_t *p4d;
 	vm_fault_t ret;
 
-	if ((flags & FAULT_FLAG_VMA_LOCK) && !vma_is_anonymous(vma)) {
-		vma_end_read(vma);
-		return VM_FAULT_RETRY;
-	}
-
 	pgd = pgd_offset(mm, address);
 	p4d = p4d_alloc(mm, pgd, address);
 	if (!p4d)