[PATCH 1/3] powerpc: Remove -mno-sched-epilog workaround

We added -mno-sched-epilog in commit 7563dc645853 (powerpc:
Work around gcc's -fno-omit-frame-pointer bug).

We shouldn't apply -fno-omit-frame-pointer on powerpc any more (it's
protected by CONFIG_FRAME_POINTER and CONFIG_SCHED_OMIT_FRAME_POINTER).

It's also an undocumented gcc option, so lets remove it.

Signed-off-by: Anton Blanchard <redacted>
---
 arch/powerpc/Makefile                    |  5 -----
 arch/powerpc/kernel/Makefile             | 12 ++++++------
 arch/powerpc/platforms/powermac/Makefile |  2 +-
 3 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/Makefile b/arch/powerpc/Makefile
index 132d9c6..c6f64e2 100644
--- a/arch/powerpc/Makefile
+++ b/arch/powerpc/Makefile

@@ -198,11 +198,6 @@ ifeq ($(CONFIG_6xx),y)
 KBUILD_CFLAGS		+= -mcpu=powerpc
 endif
 
-# Work around a gcc code-gen bug with -fno-omit-frame-pointer.
-ifeq ($(CONFIG_FUNCTION_TRACER),y)
-KBUILD_CFLAGS		+= -mno-sched-epilog
-endif
-
 cpu-as-$(CONFIG_4xx)		+= -Wa,-m405
 cpu-as-$(CONFIG_ALTIVEC)	+= -Wa,-maltivec
 cpu-as-$(CONFIG_E200)		+= -Wa,-me200

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index 502cf69..e14bda6 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile

@@ -17,14 +17,14 @@ endif
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace early boot code
-CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_cputable.o = -pg
+CFLAGS_REMOVE_prom_init.o = -pg
+CFLAGS_REMOVE_btext.o = -pg
+CFLAGS_REMOVE_prom.o = -pg
 # do not trace tracer code
-CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_ftrace.o = -pg
 # timers used by tracing
-CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_time.o = -pg
 endif
 
 obj-y				:= cputable.o ptrace.o syscalls.o \

diff --git a/arch/powerpc/platforms/powermac/Makefile b/arch/powerpc/platforms/powermac/Makefile
index 52c6ce1..e238872 100644
--- a/arch/powerpc/platforms/powermac/Makefile
+++ b/arch/powerpc/platforms/powermac/Makefile

@@ -2,7 +2,7 @@ CFLAGS_bootx_init.o  		+= -fPIC
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace early boot code
-CFLAGS_REMOVE_bootx_init.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_bootx_init.o = -pg
 endif
 
 obj-y				+= pic.o setup.o time.o feature.o pci.o \

-- 
1.9.1

mod_return_to_handler is the same as return_to_handler, except
it handles the change of the TOC (r2). Add this into
return_to_handler and remove mod_return_to_handler.

Signed-off-by: Anton Blanchard <redacted>
---
 arch/powerpc/kernel/entry_64.S | 24 +-----------------------
 arch/powerpc/kernel/ftrace.c   | 14 ++------------
 arch/powerpc/kernel/process.c  |  9 +--------
 3 files changed, 4 insertions(+), 43 deletions(-)

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 5bbd1bc..955d509 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S

@@ -1235,28 +1235,6 @@ _GLOBAL(ftrace_graph_caller)
 
 _GLOBAL(return_to_handler)
 	/* need to save return values */
-	std	r4,  -24(r1)
-	std	r3,  -16(r1)
-	std	r31, -8(r1)
-	mr	r31, r1
-	stdu	r1, -112(r1)
-
-	bl	ftrace_return_to_handler
-	nop
-
-	/* return value has real return address */
-	mtlr	r3
-
-	ld	r1, 0(r1)
-	ld	r4,  -24(r1)
-	ld	r3,  -16(r1)
-	ld	r31, -8(r1)
-
-	/* Jump back to real return address */
-	blr
-
-_GLOBAL(mod_return_to_handler)
-	/* need to save return values */
 	std	r4,  -32(r1)
 	std	r3,  -24(r1)
 	/* save TOC */

