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

[Barry Song]

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/