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

Dave Hansen
2021-11-19T15:04:17Z

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.

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.

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.