@@ -1266,7 +1244,7 @@ _GLOBAL(mod_return_to_handler)
 	stdu	r1, -112(r1)
 
 	/*
-	 * We are in a module using the module's TOC.
+	 * We might be called from a module.
 	 * Switch to our TOC to run inside the core kernel.
 	 */
 	ld	r2, PACATOC(r13)

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index 390311c..abf7921 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c

@@ -510,10 +510,6 @@ int ftrace_disable_ftrace_graph_caller(void)
 }
 #endif /* CONFIG_DYNAMIC_FTRACE */
 
-#ifdef CONFIG_PPC64
-extern void mod_return_to_handler(void);
-#endif
-
 /*
  * Hook the return address and push it in the stack of return addrs
  * in current thread info.

@@ -523,7 +519,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 	unsigned long old;
 	int faulted;
 	struct ftrace_graph_ent trace;
-	unsigned long return_hooker = (unsigned long)&return_to_handler;
+	unsigned long return_hooker;
 
 	if (unlikely(ftrace_graph_is_dead()))
 		return;

@@ -531,13 +527,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
 		return;
 
-#ifdef CONFIG_PPC64
-	/* non core kernel code needs to save and restore the TOC */
-	if (REGION_ID(self_addr) != KERNEL_REGION_ID)
-		return_hooker = (unsigned long)&mod_return_to_handler;
-#endif
-
-	return_hooker = ppc_function_entry((void *)return_hooker);
+	return_hooker = ppc_function_entry(return_to_handler);
 
 	/*
 	 * Protect against fault, even if it shouldn't

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index aa1df89..080c0b9 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c

@@ -1531,13 +1531,6 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
 	int curr_frame = current->curr_ret_stack;
 	extern void return_to_handler(void);
 	unsigned long rth = (unsigned long)return_to_handler;
-	unsigned long mrth = -1;
-#ifdef CONFIG_PPC64
-	extern void mod_return_to_handler(void);
-	rth = *(unsigned long *)rth;
-	mrth = (unsigned long)mod_return_to_handler;
-	mrth = *(unsigned long *)mrth;
-#endif
 #endif
 
 	sp = (unsigned long) stack;

@@ -1562,7 +1555,7 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
 		if (!firstframe || ip != lr) {
 			printk("["REG"] ["REG"] %pS", sp, ip, (void *)ip);
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-			if ((ip == rth || ip == mrth) && curr_frame >= 0) {
+			if ((ip == rth) && curr_frame >= 0) {
 				printk(" (%pS)",
 				       (void *)current->ret_stack[curr_frame].ret);
 				curr_frame--;

-- 
1.9.1

Instead of passing in the stack address of the link register
to be modified, just pass in the old value and return the
new value and rely on ftrace_graph_caller to do the
modification.

This removes the exception handling around the stack update -
it isn't needed and we weren't consistent about it. Later on
we would do an unprotected modification:

       if (!ftrace_graph_entry(&trace)) {
               *parent = old;

Signed-off-by: Anton Blanchard <redacted>
---
 arch/powerpc/kernel/entry_32.S | 10 +++++--
 arch/powerpc/kernel/entry_64.S | 11 ++++++--
 arch/powerpc/kernel/ftrace.c   | 59 ++++++++++--------------------------------
 3 files changed, 30 insertions(+), 50 deletions(-)

diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index 22b45a4..ad837d8 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S

@@ -1424,12 +1424,18 @@ _GLOBAL(ftrace_graph_caller)
 	lwz	r4, 44(r1)
 	subi	r4, r4, MCOUNT_INSN_SIZE
 
-	/* get the parent address */
-	addi	r3, r1, 52
+	/* Grab the LR out of the caller stack frame */
+	lwz	r3,52(r1)
 
 	bl	prepare_ftrace_return
 	nop
 
+        /*
+         * prepare_ftrace_return gives us the address we divert to.
+         * Change the LR in the callers stack frame to this.
+         */
+	stw	r3,52(r1)
+
 	MCOUNT_RESTORE_FRAME
 	/* old link register ends up in ctr reg */
 	bctr

diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index 955d509..9caab69 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S

@@ -1221,13 +1221,20 @@ _GLOBAL(ftrace_graph_caller)
 	ld	r4, 128(r1)
 	subi	r4, r4, MCOUNT_INSN_SIZE
 
-	/* get the parent address */
+	/* Grab the LR out of the caller stack frame */
 	ld	r11, 112(r1)
-	addi	r3, r11, 16
+	ld	r3, 16(r11)
 
 	bl	prepare_ftrace_return
 	nop
 
+	/*
+	 * prepare_ftrace_return gives us the address we divert to.
+	 * Change the LR in the callers stack frame to this.
+	 */
+	ld	r11, 112(r1)
+	std	r3, 16(r11)
+
 	ld	r0, 128(r1)
 	mtlr	r0
 	addi	r1, r1, 112

diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c
index abf7921..d795031 100644
--- a/arch/powerpc/kernel/ftrace.c
+++ b/arch/powerpc/kernel/ftrace.c

@@ -512,67 +512,34 @@ int ftrace_disable_ftrace_graph_caller(void)
 
 /*
  * Hook the return address and push it in the stack of return addrs
- * in current thread info.
+ * in current thread info. Return the address we want to divert to.
  */
-void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
+unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip)
 {
-	unsigned long old;
-	int faulted;
 	struct ftrace_graph_ent trace;
 	unsigned long return_hooker;
 
 	if (unlikely(ftrace_graph_is_dead()))
-		return;
+		goto out;
 
 	if (unlikely(atomic_read(&current->tracing_graph_pause)))
-		return;
+		goto out;
 
 	return_hooker = ppc_function_entry(return_to_handler);
 
-	/*
-	 * Protect against fault, even if it shouldn't
-	 * happen. This tool is too much intrusive to
-	 * ignore such a protection.
-	 */
-	asm volatile(
-		"1: " PPC_LL "%[old], 0(%[parent])\n"
-		"2: " PPC_STL "%[return_hooker], 0(%[parent])\n"
-		"   li %[faulted], 0\n"
-		"3:\n"
-
-		".section .fixup, \"ax\"\n"
-		"4: li %[faulted], 1\n"
-		"   b 3b\n"
-		".previous\n"
-
-		".section __ex_table,\"a\"\n"
-			PPC_LONG_ALIGN "\n"
-			PPC_LONG "1b,4b\n"
-			PPC_LONG "2b,4b\n"
-		".previous"
-
-		: [old] "=&r" (old), [faulted] "=r" (faulted)
-		: [parent] "r" (parent), [return_hooker] "r" (return_hooker)
-		: "memory"
-	);
-
-	if (unlikely(faulted)) {
-		ftrace_graph_stop();
-		WARN_ON(1);
-		return;
-	}
-
-	trace.func = self_addr;
+	trace.func = ip;
 	trace.depth = current->curr_ret_stack + 1;
 
 	/* Only trace if the calling function expects to */
-	if (!ftrace_graph_entry(&trace)) {
-		*parent = old;
-		return;
-	}
+	if (!ftrace_graph_entry(&trace))
+		goto out;
+
+	if (ftrace_push_return_trace(parent, ip, &trace.depth, 0) == -EBUSY)
+		goto out;
 
-	if (ftrace_push_return_trace(old, self_addr, &trace.depth, 0) == -EBUSY)
-		*parent = old;
+	parent = return_hooker;
+out:
+	return parent;
 }
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 

-- 
1.9.1

I'm running my ftrace tests on my PAsemi box with your patches and
things are not going so well.

Just this patch alone causes my first stress test to lock up, and
things don't go so well after that.

INFO: rcu_sched detected stalls on CPUs/tasks: {} (detected by 1, t=5253 jiffies, g=3603, c=3602, q=15)
INFO: Stall ended before state dump start
INFO: rcu_sched detected stalls on CPUs/tasks: {} (detected by 1, t=21008 jiffies, g=3603, c=3602, q=15)
INFO: Stall ended before state dump start

That's repeated and the system is basically useless.

-- Steve



