On Thu, Nov 27, 2025 at 09:10:56AM -0500, Peter Xu wrote:
> On Thu, Nov 27, 2025 at 01:18:10PM +0200, Mike Rapoport wrote:
> > On Tue, Nov 25, 2025 at 02:21:16PM -0500, Peter Xu wrote:
> > > Hi, Mike,
> > >
> > > On Tue, Nov 25, 2025 at 08:38:38PM +0200, Mike Rapoport wrote:
> > > > From: "Mike Rapoport (Microsoft)" <[email protected]>
> > > >
> > > > When a VMA is registered with userfaulfd in minor mode, its ->fault()
> > > > method should check if a folio exists in the page cache and if yes
> > > > ->fault() should call handle_userfault(VM_UFFD_MISSING).
> > >
> > > s/MISSING/MINOR/
> >
> > Thanks, fixed.
> >
> > > > new VM_FAULT_UFFD_MINOR there instead.
> > >
> > > Personally I'd keep the fault path as simple as possible, because that's
> > > the more frequently used path (rather than when userfaultfd is armed). I
> > > also see it slightly a pity that even with flags introduced, it only
> > > solves
> > > the MINOR problem, not MISSING.
> >
> > With David's suggestion the likely path remains unchanged.
>
> It is not about the likely, it's about introducing flags into core path
> that makes the core path harder to follow, when it's not strictly required.
ret = vma->vm_ops->fault(vmf);
if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE | VM_FAULT_RETRY |
VM_FAULT_DONE_COW | VM_FAULT_UFFD_MINOR))) {
if (ret & VM_FAULT_UFFD_MINOR)
return handle_userfault(vmf, VM_UFFD_MINOR);
return ret;
}
isn't hard to follow and it's cleaner than adding EXPORT_SYMBOL that is not
strictly required.
> Meanwhile, personally I'm also not sure if we should have "unlikely" here..
> My gut feeling is in reality we will only have two major use cases:
>
> (a) when userfaultfd minor isn't in the picture
>
> (b) when userfaultfd minor registered and actively being used (e.g. in a
> postcopy process)
>
> Then without likely, IIUC the hardware should optimize path selected hence
> both a+b performs almost equally well.
unlikely() adds a branch that hardware will predict correctly if
UFFD_MINOR is actively used.
But even misspredicted branch is nothing compared to putting a task on a
wait queue and waiting for userspace to react to the fault notification
before handle_userfault() returns the control to the fault handler.
> Just to mention, if we want, I think we have at least one more option to do
> the same thing, but without even introducing a new flag to ->fault()
> retval.
>
> That is, when we have get_folio() around, we can essentially do two faults
> in sequence, one lighter then the real one, only for minor vmas, something
> like (I didn't think deeper, so only a rough idea shown):
>
> __do_fault():
> if (uffd_minor(vma)) {
> ...
> folio = vma->get_folio(...);
> if (folio)
> return handle_userfault(vmf, VM_UFFD_MINOR);
> // fallthrough, which imply a cache miss
> }
> ret = vma->vm_ops->fault(vmf);
That's something to consider for the future, especially if we'd be able to
pull out MISSING handling as well from ->fault() handlers.
> Thanks,
> --
> Peter Xu
--
Sincerely yours,
Mike.
