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

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

[Lorenzo Stoakes (ARM)]

On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote:
> From: Hongru Zhang <zhanghongru@xiaomi.com>
>
> 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?

Though it seems the reticence is that you might end up waiting with the VMA
lock held.

If that's really critical then I'd drop this whole thing of referencing the
VMA flag in the name altogether, it's confusing and you're left wondering
why it's needed when the fault is already specified as allowing a VMA lock
via FAULT_FLAG_VMA_LOCK.

It's not at all clear it's _only_ for the arch-fault-handlers.

I'm curious anyway as to where this waiting is actually happening? Is the
waiting not _already_ happening with the VMA lock held on first attempt? Or
if not there then where? [ the fault code is such a mess don't really have
time to figure it out myself ].

Anyway if we _have_ to have this flag, then something that actually matches
the semantics here is better like:

	VM_FAULT_RETRY_WONT_BLOCK

Which is informational, doesn't add any confusion about the VMA flag, and
makes things a lot more self-documenting about what's going on here.

> together with VM_FAULT_RETRY. Architecture fault handlers use this bit
> to allow at most one retry under the per-VMA lock.
>
> This preserves the existing mmap_lock fallback behaviour for fault
> handlers that continue to return VM_FAULT_RETRY without
> VM_FAULT_MAY_USE_VMA_LOCK: major retries still enter the mmap_lock path
> with FAULT_FLAG_TRIED set, while minor retries still enter it as a fresh
> first attempt.

Yup this was a concern I think I raised in a previous version that you'd
get infinite VMA lock retries.

>
> The difference is limited to fault handlers that return VM_FAULT_RETRY
> with VM_FAULT_MAY_USE_VMA_LOCK. For them, both major and minor retries
> enter the VMA-lock retry with FAULT_FLAG_TRIED set. For major faults
> this follows the existing mmap_lock retry handling, but the retried
> fault runs under the VMA lock rather than the mmap_lock. For minor
> faults this replaces the fresh mmap_lock retry with a VMA-lock retry
> that has FAULT_FLAG_TRIED set. This can avoid an extra retry round for
> short waits. The cost is that a retried fault that blocks for an extended
> period may wait while holding the VMA lock. Fault handlers should return

Where does it wait?

> VM_FAULT_RETRY with VM_FAULT_MAY_USE_VMA_LOCK when that tradeoff is
> preferable to falling back to mmap_lock immediately.

This sentence is completely unreadable :) Too. Many. Words.

It's really hard to follow too. Was this AI-generated? Totally understand
if it's to help with prose in general but what we end up with really does
have to be clear.

Something like:

	Minor faults are retried indefinitely with the mmap lock held,
	which guarantees some forward progress, however this isn't the case
	with VMA locks, so set FAULT_FLAG_TRIED for minor faults if the VMA
	lock is used.

	< discussions of tradeoff, where the waiting actually happens >

Maybe?

>
> No current code sets VM_FAULT_MAY_USE_VMA_LOCK yet; this patch only
> prepares the retry plumbing for later users.
>
> No functional change is intended.
>
> Signed-off-by: Hongru Zhang <zhanghongru@xiaomi.com>
> Suggested-by: Barry Song <baohua@kernel.org>
> Suggested-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  arch/arm/mm/fault.c       |  6 ++++--
>  arch/arm64/mm/fault.c     |  7 +++++--
>  arch/loongarch/mm/fault.c |  6 ++++--
>  arch/powerpc/mm/fault.c   |  6 ++++--
>  arch/riscv/mm/fault.c     |  6 ++++--
>  arch/s390/mm/fault.c      |  5 +++--
>  arch/x86/mm/fault.c       |  6 ++++--

Really badly need that code to separate out fault handling code. Matthew?
:) Or perhaps he's waiting on this to land first...

It does seem that these are the arches that use VMA locks though.

Suren - is there any reason we shouldn't just enable VMA locks for every
CONFIG_MMU arch now it's headed for being default-enabled?