On Wed, 17 Sep 2014 17:07:02 +1000
Anton Blanchard [off-list ref] wrote:

On Wed, 17 Sep 2014 17:07:03 +1000
Anton Blanchard [off-list ref] wrote:

mod_return_to_handler is the same as return_to_handler, except
it handles the change of the TOC (r2). Add this into
return_to_handler and remove mod_return_to_handler.

Adding this patch actually gave me some more output. Funny that?

electra login: INFO: rcu_sched self-detected stall on CPU { 1}  (t=5250 jiffies g=3579 c=3578 q=7)
Task dump for CPU 1:
trace-cmd       R  running task        0  3553   3550 0x00008014
Call Trace:
[c0000000054a6ce0] [c000000000012b34] .show_stack+0x104/0x260 (unreliable)
[c0000000054a6dc0] [c0000000000c2270] .sched_show_task+0xd0/0x150
[c0000000054a6e40] [c0000000000eedb0] .rcu_dump_cpu_stacks+0xe0/0x150
[c0000000054a6ee0] [c0000000000f2a48] .rcu_check_callbacks+0x4f8/0x8d0
[c0000000054a7020] [c0000000000f8118] .update_process_times+0x48/0xa0
[c0000000054a70b0] [c00000000010cfe8] .tick_sched_timer+0x88/0xd0
[c0000000054a7150] [c0000000000f8b1c] .__run_hrtimer+0xcc/0x2c0
[c0000000054a7200] [c0000000000f9a98] .hrtimer_interrupt+0x158/0x330
[c0000000054a7310] [c00000000001a6b8] .__timer_interrupt+0xa8/0x280
[c0000000054a73c0] [c00000000001a920] .timer_interrupt+0x90/0x100
[c0000000054a7440] [c000000000002260] decrementer_common+0x160/0x180

--- interrupt: 901 at .trace_buffer_lock_reserve+0x2c/0x90
    LR = .trace_function+0x54/0xe0

[c0000000054a77c0] [c0000000001449cc] .function_trace_call+0x7c/0x120
[c0000000054a7840] [c00000000012d750] .ftrace_ops_no_ops+0xf0/0x170
[c0000000054a78e0] [c000000000009d8c] ftrace_call+0x4/0x8
[c0000000054a7950] [c00000000072e128] .mutex_unlock+0x18/0x70
[c0000000054a79d0] [c000000000137534] .tracing_buffers_splice_read+0x424/0x4c0
[c0000000054a7c80] [c00000000021bfe8] .do_splice_to+0xa8/0xe0
[c0000000054a7d20] [c00000000021eae4] .SyS_splice+0x694/0x6b0
[c0000000054a7e30] [c000000000009224] syscall_exit+0x0/0x98


Note, the stress test is basically this:

perf record -o perf-test.dat -a -- trace-cmd record -e all -p function hackbench 2

It actually dies as it finishes the hackbench run and starts stopping
the tracing.

-- Steve

On Wed, 17 Sep 2014 17:07:04 +1000
Anton Blanchard [off-list ref] wrote:

Instead of passing in the stack address of the link register
to be modified, just pass in the old value and return the
new value and rely on ftrace_graph_caller to do the
modification.

