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

[Dave Hansen <dave.hansen@intel.com>]

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.