>  include/linux/mm.h        | 28 ++++++++++++++++++++++++++++
>  include/linux/mm_types.h  |  4 ++++
>  9 files changed, 60 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> index e62cc4be5adf..158923b70901 100644
> --- a/arch/arm/mm/fault.c
> +++ b/arch/arm/mm/fault.c
> @@ -391,6 +391,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  	if (!(flags & FAULT_FLAG_USER))
>  		goto lock_mmap;
>
> +retry_vma:
>  	vma = lock_vma_under_rcu(mm, addr);
>  	if (!vma)
>  		goto lock_mmap;
> @@ -411,8 +412,6 @@ do_page_fault(unsigned long addr, unsigned int fsr, 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)) {
> @@ -420,6 +419,9 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
>  			goto no_context;
>  		return 0;
>  	}
> +
> +	if (fault_should_retry_under_vma_lock(fault, &flags))
> +		goto retry_vma;
>  lock_mmap:
>
>  retry:
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 0b52557652be..b17986b40ac3 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -678,6 +678,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>  	if (!(mm_flags & FAULT_FLAG_USER))
>  		goto lock_mmap;
>
> +retry_vma:
>  	vma = lock_vma_under_rcu(mm, addr);
>  	if (!vma)
>  		goto lock_mmap;
> @@ -715,8 +716,6 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>  		goto done;
>  	}
>  	count_vm_vma_lock_event(VMA_LOCK_RETRY);
> -	if (fault & VM_FAULT_MAJOR)
> -		mm_flags |= FAULT_FLAG_TRIED;
>
>  	/* Quick path to respond to signals */
>  	if (fault_signal_pending(fault, regs)) {
> @@ -724,6 +723,10 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
>  			goto no_context;
>  		return 0;
>  	}
> +
> +	if (fault_should_retry_under_vma_lock(fault, &mm_flags))
> +		goto retry_vma;
> +
>  lock_mmap:
>
>  retry:
> diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
> index 2c93d33356e5..6a946838b54b 100644
> --- a/arch/loongarch/mm/fault.c
> +++ b/arch/loongarch/mm/fault.c
> @@ -219,6 +219,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>  	if (!(flags & FAULT_FLAG_USER))
>  		goto lock_mmap;
>
> +retry_vma:
>  	vma = lock_vma_under_rcu(mm, address);
>  	if (!vma)
>  		goto lock_mmap;
> @@ -256,8 +257,6 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>  	}
>
>  	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)) {
> @@ -265,6 +264,9 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
>  			no_context(regs, write, address);
>  		return;
>  	}
> +
> +	if (fault_should_retry_under_vma_lock(fault, &flags))
> +		goto retry_vma;
>  lock_mmap:
>
>  retry:
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 806c74e0d5ab..e2a128fba408 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -487,6 +487,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>  	if (!(flags & FAULT_FLAG_USER))
>  		goto lock_mmap;
>
> +retry_vma:
>  	vma = lock_vma_under_rcu(mm, address);
>  	if (!vma)
>  		goto lock_mmap;
> @@ -511,12 +512,13 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>  		goto done;
>  	}
>  	count_vm_vma_lock_event(VMA_LOCK_RETRY);
> -	if (fault & VM_FAULT_MAJOR)
> -		flags |= FAULT_FLAG_TRIED;
>
>  	if (fault_signal_pending(fault, regs))
>  		return user_mode(regs) ? 0 : SIGBUS;
>
> +	if (fault_should_retry_under_vma_lock(fault, &flags))
> +		goto retry_vma;
> +
>  lock_mmap:
>
>  	/* When running in the kernel we expect faults to occur only to
> diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
> index 04ed6f8acae4..87b061feba51 100644
> --- a/arch/riscv/mm/fault.c
> +++ b/arch/riscv/mm/fault.c
> @@ -347,6 +347,7 @@ void handle_page_fault(struct pt_regs *regs)
>  	if (!(flags & FAULT_FLAG_USER))
>  		goto lock_mmap;
>
> +retry_vma:
>  	vma = lock_vma_under_rcu(mm, addr);
>  	if (!vma)
>  		goto lock_mmap;
> @@ -368,14 +369,15 @@ void handle_page_fault(struct pt_regs *regs)
>  		goto done;
>  	}
>  	count_vm_vma_lock_event(VMA_LOCK_RETRY);
> -	if (fault & VM_FAULT_MAJOR)
> -		flags |= FAULT_FLAG_TRIED;
>
>  	if (fault_signal_pending(fault, regs)) {
>  		if (!user_mode(regs))
>  			no_context(regs, addr);
>  		return;
>  	}
> +
> +	if (fault_should_retry_under_vma_lock(fault, &flags))
> +		goto retry_vma;
>  lock_mmap:
>
>  retry:
> diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> index 028aeb9c48d6..8e90e522436b 100644
> --- a/arch/s390/mm/fault.c
> +++ b/arch/s390/mm/fault.c
> @@ -294,6 +294,7 @@ static void do_exception(struct pt_regs *regs, int access)
>  		flags |= FAULT_FLAG_WRITE;
>  	if (!(flags & FAULT_FLAG_USER))
>  		goto lock_mmap;
> +retry_vma:
>  	vma = lock_vma_under_rcu(mm, address);
>  	if (!vma)
>  		goto lock_mmap;
> @@ -310,14 +311,14 @@ static void do_exception(struct pt_regs *regs, int access)
>  		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)) {
>  		if (!user_mode(regs))
>  			handle_fault_error_nolock(regs, 0);
>  		return;
>  	}
> +	if (fault_should_retry_under_vma_lock(fault, &flags))
> +		goto retry_vma;
>  lock_mmap:
>  retry:
>  	vma = lock_mm_and_find_vma(mm, address, regs);
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 45b99c3b1442..53c8f003fe53 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1331,6 +1331,7 @@ void do_user_addr_fault(struct pt_regs *regs,
>  	if (!(flags & FAULT_FLAG_USER))
>  		goto lock_mmap;
>
> +retry_vma:
>  	vma = lock_vma_under_rcu(mm, address);
>  	if (!vma)
>  		goto lock_mmap;
> @@ -1349,8 +1350,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 +1359,9 @@ void do_user_addr_fault(struct pt_regs *regs,
>  						 ARCH_DEFAULT_PKEY);
>  		return;
>  	}
> +
> +	if (fault_should_retry_under_vma_lock(fault, &flags))
> +		goto retry_vma;
>  lock_mmap:
>
>  retry:
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 7fabe6c66b4b..27ec6673acfe 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -727,6 +727,34 @@ static inline bool fault_flag_allow_retry_first(enum fault_flag flags)
>  	    (!(flags & FAULT_FLAG_TRIED));
>  }
>
> +/**
> + * fault_should_retry_under_vma_lock - decide whether to retry with VMA lock
> + * @fault: fault result from handle_mm_fault() under FAULT_FLAG_VMA_LOCK
> + * @flags: fault flags for the current fault, updated on retry
> + *
> + * Architecture page fault handlers call this after a VMA-lock fault returns
> + * VM_FAULT_RETRY. If the fault result also has VM_FAULT_MAY_USE_VMA_LOCK,
> + * allow one bounded retry under the VMA lock and set FAULT_FLAG_TRIED.
> + *
> + * When the fault must fall back to the mmap_lock path, preserve the existing
> + * VM_FAULT_MAJOR behavior by marking FAULT_FLAG_TRIED before the retry.
> + *
> + * Return: true if the caller should retry under the VMA lock, false if it

Oh good half of the kdoc comments in mm.h have Return and the other half have
Returns: :)) OK I guess Return is fine here.

> + * should fall back to the mmap_lock fault path.
> + */
> +static inline bool fault_should_retry_under_vma_lock(vm_fault_t fault, unsigned int *flags)
> +{
> +	if ((fault & VM_FAULT_MAY_USE_VMA_LOCK) && !(*flags & FAULT_FLAG_TRIED)) {
> +		*flags |= FAULT_FLAG_TRIED;
> +		return true;
> +	}
> +
> +	if (fault & VM_FAULT_MAJOR)
> +		*flags |= FAULT_FLAG_TRIED;
> +
> +	return false;
> +}

