[PATCH 0/3] [GIT PULL] tracing: Three more updates

[PATCH 0/3] [GIT PULL] tracing: Three more updates

[Steven Rostedt]

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

[PATCH 1/3] ftracetest: Add instance created, delete, read and enable event test

[Steven Rostedt]

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

[PATCH 2/3] ftrace: Dont disable irqs when taking the tasklist_lock read_lock

[Steven Rostedt]

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

[PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it

[Steven Rostedt]

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

Re: [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it

[Namhyung Kim]

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

Re: [PATCH 3/3] ftrace/x86: Set ftrace_stub to weak to prevent gcc from using short jumps to it

[Steven Rostedt]

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