Re: [PATCH] KVM: x86: Use gfn_to_pfn_cache for steal_time

[David Woodhouse]

On Fri, 2024-08-02 at 13:38 +0100, Matthew Wilcox wrote:
> On Fri, Aug 02, 2024 at 01:03:16PM +0100, David Woodhouse wrote:
> > On Fri, 2024-08-02 at 11:44 +0000, Carsten Stollmaier wrote:
> > > handle_userfault uses TASK_INTERRUPTIBLE, so it is interruptible by
> > > signals. do_user_addr_fault then busy-retries it if the pending signal
> > > is non-fatal. This leads to contention of the mmap_lock.
> 
> Why does handle_userfault use TASK_INTERRUPTIBLE?  We really don't
> want to stop handling a page fault just because somebody resized a
> window or a timer went off.  TASK_KILLABLE, sure.

Well, the literal answer there in this case is "because we ask it to".

The handle_userfault() function will literally do what it's told by the
fault flags: 

static inline unsigned int userfaultfd_get_blocking_state(unsigned int flags)
{
	if (flags & FAULT_FLAG_INTERRUPTIBLE)
		return TASK_INTERRUPTIBLE;

	if (flags & FAULT_FLAG_KILLABLE)
		return TASK_KILLABLE;

	return TASK_UNINTERRUPTIBLE;
}


Hence the other potential workaround I mentioned, for
do_user_addr_fault() *not* to ask it to, for faults from the kernel:

> > 
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1304,6 +1304,8 @@ void do_user_addr_fault(struct pt_regs *regs,
> >          */
> >         if (user_mode(regs))
> >                 flags |= FAULT_FLAG_USER;
> > +       else
> > +               flags &= ~FAULT_FLAG_INTERRUPTIBLE;
> >  
> >  #ifdef CONFIG_X86_64
> >         /*
> > 


But I don't know that I agree with your statement above, that we "don't
want to stop handling a page fault just because somebody resized a
window or a timer went off". 

In fact, I don't think we *do* even stop handling the page fault in
those cases; we just stop *waiting* for it to be handled. 

In fact, couldn't you contrive a test case where a thread is handling
its own uffd faults via SIGIO, where it's the opposite of what you say.
In that case the *only* way the fault actually gets handled is if we
let the signal happen instead of just waiting? 

That doesn't seem like *such* a contrived case either — that seems
perfectly reasonable for a vCPU thread, to then handle its own missing
pages?

Attachment: smime.p7s
Description: S/MIME cryptographic signature