Re: [RFC PATCH] x86/fault: move might_sleep() out of mmap read lock

Muchun Song
2021-11-22T07:00:36Z

On Fri, Nov 19, 2021 at 11:04 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 11/18/21 10:58 PM, Muchun Song wrote:
> > The mmap lock is supposed to be a contended lock sometimes, scheduling
> > to other task with holding mmap read lock does not seems to be a wise
> > choice. So move it to the front of mmap_read_trylock(). Although
> > mmap_read_lock() implies a might_sleep(), I think redundant check is
> > not a problem since this task is about to sleep and it is not a hot
> > path.
>
> This justification doesn't really do it for me. There are a billion
> ways to sleep in the fault path while the mmap lock is held. Adding one
> more cond_resched() doesn't seem like it would do much.

I agree with you that there are lots of ways to sleep in the
fault path. Just try my best to not sleep with mmap lock.

>
> In other words, I don't think there's a performance justification here.
>
> That said...
>
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 4bfed53e210e..22fd1dfafa3d 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1323,6 +1323,8 @@ void do_user_addr_fault(struct pt_regs *regs,
> > }
> > #endif
> >
> > + might_sleep();
> > +
> > /*
> > * Kernel-mode access to the user address space should only occur
> > * on well-defined single instructions listed in the exception
> > @@ -1346,13 +1348,6 @@ void do_user_addr_fault(struct pt_regs *regs,
> > }
> > retry:
> > mmap_read_lock(mm);
> > - } else {
> > - /*
> > - * The above down_read_trylock() might have succeeded in
> > - * which case we'll have missed the might_sleep() from
> > - * down_read():
> > - */
> > - might_sleep();
> > }
> >
> > vma = find_vma(mm, address);
>
> The comment is stale, which isn't great. The might_sleep() is already
> in the fast path. So, moving it up above makes a lot of sense just in
> terms of simplicity.

Without this patch, I didn't see the might_sleep() in the fast path. What
am I missing here?

Thanks.

>
> The might_sleep() does need a comment, though, about what it's _doing_
> there.
>
> So, right idea, good result, but for the wrong reasons.
>
> If you want to revise the justification and add a comment, I think this
> is something we can take.