Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry

[Hongru Zhang]

> On Thu, Aug 6, 2026 at 3:30 PM Hongru Zhang <zhanghongru06@xxxxxxxxx> wrote:
> >
> > > On Tue, Aug 4, 2026 at 8:32 PM Lorenzo Stoakes (ARM) <ljs@xxxxxxxxxx> wrote:
> > > >
> > > > On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote:
> > > > > From: Hongru Zhang <zhanghongru@xxxxxxxxxx>
> > > > >
> > > > > Page faults handled under the per-VMA lock currently fall back to the
> > > > > mmap_lock path whenever handle_mm_fault() returns VM_FAULT_RETRY. This
> > > > > means that lower-level fault handlers have no way to tell the
> > > > > architecture fault handler that the retry can safely continue under the
> > > > > per-VMA lock.
> > > > >
> > > > > Add VM_FAULT_MAY_USE_VMA_LOCK as an advisory bit that can be returned
> > > >
> > > > I don't love that name or that faulting retry behaviour is _modified_ by a
> > > > value that indicates fault resolution state... ugh.
> > > >
> > > > It's kinda confusing things 'VM_FAULT_RETRY' is 'you have to retry this
> > > > fault'.
> > > >
> > > > 'VM_FAULT_MAY_...' is starting to bring in effectively configuration
> > > > options into it and that's kinda horrible.
> > > >
> > > > I mean is there any reason we shouldn't ALWAYS do this if a VMA lock was
> > > > used?
> > > >
> > > > It's not too expensive to do a single retry with the VMA lock before
> > > > falling back to the mmap lock.
> > > >
> > > > So maybe simplify like that?
> > > >
> > > > And like that this series becomes a single patch right?
> > >
> > > This is a brilliant idea. That's a genius insight, Lorenzo.
> > >
> > > I guess the conceptual model could simply be:
> > >
> > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > > index 45b99c3b1442..3592bcc9bbd7 100644
> > > --- a/arch/x86/mm/fault.c
> > > +++ b/arch/x86/mm/fault.c
> > > @@ -1222,6 +1222,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> > >       struct mm_struct *mm;
> > >       vm_fault_t fault;
> > >       unsigned int flags = FAULT_FLAG_DEFAULT;
> > > +     bool vma_lock_retried = false;
> > >
> > >       tsk = current;
> > >       mm = tsk->mm;
> > > @@ -1331,6 +1332,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> > >       if (!(flags & FAULT_FLAG_USER))
> > >               goto lock_mmap;
> > >
> > > +vma_lock:
> > >       vma = lock_vma_under_rcu(mm, address);
> > >       if (!vma)
> > >               goto lock_mmap;
> > > @@ -1352,6 +1354,11 @@ void do_user_addr_fault(struct pt_regs *regs,
> > >       if (fault & VM_FAULT_MAJOR)
> > >               flags |= FAULT_FLAG_TRIED;
> > >
> > > +     if (!vma_lock_retried) {
> > > +             vma_lock_retried = true;
> > > +             goto vma_lock;
> > > +     }
> > > +
> > >       /* Quick path to respond to signals */
> > >       if (fault_signal_pending(fault, regs)) {
> > >               if (!user_mode(regs))
> > >
> > > Nothing else needs to change then. I wonder if there is a cleaner
> > > way to implement the idea, but it is really stunning.
> > >
> > > Best Regards
> > > Barry
> >
> > Filemap Throughput (higher is better):
> > +---------+------------+---------------------+---------------------+---------------------+
> > | Threads |  Vanilla   |       RFC v4        |          P1         |          P2         |
> > +---------+------------+---------------------+---------------------+---------------------+
> > |   40    | 1069.34 /s | 1404.47 /s (+31.3%) | 1400.13 /s (+30.9%) | 1412.38 /s (+32.1%) |
> > +---------+------------+---------------------+---------------------+---------------------+
> > |   60    | 1038.12 /s | 1682.88 /s (+62.1%) | 1683.37 /s (+62.2%) | 1685.23 /s (+62.3%) |
> > +---------+------------+---------------------+---------------------+---------------------+
> > |   80    | 1042.62 /s | 1766.72 /s (+69.5%) | 1767.83 /s (+69.6%) | 1771.73 /s (+69.9%) |
> > +---------+------------+---------------------+---------------------+---------------------+
> >
> > Swap Throughput (higher is better):
> > +--------------+-------------+----------------------+----------------------+----------------------+
> > | mmap writers |   Vanilla   |        RFC v4        |          P1          |          P2          |
> > +--------------+-------------+----------------------+----------------------+----------------------+
> > |      0       | 17303.09 /s | 18394.51 /s  (+6.3%) | 17899.48 /s  (+3.4%) | 18337.30 /s  (+6.0%) |
> > +--------------+-------------+----------------------+----------------------+----------------------+
> > |      2       | 16728.04 /s | 18591.68 /s (+11.1%) | 18346.72 /s  (+9.7%) | 18848.17 /s (+12.7%) |
> > +--------------+-------------+----------------------+----------------------+----------------------+
> > |      4       | 12596.23 /s | 18534.00 /s (+47.1%) | 16095.20 /s (+27.8%) | 18507.62 /s (+46.9%) |
> > +--------------+-------------+----------------------+----------------------+----------------------+
> >
> > The key difference between P1 and P2 is how FAULT_FLAG_TRIED is handled: P1
> > keeps the existing major-fault-only setting before the retry, while P2 sets it
> > before the VMA-lock retry for all retrying faults.
> >
> > In filemap throughput test, each reader thread operates on its own file.
> > In swap throughput test, all reader threads fault the same memory area.
> >
> >
> > P1:
> >
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 45b99c3b1442..c3ab30d32a15 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1222,6 +1222,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> >         struct mm_struct *mm;
> >         vm_fault_t fault;
> >         unsigned int flags = FAULT_FLAG_DEFAULT;
> > +       bool vma_lock_retried = false;
> >
> >         tsk = current;
> >         mm = tsk->mm;
> > @@ -1331,6 +1332,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> >         if (!(flags & FAULT_FLAG_USER))
> >                 goto lock_mmap;
> >
> > +lock_vma:
> >         vma = lock_vma_under_rcu(mm, address);
> >         if (!vma)
> >                 goto lock_mmap;
> > @@ -1360,6 +1362,12 @@ void do_user_addr_fault(struct pt_regs *regs,
> >                                                  ARCH_DEFAULT_PKEY);
> >                 return;
> >         }
> > +
> > +       if (!vma_lock_retried) {
> > +               vma_lock_retried = true;
> > +               goto lock_vma;
> > +       }
> > +
> >  lock_mmap:
> >
> >
> > P2:
> >
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 45b99c3b1442..9507b8a0fe18 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1222,6 +1222,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> >         struct mm_struct *mm;
> >         vm_fault_t fault;
> >         unsigned int flags = FAULT_FLAG_DEFAULT;
> > +       bool vma_lock_retried = false;
> >
> >         tsk = current;
> >         mm = tsk->mm;
> > @@ -1331,6 +1332,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> >         if (!(flags & FAULT_FLAG_USER))
> >                 goto lock_mmap;
> >
> > +lock_vma:
> >         vma = lock_vma_under_rcu(mm, address);
> >         if (!vma)
> >                 goto lock_mmap;
> > @@ -1349,8 +1351,6 @@ void do_user_addr_fault(struct pt_regs *regs,
> >                 goto done;
> >         }
> >         count_vm_vma_lock_event(VMA_LOCK_RETRY);
> > -       if (fault & VM_FAULT_MAJOR)
> > -               flags |= FAULT_FLAG_TRIED;
> >
> >         /* Quick path to respond to signals */
> >         if (fault_signal_pending(fault, regs)) {
> > @@ -1360,6 +1360,13 @@ void do_user_addr_fault(struct pt_regs *regs,
> >                                                  ARCH_DEFAULT_PKEY);
> >                 return;
> >         }
> > +
> > +       if (!vma_lock_retried) {
> > +               flags |= FAULT_FLAG_TRIED;
> > +               vma_lock_retried = true;
> > +               goto lock_vma;
> > +       }
> > +
> >  lock_mmap:
>
> Thanks!
>
> As Lorenzo pointed out, this would break major fault accounting, so I
> think it is better suited as P1.
>
> The performance difference you are seeing is probably because another
> thread is concurrently swapping in the same address, taking the
> folio_lock and installing the PTE. That is a separate issue and could
> be addressed by a separate patch, likely an updated version of this:
>
> mm: Don't retry page fault if folio is uptodate during swap-in
>
> https://lore.kernel.org/all/20260430040427.4672-5-baohua@xxxxxxxxxx/

Swap Throughput (higher is better):
+--------------+-------------+----------------------+----------------------+
| mmap writers |   Vanilla   |          P1          |          P3          |
+--------------+-------------+----------------------+----------------------+
|      0       | 17303.09 /s | 17899.48 /s  (+3.4%) | 18190.62 /s  (+5.1%) |
+--------------+-------------+----------------------+----------------------+
|      2       | 16728.04 /s | 18346.72 /s  (+9.7%) | 18162.03 /s  (+8.6%) |
+--------------+-------------+----------------------+----------------------+
|      4       | 12596.23 /s | 16095.20 /s (+27.8%) | 17991.45 /s (+42.8%) |
+--------------+-------------+----------------------+----------------------+

P3:

diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 45b99c3b1442..c3ab30d32a15 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1222,6 +1222,7 @@ void do_user_addr_fault(struct pt_regs *regs,
 	struct mm_struct *mm;
 	vm_fault_t fault;
 	unsigned int flags = FAULT_FLAG_DEFAULT;
+	bool vma_lock_retried = false;
 
 	tsk = current;
 	mm = tsk->mm;
@@ -1331,6 +1332,7 @@ void do_user_addr_fault(struct pt_regs *regs,
 	if (!(flags & FAULT_FLAG_USER))
 		goto lock_mmap;
 
+lock_vma:
 	vma = lock_vma_under_rcu(mm, address);
 	if (!vma)
 		goto lock_mmap;
@@ -1360,6 +1362,12 @@ void do_user_addr_fault(struct pt_regs *regs,
 						 ARCH_DEFAULT_PKEY);
 		return;
 	}
+
+	if (!vma_lock_retried) {
+		vma_lock_retried = true;
+		goto lock_vma;
+	}
+
 lock_mmap:
 
 retry:
diff --git a/mm/memory.c b/mm/memory.c
index 428eb555ecb7..8c34a857548b 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -4987,6 +4987,13 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 	}
 
 	swapcache = folio;
+	/*
+	 * If the folio is uptodate, we are likely only waiting for
+	 * another concurrent PTE mapping to complete, which should
+	 * be brief. No need to drop the lock and retry the fault.
+	 */
+	if (folio_test_uptodate(folio))
+		vmf->flags &= ~FAULT_FLAG_ALLOW_RETRY;
 	ret |= folio_lock_or_retry(folio, vmf);
 	if (ret & VM_FAULT_RETRY)
 		goto out_release;

Thanks,
Hongru