tracing: Three more updates

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

From: "Steven Rostedt (Red Hat)" <[email protected]>

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.

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

new file mode 100644
index 000000000000..5f2abd03f16b
@@ -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

From: Soumya PN <[email protected]>

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()")'

---
 kernel/trace/ftrace.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

index b1870fbd2b67..a6804823a058 100644
@@ -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

From: Steven Rostedt <[email protected]>

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 <[email protected]>
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.

---
 arch/x86/kernel/mcount_64.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index ed48a9f465f8..61924222a9e1 100644
@@ -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

Hi Steve,

Shouldn't it go to the stable tree?

Thanks,
Namhyung

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