On Fri, 2024-08-02 at 13:53 +0100, David Woodhouse wrote:
> 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".
See also "we don't want to stop waiting for a page fault, just because
somebody hit Ctrl-C, but SIGINT has a trivial handler to do some minor
cleanup before exiting so it isn't considered a *fatal* signal".
I'm very sure I'd disagree with that one :)
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
