Re: [PATCH v2 3/5] mm: introduce VM_FAULT_UFFD_MINOR fault reason

[Mike Rapoport <rppt@kernel.org>]

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)" <rppt@kernel.org>
> > > > 
> > > > 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.