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

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

[Michael Neuling]

This patch fixes a link stack corruption issue in the VDOS
get_datapage() code and adds a benchmark to test it in future.

v2:
  - Split benchmark out of commit message and put in selftests.
  - Upgrade commit message to essay.

[PATCH v2 1/2] powerpc/selftest: Add gettimeofday() benchmark

[Michael Neuling]

This adds a benchmark directory to the powerpc selftests and adds a
gettimeofday() benchmark to it.

Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Michael Neuling <mikey@neuling.org>
---
 tools/testing/selftests/powerpc/Makefile           |  2 +-
 .../testing/selftests/powerpc/benchmarks/Makefile  | 12 +++++++++
 .../selftests/powerpc/benchmarks/gettimeofday.c    | 31 ++++++++++++++++++++++
 3 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/benchmarks/Makefile
 create mode 100644 tools/testing/selftests/powerpc/benchmarks/gettimeofday.c

diff --git a/tools/testing/selftests/powerpc/Makefile b/tools/testing/selftests/powerpc/Makefile
index 03ca2e6..847adf6 100644
--- a/tools/testing/selftests/powerpc/Makefile
+++ b/tools/testing/selftests/powerpc/Makefile
@@ -12,7 +12,7 @@ CFLAGS := -Wall -O2 -flto -Wall -Werror -DGIT_VERSION='"$(GIT_VERSION)"' -I$(CUR
 
 export CFLAGS
 
-SUB_DIRS = pmu copyloops mm tm primitives stringloops vphn switch_endian dscr
+SUB_DIRS = pmu copyloops mm tm primitives stringloops vphn switch_endian dscr benchmarks
 
 endif
 
diff --git a/tools/testing/selftests/powerpc/benchmarks/Makefile b/tools/testing/selftests/powerpc/benchmarks/Makefile
new file mode 100644
index 0000000..5fa4870
--- /dev/null
+++ b/tools/testing/selftests/powerpc/benchmarks/Makefile
@@ -0,0 +1,12 @@
+TEST_PROGS := gettimeofday
+
+CFLAGS += -O2
+
+all: $(TEST_PROGS)
+
+$(TEST_PROGS): ../harness.c
+
+include ../../lib.mk
+
+clean:
+	rm -f $(TEST_PROGS) *.o
diff --git a/tools/testing/selftests/powerpc/benchmarks/gettimeofday.c b/tools/testing/selftests/powerpc/benchmarks/gettimeofday.c
new file mode 100644
index 0000000..3af3c21
--- /dev/null
+++ b/tools/testing/selftests/powerpc/benchmarks/gettimeofday.c
@@ -0,0 +1,31 @@
+/*
+ * Copyright 2015, Anton Blanchard, IBM Corp.
+ * Licensed under GPLv2.
+ */
+
+#include <sys/time.h>
+#include <stdio.h>
+
+#include "utils.h"
+
+static int test_gettimeofday(void)
+{
+	int i;
+
+	struct timeval tv_start, tv_end;
+
+	gettimeofday(&tv_start, NULL);
+
+	for(i = 0; i < 100000000; 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;
+}
+
+int main(void)
+{
+	return test_harness(test_gettimeofday, "gettimeofday");
+}
-- 
2.1.4

Re: [PATCH v2 1/2] powerpc/selftest: Add gettimeofday() benchmark

[Arnd Bergmann]

On Friday 25 September 2015 14:01:39 Michael Neuling wrote:
> This adds a benchmark directory to the powerpc selftests and adds a
> gettimeofday() benchmark to it.
> 
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> 

Any reason for keeping this powerpc specific? It seems generally useful.
and portable.

	Arnd

Re: [PATCH v2 1/2] powerpc/selftest: Add gettimeofday() benchmark

[Denis Kirjanov]

On 9/25/15, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 25 September 2015 14:01:39 Michael Neuling wrote:
>> This adds a benchmark directory to the powerpc selftests and adds a
>> gettimeofday() benchmark to it.
>>
>> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> Signed-off-by: Michael Neuling <mikey@neuling.org>
>>
>
> Any reason for keeping this powerpc specific? It seems generally useful.
> and portable.
You're right. Moreover, we can put some comment to the benchmark why
we've made such decision to add it (reference to the commit
"powerpc/vdso: Avoid link stack corruption in __get_datapage()")

>
> 	Arnd
>

Re: [PATCH v2 1/2] powerpc/selftest: Add gettimeofday() benchmark

[Gabriel Paubert]

On Fri, Sep 25, 2015 at 12:28:30PM +0300, Denis Kirjanov wrote:
> On 9/25/15, Arnd Bergmann <arnd@arndb.de> wrote:
> > On Friday 25 September 2015 14:01:39 Michael Neuling wrote:
> >> This adds a benchmark directory to the powerpc selftests and adds a
> >> gettimeofday() benchmark to it.
> >>
> >> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> >> Signed-off-by: Michael Neuling <mikey@neuling.org>
> >>
> >
> > Any reason for keeping this powerpc specific? It seems generally useful.
> > and portable.
> You're right. Moreover, we can put some comment to the benchmark why
> we've made such decision to add it (reference to the commit
> "powerpc/vdso: Avoid link stack corruption in __get_datapage()")

