* [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry @ 2026-08-04 9:52 Hongru Zhang 2026-08-04 12:31 ` Lorenzo Stoakes (ARM) 0 siblings, 1 reply; 8+ messages in thread From: Hongru Zhang @ 2026-08-04 9:52 UTC (permalink / raw) To: akpm, linux-mm Cc: david, liam, linux-kernel, ljs, baohua, mhocko, rppt, shakeel.butt, surenb, vbabka, willy, zhanghongru 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 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. 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 VM_FAULT_RETRY with VM_FAULT_MAY_USE_VMA_LOCK when that tradeoff is preferable to falling back to mmap_lock immediately. 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 ++++-- 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 + * 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; +} + #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" }, \ { VM_FAULT_COMPLETED, "COMPLETED" } struct vm_special_mapping { -- 2.43.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry 2026-08-04 9:52 [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry Hongru Zhang @ 2026-08-04 12:31 ` Lorenzo Stoakes (ARM) 2026-08-04 21:13 ` Barry Song 2026-08-05 14:12 ` [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock Hongru Zhang 0 siblings, 2 replies; 8+ messages in thread From: Lorenzo Stoakes (ARM) @ 2026-08-04 12:31 UTC (permalink / raw) To: Hongru Zhang Cc: akpm, linux-mm, david, liam, linux-kernel, baohua, mhocko, rppt, shakeel.butt, surenb, vbabka, willy, zhanghongru 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry 2026-08-04 12:31 ` Lorenzo Stoakes (ARM) @ 2026-08-04 21:13 ` Barry Song 2026-08-05 11:00 ` Lorenzo Stoakes (ARM) 2026-08-06 7:29 ` Hongru Zhang 2026-08-05 14:12 ` [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock Hongru Zhang 1 sibling, 2 replies; 8+ messages in thread From: Barry Song @ 2026-08-04 21:13 UTC (permalink / raw) To: Lorenzo Stoakes (ARM) Cc: Hongru Zhang, akpm, linux-mm, david, liam, linux-kernel, mhocko, rppt, shakeel.butt, surenb, vbabka, willy, zhanghongru 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry 2026-08-04 21:13 ` Barry Song @ 2026-08-05 11:00 ` Lorenzo Stoakes (ARM) 2026-08-06 7:29 ` Hongru Zhang 1 sibling, 0 replies; 8+ messages in thread From: Lorenzo Stoakes (ARM) @ 2026-08-05 11:00 UTC (permalink / raw) To: Barry Song Cc: Hongru Zhang, akpm, linux-mm, david, liam, linux-kernel, mhocko, rppt, shakeel.butt, surenb, vbabka, willy, zhanghongru 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry 2026-08-04 21:13 ` Barry Song 2026-08-05 11:00 ` Lorenzo Stoakes (ARM) @ 2026-08-06 7:29 ` Hongru Zhang 2026-08-06 8:06 ` Barry Song 1 sibling, 1 reply; 8+ messages in thread From: Hongru Zhang @ 2026-08-06 7:29 UTC (permalink / raw) To: baohua, ljs Cc: akpm, david, liam, linux-kernel, linux-mm, mhocko, rppt, shakeel.butt, surenb, vbabka, willy, zhanghongru06, zhanghongru > 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: ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry 2026-08-06 7:29 ` Hongru Zhang @ 2026-08-06 8:06 ` Barry Song 2026-08-07 7:49 ` Hongru Zhang 0 siblings, 1 reply; 8+ messages in thread From: Barry Song @ 2026-08-06 8:06 UTC (permalink / raw) To: Hongru Zhang Cc: ljs, akpm, david, liam, linux-kernel, linux-mm, mhocko, rppt, shakeel.butt, surenb, vbabka, willy, zhanghongru 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry 2026-08-06 8:06 ` Barry Song @ 2026-08-07 7:49 ` Hongru Zhang 0 siblings, 0 replies; 8+ messages in thread From: Hongru Zhang @ 2026-08-07 7:49 UTC (permalink / raw) To: baohua Cc: akpm, david, liam, linux-kernel, linux-mm, ljs, mhocko, rppt, shakeel.butt, surenb, vbabka, willy, zhanghongru06, zhanghongru > 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/ 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock 2026-08-04 12:31 ` Lorenzo Stoakes (ARM) 2026-08-04 21:13 ` Barry Song @ 2026-08-05 14:12 ` Hongru Zhang 1 sibling, 0 replies; 8+ messages in thread From: Hongru Zhang @ 2026-08-05 14:12 UTC (permalink / raw) To: ljs Cc: akpm, baohua, david, liam, linux-kernel, linux-mm, mhocko, rppt, shakeel.butt, surenb, vbabka, willy, zhanghongru06, zhanghongru > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-08-07 7:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-08-04 9:52 [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry Hongru Zhang 2026-08-04 12:31 ` Lorenzo Stoakes (ARM) 2026-08-04 21:13 ` Barry Song 2026-08-05 11:00 ` Lorenzo Stoakes (ARM) 2026-08-06 7:29 ` Hongru Zhang 2026-08-06 8:06 ` Barry Song 2026-08-07 7:49 ` Hongru Zhang 2026-08-05 14:12 ` [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock Hongru Zhang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).