[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <396ef026-c299-6560-fe7c-7b9932164fe3@intel.com>
Date: Fri, 19 Nov 2021 07:04:07 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: Muchun Song <songmuchun@...edance.com>,
dave.hansen@...ux.intel.com, luto@...nel.org, peterz@...radead.org,
tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, hpa@...or.com
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
duanxiongchun@...edance.com, zhengqi.arch@...edance.com
Subject: Re: [RFC PATCH] x86/fault: move might_sleep() out of mmap read lock
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.
Powered by blists - more mailing lists