From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
To: Christophe Leroy <christophe.leroy@c-s.fr>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>,
Scott Wood <oss@buserror.net>
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/5] powerpc/mm: Evaluate user_mode(regs) only once in do_page_fault()
Date: Mon, 24 Apr 2017 14:43:02 +0530 [thread overview]
Message-ID: <87r30i184h.fsf@skywalker.in.ibm.com> (raw)
In-Reply-To: <592ce73ae7cbd2383740ac31e5fed5ca5beac721.1492606298.git.christophe.leroy@c-s.fr>
Christophe Leroy <christophe.leroy@c-s.fr> writes:
> Analysis of the assembly code shows that when using user_mode(regs),
> at least the 'andi.' is redone all the time, and also
> the 'lwz ,132(r31)' most of the time. With the new form, the 'is_user'
> is mapped to cr4, then all further use of is_user results in just
> things like 'beq cr4,218 <do_page_fault+0x218>'
>
> Without the patch:
>
> 50: 81 1e 00 84 lwz r8,132(r30)
> 54: 71 09 40 00 andi. r9,r8,16384
> 58: 40 82 00 0c bne 64 <do_page_fault+0x64>
>
> 84: 81 3e 00 84 lwz r9,132(r30)
> 8c: 71 2a 40 00 andi. r10,r9,16384
> 90: 41 a2 01 64 beq 1f4 <do_page_fault+0x1f4>
>
> d4: 81 3e 00 84 lwz r9,132(r30)
> dc: 71 28 40 00 andi. r8,r9,16384
> e0: 41 82 02 08 beq 2e8 <do_page_fault+0x2e8>
>
> 108: 81 3e 00 84 lwz r9,132(r30)
> 110: 71 28 40 00 andi. r8,r9,16384
> 118: 41 82 02 28 beq 340 <do_page_fault+0x340>
>
> 1e4: 81 3e 00 84 lwz r9,132(r30)
> 1e8: 71 2a 40 00 andi. r10,r9,16384
> 1ec: 40 82 01 68 bne 354 <do_page_fault+0x354>
>
> 228: 81 3e 00 84 lwz r9,132(r30)
> 22c: 71 28 40 00 andi. r8,r9,16384
> 230: 41 82 ff c4 beq 1f4 <do_page_fault+0x1f4>
>
> 288: 71 2a 40 00 andi. r10,r9,16384
> 294: 41 a2 fe 60 beq f4 <do_page_fault+0xf4>
>
> 50c: 81 3e 00 84 lwz r9,132(r30)
> 514: 71 2a 40 00 andi. r10,r9,16384
> 518: 40 a2 fc e0 bne 1f8 <do_page_fault+0x1f8>
>
> 534: 81 3e 00 84 lwz r9,132(r30)
> 53c: 71 2a 40 00 andi. r10,r9,16384
> 540: 41 82 fc b8 beq 1f8 <do_page_fault+0x1f8>
>
> This patch creates a local var called 'is_user' which contains the
> result of user_mode(regs)
>
> With the patch:
>
> 20: 81 03 00 84 lwz r8,132(r3)
> 48: 55 09 97 fe rlwinm r9,r8,18,31,31
> 58: 2e 09 00 00 cmpwi cr4,r9,0
> 5c: 40 92 00 0c bne cr4,68 <do_page_fault+0x68>
>
> 88: 41 b2 01 90 beq cr4,218 <do_page_fault+0x218>
>
> d4: 40 92 01 d0 bne cr4,2a4 <do_page_fault+0x2a4>
>
> 120: 41 b2 00 f8 beq cr4,218 <do_page_fault+0x218>
>
> 138: 41 b2 ff a0 beq cr4,d8 <do_page_fault+0xd8>
>
> 1d4: 40 92 00 e0 bne cr4,2b4 <do_page_fault+0x2b4>
>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/mm/fault.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index b56bf472db6d..8d1639eee3af 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -202,6 +202,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
> int is_write = 0;
> int trap = TRAP(regs);
> int is_exec = trap == 0x400;
> + int is_user = user_mode(regs);
> int fault;
> int rc = 0;
> unsigned int inst = 0;
> @@ -244,7 +245,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
> * The kernel should never take an execute fault nor should it
> * take a page fault to a kernel address.
> */
> - if (!user_mode(regs) && (is_exec || (address >= TASK_SIZE))) {
> + if (!is_user && (is_exec || (address >= TASK_SIZE))) {
> rc = SIGSEGV;
> goto bail;
> }
> @@ -263,7 +264,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
> local_irq_enable();
>
> if (faulthandler_disabled() || mm == NULL) {
> - if (!user_mode(regs)) {
> + if (!is_user) {
> rc = SIGSEGV;
> goto bail;
> }
> @@ -284,10 +285,10 @@ 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 && user_mode(regs))
> + if (is_write && is_user)
> __get_user(inst, (unsigned int __user *)regs->nip);
>
> - if (user_mode(regs))
> + if (is_user)
> flags |= FAULT_FLAG_USER;
>
> /* When running in the kernel we expect faults to occur only to
> @@ -306,7 +307,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
> * thus avoiding the deadlock.
> */
> if (!down_read_trylock(&mm->mmap_sem)) {
> - if (!user_mode(regs) && !search_exception_tables(regs->nip))
> + if (!is_user && !search_exception_tables(regs->nip))
> goto bad_area_nosemaphore;
>
> retry:
> @@ -506,7 +507,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
>
> bad_area_nosemaphore:
> /* User mode accesses cause a SIGSEGV */
> - if (user_mode(regs)) {
> + if (is_user) {
> _exception(SIGSEGV, regs, code, address);
> goto bail;
> }
> --
> 2.12.0
next prev parent reply other threads:[~2017-04-24 9:14 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-19 12:56 [PATCH 0/5] powerpc/mm: some cleanup of do_page_fault() Christophe Leroy
2017-04-19 12:56 ` [PATCH 1/5] powerpc/mm: only call store_updates_sp() on stores in do_page_fault() Christophe Leroy
2017-04-24 9:10 ` Aneesh Kumar K.V
2017-06-05 10:21 ` [1/5] " Michael Ellerman
2017-04-19 12:56 ` [PATCH 2/5] powerpc/mm: split store_updates_sp() in two parts " Christophe Leroy
2017-04-24 9:11 ` Aneesh Kumar K.V
2017-06-02 9:26 ` Michael Ellerman
2017-06-02 9:39 ` Christophe LEROY
2017-06-02 12:11 ` Benjamin Herrenschmidt
2017-06-02 12:31 ` Christophe LEROY
2017-06-05 10:49 ` Michael Ellerman
2017-06-05 10:49 ` Michael Ellerman
2017-06-05 10:45 ` Michael Ellerman
2017-06-05 10:45 ` Michael Ellerman
2017-06-05 17:48 ` christophe leroy
2017-06-06 11:00 ` Michael Ellerman
2017-06-06 11:00 ` Michael Ellerman
2017-06-06 13:29 ` Christophe LEROY
2017-04-19 12:56 ` [PATCH 3/5] powerpc/mm: remove a redundant test " Christophe Leroy
2017-04-19 12:56 ` [PATCH 4/5] powerpc/mm: Evaluate user_mode(regs) only once " Christophe Leroy
2017-04-24 9:13 ` Aneesh Kumar K.V [this message]
2017-04-19 12:56 ` [PATCH 5/5] powerpc/mm: The 8xx doesn't call do_page_fault() for breakpoints Christophe Leroy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87r30i184h.fsf@skywalker.in.ibm.com \
--to=aneesh.kumar@linux.vnet.ibm.com \
--cc=benh@kernel.crashing.org \
--cc=christophe.leroy@c-s.fr \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mpe@ellerman.id.au \
--cc=oss@buserror.net \
--cc=paulus@samba.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.