From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754171AbeEWHRV (ORCPT ); Wed, 23 May 2018 03:17:21 -0400 Received: from mail-pf0-f193.google.com ([209.85.192.193]:44196 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753985AbeEWHRU (ORCPT ); Wed, 23 May 2018 03:17:20 -0400 X-Google-Smtp-Source: AB8JxZrQffYkNcdKfLL0USNSMSYUxLApjzt3GOa0B3q0J2W9NFnNZu/sN5AG4bSGWIWwfNS/7PTvXw== Date: Wed, 23 May 2018 17:17:10 +1000 From: Nicholas Piggin To: Christophe Leroy Cc: Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v8] powerpc/mm: Only read faulting instruction when necessary in do_page_fault() Message-ID: <20180523171710.00792240@roar.ozlabs.ibm.com> In-Reply-To: <61ef5847cedae91eb5415f40fb4276585950cb43.1527058032.git.christophe.leroy@c-s.fr> References: <61ef5847cedae91eb5415f40fb4276585950cb43.1527058032.git.christophe.leroy@c-s.fr> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 23 May 2018 09:01:19 +0200 (CEST) Christophe Leroy wrote: > Commit a7a9dcd882a67 ("powerpc: Avoid taking a data miss on every > userspace instruction miss") has shown that limiting the read of > faulting instruction to likely cases improves performance. > > This patch goes further into this direction by limiting the read > of the faulting instruction to the only cases where it is likely > needed. > > On an MPC885, with the same benchmark app as in the commit referred > above, we see a reduction of about 3900 dTLB misses (approx 3%): > > Before the patch: > Performance counter stats for './fault 500' (10 runs): > > 683033312 cpu-cycles ( +- 0.03% ) > 134538 dTLB-load-misses ( +- 0.03% ) > 46099 iTLB-load-misses ( +- 0.02% ) > 19681 faults ( +- 0.02% ) > > 5.389747878 seconds time elapsed ( +- 0.06% ) > > With the patch: > > Performance counter stats for './fault 500' (10 runs): > > 682112862 cpu-cycles ( +- 0.03% ) > 130619 dTLB-load-misses ( +- 0.03% ) > 46073 iTLB-load-misses ( +- 0.05% ) > 19681 faults ( +- 0.01% ) > > 5.381342641 seconds time elapsed ( +- 0.07% ) > > The proper work of the huge stack expansion was tested with the > following app: > > int main(int argc, char **argv) > { > char buf[1024 * 1025]; > > sprintf(buf, "Hello world !\n"); > printf(buf); > > exit(0); > } > > Signed-off-by: Christophe Leroy > --- > v8: Back to a single patch as it now makes no sense to split the first part in two. The third patch has no > dependencies with the ones before, so it will be resend independantly. As suggested by Nicholas, the > patch now does the get_user() stuff inside bad_stack_expansion(), that's a mid way between v5 and v7. > > v7: Following comment from Nicholas on v6 on possibility of the page getting removed from the pagetables > between the fault and the read, I have reworked the patch in order to do the get_user() in > __do_page_fault() directly in order to reduce complexity compared to version v5 > > v6: Rebased on latest powerpc/merge branch ; Using __get_user_inatomic() instead of get_user() in order > to move it inside the semaphored area. That removes all the complexity of the patch. > > v5: Reworked to fit after Benh do_fault improvement and rebased on top of powerpc/merge (65152902e43fef) > > v4: Rebased on top of powerpc/next (f718d426d7e42e) and doing access_ok() verification before __get_user_xxx() > > v3: Do a first try with pagefault disabled before releasing the semaphore > > v2: Changes 'if (cond1) if (cond2)' by 'if (cond1 && cond2)' > > arch/powerpc/mm/fault.c | 63 +++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 45 insertions(+), 18 deletions(-) > > diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c > index 0c99f9b45e8f..7f9363879f4a 100644 > --- a/arch/powerpc/mm/fault.c > +++ b/arch/powerpc/mm/fault.c > @@ -66,15 +66,11 @@ static inline bool notify_page_fault(struct pt_regs *regs) > } > > /* > - * Check whether the instruction at regs->nip is a store using > + * Check whether the instruction inst is a store using > * an update addressing form which will update r1. > */ > -static bool store_updates_sp(struct pt_regs *regs) > +static bool store_updates_sp(unsigned int inst) > { > - unsigned int inst; > - > - if (get_user(inst, (unsigned int __user *)regs->nip)) > - return false; > /* check for 1 in the rA field */ > if (((inst >> 16) & 0x1f) != 1) > return false; > @@ -233,9 +229,10 @@ static bool bad_kernel_fault(bool is_exec, unsigned long error_code, > return is_exec || (address >= TASK_SIZE); > } > > -static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, > - struct vm_area_struct *vma, > - bool store_update_sp) > +/* Return value is true if bad (sem. released), false if good, -1 for retry */ > +static int bad_stack_expansion(struct pt_regs *regs, unsigned long address, > + struct vm_area_struct *vma, unsigned int flags, > + bool is_retry) > { > /* > * N.B. The POWER/Open ABI allows programs to access up to > @@ -247,10 +244,15 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, > * expand to 1MB without further checks. > */ > if (address + 0x100000 < vma->vm_end) { > + struct mm_struct *mm = current->mm; > + unsigned int __user *nip = (unsigned int __user *)regs->nip; > + unsigned int inst; > /* get user regs even if this fault is in kernel mode */ > struct pt_regs *uregs = current->thread.regs; > - if (uregs == NULL) > + if (uregs == NULL) { > + up_read(&mm->mmap_sem); > return true; > + } > > /* > * A user-mode access to an address a long way below > @@ -264,8 +266,30 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address, > * between the last mapped region and the stack will > * expand the stack rather than segfaulting. > */ > - if (address + 2048 < uregs->gpr[1] && !store_update_sp) > - return true; > + if (address + 2048 >= uregs->gpr[1]) > + return false; > + if (is_retry) > + return false; > + > + if ((flags & FAULT_FLAG_WRITE) && (flags & FAULT_FLAG_USER) && > + access_ok(VERIFY_READ, nip, sizeof(inst))) { > + int res; > + > + pagefault_disable(); > + res = __get_user_inatomic(inst, nip); > + pagefault_enable(); > + if (res) { > + up_read(&mm->mmap_sem); > + res = __get_user(inst, nip); > + if (!res && store_updates_sp(inst)) > + return -1; > + return true; > + } > + if (store_updates_sp(inst)) > + return false; > + } > + up_read(&mm->mmap_sem); Starting to look pretty good... I think probably I prefer the mmap_sem drop going into the caller so we don't don't drop in the child function. I thought the retry logic was a little bit complex too, what do you think of using fault_in_pages_readable and just doing a full retry to avoid some of this complexity? > + return true; > } > return false; > } > @@ -403,7 +427,8 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, > int is_user = user_mode(regs); > int is_write = page_fault_is_write(error_code); > int fault, major = 0; > - bool store_update_sp = false; > + bool is_retry = false; > + int is_bad; > > if (notify_page_fault(regs)) > return 0; > @@ -454,9 +479,6 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, > * can result in fault, which will cause a deadlock when called with > * mmap_sem held > */ > - if (is_write && is_user) > - store_update_sp = store_updates_sp(regs); > - > if (is_user) > flags |= FAULT_FLAG_USER; > if (is_write) > @@ -503,8 +525,13 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, > return bad_area(regs, address); > > /* The stack is being expanded, check if it's valid */ > - if (unlikely(bad_stack_expansion(regs, address, vma, store_update_sp))) > - return bad_area(regs, address); > + is_bad = bad_stack_expansion(regs, address, vma, flags, is_retry); > + if (unlikely(is_bad == -1)) { > + is_retry = true; > + goto retry; > + } > + if (unlikely(is_bad)) > + return bad_area_nosemaphore(regs, address); Suggest making the return so that you can do a single unlikely test for the retry or bad case, and then distinguish the retry in there. Code generation should be better. Thanks, Nick