This removes the exception handling around the stack update -
it isn't needed and we weren't consistent about it. Later on
we would do an unprotected modification:

       if (!ftrace_graph_entry(&trace)) {
               *parent = old;

First I'll say this is something I've been wanting to do with x86 for
some time. That said...

With this patch, things move much further in my tests. The stress test
passes again. But then it fails on my stack trace test. Which is
because this is what I have in the stack traces:

           sleep-3557  [000] d...   100.206808: <stack trace>
 => 0
 => 0
 => 0
 => 0
 => 0
 => 0
 => 0
 => 0


Where without the patches I have something like this:

          sleep-3641  [001] d...   304.023550: <stack trace>
 => .ftrace_raw_event_sched_switch
 => .__schedule
 => .schedule
 => .do_nanosleep
 => .hrtimer_nanosleep
 => .compat_SyS_nanosleep
 => syscall_exit
 => 0

This could be broken from the earlier patches, I haven't run just this
test. I probably should on them.

I've attached the test.

-- Steve

On Tue, 23 Sep 2014 19:46:04 -0400
Steven Rostedt [off-list ref] wrote:

 

This could be broken from the earlier patches, I haven't run just this
test. I probably should on them.

I went back and tested, and it breaks under the first patch.

-- Steve

Hi Steve,

quoted

This could be broken from the earlier patches, I haven't run just
this test. I probably should on them.

I went back and tested, and it breaks under the first patch.

Thanks for testing. It looks like some toolchains have issues
other than the -fno-no-omit-frame-pointer one, and -mno-sched-epilog
works around it.

I'll drop that patch and respin.

Anton

On Wed, 2014-09-24 at 12:22 +1000, Anton Blanchard wrote:

Hi Steve,

quoted

quoted

This could be broken from the earlier patches, I haven't run just
this test. I probably should on them.

I went back and tested, and it breaks under the first patch.

Thanks for testing. It looks like some toolchains have issues
other than the -fno-no-omit-frame-pointer one, and -mno-sched-epilog
works around it.

I'll drop that patch and respin.

Or maybe do a toolchain check / or enable it in LE ?

Ben.

Hi Ben,

quoted

I'll drop that patch and respin.

Or maybe do a toolchain check / or enable it in LE ?

We are scratching our heads trying to remember details of the issue
right now. In retrospect we should have linked the gcc bugzilla or
gcc commit details in the kernel commit message :)

Steve: what gcc version are you building with?

Anton

On Wed, 24 Sep 2014 12:33:07 +1000
Anton Blanchard [off-list ref] wrote:

Hi Ben,

quoted

quoted

I'll drop that patch and respin.

Or maybe do a toolchain check / or enable it in LE ?

We are scratching our heads trying to remember details of the issue
right now. In retrospect we should have linked the gcc bugzilla or
gcc commit details in the kernel commit message :)

Steve: what gcc version are you building with?

powerpc64-linux-gcc (GCC) 4.6.3

I got it from here:

  https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.6.3/

-- Steve

On Wed, Sep 24, 2014 at 12:33:07PM +1000, Anton Blanchard wrote:

We are scratching our heads trying to remember details of the issue
right now. In retrospect we should have linked the gcc bugzilla or
gcc commit details in the kernel commit message :)

There have been many GCC bugs in this area.

30282 (for 32-bit)
44199 (for 64-bit)
52828 (for everything, and this one should finally handle things for good)
Also a bunch of duplicates, and I'm sure I've missed some more.

The original issue as far as I remember: when using a frame pointer, GCC
would sometimes schedule the epilogue to do the stack adjust before
restoring all regs from the stack.  Then an interrupt comes in, those
saved regs are clobbered, kaboom.  We cannot disable the frame pointer
because -pg forces it (although PowerPC does not need it).  The
-mno-sched-epilog flag is a workaround: the epilogue (and prologue) will
not be reordered by instruction scheduling.  Slow code is better than
blowing up fast ;-)


Segher

Hi Segher,

On Wed, Sep 24, 2014 at 12:33:07PM +1000, Anton Blanchard wrote:

quoted

We are scratching our heads trying to remember details of the issue
right now. In retrospect we should have linked the gcc bugzilla or
gcc commit details in the kernel commit message :)

There have been many GCC bugs in this area.

30282 (for 32-bit)
44199 (for 64-bit)
52828 (for everything, and this one should finally handle things for
good) Also a bunch of duplicates, and I'm sure I've missed some more.

The original issue as far as I remember: when using a frame pointer,
GCC would sometimes schedule the epilogue to do the stack adjust
before restoring all regs from the stack.  Then an interrupt comes
in, those saved regs are clobbered, kaboom.  We cannot disable the
frame pointer because -pg forces it (although PowerPC does not need
it).  The -mno-sched-epilog flag is a workaround: the epilogue (and
prologue) will not be reordered by instruction scheduling.  Slow code
is better than blowing up fast ;-)

Thanks for explaining it! It does look like the last issue wasn't
fixed until gcc 4.8. We'll drop that patch.

Anton