[PATCH] powerpc/vdso: Avoid link stack corruption in __get_datapage()

The 32 and 64 bit variants of __get_datapage() use a "bcl; mflr" to
determine the loaded address of the VDSO. The current version of these
attempt to use the special bcl variant which avoids pushing to the
link stack.

Unfortunately it uses bcl+8 rather than the required bcl+4. Hence the
current code results in link stack corruption and the resulting
performance degradation (due to branch mis-prediction).

This patch moves us to bcl+4 by moving __kernel_datapage_offset
out of __get_datapage().

With this patch, running the below benchmark we get a bump in
performance on POWER8 for gettimeofday() (which uses
__get_datapage()).

64bit gets ~4% improvement:
  Without patch:
    # ./tb
    time =3D 0.180321
  With patch:
    # ./tb
    time =3D 0.187408

32bit gets ~9% improvement:
  Without patch:
    # ./tb
    time =3D 0.276551
  With patch:
    # ./tb
    time =3D 0.252767

Testcase tb.c (stolen from Anton)
  /* gcc -O2 tb.c -o tb */
  #include <sys/time.h>
  #include <stdio.h>

  int main()
  {
	  int i;

	  struct timeval tv_start, tv_end;

	  gettimeofday(&tv_start, NULL);

	  for(i =3D 0; i < 10000000; i++) {
		  gettimeofday(&tv_end, NULL);
	  }

	  printf("time =3D %.6f\n", tv_end.tv_sec - tv_start.tv_sec + (tv_end.tv_u=
sec - tv_start.tv_usec) * 1e-6);

	  return 0;
  }

Signed-off-by: Michael Neuling <redacted>
Reported-by: Aaron Sawdey <redacted>

diff --git a/arch/powerpc/kernel/vdso32/datapage.S b/arch/powerpc/kernel/vd=

so32/datapage.S
index dc21e89..59cf5f4 100644

--- a/arch/powerpc/kernel/vdso32/datapage.S
+++ b/arch/powerpc/kernel/vdso32/datapage.S

@@ -16,6 +16,10 @@
 #include <asm/vdso.h>

=20
 	.text
