Re: [RFC PATCH v4 1/3] mm: allow page faults to request VMA-lock retry
Hongru Zhang <[email protected]>
| Newsgroups | org.kernel.vger.linux-kernel,org.kvack.linux-mm |
|---|---|
| Message-ID | <[email protected]> |
> On Tue, Aug 4, 2026 at 8:32 PM Lorenzo Stoakes (ARM) <[email protected]> wrote: > > > > On Tue, Aug 04, 2026 at 05:52:19PM +0800, Hongru Zhang wrote: > > > From: Hongru Zhang <[email protected]> > > > > > > Page faults handled under the per-VMA lock currently fall back to the > > > mmap_lock path whenever handle_mm_fault() returns VM_FAULT_RETRY. This > > > means that lower-level fault handlers have no way to tell the > > > architecture fault handler that the retry can safely continue under the > > > per-VMA lock. > > > > > > Add VM_FAULT_MAY_USE_VMA_LOCK as an advisory bit that can be returned > > > > I don't love that name or that faulting retry behaviour is _modified_ by a > > value that indicates fault resolution state... ugh. > > > > It's kinda confusing things 'VM_FAULT_RETRY' is 'you have to retry this > > fault'. > > > > 'VM_FAULT_MAY_...' is starting to bring in effectively configuration > > options into it and that's kinda horrible. > > > > I mean is there any reason we shouldn't ALWAYS do this if a VMA lock was > > used? > > > > It's not too expensive to do a single retry with the VMA lock before > > falling back to the mmap lock. > > > > So maybe simplify like that? > > > > And like that this series becomes a single patch right? > > This is a brilliant idea. That's a genius insight, Lorenzo. > > 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: