Re: [PATCH v6] mm: retry page faults once under the per-VMA lock

[Barry Song <baohua@kernel.org>]

On Fri, Oct 2, 2026 at 5:12 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Sep 28, 2026 at 10:48:41AM +0800, Barry Song wrote:
> > On Mon, Sep 28, 2026 at 6:55 AM Matthew Wilcox <willy@infradead.org> wrote:
> > > So while doing my slides, I realised that what we need to avoid doing
> > > is (a) sleeping while holding the mmap_lock (b) returning RETRY while
> > > holding the VMA lock
> > >
> > > And that turns out to be as simple as this patch:
> > >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index dd09c438fa23..94ed2333f8d8 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -723,6 +723,8 @@ enum {
> > >   */
> > >  static inline bool fault_flag_allow_retry_first(enum fault_flag flags)
> > >  {
> > > +       if (flags & FAULT_FLAG_VMA_LOCK)
> > > +               return false;
> > >         return (flags & FAULT_FLAG_ALLOW_RETRY) &&
> > >             (!(flags & FAULT_FLAG_TRIED));
> > >  }
> > >
> > > OK, this is a hack.  The function is spectacularly badly named, and
> > > needs to be renamed before a patch can go upstream.  But this should
> > > fix the contention on mmap_lock.
> >
> > Thanks for your suggestion.
> > This is exactly what we did in Android Common Kernel before we had
> > Lorenzo's proposal (bypassing `fault_flag_allow_retry_first()`):
> >
> > https://android.googlesource.com/kernel/common/+/1b9b045a586245cc1c29b2747c6586234c7f5bad%5E%21/#F2
>
> Looks like that one didn't cover __folio_lock_or_retry(), but that
> doesn't invalidate your point.
Yep. `__folio_lock_or_retry()` will make the same thing true for anon
VMAs, so we intentionally made the hook valid only for file VMAs.Only touching the file retry path seems to involve less VMA
contention.

>
> > Note that Lorenzo's proposal avoids mmap_lock contention without
> > introducing any new VMA lock contention. It also doesn't require a new
> > flag that would break KMI. So this is clearly the preferred approach.
>
> But it does retry multiple times in cases where we know the fault
> will always fail (eg the fault is on a device-private VMA)
>
Right now, these might require a single extra retry for device-private
and `__vmf_anon_prepare()` cases. The commit log also mentions this.

"Some faults may
retry unnecessarily, for example, those in __vmf_anon_prepare() or
device-private fault handling, which require the mmap_lock. However,
these cases are expected to be infrequent and only add one cheap
per-VMA lock attempt."

If we do want to remove the extra VMA lock attempt, we might still
need an additional flag such as VM_FAULT_NEED_MMAP_LOCK:

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 2cecf6ba6df7..2821e3bd7462 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -620,6 +620,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
 	struct vm_area_struct *vma;
 	int si_code;
 	int pkey = -1;
+	bool vma_lock_retried = false;
 
 	if (kprobe_page_fault(regs, esr))
 		return 0;
@@ -688,6 +689,7 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
 	if (!(mm_flags & FAULT_FLAG_USER))
 		goto lock_mmap;
 
+lock_vma:
 	vma = lock_vma_under_rcu(mm, addr);
 	if (!vma)
 		goto lock_mmap;
@@ -734,6 +736,12 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
 			goto no_context;
 		return 0;
 	}
+
+	if (!vma_lock_retried && !(fault & VM_FAULT_NEED_MMAP_LOCK)) {
+		vma_lock_retried = true;
+		goto lock_vma;
+	}
+
 lock_mmap:
 
 retry:
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 6141160ec652..baadab598d98 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -1734,10 +1734,11 @@ enum vm_fault_reason {
 	VM_FAULT_NOPAGE         = (__force vm_fault_t)0x000100,
 	VM_FAULT_LOCKED         = (__force vm_fault_t)0x000200,
 	VM_FAULT_RETRY          = (__force vm_fault_t)0x000400,
-	VM_FAULT_FALLBACK       = (__force vm_fault_t)0x000800,
-	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_NEED_MMAP_LOCK     = (__force vm_fault_t)0x000800,
+	VM_FAULT_FALLBACK       = (__force vm_fault_t)0x001000,
+	VM_FAULT_DONE_COW       = (__force vm_fault_t)0x002000,
+	VM_FAULT_NEEDDSYNC      = (__force vm_fault_t)0x004000,
+	VM_FAULT_COMPLETED      = (__force vm_fault_t)0x008000,
 	VM_FAULT_HINDEX_MASK    = (__force vm_fault_t)0x0f0000,
 };
 
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index ddc631a388b9..64e08c6153cf 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -1480,7 +1480,7 @@ vm_fault_t do_huge_pmd_device_private(struct vm_fault *vmf)
 
 	if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
 		vma_end_read(vma);
-		return VM_FAULT_RETRY;
+		return VM_FAULT_RETRY | VM_FAULT_NEED_MMAP_LOCK;
 	}
 
 	ptl = pmd_lock(vma->vm_mm, vmf->pmd);
diff --git a/mm/memory.c b/mm/memory.c
index 1f83a26f8733..89c8c8a52c16 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3980,7 +3980,7 @@ vm_fault_t __vmf_anon_prepare(struct vm_fault *vmf)
 		return 0;
 	if (vmf->flags & FAULT_FLAG_VMA_LOCK) {
 		if (!mmap_read_trylock(vma->vm_mm))
-			return VM_FAULT_RETRY;
+			return VM_FAULT_RETRY | VM_FAULT_NEED_MMAP_LOCK;
 	}
 	if (__anon_vma_prepare(vma))
 		ret = VM_FAULT_OOM;
@@ -4937,7 +4937,7 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 				 * under VMA lock.
 				 */
 				vma_end_read(vma);
-				ret = VM_FAULT_RETRY;
+				ret = VM_FAULT_RETRY | VM_FAULT_NEED_MMAP_LOCK;
 				goto out;
 			}
 
-- 
2.39.3 (Apple Git-146)