* [PATCH 2/3] powerpc/ftrace: Remove mod_return_to_handler
2014-09-17 7:07 [PATCH 1/3] powerpc: Remove -mno-sched-epilog workaround Anton Blanchard
@ 2014-09-17 7:07 ` Anton Blanchard
2014-09-23 23:20 ` Steven Rostedt
2014-09-17 7:07 ` [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return Anton Blanchard
2014-09-23 23:11 ` [PATCH 1/3] powerpc: Remove -mno-sched-epilog workaround Steven Rostedt
2 siblings, 1 reply; 13+ messages in thread
From: Anton Blanchard @ 2014-09-17 7:07 UTC (permalink / raw)
To: benh, paulus, mpe, rostedt; +Cc: linuxppc-dev
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(¤t->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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 2/3] powerpc/ftrace: Remove mod_return_to_handler
2014-09-17 7:07 ` [PATCH 2/3] powerpc/ftrace: Remove mod_return_to_handler Anton Blanchard
@ 2014-09-23 23:20 ` Steven Rostedt
0 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-09-23 23:20 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-17 7:07 [PATCH 1/3] powerpc: Remove -mno-sched-epilog workaround Anton Blanchard
2014-09-17 7:07 ` [PATCH 2/3] powerpc/ftrace: Remove mod_return_to_handler Anton Blanchard
@ 2014-09-17 7:07 ` Anton Blanchard
2014-09-23 23:46 ` Steven Rostedt
2014-09-23 23:11 ` [PATCH 1/3] powerpc: Remove -mno-sched-epilog workaround Steven Rostedt
2 siblings, 1 reply; 13+ messages in thread
From: Anton Blanchard @ 2014-09-17 7:07 UTC (permalink / raw)
To: benh, paulus, mpe, rostedt; +Cc: linuxppc-dev
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(¤t->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
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-17 7:07 ` [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return Anton Blanchard
@ 2014-09-23 23:46 ` Steven Rostedt
2014-09-23 23:54 ` Steven Rostedt
0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2014-09-23 23:46 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev
[-- 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
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-23 23:46 ` Steven Rostedt
@ 2014-09-23 23:54 ` Steven Rostedt
2014-09-24 2:22 ` Anton Blanchard
0 siblings, 1 reply; 13+ messages in thread
From: Steven Rostedt @ 2014-09-23 23:54 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-23 23:54 ` Steven Rostedt
@ 2014-09-24 2:22 ` Anton Blanchard
2014-09-24 2:24 ` Benjamin Herrenschmidt
0 siblings, 1 reply; 13+ messages in thread
From: Anton Blanchard @ 2014-09-24 2:22 UTC (permalink / raw)
To: Steven Rostedt; +Cc: paulus, linuxppc-dev
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-24 2:22 ` Anton Blanchard
@ 2014-09-24 2:24 ` Benjamin Herrenschmidt
2014-09-24 2:33 ` Anton Blanchard
0 siblings, 1 reply; 13+ messages in thread
From: Benjamin Herrenschmidt @ 2014-09-24 2:24 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev, Steven Rostedt
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-24 2:24 ` Benjamin Herrenschmidt
@ 2014-09-24 2:33 ` Anton Blanchard
2014-09-24 2:44 ` Steven Rostedt
2014-09-27 0:22 ` Segher Boessenkool
0 siblings, 2 replies; 13+ messages in thread
From: Anton Blanchard @ 2014-09-24 2:33 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: paulus, linuxppc-dev, Steven Rostedt, Alan Modra
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-24 2:33 ` Anton Blanchard
@ 2014-09-24 2:44 ` Steven Rostedt
2014-09-27 0:22 ` Segher Boessenkool
1 sibling, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-09-24 2:44 UTC (permalink / raw)
To: Anton Blanchard; +Cc: Alan Modra, paulus, linuxppc-dev
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-24 2:33 ` Anton Blanchard
2014-09-24 2:44 ` Steven Rostedt
@ 2014-09-27 0:22 ` Segher Boessenkool
2014-10-28 4:55 ` Anton Blanchard
1 sibling, 1 reply; 13+ messages in thread
From: Segher Boessenkool @ 2014-09-27 0:22 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev, Steven Rostedt, Alan Modra
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return
2014-09-27 0:22 ` Segher Boessenkool
@ 2014-10-28 4:55 ` Anton Blanchard
0 siblings, 0 replies; 13+ messages in thread
From: Anton Blanchard @ 2014-10-28 4:55 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: paulus, linuxppc-dev, Steven Rostedt, Alan Modra
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] powerpc: Remove -mno-sched-epilog workaround
2014-09-17 7:07 [PATCH 1/3] powerpc: Remove -mno-sched-epilog workaround Anton Blanchard
2014-09-17 7:07 ` [PATCH 2/3] powerpc/ftrace: Remove mod_return_to_handler Anton Blanchard
2014-09-17 7:07 ` [PATCH 3/3] powerpc/ftrace: simplify prepare_ftrace_return Anton Blanchard
@ 2014-09-23 23:11 ` Steven Rostedt
2 siblings, 0 replies; 13+ messages in thread
From: Steven Rostedt @ 2014-09-23 23:11 UTC (permalink / raw)
To: Anton Blanchard; +Cc: paulus, linuxppc-dev
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 \
^ permalink raw reply [flat|nested] 13+ messages in thread