[PATCH] powerpc: Avoid taking a data miss on every userspace instruction miss

[PATCH] powerpc: Avoid taking a data miss on every userspace instruction miss

[Anton Blanchard]

From: Anton Blanchard <anton@samba.org>

Early on in do_page_fault() we call store_updates_sp(), regardless of
the type of exception. For an instruction miss this doesn't make
sense, because we only use this information to detect if a data miss
is the result of a stack expansion instruction or not.

Worse still, it results in a data miss within every userspace
instruction miss handler, because we try and load the very instruction
we are about to install a pte for!

A simple exec microbenchmark runs 6% faster on POWER8 with this fix:

 #include <stdlib.h>
 #include <stdio.h>
 #include <unistd.h>

int main(int argc, char *argv[])
{
	unsigned long left = atol(argv[1]);
	char leftstr[16];

	if (left-- == 0)
		return 0;

	sprintf(leftstr, "%ld", left);
	execlp(argv[0], argv[0], leftstr, NULL);
	perror("exec failed\n");

	return 0;
}

Pass the number of iterations on the command line (eg 10000) and time
how long it takes to execute.

Signed-off-by: Anton Blanchard <anton@samba.org>
---
 arch/powerpc/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index fd6484fc2fa9..3a7d580fdc59 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -287,7 +287,7 @@ 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 (user_mode(regs))
+	if (!is_exec && user_mode(regs))
 		store_update_sp = store_updates_sp(regs);
 
 	if (user_mode(regs))
-- 
2.11.0

Re: [PATCH] powerpc: Avoid taking a data miss on every userspace instruction miss

[LEROY Christophe]

Anton Blanchard <anton@ozlabs.org> a =C3=A9crit=C2=A0:

> From: Anton Blanchard <anton@samba.org>
>
> Early on in do_page_fault() we call store_updates_sp(), regardless of
> the type of exception. For an instruction miss this doesn't make
> sense, because we only use this information to detect if a data miss
> is the result of a stack expansion instruction or not.
>
> Worse still, it results in a data miss within every userspace
> instruction miss handler, because we try and load the very instruction
> we are about to install a pte for!
>
> A simple exec microbenchmark runs 6% faster on POWER8 with this fix:
>
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <unistd.h>
>
> int main(int argc, char *argv[])
> {
> 	unsigned long left =3D atol(argv[1]);
> 	char leftstr[16];
>
> 	if (left-- =3D=3D 0)
> 		return 0;
>
> 	sprintf(leftstr, "%ld", left);
> 	execlp(argv[0], argv[0], leftstr, NULL);
> 	perror("exec failed\n");
>
> 	return 0;
> }
>
> Pass the number of iterations on the command line (eg 10000) and time
> how long it takes to execute.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
>  arch/powerpc/mm/fault.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index fd6484fc2fa9..3a7d580fdc59 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -287,7 +287,7 @@ int do_page_fault(struct pt_regs *regs, unsigned=20=
=20
>=20long address,
>  	 * can result in fault, which will cause a deadlock when called with
>  	 * mmap_sem held
>  	 */
> -	if (user_mode(regs))
> +	if (!is_exec && user_mode(regs))

Shouldn't it also check 'is_write' ?
If it is a store, is_write should be set, shouldn't it ?

Christophe

>  		store_update_sp =3D store_updates_sp(regs);
>
>  	if (user_mode(regs))
> --
> 2.11.0

Re: [PATCH] powerpc: Avoid taking a data miss on every userspace instruction miss

[Anton Blanchard]

Hi Christophe,

> > -	if (user_mode(regs))
> > +	if (!is_exec && user_mode(regs))  
> 
> Shouldn't it also check 'is_write' ?
> If it is a store, is_write should be set, shouldn't it ?

Thanks, Ben had the same suggestion. I'll add that further optimisation
in a subsequent patch.

Anton

Re: [PATCH] powerpc: Avoid taking a data miss on every userspace instruction miss

[Christophe LEROY]

Hi Anton,

Le 04/04/2017 à 00:00, Anton Blanchard a écrit :
> Hi Christophe,
>
>>> -	if (user_mode(regs))
>>> +	if (!is_exec && user_mode(regs))
>>
>> Shouldn't it also check 'is_write' ?
>> If it is a store, is_write should be set, shouldn't it ?
>
> Thanks, Ben had the same suggestion. I'll add that further optimisation
> in a subsequent patch.
>
> Anton
>

For your information, I made some benchmark test using 'perf stat' with 
your app on MPC8321 and MPC885, and I got the following results:

MPC8321 before the change:

  Performance counter stats for './fault 1000' (10 runs):

        4491.971466      cpu-clock (msec) 
               ( +-  0.03% )
              47386      faults 
               ( +-  0.02% )

        4.727864465 seconds time elapsed 
          ( +-  0.17% )


MPC8321 after your change:

  Performance counter stats for './fault 1000' (10 runs):

        4278.738845      cpu-clock (msec) 
               ( +-  0.02% )
              35181      faults 
               ( +-  0.02% )

        4.504443891 seconds time elapsed 
          ( +-  0.19% )


MPC8321 after changing !is_exec by is_write

  Performance counter stats for './fault 1000' (10 runs):

        4268.187261      cpu-clock (msec) 
               ( +-  0.03% )
              35181      faults 
               ( +-  0.01% )

        4.489207922 seconds time elapsed 
          ( +-  0.20% )





MPC885 before the change:

  Performance counter stats for './fault 500' (10 runs):

          726605854      cpu-cycles 
               ( +-  0.03% )
             176067      dTLB-load-misses 
               ( +-  0.08% )
              52722      iTLB-load-misses 
               ( +-  0.01% )
              25718      faults 
               ( +-  0.03% )

        5.795924654 seconds time elapsed 
          ( +-  0.14% )


MPC885 after your change:

  Performance counter stats for './fault 500' (10 runs):

          711233251      cpu-cycles 
               ( +-  0.04% )
             152462      dTLB-load-misses 
               ( +-  0.09% )
              52715      iTLB-load-misses 
               ( +-  0.01% )
              19611      faults 
               ( +-  0.02% )

        5.673784606 seconds time elapsed 
          ( +-  0.14% )


MPC885 after changing !is_exec by is_write

  Performance counter stats for './fault 500' (10 runs):

          710904083      cpu-cycles 
               ( +-  0.05% )
             147162      dTLB-load-misses 
               ( +-  0.06% )
              52716      iTLB-load-misses 
               ( +-  0.01% )
              19610      faults 
               ( +-  0.02% )

        5.672091139 seconds time elapsed 
          ( +-  0.15% )



Christophe

Re: [PATCH] powerpc: Avoid taking a data miss on every userspace instruction miss

[Michael Ellerman]

Christophe LEROY <christophe.leroy@c-s.fr> writes:

> Hi Anton,
>
> Le 04/04/2017 =C3=A0 00:00, Anton Blanchard a =C3=A9crit :
>> Hi Christophe,
>>
>>>> -	if (user_mode(regs))
>>>> +	if (!is_exec && user_mode(regs))
>>>
>>> Shouldn't it also check 'is_write' ?
>>> If it is a store, is_write should be set, shouldn't it ?
>>
>> Thanks, Ben had the same suggestion. I'll add that further optimisation
>> in a subsequent patch.
>>
>
> For your information, I made some benchmark test using 'perf stat' with=20
> your app on MPC8321 and MPC885, and I got the following results:

 MPC8321:
 before        47386      faults=20
 after         35181      faults        -12205
 is_write      35181      faults        -12205=20

So that's good.

 MPC885:
 before:      176067      dTLB-load-misses=20
               52722      iTLB-load-misses=20
               25718      faults=20
 after:       152462      dTLB-load-misses    -23605
               52715      iTLB-load-misses        -7
               19611      faults               -6107

 is_write:    147162      dTLB-load-misses    -28905
               52716      iTLB-load-misses        -6
               19610      faults               -6108

Also good, and shows that is_write idea would be even better.

cheers

Re: powerpc: Avoid taking a data miss on every userspace instruction miss

[Michael Ellerman]

On Mon, 2017-04-03 at 06:41:02 UTC, Anton Blanchard wrote:
> From: Anton Blanchard <anton@samba.org>
> 
> Early on in do_page_fault() we call store_updates_sp(), regardless of
> the type of exception. For an instruction miss this doesn't make
> sense, because we only use this information to detect if a data miss
> is the result of a stack expansion instruction or not.
> 
> Worse still, it results in a data miss within every userspace
> instruction miss handler, because we try and load the very instruction
> we are about to install a pte for!
> 
> A simple exec microbenchmark runs 6% faster on POWER8 with this fix:
> 
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <unistd.h>
> 
> int main(int argc, char *argv[])
> {
> 	unsigned long left = atol(argv[1]);
> 	char leftstr[16];
> 
> 	if (left-- == 0)
> 		return 0;
> 
> 	sprintf(leftstr, "%ld", left);
> 	execlp(argv[0], argv[0], leftstr, NULL);
> 	perror("exec failed\n");
> 
> 	return 0;
> }
> 
> Pass the number of iterations on the command line (eg 10000) and time
> how long it takes to execute.
> 
> Signed-off-by: Anton Blanchard <anton@samba.org>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/a7a9dcd882a67b68568868b988289f

cheers

Re: powerpc: Avoid taking a data miss on every userspace instruction miss

[Balbir Singh]

On Thu, 2017-04-06 at 23:06 +1000, Michael Ellerman wrote:
> On Mon, 2017-04-03 at 06:41:02 UTC, Anton Blanchard wrote:
> > From: Anton Blanchard <anton@samba.org>
> > 
> Applied to powerpc next, thanks.
> 
> https://git.kernel.org/powerpc/c/a7a9dcd882a67b68568868b988289f
>

FYI: The version you applied does not have checks for is_write

Balbir Singh. 

Re: powerpc: Avoid taking a data miss on every userspace instruction miss

[Anton Blanchard]

Hi Balbir,

> FYI: The version you applied does not have checks for is_write

Yeah, we decided to do that in a follow up patch. I'm ok if someone
gets to it before me :)