+	.global	__kernel_datapage_offset;
+__kernel_datapage_offset:
+	.long	0
+
 V_FUNCTION_BEGIN(__get_datapage)
   .cfi_startproc
 	/* We don't want that exposed or overridable as we want other objects

@@ -27,13 +31,11 @@ V_FUNCTION_BEGIN(__get_datapage)
 	mflr	r0
   .cfi_register lr,r0

=20
-	bcl	20,31,1f
-	.global	__kernel_datapage_offset;
-__kernel_datapage_offset:
-	.long	0
-1:
+	bcl	20,31,data_page_branch
+data_page_branch:
 	mflr	r3
 	mtlr	r0
+	addi	r3, r3, __kernel_datapage_offset-data_page_branch
 	lwz	r0,0(r3)
 	add	r3,r0,r3
 	blr

diff --git a/arch/powerpc/kernel/vdso64/datapage.S b/arch/powerpc/kernel/vd=

so64/datapage.S
index 79796de..2f01c4a 100644

--- a/arch/powerpc/kernel/vdso64/datapage.S
+++ b/arch/powerpc/kernel/vdso64/datapage.S

@@ -16,6 +16,10 @@
 #include <asm/vdso.h>

=20
 	.text
+.global	__kernel_datapage_offset;
+__kernel_datapage_offset:
+	.long	0
+
 V_FUNCTION_BEGIN(__get_datapage)
   .cfi_startproc
 	/* We don't want that exposed or overridable as we want other objects

@@ -27,13 +31,11 @@ V_FUNCTION_BEGIN(__get_datapage)
 	mflr	r0
   .cfi_register lr,r0

=20
-	bcl	20,31,1f
-	.global	__kernel_datapage_offset;
-__kernel_datapage_offset:
-	.long	0
-1:
+	bcl	20,31,data_page_branch
+data_page_branch:
 	mflr	r3
 	mtlr	r0
+	addi	r3, r3, __kernel_datapage_offset-data_page_branch
 	lwz	r0,0(r3)
 	add	r3,r0,r3
 	blr

On 9/23/15, Michael Neuling [off-list ref] wrote:

The 32 and 64 bit variants of __get_datapage() use a "bcl; mflr" to
determine the loaded address of the VDSO. The current version of these
attempt to use the special bcl variant which avoids pushing to the
link stack.

Unfortunately it uses bcl+8 rather than the required bcl+4. Hence the
current code results in link stack corruption and the resulting
performance degradation (due to branch mis-prediction).

This patch moves us to bcl+4 by moving __kernel_datapage_offset
out of __get_datapage().

With this patch, running the below benchmark we get a bump in
performance on POWER8 for gettimeofday() (which uses
__get_datapage()).

64bit gets ~4% improvement:
  Without patch:
    # ./tb
    time = 0.180321
  With patch:
    # ./tb
    time = 0.187408

32bit gets ~9% improvement:
  Without patch:
    # ./tb
    time = 0.276551
  With patch:
    # ./tb
    time = 0.252767

I've got the following results on POWER7 64bit
without the patch:
# ./tb
time = 0.263337
# ./tb
time = 0.251273
# ./tb
time = 0.258453
# ./tb
time = 0.260189

with the patch:
# ./tb
time = 0.241517
# ./tb
time = 0.241973
# ./tb
time = 0.239365
# ./tb
time = 0.240109

One thing that occurred to me was that since the __kernel_datapage_offset
is located immediately before the function, the offset is small and you
could get rid of the addi and just fold it into the lwz.


Aaron Sawdey, Ph.D.  sawdey@us.ibm.com
050-2/C113  (507) 253-7520 TL: 553-7520 home: 507/263-0782
IBM Linux Technology Center - PPC Toolchain



From:	Denis Kirjanov <redacted>
To:	Michael Neuling <redacted>
Cc:	Michael Ellerman <redacted>, benh
            [off-list ref], Aaron Sawdey/Rochester/IBM@IBMUS,
            linuxppc-dev [off-list ref], Anton Blanchard
            [off-list ref], Steve Munroe/Rochester/IBM@IBMUS
Date:	09/23/2015 01:22 PM
Subject:	Re: [PATCH] powerpc/vdso: Avoid link stack corruption in
            __get_datapage()



On 9/23/15, Michael Neuling [off-list ref] wrote:

The 32 and 64 bit variants of __get_datapage() use a "bcl; mflr" to
determine the loaded address of the VDSO. The current version of these
attempt to use the special bcl variant which avoids pushing to the
link stack.

Unfortunately it uses bcl+8 rather than the required bcl+4. Hence the
current code results in link stack corruption and the resulting
performance degradation (due to branch mis-prediction).

This patch moves us to bcl+4 by moving __kernel_datapage_offset
out of __get_datapage().

With this patch, running the below benchmark we get a bump in
performance on POWER8 for gettimeofday() (which uses
__get_datapage()).

64bit gets ~4% improvement:
  Without patch:
    # ./tb
    time = 0.180321
  With patch:
    # ./tb
    time = 0.187408

32bit gets ~9% improvement:
  Without patch:
    # ./tb
    time = 0.276551
  With patch:
    # ./tb
    time = 0.252767

I've got the following results on POWER7 64bit
without the patch:
# ./tb
time = 0.263337
# ./tb
time = 0.251273
# ./tb
time = 0.258453
# ./tb
time = 0.260189

with the patch:
# ./tb
time = 0.241517
# ./tb
time = 0.241973
# ./tb
time = 0.239365
# ./tb
time = 0.240109

Testcase tb.c (stolen from Anton)
  /* gcc -O2 tb.c -o tb */
  #include <sys/time.h>
  #include <stdio.h>

  int main()
  {
		   int i;

		   struct timeval tv_start, tv_end;

		   gettimeofday(&tv_start, NULL);

		   for(i = 0; i < 10000000; i++) {
		 		   gettimeofday(&tv_end, NULL);
		   }

		   printf("time = %.6f\n", tv_end.tv_sec - tv_start.tv_sec +

(tv_end.tv_usec

objects

objects

On 23 September 2015 16:05:02 GMT+10:00, Michael Neuling [off-list ref] wrote:

The 32 and 64 bit variants of __get_datapage() use a "bcl; mflr" to
determine the loaded address of the VDSO. The current version of these
attempt to use the special bcl variant which avoids pushing to the
link stack.

Unfortunately it uses bcl+8 rather than the required bcl+4. Hence the
current code results in link stack corruption and the resulting
performance degradation (due to branch mis-prediction).

This patch moves us to bcl+4 by moving __kernel_datapage_offset
out of __get_datapage().

With this patch, running the below benchmark we get a bump in
performance on POWER8 for gettimeofday() (which uses
__get_datapage()).

64bit gets ~4% improvement:
 Without patch:
   # ./tb
   time = 0.180321
 With patch:
   # ./tb
   time = 0.187408

32bit gets ~9% improvement:
 Without patch:
   # ./tb
   time = 0.276551
 With patch:
   # ./tb
   time = 0.252767

Testcase tb.c (stolen from Anton)
 /* gcc -O2 tb.c -o tb */
 #include <sys/time.h>
 #include <stdio.h>

 int main()
 {
  int i;

  struct timeval tv_start, tv_end;

  gettimeofday(&tv_start, NULL);

  for(i = 0; i < 10000000; i++) {
	  gettimeofday(&tv_end, NULL);
  }

  printf("time = %.6f\n", tv_end.tv_sec - tv_start.tv_sec +
(tv_end.tv_usec - tv_start.tv_usec) * 1e-6);

  return 0;
 }

You know where test cases are supposed to go.

I know it's not a pass/fail test, but it's still useful. If it's in the tree it will get run as part of automated test runs and we will have a record of the result over time.

cheers
-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

I've got the following results on POWER7 64bit
without the patch:
# ./tb
time =3D 0.263337
# ./tb
time =3D 0.251273
# ./tb
time =3D 0.258453
# ./tb
time =3D 0.260189
=20
with the patch:
# ./tb
time =3D 0.241517
# ./tb
time =3D 0.241973
# ./tb
time =3D 0.239365
# ./tb
time =3D 0.240109

Nice, I was expecting a bump there too but didn't test..  Looks like
about 7% to me. =20

Thanks,
Mikey

On 9/24/15, Michael Ellerman [off-list ref] wrote:

On 23 September 2015 16:05:02 GMT+10:00, Michael Neuling [off-list ref]
wrote:

quoted

The 32 and 64 bit variants of __get_datapage() use a "bcl; mflr" to
determine the loaded address of the VDSO. The current version of these
attempt to use the special bcl variant which avoids pushing to the
link stack.

Unfortunately it uses bcl+8 rather than the required bcl+4. Hence the
current code results in link stack corruption and the resulting
performance degradation (due to branch mis-prediction).

This patch moves us to bcl+4 by moving __kernel_datapage_offset
out of __get_datapage().

With this patch, running the below benchmark we get a bump in
performance on POWER8 for gettimeofday() (which uses
__get_datapage()).

64bit gets ~4% improvement:
 Without patch:
   # ./tb
   time = 0.180321
 With patch:
   # ./tb
   time = 0.187408

32bit gets ~9% improvement:
 Without patch:
   # ./tb
   time = 0.276551
 With patch:
   # ./tb
   time = 0.252767

Testcase tb.c (stolen from Anton)
 /* gcc -O2 tb.c -o tb */
 #include <sys/time.h>
 #include <stdio.h>

 int main()
 {
  int i;

  struct timeval tv_start, tv_end;

  gettimeofday(&tv_start, NULL);

  for(i = 0; i < 10000000; i++) {
	  gettimeofday(&tv_end, NULL);
  }

  printf("time = %.6f\n", tv_end.tv_sec - tv_start.tv_sec +
(tv_end.tv_usec - tv_start.tv_usec) * 1e-6);

  return 0;
 }

You know where test cases are supposed to go.

I know it's not a pass/fail test, but it's still useful. If it's in the tree
it will get run as part of automated test runs and we will have a record of
the result over time.

I can send a patch for it.

cheers
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

On Thu, 2015-09-24 at 13:10 +0300, Denis Kirjanov wrote:

On 9/24/15, Michael Ellerman [off-list ref] wrote:

quoted

On 23 September 2015 16:05:02 GMT+10:00, Michael Neuling [off-list ref]
wrote:

quoted

Testcase tb.c (stolen from Anton)
 /* gcc -O2 tb.c -o tb */
 #include <sys/time.h>
 #include <stdio.h>

 int main()
 {
  int i;

  struct timeval tv_start, tv_end;

  gettimeofday(&tv_start, NULL);

  for(i = 0; i < 10000000; i++) {
	  gettimeofday(&tv_end, NULL);
  }

  printf("time = %.6f\n", tv_end.tv_sec - tv_start.tv_sec +
(tv_end.tv_usec - tv_start.tv_usec) * 1e-6);

  return 0;
 }

You know where test cases are supposed to go.

I know it's not a pass/fail test, but it's still useful. If it's in the tree
it will get run as part of automated test runs and we will have a record of
the result over time.

I can send a patch for it.

Mikey was working on it I think.

cheers