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

[Lorenzo Stoakes (ARM)]

On Mon, Sep 21, 2026 at 07:57:23PM +0100, Matthew Wilcox wrote:
> On Mon, Sep 21, 2026 at 06:54:25PM +0100, Lorenzo Stoakes (ARM) wrote:
> > OK I finally got to this...
>
> Brave ;-)

Or stupid? It's a blurry line ;)

>
> > I fear we're going to go round in circles on this/have endless revisions of
> > the same thing.
> >
> > So 2 things the patch needs to answer, more or less right away:
> >
> > 1. What are you actually optimising for?
> >
> > 2. Where does the hurt happen?
> >
> > Your patch does neither, then goes on to arm wave away all the complexity
> > and presents a 'simple' solution that I feel brushes a lot under the rug.
> >
> > Answer to 1 is 'cold app startup time for a specific set of apps'
> > 2 is filemap_fault() AFAICT.
> >
> > You should definitely be stating this upfront.
> >
> > And I also seem to remember that zygote + multi-threaded apps = fork
> > blocking is part of the problem here, which is why there was push-back on
> > just holding the VMA lock over I/O.
> >
> > Looking through filemap_fault() makes me want to cry and looking through
> > the other retry logic in the fault code makes me want to live on an island
> > with parakeets and coconuts forgetting any of this even exists...
>
> I think that's the right response.  It's why all the feedback from people

I mean, it's the instinctive move when you dig into all this again :) my eyes!
Burn it all down etc.

BUT. The other thing that came to mind was 'fix this bloody mess!'.

And I also worry with this patch that we might end up deferring NECESSARY work
to do that.

> who haven't even tried to follow the fault path (eg the session at LSFMM)
> is completely useless.  The fault path is WAY TOO COMPLEX.  I'll have

Yes. YES YES YES.

I was thinking honestly I'd look at this patch and come to the conclusion that
yes it is this simple and now I'm going to have to argue with mm people but
fine.

But no, I was just reminded how bloody complicated it all is.

> more to say on this in two weeks at Plumbers.

I look forward to it :)

>
> > VM_FAULT_RETRY can mean a million different things:
> >
> > 1. I dropped the lock and waited for the folio to be unlocked.
> >
> > do_swap_page(), remove_device_exclusive_entry() -> folio_lock_or_retry()
> >
> > 2. I dropped the lock to start I/O it may or may not be ready when you come
> >    back.
> >
> > filemap_fault() after do_sync_mmap_readahead() or page not uptodate
> > synchronous read with lock dropped <- what you are optimising for, or
> > shmem_falloc_wait() waiting for a hole punch.
> >
> > The maybe_unlock_mmap_for_io() stuff.
>
> I think these two are the same case?  At least by my reading, case 2
> also waits for the folio to be unlocked (ie the read completed).
> do_sync_mmap_readahead() doesn't do that, but we then call
> __filemap_get_folio() which shuld return the appropriate folio,
> then we call __folio_lock_killable() or __folio_lock().

Right yeah.

>
> > 3. I dropped the lock and nothing was waited for, OOM me on retry!
> >
> > filemap_fault() allocation failure, gotta retry because we dropped the
> > lock!
>
> Oh, yeah, that's icky.  Hadn't even crawled onto my list of problems
> with this function.

Yup. Man.

>
> > 4. I can't do this under the VMA lock, use an mmap lock
> >
> > vmf_can_call_fault(), any vm_ops without ->map_pages, __vmf_anon_prepare()
> > when mmap_read_trylock() lost, also hugetlb_fault() horror shows.
> >
> > Also device-private swap entries.
>
> This one really is my fault / Suren's fault / your fault ;-)

Mea culpa... We-a culpa?

>
> We should not have overloaded VM_FAULT_RETRY for this case.  We should
> have had a distinct VM_FAULT_NEEDS_MMAP_LOCK code.  But I am *scared*
> to try to separate the two at this point.  It was hard to make sure we
> got all the cases as we pushed it down, and now it's going to be worse.
>
> But I don't think that hugetlb_fault() deserves any stick here:
>          * We must check to release the per-VMA lock. __vmf_anon_prepare() in
>          * hugetlb_wp() is the only way ret can be set to VM_FAULT_RETRY.
> is the comment on the only two mentions of VM_FAULT_RETRY in hugetlb.

It does feel like we should just have

>
> I do have a plan to get rid of those two mentions actually ...
> but let's talk about in in Prague.

Yes, indeed.

>
> > 5. Userfaultfd! Because of course!
> >
> > Userspace has to do something.
> >
> > handle_userfault() after sleeping for uffd handler, retry should succeed.
> >
> > ctx->released -> yield to releasing thread.
>
> I find it really hard to reason about uffd.

Yeah me also.

The 2 fundamental issues with uffd (aside from code quality) is that:

a. It's the exception to a lot of rules
b. It duplicates its own logic

Similar to hugetlb.

And these problems grow as we add more functionality to mm.

I fear the device private stuff is becoming this a bit (albeit, not as bad).