Why gettimeofday? Isn't clock_gettime the modern variant?

BTW: dows anyone receive 2 copies of every messge in this thread ?

I do, and I suspect that this is due to the Cc: list having both
linuxppc-dev@ozlabs.org and linuxppc-dev@lists.ozlabs.org. I removed the
former for this reply.

    Gabriel

Re: [PATCH v2 1/2] powerpc/selftest: Add gettimeofday() benchmark

[Denis Kirjanov]

On 9/25/15, Gabriel Paubert <paubert@iram.es> wrote:
> On Fri, Sep 25, 2015 at 12:28:30PM +0300, Denis Kirjanov wrote:
>> On 9/25/15, Arnd Bergmann <arnd@arndb.de> wrote:
>> > On Friday 25 September 2015 14:01:39 Michael Neuling wrote:
>> >> This adds a benchmark directory to the powerpc selftests and adds a
>> >> gettimeofday() benchmark to it.
>> >>
>> >> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> >> Signed-off-by: Michael Neuling <mikey@neuling.org>
>> >>
>> >
>> > Any reason for keeping this powerpc specific? It seems generally
>> > useful.
>> > and portable.
>> You're right. Moreover, we can put some comment to the benchmark why
>> we've made such decision to add it (reference to the commit
>> "powerpc/vdso: Avoid link stack corruption in __get_datapage()")
>
> Why gettimeofday? Isn't clock_gettime the modern variant?

Mostly, I think, for historical reasons. Both of them are implemented
as vsyscalls (on ppc, ) so why bother?

BTW: Found an interesting vdso testsuite on github:
https://github.com/nlynch-mentor/vdsotest under GPL. Not sure if we
really need such similar thing in the kernel selftest sources.

>
> BTW: dows anyone receive 2 copies of every messge in this thread ?
>
> I do, and I suspect that this is due to the Cc: list having both
> linuxppc-dev@ozlabs.org and linuxppc-dev@lists.ozlabs.org. I removed the
> former for this reply.
>
>     Gabriel
>

Re: [PATCH v2 1/2] powerpc/selftest: Add gettimeofday() benchmark

[Michael Neuling]

On Fri, 2015-09-25 at 11:37 +0200, Gabriel Paubert wrote:
> On Fri, Sep 25, 2015 at 12:28:30PM +0300, Denis Kirjanov wrote:
> > On 9/25/15, Arnd Bergmann <arnd@arndb.de> wrote:
> > > On Friday 25 September 2015 14:01:39 Michael Neuling wrote:
> > >> This adds a benchmark directory to the powerpc selftests and adds a
> > >> gettimeofday() benchmark to it.
> > >>
> > >> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> > >> Signed-off-by: Michael Neuling <mikey@neuling.org>
> > >>
> > >
> > > Any reason for keeping this powerpc specific? It seems generally usef=
ul.
> > > and portable.
> > You're right. Moreover, we can put some comment to the benchmark why
> > we've made such decision to add it (reference to the commit
> > "powerpc/vdso: Avoid link stack corruption in __get_datapage()")
>=20
> Why gettimeofday? Isn't clock_gettime the modern variant?

