* [PATCH 0/3] [GIT PULL] tracing: Three more updates
@ 2016-05-22 20:28 Steven Rostedt
2016-05-22 20:28 ` [PATCH 1/3] ftracetest: Add instance created, delete, read and enable event test Steven Rostedt
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Steven Rostedt @ 2016-05-22 20:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Linus Torvalds, Ingo Molnar, Andrew Morton
Linus,
Three more changes.
1) I forgot that I had another selftest to stress test the ftrace
instance creation. It was actually suppose to go into the 4.6
merge window, but I never committed it. I almost forgot about it
again, but noticed it was missing from your tree.
2) Soumya PN sent me a clean up patch to not disable interrupts when
taking the tasklist_lock for read, as it's unnecessary because
that lock is never taken for write in irq context.
3) Newer gcc's can cause the jump in the function_graph code to the
global ftrace_stub label to be a short jump instead of a long one.
As that jump is dynamically converted to jump to the trace code to
do function graph tracing, and that conversion expects a long jump
it can corrupt the ftrace_stub itself (it's directly after that call).
One way to prevent gcc from using a short jump is to declare the
ftrace_stub as a weak function, which we do here to keep gcc from
optimizing too much.
Please pull the latest trace-v4.7-2 tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
trace-v4.7-2
Tag SHA1: 01ed489ad3660b3813d93d63b0b9258e08b15f7f
Head SHA1: 8329e818f14926a6040df86b2668568bde342ebf
Soumya PN (1):
ftrace: Don't disable irqs when taking the tasklist_lock read_lock
Steven Rostedt (1):
ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it
Steven Rostedt (Red Hat) (1):
ftracetest: Add instance created, delete, read and enable event test
----
arch/x86/kernel/mcount_64.S | 3 +-
kernel/trace/ftrace.c | 5 +-
.../ftrace/test.d/instances/instance-event.tc | 143 +++++++++++++++++++++
3 files changed, 147 insertions(+), 4 deletions(-)
create mode 100644 tools/testing/selftests/ftrace/test.d/instances/instance-event.tc
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/3] ftracetest: Add instance created, delete, read and enable event test
2016-05-22 20:28 [PATCH 0/3] [GIT PULL] tracing: Three more updates Steven Rostedt
@ 2016-05-22 20:28 ` Steven Rostedt
2016-05-22 20:28 ` [PATCH 2/3] ftrace: Dont disable irqs when taking the tasklist_lock read_lock Steven Rostedt
2016-05-22 20:28 ` [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it Steven Rostedt
2 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2016-05-22 20:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Linus Torvalds, Ingo Molnar, Andrew Morton
[-- Attachment #1: 0001-ftracetest-Add-instance-created-delete-read-and-enab.patch --]
[-- Type: text/plain, Size: 3052 bytes --]
From: "Steven Rostedt (Red Hat)" <rostedt@goodmis.org>
Add a new ftrace test that creates three threads. One that creates and
removes an ftrace instance, one that reads the instance, and one that enables
and disables events in the instance. This is a stress test for accessing and
removing instances at the same time.
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
.../ftrace/test.d/instances/instance-event.tc | 143 +++++++++++++++++++++
1 file changed, 143 insertions(+)
create mode 100644 tools/testing/selftests/ftrace/test.d/instances/instance-event.tc
diff --git a/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc b/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc
new file mode 100644
index 000000000000..5f2abd03f16b
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/instances/instance-event.tc
@@ -0,0 +1,143 @@
+#!/bin/sh
+# description: Test creation and deletion of trace instances while setting an event
+
+if [ ! -d instances ] ; then
+ echo "no instance directory with this kernel"
+ exit_unsupported;
+fi
+
+fail() { # mesg
+ rmdir foo 2>/dev/null
+ echo $1
+ set -e
+ exit $FAIL
+}
+
+cd instances
+
+# we don't want to fail on error
+set +e
+
+mkdir x
+rmdir x
+result=$?
+
+if [ $result -ne 0 ]; then
+ echo "instance rmdir not supported"
+ exit_unsupported
+fi
+
+instance_slam() {
+ while :; do
+ mkdir foo 2> /dev/null
+ rmdir foo 2> /dev/null
+ done
+}
+
+instance_read() {
+ while :; do
+ cat foo/trace 1> /dev/null 2>&1
+ done
+}
+
+instance_set() {
+ while :; do
+ echo 1 > foo/events/sched/sched_switch
+ done 2> /dev/null
+}
+
+instance_slam &
+p1=$!
+echo $p1
+
+instance_set &
+p2=$!
+echo $p2
+
+instance_read &
+p3=$!
+echo $p3
+
+sleep 1
+
+kill -1 $p3
+kill -1 $p2
+kill -1 $p1
+
+echo "Wait for processes to finish"
+wait $p1 $p2 $p3
+echo "all processes finished, wait for cleanup"
+sleep 1
+
+mkdir foo
+ls foo > /dev/null
+rmdir foo
+if [ -d foo ]; then
+ fail "foo still exists"
+fi
+exit 0
+
+
+
+
+instance_slam() {
+ while :; do
+ mkdir x
+ mkdir y
+ mkdir z
+ rmdir x
+ rmdir y
+ rmdir z
+ done 2>/dev/null
+}
+
+instance_slam &
+x=`jobs -l`
+p1=`echo $x | cut -d' ' -f2`
+echo $p1
+
+instance_slam &
+x=`jobs -l | tail -1`
+p2=`echo $x | cut -d' ' -f2`
+echo $p2
+
+instance_slam &
+x=`jobs -l | tail -1`
+p3=`echo $x | cut -d' ' -f2`
+echo $p3
+
+instance_slam &
+x=`jobs -l | tail -1`
+p4=`echo $x | cut -d' ' -f2`
+echo $p4
+
+instance_slam &
+x=`jobs -l | tail -1`
+p5=`echo $x | cut -d' ' -f2`
+echo $p5
+
+ls -lR >/dev/null
+sleep 1
+
+kill -1 $p1
+kill -1 $p2
+kill -1 $p3
+kill -1 $p4
+kill -1 $p5
+
+echo "Wait for processes to finish"
+wait $p1 $p2 $p3 $p4 $p5
+echo "all processes finished, wait for cleanup"
+
+mkdir x y z
+ls x y z
+rmdir x y z
+for d in x y z; do
+ if [ -d $d ]; then
+ fail "instance $d still exists"
+ fi
+done
+
+set -e
+
+exit 0
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/3] ftrace: Dont disable irqs when taking the tasklist_lock read_lock
2016-05-22 20:28 [PATCH 0/3] [GIT PULL] tracing: Three more updates Steven Rostedt
2016-05-22 20:28 ` [PATCH 1/3] ftracetest: Add instance created, delete, read and enable event test Steven Rostedt
@ 2016-05-22 20:28 ` Steven Rostedt
2016-05-22 20:28 ` [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it Steven Rostedt
2 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2016-05-22 20:28 UTC (permalink / raw)
To: linux-kernel; +Cc: Linus Torvalds, Ingo Molnar, Andrew Morton, Soumya PN
[-- Attachment #1: 0002-ftrace-Don-t-disable-irqs-when-taking-the-tasklist_l.patch --]
[-- Type: text/plain, Size: 2108 bytes --]
From: Soumya PN <soumya.p.n@hpe.com>
In ftrace.c inside the function alloc_retstack_tasklist() (which will be
invoked when function_graph tracing is on) the tasklist_lock is being
held as reader while iterating through a list of threads. Here the lock
is being held as reader with irqs disabled. The tasklist_lock is never
write_locked in interrupt context so it is safe to not disable interrupts
for the duration of read_lock in this block which, can be significant,
given the block of code iterates through all threads. Hence changing the
code to call read_lock() and read_unlock() instead of read_lock_irqsave()
and read_unlock_irqrestore().
A similar change was made in commits: 8063e41d2ffc ("tracing: Change
syscall_*regfunc() to check PF_KTHREAD and use for_each_process_thread()")'
and 3472eaa1f12e ("sched: normalize_rt_tasks(): Don't use _irqsave for
tasklist_lock, use task_rq_lock()")'
Link: http://lkml.kernel.org/r/1463500874-77480-1-git-send-email-soumya.p.n@hpe.com
Signed-off-by: Soumya PN <soumya.p.n@hpe.com>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
kernel/trace/ftrace.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b1870fbd2b67..a6804823a058 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -5713,7 +5713,6 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
{
int i;
int ret = 0;
- unsigned long flags;
int start = 0, end = FTRACE_RETSTACK_ALLOC_SIZE;
struct task_struct *g, *t;
@@ -5729,7 +5728,7 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
}
}
- read_lock_irqsave(&tasklist_lock, flags);
+ read_lock(&tasklist_lock);
do_each_thread(g, t) {
if (start == end) {
ret = -EAGAIN;
@@ -5747,7 +5746,7 @@ static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
} while_each_thread(g, t);
unlock:
- read_unlock_irqrestore(&tasklist_lock, flags);
+ read_unlock(&tasklist_lock);
free:
for (i = start; i < end; i++)
kfree(ret_stack_list[i]);
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it
2016-05-22 20:28 [PATCH 0/3] [GIT PULL] tracing: Three more updates Steven Rostedt
2016-05-22 20:28 ` [PATCH 1/3] ftracetest: Add instance created, delete, read and enable event test Steven Rostedt
2016-05-22 20:28 ` [PATCH 2/3] ftrace: Dont disable irqs when taking the tasklist_lock read_lock Steven Rostedt
@ 2016-05-22 20:28 ` Steven Rostedt
2016-06-20 0:46 ` Namhyung Kim
2 siblings, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2016-05-22 20:28 UTC (permalink / raw)
To: linux-kernel
Cc: Linus Torvalds, Ingo Molnar, Andrew Morton, Matt Fleming,
Namhyung Kim, Masami Hiramatsu
[-- Attachment #1: 0003-ftrace-x86-Set-ftrace_stub-to-weak-to-prevent-gcc-fr.patch --]
[-- Type: text/plain, Size: 2157 bytes --]
From: Steven Rostedt <rostedt@goodmis.org>
Matt Fleming reported seeing crashes when enabling and disabling
function profiling which uses function graph tracer. Later Namhyung Kim
hit a similar issue and he found that the issue was due to the jmp to
ftrace_stub in ftrace_graph_call was only two bytes, and when it was
changed to jump to the tracing code, it overwrote the ftrace_stub that
was after it.
Masami Hiramatsu bisected this down to a binutils change:
8dcea93252a9ea7dff57e85220a719e2a5e8ab41 is the first bad commit
commit 8dcea93252a9ea7dff57e85220a719e2a5e8ab41
Author: H.J. Lu <hjl.tools@gmail.com>
Date: Fri May 15 03:17:31 2015 -0700
Add -mshared option to x86 ELF assembler
This patch adds -mshared option to x86 ELF assembler. By default,
assembler will optimize out non-PLT relocations against defined non-weak
global branch targets with default visibility. The -mshared option tells
the assembler to generate code which may go into a shared library
where all non-weak global branch targets with default visibility can
be preempted. The resulting code is slightly bigger. This option
only affects the handling of branch instructions.
Declaring ftrace_stub as a weak call prevents gas from using two byte
jumps to it, which would be converted to a jump to the function graph
code.
Link: http://lkml.kernel.org/r/20160516230035.1dbae571@gandalf.local.home
Reported-by: Matt Fleming <matt@codeblueprint.co.uk>
Reported-by: Namhyung Kim <namhyung@kernel.org>
Tested-by: Matt Fleming <matt@codeblueprint.co.uk>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
arch/x86/kernel/mcount_64.S | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
index ed48a9f465f8..61924222a9e1 100644
--- a/arch/x86/kernel/mcount_64.S
+++ b/arch/x86/kernel/mcount_64.S
@@ -182,7 +182,8 @@ GLOBAL(ftrace_graph_call)
jmp ftrace_stub
#endif
-GLOBAL(ftrace_stub)
+/* This is weak to keep gas from relaxing the jumps */
+WEAK(ftrace_stub)
retq
END(ftrace_caller)
--
2.8.0.rc3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it
2016-05-22 20:28 ` [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it Steven Rostedt
@ 2016-06-20 0:46 ` Namhyung Kim
2016-06-20 13:40 ` Steven Rostedt
0 siblings, 1 reply; 6+ messages in thread
From: Namhyung Kim @ 2016-06-20 0:46 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Linus Torvalds, Ingo Molnar, Andrew Morton,
Matt Fleming, Masami Hiramatsu
Hi Steve,
On Sun, May 22, 2016 at 04:28:49PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
>
> Matt Fleming reported seeing crashes when enabling and disabling
> function profiling which uses function graph tracer. Later Namhyung Kim
> hit a similar issue and he found that the issue was due to the jmp to
> ftrace_stub in ftrace_graph_call was only two bytes, and when it was
> changed to jump to the tracing code, it overwrote the ftrace_stub that
> was after it.
>
> Masami Hiramatsu bisected this down to a binutils change:
>
> 8dcea93252a9ea7dff57e85220a719e2a5e8ab41 is the first bad commit
> commit 8dcea93252a9ea7dff57e85220a719e2a5e8ab41
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date: Fri May 15 03:17:31 2015 -0700
>
> Add -mshared option to x86 ELF assembler
>
> This patch adds -mshared option to x86 ELF assembler. By default,
> assembler will optimize out non-PLT relocations against defined non-weak
> global branch targets with default visibility. The -mshared option tells
> the assembler to generate code which may go into a shared library
> where all non-weak global branch targets with default visibility can
> be preempted. The resulting code is slightly bigger. This option
> only affects the handling of branch instructions.
>
> Declaring ftrace_stub as a weak call prevents gas from using two byte
> jumps to it, which would be converted to a jump to the function graph
> code.
>
> Link: http://lkml.kernel.org/r/20160516230035.1dbae571@gandalf.local.home
Shouldn't it go to the stable tree?
Thanks,
Namhyung
>
> Reported-by: Matt Fleming <matt@codeblueprint.co.uk>
> Reported-by: Namhyung Kim <namhyung@kernel.org>
> Tested-by: Matt Fleming <matt@codeblueprint.co.uk>
> Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> arch/x86/kernel/mcount_64.S | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
> index ed48a9f465f8..61924222a9e1 100644
> --- a/arch/x86/kernel/mcount_64.S
> +++ b/arch/x86/kernel/mcount_64.S
> @@ -182,7 +182,8 @@ GLOBAL(ftrace_graph_call)
> jmp ftrace_stub
> #endif
>
> -GLOBAL(ftrace_stub)
> +/* This is weak to keep gas from relaxing the jumps */
> +WEAK(ftrace_stub)
> retq
> END(ftrace_caller)
>
> --
> 2.8.0.rc3
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it
2016-06-20 0:46 ` Namhyung Kim
@ 2016-06-20 13:40 ` Steven Rostedt
0 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2016-06-20 13:40 UTC (permalink / raw)
To: Namhyung Kim
Cc: linux-kernel, Linus Torvalds, Ingo Molnar, Andrew Morton,
Matt Fleming, Masami Hiramatsu
On Mon, 20 Jun 2016 09:46:02 +0900
Namhyung Kim <namhyung@kernel.org> wrote:
> Hi Steve,
>
> On Sun, May 22, 2016 at 04:28:49PM -0400, Steven Rostedt wrote:
> > From: Steven Rostedt <rostedt@goodmis.org>
> >
> > Matt Fleming reported seeing crashes when enabling and disabling
> > function profiling which uses function graph tracer. Later Namhyung Kim
> > hit a similar issue and he found that the issue was due to the jmp to
> > ftrace_stub in ftrace_graph_call was only two bytes, and when it was
> > changed to jump to the tracing code, it overwrote the ftrace_stub that
> > was after it.
> >
> > Masami Hiramatsu bisected this down to a binutils change:
> >
> > 8dcea93252a9ea7dff57e85220a719e2a5e8ab41 is the first bad commit
> > commit 8dcea93252a9ea7dff57e85220a719e2a5e8ab41
> > Author: H.J. Lu <hjl.tools@gmail.com>
> > Date: Fri May 15 03:17:31 2015 -0700
> >
> > Add -mshared option to x86 ELF assembler
> >
> > This patch adds -mshared option to x86 ELF assembler. By default,
> > assembler will optimize out non-PLT relocations against defined non-weak
> > global branch targets with default visibility. The -mshared option tells
> > the assembler to generate code which may go into a shared library
> > where all non-weak global branch targets with default visibility can
> > be preempted. The resulting code is slightly bigger. This option
> > only affects the handling of branch instructions.
> >
> > Declaring ftrace_stub as a weak call prevents gas from using two byte
> > jumps to it, which would be converted to a jump to the function graph
> > code.
> >
> > Link: http://lkml.kernel.org/r/20160516230035.1dbae571@gandalf.local.home
>
> Shouldn't it go to the stable tree?
I had this discussion with Matt too. If you want to request this for
stable, feel free to do so. It's an issue with new compilers. My
thought was, if you are using an older kernel, you are probably using
an older compiler too. ;-)
-- Steve
>
>
> >
> > Reported-by: Matt Fleming <matt@codeblueprint.co.uk>
> > Reported-by: Namhyung Kim <namhyung@kernel.org>
> > Tested-by: Matt Fleming <matt@codeblueprint.co.uk>
> > Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
> > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> > ---
> > arch/x86/kernel/mcount_64.S | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/x86/kernel/mcount_64.S b/arch/x86/kernel/mcount_64.S
> > index ed48a9f465f8..61924222a9e1 100644
> > --- a/arch/x86/kernel/mcount_64.S
> > +++ b/arch/x86/kernel/mcount_64.S
> > @@ -182,7 +182,8 @@ GLOBAL(ftrace_graph_call)
> > jmp ftrace_stub
> > #endif
> >
> > -GLOBAL(ftrace_stub)
> > +/* This is weak to keep gas from relaxing the jumps */
> > +WEAK(ftrace_stub)
> > retq
> > END(ftrace_caller)
> >
> > --
> > 2.8.0.rc3
> >
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-20 13:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-22 20:28 [PATCH 0/3] [GIT PULL] tracing: Three more updates Steven Rostedt
2016-05-22 20:28 ` [PATCH 1/3] ftracetest: Add instance created, delete, read and enable event test Steven Rostedt
2016-05-22 20:28 ` [PATCH 2/3] ftrace: Dont disable irqs when taking the tasklist_lock read_lock Steven Rostedt
2016-05-22 20:28 ` [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it Steven Rostedt
2016-06-20 0:46 ` Namhyung Kim
2016-06-20 13:40 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).