>
> > 6. Fatal signal
> >
> > Arch handlers rely on this, so retry also means 'check signals'. Fun.
> >
> > 7. Some driver insanity
> >
> > It means all things to all people. Used for waiting on stuff like
> >
> > LLM says:
> >
> > - TTM at drivers/gpu/drm/ttm/ttm_bo_vm.c:62 and 144, dma_resv contention
> >   and GPU idle wait. Consumers in amdgpu, i915, nouveau, radeon and vmwgfx
> >   re-derive "was the reservation unlocked" from ret == VM_FAULT_RETRY &&
> >   !NOWAIT.
> >
> > - xe_bo_cpu_fault_fastpath() at drivers/gpu/drm/xe/xe_bo.c:2021, where
> >   RETRY is the default return value, including for "runtime PM not active".
> >
> > - panthor at drivers/gpu/drm/panthor/panthor_gem.c:831 onwards, where
> >   dma_resv_trylock() failure is RETRY and the same error is NOPAGE, SIGBUS
> >   or RETRY depending on mmap_lock_held.
> >
> > - sgx_vepc_fault() at arch/x86/kernel/cpu/sgx/virt.c:91, EBUSY from
> >   __sgx_vepc_fault().  All of these call mmap_read_unlock() directly rather
> >   than release_fault_lock(). They are only safe because none has
> >   ->map_pages, so vmf_can_call_fault() bounces them before ->fault
> >   runs. The VMA-lock design rests on that proxy.
> >
> > So yeah. All that. Wow.
> >
> > 8. Nothing!
> >
> > LLM says:
> >
> >   - page_mkwrite() returns in fs/exfat/file.c:951 (inode_trylock lost),
>
> That's just wrong.  It turns the pagefault path into a spin on
> the inode lock!

Oh lord. So it's... worse? :)

>
> >     fs/nfs/file.c:699, fs/netfs/buffered_write.c:576 and
> >     fs/orangefs/inode.c:635 onwards, including the combination
> >     VM_FAULT_LOCKED | VM_FAULT_RETRY.
>
> I think your AI is confused.  I don't see how nfs_vm_page_mkwrite()

1st rule of LLMs is they can always hallucinate/get things terrible wrong.

But yeah this was me being lazy but in my defence it was nearly 7pm and I was
still at the office with a long drive ahead of me ;)

> can return VM_FAULT_LOCKED | VM_FAULT_RETRY.  Nor netfs.  Orangefs
> is wrong and needs to be fixed ... once I figure out what it actually
> wants.

Well volunteered :>)

>
> >     do_page_mkwrite() at mm/memory.c only
> >     passes through ERROR and NOPAGE, and do_shared_fault() and
> >     wp_page_shared() then ignore the returned value entirely. The write
> >     fault completes as if page_mkwrite had succeeded.
>
> That's probably OK.  I think?

I think we probably need more flags, honestly. 'Retry' is too vague.

But on the other hand do we want a flag for every possible weird specific caes?

OK OK I'll wait for Prague for the simplification :)


>
> > And maybe I'm missing some stuff too.
>
> There's also the VM_FAULT_COMPLETED insanity ...
>
> > But I worry a LOT more that it's just a HACK (and I know Barry was nice
> > about the idea and I appreciate it but I have to be honest).
> >
> > Instead of dealing with any of the above, we just leave the mess in place +
> > just retry the operation under VMA because we happen to know, for this ONE
> > workload, it works out better.
> >
> > And PROBABLY it doesn't add too much overhead to anything else.
> >
> > SO.
> >
> > I've written too much again, let's sum it up.
> >
> > 1. WE HAVE TO DECIDE whether we want to accept the hack because it
> >    helps in a known case and probably doesn't harm any other cases.
> >
> > 2. WE HAVE TO FIX THIS DAMN MESS. Even if we take something like this WE
> >    HAVE TO FIX IT.
> >
> > I'm inclined to rip out the whole retry thing altogether one way or another
> > but I leave that to Matthew to figure out :)
>
> I think there's a bathtub with a baby in it over there ...

Yup as above, instinct to nuke from orbit etc. ;) but something (TM) needs to be
done.

>
> There's considerable scope for simplification here.
> lock_folio_maybe_drop_mmap() makes my eyeballs bleed every time I look
> at it.  But we do need smoe kind of restart-the-fault mechanism.

Right, yeah.

It does feel like there's a solution lurking here in the undergrowth, so again
Prague etc. etc.

>
> > If we decide we DO want the hack, then DEAR LORD can we not have this
> > horrible duplication across arches? I seem to remember you agreed to take
> > that out ([0]), and maybe it's pending what Matthew wants to do, but is
> > there not a way to avoid that?
>
> We desperately need to move more of the fault handling path out of the
> architectures.

That's definitely part of the solution!

>
> > I REALLY want to hear from Matthew on all this, I don't think we can move
> > ahead without his clear feedback.
>
> Thanks for dragging me back into this ;-)

No worries ;)

Though I do have to ask again - do we want this patch as-is as a temporary
solution? Or maybe just for x86/arm64 (god I hate the duplication of the arch
code)

I'm not sure that it does all that much harm?

It's not necessary either/or it can be an and. But I'd want your OK on taking
this.

--
Cheers, Lorenzo