We can do either here really.  I just wanted to test __get_datapage()
really.  I just stole the benchmark from Anton, which turned into mpe
wanting it in selftests.  I dumped it in powerpc mostly because I knew
mpe would take it (ie. I'm was being lazy).

> BTW: dows anyone receive 2 copies of every messge in this thread ?

I'm not.

Mikey

>=20
> I do, and I suspect that this is due to the Cc: list having both
> linuxppc-dev@ozlabs.org and linuxppc-dev@lists.ozlabs.org. I removed the
> former for this reply.
>=20
>     Gabriel

Re: [PATCH v2 1/2] powerpc/selftest: Add gettimeofday() benchmark

[Denis Kirjanov]

On 9/28/15, Michael Neuling <mikey@neuling.org> wrote:
> On Fri, 2015-09-25 at 11:37 +0200, Gabriel Paubert wrote:
>> On Fri, Sep 25, 2015 at 12:28:30PM +0300, Denis Kirjanov wrote:
>> > On 9/25/15, Arnd Bergmann <arnd@arndb.de> wrote:
>> > > On Friday 25 September 2015 14:01:39 Michael Neuling wrote:
>> > >> This adds a benchmark directory to the powerpc selftests and adds a
>> > >> gettimeofday() benchmark to it.
>> > >>
>> > >> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
>> > >> Signed-off-by: Michael Neuling <mikey@neuling.org>
>> > >>
>> > >
>> > > Any reason for keeping this powerpc specific? It seems generally
>> > > useful.
>> > > and portable.
>> > You're right. Moreover, we can put some comment to the benchmark why
>> > we've made such decision to add it (reference to the commit
>> > "powerpc/vdso: Avoid link stack corruption in __get_datapage()")
>>
>> Why gettimeofday? Isn't clock_gettime the modern variant?
>
> We can do either here really.  I just wanted to test __get_datapage()
> really.  I just stole the benchmark from Anton, which turned into mpe
> wanting it in selftests.  I dumped it in powerpc mostly because I knew
> mpe would take it (ie. I'm was being lazy).

Yeah, so the quickest way would be to apply the patch as is. Then
someone can make it generic.
So now it depends on Michael's decision :)
>
>> BTW: dows anyone receive 2 copies of every messge in this thread ?
>
> I'm not.
>
> Mikey
>
>>
>> I do, and I suspect that this is due to the Cc: list having both
>> linuxppc-dev@ozlabs.org and linuxppc-dev@lists.ozlabs.org. I removed the
>> former for this reply.
>>
>>     Gabriel
>
>

Re: [PATCH v2 1/2] powerpc/selftest: Add gettimeofday() benchmark

[Michael Ellerman]

On Mon, 2015-09-28 at 12:56 +0300, Denis Kirjanov wrote:
> On 9/28/15, Michael Neuling <mikey@neuling.org> wrote:
> > On Fri, 2015-09-25 at 11:37 +0200, Gabriel Paubert wrote:
> >> On Fri, Sep 25, 2015 at 12:28:30PM +0300, Denis Kirjanov wrote:
> >> > On 9/25/15, Arnd Bergmann <arnd@arndb.de> wrote:
> >> > > On Friday 25 September 2015 14:01:39 Michael Neuling wrote:
> >> > >> This adds a benchmark directory to the powerpc selftests and adds a
> >> > >> gettimeofday() benchmark to it.
> >> > >>
> >> > >> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> >> > >> Signed-off-by: Michael Neuling <mikey@neuling.org>
> >> > >>
> >> > >
> >> > > Any reason for keeping this powerpc specific? It seems generally
> >> > > useful.
> >> > > and portable.
> >> > You're right. Moreover, we can put some comment to the benchmark why
> >> > we've made such decision to add it (reference to the commit
> >> > "powerpc/vdso: Avoid link stack corruption in __get_datapage()")
> >>
> >> Why gettimeofday? Isn't clock_gettime the modern variant?
> >
> > We can do either here really.  I just wanted to test __get_datapage()
> > really.  I just stole the benchmark from Anton, which turned into mpe
> > wanting it in selftests.  I dumped it in powerpc mostly because I knew
> > mpe would take it (ie. I'm was being lazy).
> 
> Yeah, so the quickest way would be to apply the patch as is. Then
> someone can make it generic.

> So now it depends on Michael's decision :)

Yeah I'll merge it and then folks can do whatever they like.

cheers

Re: [v2,1/2] powerpc/selftest: Add gettimeofday() benchmark

[Michael Ellerman]

On Fri, 2015-25-09 at 04:01:39 UTC, Michael Neuling wrote:
> This adds a benchmark directory to the powerpc selftests and adds a
> gettimeofday() benchmark to it.
> 
> Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Michael Neuling <mikey@neuling.org>

Applied to powerpc next, thanks.

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

cheers

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

[Michael Neuling]

powerpc has a link register (lr) used for calling functions. We "bl
<func>" to call a function, and "blr" to return back to the call site.

The lr is only a single register, so if we call another function from
inside this function (ie. nested calls), software must save away the
lr on the software stack before calling the new function. Before
returning (ie. before the "blr"), the lr is restored by software from
the software stack.

This makes branch prediction quite difficult for the processor as it
will only know the branch target just before the "blr".

To help with this, modern powerpc processors keep a (non-architected)
hardware stack of lr called a "link stack". When a "bl <func>" is
run, the lr is pushed onto this stack. When a "blr" is called, the
branch predictor pops the lr value from the top of the link stack, and
uses it to predict the branch target. Hence the processor pipeline
knows a lot earlier the branch target.

This works great but there are some cases where you call "bl" but
without a matching "blr". Once such case is when trying to determine
the program counter (which can't be read directly). Here you "bl+4;
mflr" to get the program counter. If you do this, the link stack will
get out of sync with reality, causing the branch predictor to
mis-predict subsequent function returns.

To avoid this, modern micro-architectures have a special case of bl.
Using the form "bcl 20,31,+4", ensures the processor doesn't push to
the link stack.

The 32 and 64 bit variants of __get_datapage() use a "bl; mflr" to
determine the loaded address of the VDSO. The current versions of
these attempt to use this special bl variant.

Unfortunately they use +8 rather than the required +4. Hence the
current code results in the link stack getting out of sync with
reality and hence the resulting performance degradation.

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

With this patch, running a gettimeofday() (which uses
__get_datapage()) microbenchmark we get a decent bump in performance
on POWER7/8.

For the benchmark in tools/testing/selftests/powerpc/benchmarks/gettimeofday.c
  POWER8:
    64bit gets ~4% improvement
    32bit gets ~9% improvement
  POWER7:
    64bit gets ~7% improvement

Signed-off-by: Michael Neuling <mikey@neuling.org>
Reported-by: Aaron Sawdey <sawdey@us.ibm.com>

---
Giant-commit-log-essay-suggested-by: mpe
---
 arch/powerpc/kernel/vdso32/datapage.S | 12 +++++++-----
 arch/powerpc/kernel/vdso64/datapage.S | 12 +++++++-----
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/kernel/vdso32/datapage.S b/arch/powerpc/kernel/vdso32/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>
 
 	.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
 
-	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/vdso64/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>
 
 	.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
 
-	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
-- 
2.1.4

Re: [v2, 2/2] powerpc/vdso: Avoid link stack corruption in __get_datapage()

[Michael Ellerman]

On Fri, 2015-25-09 at 04:01:40 UTC, Michael Neuling wrote:
> powerpc has a link register (lr) used for calling functions. We "bl
> <func>" to call a function, and "blr" to return back to the call site.

<snip, geez that was long>

> For the benchmark in tools/testing/selftests/powerpc/benchmarks/gettimeofday.c
>   POWER8:
>     64bit gets ~4% improvement
>     32bit gets ~9% improvement
>   POWER7:
>     64bit gets ~7% improvement
> 
> Signed-off-by: Michael Neuling <mikey@neuling.org>
> Reported-by: Aaron Sawdey <sawdey@us.ibm.com>

Applied to powerpc next, thanks.

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

cheers

Re: [v2, 2/2] powerpc/vdso: Avoid link stack corruption in __get_datapage()

[Benjamin Herrenschmidt]

On Fri, 2015-10-02 at 17:47 +1000, Michael Ellerman wrote:
> On Fri, 2015-25-09 at 04:01:40 UTC, Michael Neuling wrote:
> > powerpc has a link register (lr) used for calling functions. We "bl
> > <func>" to call a function, and "blr" to return back to the call
> > site.
> 
> <snip, geez that was long>
> 
> > For the benchmark in
> > tools/testing/selftests/powerpc/benchmarks/gettimeofday.c
> >   POWER8:
> >     64bit gets ~4% improvement
> >     32bit gets ~9% improvement
> >   POWER7:
> >     64bit gets ~7% improvement
> > 
> > Signed-off-by: Michael Neuling <mikey@neuling.org>
> > Reported-by: Aaron Sawdey <sawdey@us.ibm.com>
> 
> Applied to powerpc next, thanks.
> 
> https://git.kernel.org/powerpc/c/c974809a26a13e40254dbe3c

I'd argue this is a bug fix and should hit stable too..

Cheers,
Ben.