Re: [RFC PATCH v3] mm: retry page faults under per-VMA lock when mmap_lock is not required

[Suren Baghdasaryan]

On Fri, Jul 10, 2026 at 7:54 AM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> On Tue, Jul 7, 2026 at 9:52 AM Lorenzo Stoakes <ljs@xxxxxxxxxx> wrote:
> >
> > +cc Vlata, the only one from the MEMORY MAPPING - LOCKING people excluded,
> > and Willy as he's been heavily involved. But see below you need to cc- way
> > more.
> >
> > (nitty I know) Maybe subject should be 'mm: default to VMA lock on
> > VM_FAULT_RETRY' or something just to make it clear it's about the retry
> > path?
> >
> > Also I guess you and Barry agreed to you taking this over? :)
> >
> > You missed a million people from cc- by the way. This time I really cannot
> > be bothered to add them all as I usually do, but it is MANDATORY to cc
> > maintainers and really you should be cc'ing reviewers too every time.
> >
> > Again I feel I need to write a bot that just autoreplies about this :)
> >
> > $ scripts/get_maintainer.pl --nogit --nogit-fallback your-patch.patch
> > <... ~36 people in total ...>
> > < lists >
> > < etc. >
> >
> > Please fix this on respin.
> >
> > On Fri, Jun 26, 2026 at 03:50:19PM +0800, Hongru Zhang wrote:
> > > From: Hongru Zhang <zhanghongru@xxxxxxxxxx>
> > >
> > > Currently, when a page fault returns VM_FAULT_RETRY under the per-VMA
> > > lock due to folio_lock() failing, mainly because the folio is under
> > > I/O, the architecture fault handler unconditionally falls back to
> > > retrying with mmap_lock. This leads to mmap_lock contention.
> > >
> > > This patch introduces VM_FAULT_RETRY_HARD to mark the paths that
> >
> > I'm going to have to bikeshed the name :)
> >
> > While I like the idea of 'retry hard' like the movie 'die hard' :P I think,
> > since this is explicitly about the mmap lock, we should reference that?
> >
> > So maybe VM_FAULT_RETRY_MMAP?
> >
> > or VM_FAULT_RETRY_HARDER_WITH_A_VENGEANCE? ;)
> >
> > > require mmap_lock. Retries now stay under the per-VMA lock by default,
> > > and only marked paths fall back to the mmap_lock retry path.
> >
> > I think the commit message should be much clearer about the logic here.
> >
> > You are making it such that VM_FAULT_RETRY does its retry pass using a VMA
> > lock, but it retains the same 'retry and set FAULT_FLAG_TRIED' logic as
> > before.
> >
> > HOWEVER.
> >
> > On the VMA lock path the retry flag is only set if VM_FAULT_MAJOR. So now
> > you're allowing _unbounded_ VMA lock attempts (which is concerning and I
> > don't think intended by the lock code).
> >
> > None of this is clear right now, you need to documment it.
> >
> > >
> > > Based on the stress model from Kunwu Chan and Wang Lian in v2, we
> > > adapted a benchmark for a 20-core desktop environment (reduced thread
> > > count, adjusted memcg limits, 20-core Intel i7-12700). The benchmark
> > > uses concurrent page faults under memcg pressure (forcing reclaim and
> > > VM_FAULT_RETRY) with parallel munmap to amplify mmap_lock read-write
> > > contention.
> > >
> > > Throughput (higher is better):
> > > +---------+------------+------------+-------------+
> > > | Threads |  Vanilla   |  Patched   | Improvement |
> > > +---------+------------+------------+-------------+
> > > |   40    | 1071.11 /s | 1301.11 /s |   +21.5%    |
> > > +---------+------------+------------+-------------+
> > > |   60    | 1043.15 /s | 1472.36 /s |   +41.1%    |
> > > +---------+------------+------------+-------------+
> > > |   80    | 1049.39 /s | 1665.65 /s |   +58.7%    |
> > > +---------+------------+------------+-------------+
> > >
> > > mmap_lock contention count (lower is better):
> > > +---------+-----------+---------+-----------+
> > > | Threads |  Vanilla  | Patched | Reduction |
> > > +---------+-----------+---------+-----------+
> > > |   40    | 3,217,904 |  51,201 |  -98.4%   |
> > > +---------+-----------+---------+-----------+
> > > |   60    | 4,419,149 |  55,711 |  -98.7%   |
> > > +---------+-----------+---------+-----------+
> > > |   80    | 5,395,730 |  66,184 |  -98.8%   |
> > > +---------+-----------+---------+-----------+
> >
> > Yeah I worry a bit about the unbounded nature of things here, the fact
> > we're seeing such a difference could also be offset by extreme busy waiting
> > with the unbounded VMA lock retries.
> >
> > Let's experiment based on limiting this surely?
> >
> > >
> > > Benchmark and test scripts:
> > > https://gist.github.com/zhr250/c36c2c54d9351df37e12fd072d4926ef
> > >
> > > Suggested-by: Barry Song <baohua@xxxxxxxxxx>
> > > Suggested-by: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > > Signed-off-by: Hongru Zhang <zhanghongru@xxxxxxxxxx>
> > > ---
> > > Changes since v2:
> > > - v2 required each retry path to explicitly opt in to VMA lock retry
> > >   (VM_FAULT_RETRY_VMA). This patch inverts the logic: retries default
> > >   to VMA lock, and only paths requiring mmap_lock opt out
> > >   (VM_FAULT_RETRY_HARD)
> > > - This patch corresponds to v2 patch 1/5; the remaining optimizations
> > >   will be submitted separately
> > > - Rebased on mm-unstable
> > >
> > > Link to v2:
> > > https://lore.kernel.org/all/20260430040427.4672-1-baohua@xxxxxxxxxx/
> > >
> > > Link to v1:
> > > https://lore.kernel.org/all/20251127011438.6918-1-21cnbao@xxxxxxxxx/
> >
> > Hmm it's a bit odd for you to send an independent series and link to
> > somebody else's series as v1/v2 but anyway as long as Barry and you agree
> > with that approach that's fine!
> >
> > >
> > >  arch/arm/mm/fault.c       | 4 ++++
> > >  arch/arm64/mm/fault.c     | 4 ++++
> > >  arch/loongarch/mm/fault.c | 4 ++++
> > >  arch/powerpc/mm/fault.c   | 4 ++++
> > >  arch/riscv/mm/fault.c     | 4 ++++
> > >  arch/s390/mm/fault.c      | 4 ++++
> > >  arch/x86/mm/fault.c       | 4 ++++
> > >  include/linux/mm_types.h  | 9 +++++----
> > >  mm/huge_memory.c          | 2 +-
> > >  mm/memory.c               | 6 +++---
> > >  10 files changed, 37 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
> > > index e62cc4be5adf..2f909fb4f7db 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;
> > > @@ -420,6 +421,9 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
> > >                       goto no_context;
> > >               return 0;
> > >       }
> >
> > OK so this is really important - and it seems to be the case regardless of
> > arch.
> >
> > When VM_FAULT_RETRY is encountered under the VMA lock, we do:
> >
> >         if (fault & VM_FAULT_MAJOR)
> >                 flags |= FAULT_FLAG_TRIED;
> >
> > Which then allows _under the mmap lock_ 2 more retries.
> >
> > You're now (silently!) going to:
> >
> >         if (!(fault & VM_FAULT_MAJOR))
> >                 /* unbounded infinite VMA lock retries */
> >
> > This seems very unwise (TM).
> >
> > We should bound this somehow. A maximum number of retries? Only do the
> > minor fault thing once?
> >
> > Could you benchmark how it looks at different retry counts? We could also
> > consider other strategies, but unbounded attempts here seems not good.
> >
> > Another thing - is there any way to dedupe this code...? We're open coding
> > a lot of crap now in each of the arches and it's not good.
> >
> > A series like this is the place to fix stuff like that, not keep adding to
> > the technical debt.
> >
> > Hongru - could you see if that's possible as preceeding patches please?
> >
> > > +     if (!(fault & VM_FAULT_RETRY_HARD))
> > > +             goto retry_vma;
> >
> > Maybe it's worth being explicit that this is the VM_FAULT_RETRY path? Otherwise
> > it's not so clear.
> >
> > You happen to only fall through here if VM_FAULT_RETRY is set.
> >
> > So e.g.:
> >
> > /**
> >  * fault_can_retry_vma_lock() - Are we able to retry a page fault holding
> >  * only the VMA lock?
> >  * @fault: The fault to check.
> >  *
> >  * We default to performing the VM_FAULT_RETRY/FAULT_FLAG_TRIED dance under
> >  * VMA lock. This allows us to avoid mmap contention on the likely case
> >  * that we encountered an ephemeral inability to VMA lock (e.g. the VMA was
> >  * write locked at the time).
> >  *
> >  * Returns: true if we can retry under the VMA lock, false otherwise.
> >  */
> > static bool fault_can_retry_vma_lock(vm_fault_t fault)
> > {
> >         VM_WARN_ON_ONCE(!(fault & VM_FAULT_RETRY));
> >
> >         return fault & VM_FAULT_RETRY_HARD;
> > }
> >
> > And put that somewhere all the arch fault logic can access.
> >
> > Then:
> >
> >         if (fault_can_retry_vma_lock(fault))
> >                 goto retry_vma;
> >
> > Is self-documenting, and the assert assures that no mistake was made with
> > it.
> >
> > Same comments for all the arches, obviously.
> >
> > > +
> > >  lock_mmap:
> > >
> > >  retry:
> > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > > index 739800835920..695a09486795 100644
> > > --- a/arch/arm64/mm/fault.c
> > > +++ b/arch/arm64/mm/fault.c
> > > @@ -673,6 +673,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;
> > > @@ -719,6 +720,9 @@ static int __kprobes do_page_fault(unsigned long far, unsigned long esr,
> > >                       goto no_context;
> > >               return 0;
> > >       }
> > > +     if (!(fault & VM_FAULT_RETRY_HARD))
> > > +             goto retry_vma;
> > > +
> > >  lock_mmap:
> > >
> > >  retry:
> > > diff --git a/arch/loongarch/mm/fault.c b/arch/loongarch/mm/fault.c
> > > index 2c93d33356e5..d7dd55722e52 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;
> > > @@ -265,6 +266,9 @@ static void __kprobes __do_page_fault(struct pt_regs *regs,
> > >                       no_context(regs, write, address);
> > >               return;
> > >       }
> > > +     if (!(fault & VM_FAULT_RETRY_HARD))
> > > +             goto retry_vma;
> > > +
> > >  lock_mmap:
> > >
> > >  retry:
> > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> > > index 806c74e0d5ab..65ec9a8252e7 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;
> > > @@ -517,6 +518,9 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
> > >       if (fault_signal_pending(fault, regs))
> > >               return user_mode(regs) ? 0 : SIGBUS;
> > >
> > > +     if (!(fault & VM_FAULT_RETRY_HARD))
> > > +             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..6c68414a1224 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;
> > > @@ -376,6 +377,9 @@ void handle_page_fault(struct pt_regs *regs)
> > >                       no_context(regs, addr);
> > >               return;
> > >       }
> > > +     if (!(fault & VM_FAULT_RETRY_HARD))
> > > +             goto retry_vma;
> > > +
> > >  lock_mmap:
> > >
> > >  retry:
> > > diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
> > > index 028aeb9c48d6..45a6c35044cc 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;
> > > @@ -318,6 +319,9 @@ static void do_exception(struct pt_regs *regs, int access)
> > >                       handle_fault_error_nolock(regs, 0);
> > >               return;
> > >       }
> > > +     if (!(fault & VM_FAULT_RETRY_HARD))
> > > +             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 63de8e8684f2..cb80070271d0 100644
> > > --- a/arch/x86/mm/fault.c
> > > +++ b/arch/x86/mm/fault.c
> > > @@ -1322,6 +1322,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;
> > > @@ -1351,6 +1352,9 @@ void do_user_addr_fault(struct pt_regs *regs,
> > >                                                ARCH_DEFAULT_PKEY);
> > >               return;
> > >       }
> > > +     if (!(fault & VM_FAULT_RETRY_HARD))
> > > +             goto retry_vma;
> > > +
> > >  lock_mmap:
> > >
> > >  retry:
> > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > > index 5ef78617ce93..58db6f0af6fd 100644
> > > --- a/include/linux/mm_types.h
> > > +++ b/include/linux/mm_types.h
> >
> > Please also update the comment block to add the new flag.
> >
> > > @@ -1665,10 +1665,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_RETRY_HARD     = (__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,
> >
> > Hmm I suppose there's no harm in renumbering here to keep the retries
> > together :)
> >
> > I wonder if we shouldn't invert this VM_FAULT_RETRY | VM_FAULT_RETRY_HARD
> > thing though and instead have:
> >
> > #define VM_FAULT_RETRY_MASK (VM_FAULT_RETRY | VM_FAULT_RETRY_MMAP)
> >
> > And convert code that checks VM_FAULT_RETRY to check VM_FAULT_RETRY_MASK
> > instead, then there'd be no need for an awkward check/or confusion around
> > the need for VM_FAULT_RETRY to always be set in both cases.
> >
> > You're already basically touching all the files that reference
> > VM_FAULT_RETRY so it's not actually much more churn.
> >
> > >       VM_FAULT_HINDEX_MASK    = (__force vm_fault_t)0x0f0000,
> > >  };
> > >
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index 94bd656eeaf8..ad0129d579bc 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -1398,7 +1398,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_RETRY_HARD;
> >
> > Why are we specifically wanting this for PMD device private entries?
> >
> > You should add a comment, and the commit message should say why.
> >
> > Actually, maybe in this common case of vmf->flags & FAULT_FLAG_VMA_LOCK ->
> > harder you can make a helper?
> >
> > Like:
> >
> > /** blahhhh some kdoc comment */
> > static vm_fault_t retry_under_mmap_if_vma_locked(struct vm_fault *vmf)
> > {
> >         if (!(vmf->flags & FAULT_FLAG_VMA_LOCK))
> >                 return 0;
> >
> >         vma_end_read(vma);
> >         return VM_FAULT_RETRY | VM_FAULT_RETRY_HARD;
> > }
> >
> > And:
> >
> >         vm_fault_t ret;
> >
> >         ret = retry_under_mmap_if_vma_locked(vmf);
> >         if (ret)
> >                 return ret;
> >
> > Or something like this.
> >
> > >       }
> > >
> > >       ptl = pmd_lock(vma->vm_mm, vmf->pmd);
> > > diff --git a/mm/memory.c b/mm/memory.c
> > > index ff338c2abe92..7f2a30b5efca 100644
> > > --- a/mm/memory.c
> > > +++ b/mm/memory.c
> > > @@ -3797,7 +3797,7 @@ static inline vm_fault_t vmf_can_call_fault(const struct vm_fault *vmf)
> > >       if (vma->vm_ops->map_pages || !(vmf->flags & FAULT_FLAG_VMA_LOCK))
> > >               return 0;
> > >       vma_end_read(vma);
> > > -     return VM_FAULT_RETRY;
> > > +     return VM_FAULT_RETRY | VM_FAULT_RETRY_HARD;
> >
> > You could use my helper like above.
> >
> > >  }
> > >
> > >  /**
> > > @@ -3824,7 +3824,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_RETRY_HARD;
> >
> > This really makes me think VM_FAULT_RETRY_MMAP or something explicit would
> > be better, otherwise you have to add a comment explaining you're doing this
> > because you need the mmap read lock here since the VMA lock + speculative
> > mmap read lock acquisition didn't work out.
> >
> > >       }
> > >       if (__anon_vma_prepare(vma))
> > >               ret = VM_FAULT_OOM;
> > > @@ -4778,7 +4778,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_RETRY_HARD;
> >
> > Again, helper above self-documents.
> >
> > >                               goto out;
> > >                       }
> > >
> > >
> > > base-commit: 81652c5a65d4ae28e9b18c16ef917a40025c3653
> > > --
> > > 2.43.0
> > >
> >
> > Have you audited every single VM_FAULT_RETRY case? Because getting this
> > wrong could be a serious problem and break things. You should include a
> > list of call sites in your patch and explain why it's fine in each case.
>
> That's my main worry with this patch as well. I think a better
> approach would keep VM_FAULT_RETRY as is (falling back to mmap_lock),
> introduce VM_FAULT_RETRY_VMA (or whatever is best to indicate a retry
> with VMA lock) and then convert individual VM_FAULT_RETRY cases to
> VM_FAULT_RETRY_VMA with proper reasoning and explanation why it is
> safe. Less churn, gradual conversion, and happy reviewers.
>
> >
> > One case I noticed is uffd:
> >
> >         ret = VM_FAULT_RETRY;
> >         ...
> >         if (unlikely(READ_ONCE(ctx->released))) {
> >                 /*
> >                  * If a concurrent release is detected, do not return
> >                  * VM_FAULT_SIGBUS or VM_FAULT_NOPAGE, but instead always
> >                  * return VM_FAULT_RETRY with lock released proactively.
> > .       ...
> >                  * If we were to return VM_FAULT_NOPAGE, it would work for
> >                  * the fault path, in which the lock will be released
> >                  * later.  However for GUP, faultin_page() does nothing
> >                  * special on NOPAGE, so GUP would spin retrying without
> >                  * releasing the mmap read lock, causing possible livelock.
> >                  *
> >                  * Here only VM_FAULT_RETRY would make sure the mmap lock
> >                  * be released immediately, so that the thread concurrently
> >                  * releasing the userfault would always make progress.
> >                  */
> >                  release_fault_lock(vmf);
> >                  goto out;
> >         }
> >
> > The comment needs updating (Suren?) to reflect the VMA lock, basically with
> > VMA lock held this is meaningless.
>
> Right, I missed it when enabling per-VMA locks for userfaultfd faults
> in [1]. The comment is true if we already fell back to mmap_lock but
> not if we are handling the fault under VMA lock. I should amend it.
>
> [1] https://lore.kernel.org/all/20230630211957.1341547-1-surenb@xxxxxxxxxx/
>
> >
> > We're actually safe here in general as if the VMA lock is contended on a
> > competing write lock we drop to mmap lock.
> >
> > A racing uffd release thread does an unconditional vma_start_write()
> > _after_ setting ctx->released:
> >
> > static int userfaultfd_release(struct inode *inode, struct file *file)
> > {
> >         ...
> >         WRITE_ONCE(ctx->released, true);
> >         userfaultfd_release_all(mm, ctx);
> >         ...
> > }
> >
> > userfaultfd_release_all()
> > <mmap write lock>
> > -> userfaultfd_clear_vma()
> > -> vma_modify_flags_uffd()
> > -> vma_modify()
> > -> vma_merge_existing_range()   -> vma_start_write() LIVELOCK
> > OR split_vma() -> __split_vma() -> vma_start_write() LIVELOCK
> >
> > So yeah, not sure on this one, maybe we're fine just to use VMA lock still?
> >
> > Suren - any thoughts?
>
> If we retry without falling back to mmap_lock then
> userfaultfd_release() can end up write-locking mmap_lock but losing
> the race for VMA lock to the retrying pagefault handler. This means it
> would block while holding mmap_lock for write - not good.

I was thinking more about this... Writer blocking on a VMA write lock
while holding write side of the mmap_lock is what all VMA modifyers
do, so userfaultfd_release() doing that should be fine. As long as the
usual locking order of taking mmap_lock first then locking VMA is
followed, we should be ok.

> With pagefault handler falling back to mmap_lock, either
> userfaultfd_release() wins the race for mmap_lock or pagefault
> handler, so I think we are safe.
>
> >
> > You definitely need to explain your reasoning in the commit message at
> > least.
> >
> > Also commit 29a22b9e08d7 ("mm: handle userfaults under VMA lock") did fail
> > to update this comment to reflect VMA locks - we should fix that too -
> > Suren? :)
>
> Yes, I will correct my mistake and send a small fixup.
>
> >
> >
> > Also another thought - should we allow FAULT_FLAG_RETRY_NOWAIT to retry
> > with the VMA lock? As this could also be some unbounded spinning? But then
> > again, if we bound the spinning that might be ok.
>
> Hmm. If we do that, let's make it a separate series. Too many moving
> parts is never a good idea.
>
> >
> > Thanks, Lorenzo