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

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

[Anton Blanchard]

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 <anton@samba.org>
---
 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

[PATCH 2/3] powerpc/ftrace: Remove mod_return_to_handler

[Anton Blanchard]

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 <anton@samba.org>
---
 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

[PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Anton Blanchard]

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 <anton@samba.org>
---
 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

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

[Steven Rostedt]

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 <anton@samba.org> wrote:

> 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 <anton@samba.org>
> ---
>  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 \

Re: [PATCH 2/3] powerpc/ftrace: Remove mod_return_to_handler

[Steven Rostedt]

On Wed, 17 Sep 2014 17:07:03 +1000
Anton Blanchard <anton@samba.org> 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

Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Steven Rostedt]

[-- Attachment #1: Type: text/plain, Size: 1330 bytes --]

On Wed, 17 Sep 2014 17:07:04 +1000
Anton Blanchard <anton@samba.org> 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

[-- Attachment #2: ftrace-test-event-stacktrace --]
[-- Type: application/octet-stream, Size: 3187 bytes --]

#!/bin/bash

find_debugfs() {
    debugfs=`cat /proc/mounts | while read mount dir type opts a b; do
	if [ $mount == "debugfs" ]; then
	    echo $dir;
	    break
	fi
    done`
    if [ -z "$debugfs" ]; then
	if ! mount -t debugfs nodev /sys/kernel/debug; then
	    echo "FAILED to mount debugfs"
	    exit -1
	fi
	echo "/sys/kernel/debug"
    else
	echo $debugfs
    fi
}

debugfs=`find_debugfs`
TRACEDIR="$debugfs/tracing"
EVENTDIR=$TRACEDIR/events
EVENT=$EVENTDIR/sched/sched_switch
EVENT_ENABLE=$EVENT/enable
TRIGGER_FILE=$EVENT/trigger
TRACE_FILE=$TRACEDIR/trace
TRACING_ON=$TRACEDIR/tracing_on

# set -x

if [ ! -f $TRIGGER_FILE ]; then
    echo "triggers are not set for this kernel"
    exit 0
fi

function cnt_trace() {
	    grep -v '^#' $TRACE_FILE | wc -l
}

function clear_triggers() {
    grep -v '^#' $TRIGGER_FILE | while read t; do
	tr=`echo $t | sed -e s'/:.*//'`
	if [ $tr == "enable_event" -o $tr == "disable_event" ]; then
	    tr=`echo $t | sed -e s'/\([^:]*:[^:]*:[^:]*\):.*/\1/'`
	fi
	echo "!$tr" > $TRIGGER_FILE
    done
}

function check_events() {
    events=$1
    stack=$2
    count=$3

    if [ $events -eq 1 ]; then
	if ! grep -q 'sched_switch:' $TRACE_FILE ; then
	    echo "Expected event but it wasn't there"
	    exit -1
	fi
    else
	if grep -q 'sched_switch:' $TRACE_FILE ; then
	    echo "Expected no events but events were found"
	    exit -1
	fi
    fi

    if [ $stack -eq 1 ]; then
	if [ $count -eq 0 ]; then
	    # unlimited, find the sleep call
	    if ! grep -q '^ => .*sleep' $TRACE_FILE; then
		echo "Could not find sleep call"
		exit -1
	    fi
	else
	    # count the stack traces
	    cnt=`grep '<stack trace>' $TRACE_FILE | wc -l`;
	    if [ $cnt -ne $count ]; then
		echo "Expected $count hits but only see $cnt"
		exit -1
	    fi
	fi
    else
	if grep -q '^ =>' $TRACE_FILE; then
	    echo "Found stack trace when not enabled"
	    exit -1
	fi
    fi
}

function do_reset() {
    clear_triggers
    echo nop > $TRACEDIR/current_tracer || exit -1
    echo 0 > $TRACEDIR/events/enable || exit -1
    echo > $TRACE_FILE || exit -1
    echo 1 > $TRACING_ON || exit -1
}

#set -x

echo '** CLEAR TRACE'
do_reset

# check stacktrace available
if ! grep -q stacktrace $TRIGGER_FILE; then
    echo "stacktrace trigger not available"
    exit 0
fi

cnt=`cnt_trace`
if [ $cnt -ne 0 ]; then
    echo "Found junk in trace file, exiting"
    exit -1
fi

echo "Enable only events"

echo 1 > $EVENT_ENABLE
sleep 1
echo 0 > $TRACING_ON

check_events 1 0 0
do_reset

echo "Enable only stack trace"

echo 0 > $TRACING_ON
echo stacktrace > $TRIGGER_FILE

echo 1 > $TRACING_ON
sleep 1
echo 0 > $TRACING_ON

check_events 0 1 0
do_reset


echo "Enable both stack trace and events"

echo 0 > $TRACING_ON
echo stacktrace > $TRIGGER_FILE
echo 1 > $EVENT_ENABLE

echo 1 > $TRACING_ON
sleep 1
echo 0 > $TRACING_ON

check_events 1 1 0
do_reset

for i in 1 2 3; do
    if [ $i -gt 1 ]; then
	s='s'
    else
	s=''
    fi
    echo "Enable stack trace only $i time$s"

    echo 1 > $TRACING_ON
    echo stacktrace:$i > $TRIGGER_FILE
    echo 1 > $EVENT_ENABLE

    sleep 1
    echo 0 > $TRACING_ON

    check_events 1 1 $i
    do_reset
done


echo '** SUCCESS'

exit 0

Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Steven Rostedt]

On Tue, 23 Sep 2014 19:46:04 -0400
Steven Rostedt <rostedt@goodmis.org> 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

Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Anton Blanchard]

Hi Steve,

> > 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

Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Benjamin Herrenschmidt]

On Wed, 2014-09-24 at 12:22 +1000, Anton Blanchard wrote:
> Hi Steve,
> 
> > > 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.

Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Anton Blanchard]

Hi Ben,

> > 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

Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Steven Rostedt]

On Wed, 24 Sep 2014 12:33:07 +1000
Anton Blanchard <anton@samba.org> wrote:

> Hi Ben,
> 
> > > 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

Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Segher Boessenkool]

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

Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return

[Anton Blanchard]

Hi Segher,

> 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 ;-)

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

Anton