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

On Wed, Aug 05, 2026 at 05:13:49AM +0800, Barry Song wrote:
> On Tue, Aug 4, 2026 at 8:32 PM Lorenzo Stoakes (ARM) <[email protected]> wrote:
> >
> > On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote:
> > > From: Hongru Zhang <[email protected]>
> > >
> > > 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.

Haha thanks! :)

>
> 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))
>

I seem to remember Willy didn't love the idea of '1 more try with the VMA lock'
but this isn't _quite_ doing that.

If we spuriously can't get the VMA lock then this gives up immediately and goes
to the mmap logic without a retry, so we're not doing that on lock contention at
least.

(We could fix that with vma_start_read_unlocked() though which would handle
write lock contention by sleeping on mmap read lock until the VMA lock can be
obtained - though we have to be careful about possible lock inversion vs. a
writer maybe?).

So it only retries quickly if a retry is requested by the fault logic.

I guess it does end up working nicely then - because if the retry can
immediately succeed with a VMA lock again then it does that, but if it can't
then it falls through to the mmap lock quickly.

(And use of vma_start_read_unlocked() would make that more reliable vs. lock
contention.)

I think there were cases where we thought that might be the case (though it then
makes you wonder why exactly the fault needs a retry?)

(This is assuming nothing in the fault path would sleep holding the VMA lock,
which I don't think can happen?).

> Nothing else needs to change then. I wonder if there is a cleaner
> way to implement the idea, but it is really stunning.

Thanks again :>) I'm not quite sure this is really all that clever, but that's
nice of you :)

>
> Best Regards
> Barry

--
Cheers, Lorenzo