Anton

Re: powerpc: Avoid taking a data miss on every userspace instruction miss

[Michael Ellerman]

Balbir Singh <bsingharora@gmail.com> writes:

> On Thu, 2017-04-06 at 23:06 +1000, Michael Ellerman wrote:
>> On Mon, 2017-04-03 at 06:41:02 UTC, Anton Blanchard wrote:
>> > From: Anton Blanchard <anton@samba.org>
>> > 
>> Applied to powerpc next, thanks.
>> 
>> https://git.kernel.org/powerpc/c/a7a9dcd882a67b68568868b988289f
>>
>
> FYI: The version you applied does not have checks for is_write

The version I was sent didn't have it :)

cheers

[PATCH] powerpc: Avoid taking a data miss on every userspace instruction miss

[Anton Blanchard]

From: Anton Blanchard <anton@samba.org>

Early on in do_page_fault() we call store_updates_sp(), regardless of
the type of exception. For an instruction miss this doesn't make
sense, because we only use this information to detect if a data miss
is the result of a stack expansion instruction or not.

Worse still, it results in a data miss within every userspace
instruction miss handler, because we try and load the very instruction
we are about to install a pte for!

A simple exec microbenchmark runs 6% faster on POWER8 with this fix:

int main(int argc, char *argv[])
{
	unsigned long left = atol(argv[1]);
	char leftstr[16];

	if (left-- == 0)
		return 0;

	sprintf(leftstr, "%ld", left);
	execlp(argv[0], argv[0], leftstr, NULL);
	perror("exec failed\n");

	return 0;
}

Signed-off-by: Anton Blanchard <anton@samba.org>
---
 arch/powerpc/mm/fault.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index fd6484fc2fa9..3a7d580fdc59 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -287,7 +287,7 @@ 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 (user_mode(regs))
+	if (!is_exec && user_mode(regs))
 		store_update_sp = store_updates_sp(regs);
 
 	if (user_mode(regs))
-- 
2.11.0