I really hate this function. It's not doing what it says it is (it's
modifying fault behaviour too via the flags parameter AND setting state for
major faults), it's combining VMA fault path handling AND mmap major fault
handling it's doing ugly horrors with an output parameter.

I think it's better just as a straight-up predicate. Yes there'll be
duplication on setting FAULT_FLAG_TRIED. It sucks, but you're _already_
duplicating every single invocation and goto anyway.

The fix for that is finally de-duplicating the arch fault code properly.

So it'd be like:

	< kdoc etc. >
	static inline bool should_retry_fault_under_vma_lock(vm_fault fault_type,
			unsigned int flags)
	{
		/* Don't wait holding the VMA lock. */
		if (!(fault_type & VM_FAULT_RETRY_WONT_BLOCK))
			return false;
		/* Already retried the fault under the VMA lock. */
		if (flags & FAULT_FLAG_TRIED)
			return false;
		return true;
	}


-	if (fault_should_retry_under_vma_lock(fault, &flags))
-		goto retry_vma;
+	if (should_retry_fault_under_vma_lock(fault, flags)) {
+		flags |= FAULT_FLAG_TRIED;
+		goto retry_vma;
+	}
+
+	if (fault & VM_FAULT_MAJOR)
+		flags |= FAULT_FLAG_TRIED;

This way also you explicitly see where FAULT_FLAG_TRIED is set.

> +
>  #define FAULT_FLAG_TRACE \
>  	{ FAULT_FLAG_WRITE,		"WRITE" }, \
>  	{ FAULT_FLAG_MKWRITE,		"MKWRITE" }, \
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index b5d4cd3b067b..46a832757109 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1684,6 +1684,8 @@ typedef __bitwise unsigned int vm_fault_t;
>   * @VM_FAULT_NOPAGE:		->fault installed the pte, not return page
>   * @VM_FAULT_LOCKED:		->fault locked the returned page
>   * @VM_FAULT_RETRY:		->fault blocked, must retry
> + * @VM_FAULT_MAY_USE_VMA_LOCK:	->fault blocked, retry may be handled under
> + *				the VMA lock
>   * @VM_FAULT_FALLBACK:		huge page fault failed, fall back to small
>   * @VM_FAULT_DONE_COW:		->fault has fully handled COW
>   * @VM_FAULT_NEEDDSYNC:		->fault did not modify page tables and needs
> @@ -1707,6 +1709,7 @@ enum vm_fault_reason {
>  	VM_FAULT_DONE_COW       = (__force vm_fault_t)0x001000,
>  	VM_FAULT_NEEDDSYNC      = (__force vm_fault_t)0x002000,
>  	VM_FAULT_COMPLETED      = (__force vm_fault_t)0x004000,
> +	VM_FAULT_MAY_USE_VMA_LOCK = (__force vm_fault_t)0x008000,
>  	VM_FAULT_HINDEX_MASK    = (__force vm_fault_t)0x0f0000,
>  };
>
> @@ -1731,6 +1734,7 @@ enum vm_fault_reason {
>  	{ VM_FAULT_FALLBACK,            "FALLBACK" },	\
>  	{ VM_FAULT_DONE_COW,            "DONE_COW" },	\
>  	{ VM_FAULT_NEEDDSYNC,           "NEEDDSYNC" },	\
> +	{ VM_FAULT_MAY_USE_VMA_LOCK,    "MAY_USE_VMA_LOCK" },	\

This makes the naming confusion even worse - because now we have
FAULT_FLAG_VMA_LOCK and this which 'specifies' may use VMA lock whereas
really it means 'may use once on retry'...

So again, as per above, I think it's better to actually have this
communicate the _reason_ why it seems OK to use the VMA lock on retry
rather than actually specifying that the caller should.

>  	{ VM_FAULT_COMPLETED,           "COMPLETED" }
>
>  struct vm_special_mapping {
> --
> 2.43.0
>

BTW in sanitize_fault_flags() there's:

	/*
	 * Per-VMA locks can't be used with FAULT_FLAG_RETRY_NOWAIT because of
	 * the assumption that lock is dropped on VM_FAULT_RETRY.
	 */
	if (WARN_ON_ONCE((*flags &
			(FAULT_FLAG_VMA_LOCK | FAULT_FLAG_RETRY_NOWAIT)) ==
			(FAULT_FLAG_VMA_LOCK | FAULT_FLAG_RETRY_NOWAIT)))
		return VM_FAULT_SIGSEGV;

Except now VM_FAULT_RETRY doesn't drop the lock :) I think the comment
needs to be updated to say 'dropped on VM_FAULT_RETRY after one attempt'.

Also I think there's an issue with major/minor fault counting as a result
of this, in mm_account_fault():

	/*
	 * We define the fault as a major fault when the final successful fault
	 * is VM_FAULT_MAJOR, or if it retried (which implies that we couldn't
	 * handle it immediately previously).
	 */
	major = (ret & VM_FAULT_MAJOR) || (flags & FAULT_FLAG_TRIED);

	if (major)
		current->maj_flt++;
	else
		current->min_flt++;

Now you're getting minor faults being counted as major ones?

If intended, that's a user-visible change that should be documented and
defended in the commit msg.

If not, you could do some REALLY gross checks making things _even more
complicated_ here.

But maybe something like:

	< kdoc comment etc. >
	static bool is_major_fault(vm_fault_t fault_type, unsigned int fault_flags)
	{
		/* Explicitly marked as major. */
		if (fault_type & VM_FAULT_MAJOR)
			return true;
		/* If no retry occurred, minor. */
		if (!(fault_flags & FAULT_FLAG_TRIED))
			return false;
		/* Quickly retrying fault under the VMA lock implies minor. */
		return !(fault_flags & FAULT_FLAG_VMA_LOCK);
	}

	Then the code above becomes:

	if (is_major_fault(ret, flags))
		current->maj_flt++;
	else
		current->min_flt++;

--
Cheers, Lorenzo

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

[Barry Song]

On Tue, Aug 4, 2026 at 8:32 PM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
>
> On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote:
> > From: Hongru Zhang <zhanghongru@xiaomi.com>
> >
> > 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

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

[Lorenzo Stoakes (ARM)]

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) <ljs@kernel.org> wrote:
> >
> > On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote:
> > > From: Hongru Zhang <zhanghongru@xiaomi.com>
> > >
> > > 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

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

[Hongru Zhang]

> On Tue, Aug 4, 2026 at 8:32 PM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> >
> > On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote:
> > > From: Hongru Zhang <zhanghongru@xiaomi.com>
> > >
> > > 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:

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@gmail.com> wrote:
>
> > On Tue, Aug 4, 2026 at 8:32 PM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> > >
> > > On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote:
> > > > From: Hongru Zhang <zhanghongru@xiaomi.com>
> > > >
> > > > 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@kernel.org/

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

[Hongru Zhang]

> On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote:
> > From: Hongru Zhang <zhanghongru@xiaomi.com>
> >
> > 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.

vm_fault_t do_swap_page(struct vm_fault *vmf)
{
	...
	entry = softleaf_from_pte(vmf->orig_pte);
	if (unlikely(!softleaf_is_swap(entry))) {
		...
		} else if (softleaf_is_device_private(entry)) {
			if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
				/*
				 * migrate_to_ram is not yet ready to operate
				 * under VMA lock.
				 */
				vma_end_read(vma);
				ret = VM_FAULT_RETRY;
				goto out;
			}
	...
}

This is one example why I made the v4 retry opt-in. Device-private entries
currently use VM_FAULT_RETRY to force a fallback from the VMA-lock path to
the mmap_lock path, because migrate_to_ram() is not VMA-lock-ready yet. An
unconditional VMA-lock retry would not make this path progress under the VMA
lock; it would only add one bounded extra VMA-lock attempt before falling
back to mmap_lock.

>
> So maybe simplify like that?
>
> And like that this series becomes a single patch right?
>

Agreed. I'll try the "always retry once under VMA lock" approach and report
performance results in a later email.

> Though it seems the reticence is that you might end up waiting with the VMA
> lock held.
>
> If that's really critical then I'd drop this whole thing of referencing the
> VMA flag in the name altogether, it's confusing and you're left wondering
> why it's needed when the fault is already specified as allowing a VMA lock
> via FAULT_FLAG_VMA_LOCK.
>
> It's not at all clear it's _only_ for the arch-fault-handlers.
>
> I'm curious anyway as to where this waiting is actually happening? Is the
> waiting not _already_ happening with the VMA lock held on first attempt? Or
> if not there then where? [ the fault code is such a mess don't really have
> time to figure it out myself ].
>
> Anyway if we _have_ to have this flag, then something that actually matches
> the semantics here is better like:
>
> 	VM_FAULT_RETRY_WONT_BLOCK
>
> Which is informational, doesn't add any confusion about the VMA flag, and
> makes things a lot more self-documenting about what's going on here.
>
> > together with VM_FAULT_RETRY. Architecture fault handlers use this bit
> > to allow at most one retry under the per-VMA lock.
> >
> > This preserves the existing mmap_lock fallback behaviour for fault
> > handlers that continue to return VM_FAULT_RETRY without
> > VM_FAULT_MAY_USE_VMA_LOCK: major retries still enter the mmap_lock path
> > with FAULT_FLAG_TRIED set, while minor retries still enter it as a fresh
> > first attempt.
>
> Yup this was a concern I think I raised in a previous version that you'd
> get infinite VMA lock retries.
>
> >
> > The difference is limited to fault handlers that return VM_FAULT_RETRY
> > with VM_FAULT_MAY_USE_VMA_LOCK. For them, both major and minor retries
> > enter the VMA-lock retry with FAULT_FLAG_TRIED set. For major faults
> > this follows the existing mmap_lock retry handling, but the retried
> > fault runs under the VMA lock rather than the mmap_lock. For minor
> > faults this replaces the fresh mmap_lock retry with a VMA-lock retry
> > that has FAULT_FLAG_TRIED set. This can avoid an extra retry round for
> > short waits. The cost is that a retried fault that blocks for an extended
> > period may wait while holding the VMA lock. Fault handlers should return
>
> Where does it wait?

For filemap, the main wait I had in mind is in lock_folio_maybe_drop_mmap().
On the first attempt, maybe_unlock_mmap_for_io() can drop the fault lock
before waiting. On the VMA-lock retry, FAULT_FLAG_TRIED is already set, so
maybe_unlock_mmap_for_io() can no longer drop it. If folio_trylock() fails,
__folio_lock()/__folio_lock_killable() may then wait while the VMA lock is
still held.

There is also a less frequent not-uptodate path: after
maybe_unlock_mmap_for_io() keeps the fault lock on a FAULT_FLAG_TRIED
retry, filemap_read_folio() can wait for the synchronous read to complete.

I instrumented both places locally. The folio-lock wait path was common in
the shared-file sequential workload; the read_folio path also happened,
but much less frequently and mainly in the shared-file random cases.

>
> > VM_FAULT_RETRY with VM_FAULT_MAY_USE_VMA_LOCK when that tradeoff is
> > preferable to falling back to mmap_lock immediately.
>
> This sentence is completely unreadable :) Too. Many. Words.
>
> It's really hard to follow too. Was this AI-generated? Totally understand
> if it's to help with prose in general but what we end up with really does
> have to be clear.
>
> Something like:
>
> 	Minor faults are retried indefinitely with the mmap lock held,
> 	which guarantees some forward progress, however this isn't the case
> 	with VMA locks, so set FAULT_FLAG_TRIED for minor faults if the VMA
> 	lock is used.
> 
> 	< discussions of tradeoff, where the waiting actually happens >
> 
> Maybe?
>
> >
> > No current code sets VM_FAULT_MAY_USE_VMA_LOCK yet; this patch only
> > prepares the retry plumbing for later users.
> >
> > No functional change is intended.
> >
> > Signed-off-by: Hongru Zhang <zhanghongru@xiaomi.com>
> > Suggested-by: Barry Song <baohua@kernel.org>
> > Suggested-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >  arch/arm/mm/fault.c       |  6 ++++--
> >  arch/arm64/mm/fault.c     |  7 +++++--
> >  arch/loongarch/mm/fault.c |  6 ++++--
> >  arch/powerpc/mm/fault.c   |  6 ++++--
> >  arch/riscv/mm/fault.c     |  6 ++++--
> >  arch/s390/mm/fault.c      |  5 +++--
> >  arch/x86/mm/fault.c       |  6 ++++--
>
> Really badly need that code to separate out fault handling code. Matthew?
> :) Or perhaps he's waiting on this to land first...
>
> It does seem that these are the arches that use VMA locks though.
>
> Suren - is there any reason we shouldn't just enable VMA locks for every
> CONFIG_MMU arch now it's headed for being default-enabled?
>
>
> >  include/linux/mm.h        | 28 ++++++++++++++++++++++++++++
> >  include/linux/mm_types.h  |  4 ++++
> >  9 files changed, 60 insertions(+), 14 deletions(-)
> >
> > diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> > index e62cc4be5adf..158923b70901 100644
> > --- a/arch/arm/mm/fault.c
> > +++ b/arch/arm/mm/fault.c
> > @@ -391,6 +391,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> >  	if (!(flags & FAULT_FLAG_USER))
> >  		goto lock_mmap;
> >
> > +retry_vma:
> >  	vma = lock_vma_under_rcu(mm, addr);
> >  	if (!vma)
> >  		goto lock_mmap;
> > @@ -411,8 +412,6 @@ do_page_fault(unsigned long addr, unsigned int fsr, 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)) {
> > @@ -420,6 +419,9 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> >  			goto no_context;
> >  		return 0;
> >  	}
> > +
> > +	if (fault_should_retry_under_vma_lock(fault, &flags))
> > +		goto retry_vma;
> >  lock_mmap:
> >
> >  retry:
> > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > index 0b52557652be..b17986b40ac3 100644
> > --- a/arch/arm64/mm/fault.c
> > +++ b/arch/arm64/mm/fault.c
> > @@ -678,6 +678,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
> >  	if (!(mm_flags & FAULT_FLAG_USER))
> >  		goto lock_mmap;
> >
> > +retry_vma:
> >  	vma = lock_vma_under_rcu(mm, addr);
> >  	if (!vma)
> >  		goto lock_mmap;
> > @@ -715,8 +716,6 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
> >  		goto done;
> >  	}
> >  	count_vm_vma_lock_event(VMA_LOCK_RETRY);
> > -	if (fault & VM_FAULT_MAJOR)
> > -		mm_flags |= FAULT_FLAG_TRIED;
> >
> >  	/* Quick path to respond to signals */
> >  	if (fault_signal_pending(fault, regs)) {
> > @@ -724,6 +723,10 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
> >  			goto no_context;
> >  		return 0;
> >  	}
> > +
> > +	if (fault_should_retry_under_vma_lock(fault, &mm_flags))
> > +		goto retry_vma;
> > +
> >  lock_mmap:
> >
> >  retry:
> > diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
> > index 2c93d33356e5..6a946838b54b 100644
> > --- a/arch/loongarch/mm/fault.c
> > +++ b/arch/loongarch/mm/fault.c
> > @@ -219,6 +219,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
> >  	if (!(flags & FAULT_FLAG_USER))
> >  		goto lock_mmap;
> >
> > +retry_vma:
> >  	vma = lock_vma_under_rcu(mm, address);
> >  	if (!vma)
> >  		goto lock_mmap;
> > @@ -256,8 +257,6 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
> >  	}
> >
> >  	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)) {
> > @@ -265,6 +264,9 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
> >  			no_context(regs, write, address);
> >  		return;
> >  	}
> > +
> > +	if (fault_should_retry_under_vma_lock(fault, &flags))
> > +		goto retry_vma;
> >  lock_mmap:
> >
> >  retry:
> > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > index 806c74e0d5ab..e2a128fba408 100644
> > --- a/arch/powerpc/mm/fault.c
> > +++ b/arch/powerpc/mm/fault.c
> > @@ -487,6 +487,7 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
> >  	if (!(flags & FAULT_FLAG_USER))
> >  		goto lock_mmap;
> >
> > +retry_vma:
> >  	vma = lock_vma_under_rcu(mm, address);
> >  	if (!vma)
> >  		goto lock_mmap;
> > @@ -511,12 +512,13 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
> >  		goto done;
> >  	}
> >  	count_vm_vma_lock_event(VMA_LOCK_RETRY);
> > -	if (fault & VM_FAULT_MAJOR)
> > -		flags |= FAULT_FLAG_TRIED;
> >
> >  	if (fault_signal_pending(fault, regs))
> >  		return user_mode(regs) ? 0 : SIGBUS;
> >
> > +	if (fault_should_retry_under_vma_lock(fault, &flags))
> > +		goto retry_vma;
> > +
> >  lock_mmap:
> >
> >  	/* When running in the kernel we expect faults to occur only to
> > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
> > index 04ed6f8acae4..87b061feba51 100644
> > --- a/arch/riscv/mm/fault.c
> > +++ b/arch/riscv/mm/fault.c
> > @@ -347,6 +347,7 @@ void handle_page_fault(struct pt_regs *regs)
> >  	if (!(flags & FAULT_FLAG_USER))
> >  		goto lock_mmap;
> >
> > +retry_vma:
> >  	vma = lock_vma_under_rcu(mm, addr);
> >  	if (!vma)
> >  		goto lock_mmap;
> > @@ -368,14 +369,15 @@ void handle_page_fault(struct pt_regs *regs)
> >  		goto done;
> >  	}
> >  	count_vm_vma_lock_event(VMA_LOCK_RETRY);
> > -	if (fault & VM_FAULT_MAJOR)
> > -		flags |= FAULT_FLAG_TRIED;
> >
> >  	if (fault_signal_pending(fault, regs)) {
> >  		if (!user_mode(regs))
> >  			no_context(regs, addr);
> >  		return;
> >  	}
> > +
> > +	if (fault_should_retry_under_vma_lock(fault, &flags))
> > +		goto retry_vma;
> >  lock_mmap:
> >
> >  retry:
> > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > index 028aeb9c48d6..8e90e522436b 100644
> > --- a/arch/s390/mm/fault.c
> > +++ b/arch/s390/mm/fault.c
> > @@ -294,6 +294,7 @@ static void do_exception(struct pt_regs *regs, int access)
> >  		flags |= FAULT_FLAG_WRITE;
> >  	if (!(flags & FAULT_FLAG_USER))
> >  		goto lock_mmap;
> > +retry_vma:
> >  	vma = lock_vma_under_rcu(mm, address);
> >  	if (!vma)
> >  		goto lock_mmap;
> > @@ -310,14 +311,14 @@ static void do_exception(struct pt_regs *regs, int access)
> >  		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)) {
> >  		if (!user_mode(regs))
> >  			handle_fault_error_nolock(regs, 0);
> >  		return;
> >  	}
> > +	if (fault_should_retry_under_vma_lock(fault, &flags))
> > +		goto retry_vma;
> >  lock_mmap:
> >  retry:
> >  	vma = lock_mm_and_find_vma(mm, address, regs);
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 45b99c3b1442..53c8f003fe53 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1331,6 +1331,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> >  	if (!(flags & FAULT_FLAG_USER))
> >  		goto lock_mmap;
> >
> > +retry_vma:
> >  	vma = lock_vma_under_rcu(mm, address);
> >  	if (!vma)
> >  		goto lock_mmap;
> > @@ -1349,8 +1350,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 +1359,9 @@ void do_user_addr_fault(struct pt_regs *regs,
> >  						 ARCH_DEFAULT_PKEY);
> >  		return;
> >  	}
> > +
> > +	if (fault_should_retry_under_vma_lock(fault, &flags))
> > +		goto retry_vma;
> >  lock_mmap:
> >
> >  retry:
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 7fabe6c66b4b..27ec6673acfe 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -727,6 +727,34 @@ static inline bool fault_flag_allow_retry_first(enum fault_flag flags)
> >  	    (!(flags & FAULT_FLAG_TRIED));
> >  }
> >
> > +/**
> > + * fault_should_retry_under_vma_lock - decide whether to retry with VMA lock
> > + * @fault: fault result from handle_mm_fault() under FAULT_FLAG_VMA_LOCK
> > + * @flags: fault flags for the current fault, updated on retry
> > + *
> > + * Architecture page fault handlers call this after a VMA-lock fault returns
> > + * VM_FAULT_RETRY. If the fault result also has VM_FAULT_MAY_USE_VMA_LOCK,
> > + * allow one bounded retry under the VMA lock and set FAULT_FLAG_TRIED.
> > + *
> > + * When the fault must fall back to the mmap_lock path, preserve the existing
> > + * VM_FAULT_MAJOR behavior by marking FAULT_FLAG_TRIED before the retry.
> > + *
> > + * Return: true if the caller should retry under the VMA lock, false if it
>
> Oh good half of the kdoc comments in mm.h have Return and the other half have
> Returns: :)) OK I guess Return is fine here.
>
> > + * should fall back to the mmap_lock fault path.
> > + */
> > +static inline bool fault_should_retry_under_vma_lock(vm_fault_t fault, unsigned int *flags)
> > +{
> > +	if ((fault & VM_FAULT_MAY_USE_VMA_LOCK) && !(*flags & FAULT_FLAG_TRIED)) {
> > +		*flags |= FAULT_FLAG_TRIED;
> > +		return true;
> > +	}
> > +
> > +	if (fault & VM_FAULT_MAJOR)
> > +		*flags |= FAULT_FLAG_TRIED;
> > +
> > +	return false;
> > +}
>
> I really hate this function. It's not doing what it says it is (it's
> modifying fault behaviour too via the flags parameter AND setting state for
> major faults), it's combining VMA fault path handling AND mmap major fault
> handling it's doing ugly horrors with an output parameter.
>
> I think it's better just as a straight-up predicate. Yes there'll be
> duplication on setting FAULT_FLAG_TRIED. It sucks, but you're _already_
> duplicating every single invocation and goto anyway.
>
> The fix for that is finally de-duplicating the arch fault code properly.
>
> So it'd be like:
>
> 	< kdoc etc. >
> 	static inline bool should_retry_fault_under_vma_lock(vm_fault fault_type,
> 			unsigned int flags)
> 	{
> 		/* Don't wait holding the VMA lock. */
> 		if (!(fault_type & VM_FAULT_RETRY_WONT_BLOCK))
> 			return false;
> 		/* Already retried the fault under the VMA lock. */
> 		if (flags & FAULT_FLAG_TRIED)
> 			return false;
> 		return true;
> 	}
>
>
> -	if (fault_should_retry_under_vma_lock(fault, &flags))
> -		goto retry_vma;
> +	if (should_retry_fault_under_vma_lock(fault, flags)) {
> +		flags |= FAULT_FLAG_TRIED;
> +		goto retry_vma;
> +	}
> +
> +	if (fault & VM_FAULT_MAJOR)
> +		flags |= FAULT_FLAG_TRIED;
>
> This way also you explicitly see where FAULT_FLAG_TRIED is set.
>
> > +
> >  #define FAULT_FLAG_TRACE \
> >  	{ FAULT_FLAG_WRITE,		"WRITE" }, \
> >  	{ FAULT_FLAG_MKWRITE,		"MKWRITE" }, \
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index b5d4cd3b067b..46a832757109 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -1684,6 +1684,8 @@ typedef __bitwise unsigned int vm_fault_t;
> >   * @VM_FAULT_NOPAGE:		->fault installed the pte, not return page
> >   * @VM_FAULT_LOCKED:		->fault locked the returned page
> >   * @VM_FAULT_RETRY:		->fault blocked, must retry
> > + * @VM_FAULT_MAY_USE_VMA_LOCK:	->fault blocked, retry may be handled under
> > + *				the VMA lock
> >   * @VM_FAULT_FALLBACK:		huge page fault failed, fall back to small
> >   * @VM_FAULT_DONE_COW:		->fault has fully handled COW
> >   * @VM_FAULT_NEEDDSYNC:		->fault did not modify page tables and needs
> > @@ -1707,6 +1709,7 @@ enum vm_fault_reason {
> >  	VM_FAULT_DONE_COW       = (__force vm_fault_t)0x001000,
> >  	VM_FAULT_NEEDDSYNC      = (__force vm_fault_t)0x002000,
> >  	VM_FAULT_COMPLETED      = (__force vm_fault_t)0x004000,
> > +	VM_FAULT_MAY_USE_VMA_LOCK = (__force vm_fault_t)0x008000,
> >  	VM_FAULT_HINDEX_MASK    = (__force vm_fault_t)0x0f0000,
> >  };
> >
> > @@ -1731,6 +1734,7 @@ enum vm_fault_reason {
> >  	{ VM_FAULT_FALLBACK,            "FALLBACK" },	\
> >  	{ VM_FAULT_DONE_COW,            "DONE_COW" },	\
> >  	{ VM_FAULT_NEEDDSYNC,           "NEEDDSYNC" },	\
> > +	{ VM_FAULT_MAY_USE_VMA_LOCK,    "MAY_USE_VMA_LOCK" },	\
>
> This makes the naming confusion even worse - because now we have
> FAULT_FLAG_VMA_LOCK and this which 'specifies' may use VMA lock whereas
> really it means 'may use once on retry'...
>
> So again, as per above, I think it's better to actually have this
> communicate the _reason_ why it seems OK to use the VMA lock on retry
> rather than actually specifying that the caller should.
>

Will address in the next revision if we keep this approach.

> >  	{ VM_FAULT_COMPLETED,           "COMPLETED" }
> >
> >  struct vm_special_mapping {
> > --
> > 2.43.0
> >
>
> BTW in sanitize_fault_flags() there's:
>
> 	/*
> 	 * Per-VMA locks can't be used with FAULT_FLAG_RETRY_NOWAIT because of
> 	 * the assumption that lock is dropped on VM_FAULT_RETRY.
> 	 */
> 	if (WARN_ON_ONCE((*flags &
> 			(FAULT_FLAG_VMA_LOCK | FAULT_FLAG_RETRY_NOWAIT)) ==
> 			(FAULT_FLAG_VMA_LOCK | FAULT_FLAG_RETRY_NOWAIT)))
> 		return VM_FAULT_SIGSEGV;
>
> Except now VM_FAULT_RETRY doesn't drop the lock :) I think the comment
> needs to be updated to say 'dropped on VM_FAULT_RETRY after one attempt'.

The patch doesn't change VM_FAULT_RETRY semantics: returning VM_FAULT_RETRY
still means the lock has been dropped, whether it's the mmap lock or the VMA
lock. The retry path re-acquires the VMA lock before the second attempt. So
I think the existing comment is still accurate?

> Also I think there's an issue with major/minor fault counting as a result
> of this, in mm_account_fault():
>
> 	/*
> 	 * We define the fault as a major fault when the final successful fault
> 	 * is VM_FAULT_MAJOR, or if it retried (which implies that we couldn't
> 	 * handle it immediately previously).
> 	 */
> 	major = (ret & VM_FAULT_MAJOR) || (flags & FAULT_FLAG_TRIED);
>
> 	if (major)
> 		current->maj_flt++;
> 	else
> 		current->min_flt++;
>
> Now you're getting minor faults being counted as major ones?
>
> If intended, that's a user-visible change that should be documented and
> defended in the commit msg.
>
> If not, you could do some REALLY gross checks making things _even more
> complicated_ here.
>
> But maybe something like:
>
> 	< kdoc comment etc. >
> 	static bool is_major_fault(vm_fault_t fault_type, unsigned int fault_flags)
> 	{
> 		/* Explicitly marked as major. */
> 		if (fault_type & VM_FAULT_MAJOR)
> 			return true;
> 		/* If no retry occurred, minor. */
> 		if (!(fault_flags & FAULT_FLAG_TRIED))
> 			return false;
> 		/* Quickly retrying fault under the VMA lock implies minor. */
> 		return !(fault_flags & FAULT_FLAG_VMA_LOCK);
> 	}
>
> 	Then the code above becomes:
>
> 	if (is_major_fault(ret, flags))
> 		current->maj_flt++;
> 	else
> 		current->min_flt++;
>
> --
> Cheers, Lorenzo

With FAULT_FLAG_TRIED set, the retry attempt behaves identically regardless
of whether it's under the mmap lock or the VMA lock. Is a change still
needed here?

Thanks,
Hongru