[PATCH] tracing/function-return-tracer: set a more human readable output

[PATCH] tracing/function-return-tracer: set a more human readable output

[Frederic Weisbecker]

Impact: feature

This patch sets a C-like output for the function graph tracing.
For this aim, we now call two handler for each function: one on the entry
and one other on return. This way we can draw a well-ordered call stack.

The pid of the previous trace is loosely stored to be compared against
the one of the current trace to see if there were a context switch.

Without this little feature, the call tree would seem broken at
some locations.
We could use the sched_tracer to capture these sched_events but this
way of processing is much more simpler.

2 spaces have been chosen for indentation to fit the screen while deep
calls. The time of execution in nanosecs is printed just after closed
braces, it seems more easy this way to find the corresponding function.
If the time was printed as a first column, it would be not so easy to
find the corresponding function if it is called on a deep depth.

I plan to output the return value but on 32 bits CPU, the return value
can be 32 or 64, and its difficult to guess on which case we are.
I don't know what would be the better solution on X86-32: only print
eax (low-part) or even edx (high-part).

Actually it's thee same problem when a function return a 8 bits value, the
high part of eax could contain junk values...

Here is an example of trace:

sys_read() {
  fget_light() {
  } 526
  vfs_read() {
    rw_verify_area() {
      security_file_permission() {
        cap_file_permission() {
        } 519
      } 1564
    } 2640
    do_sync_read() {
      pipe_read() {
        __might_sleep() {
        } 511
        pipe_wait() {
          prepare_to_wait() {
          } 760
          deactivate_task() {
            dequeue_task() {
              dequeue_task_fair() {
                dequeue_entity() {
                  update_curr() {
                    update_min_vruntime() {
                    } 504
                  } 1587
                  clear_buddies() {
                  } 512
                  add_cfs_task_weight() {
                  } 519
                  update_min_vruntime() {
                  } 511
                } 5602
                dequeue_entity() {
                  update_curr() {
                    update_min_vruntime() {
                    } 496
                  } 1631
                  clear_buddies() {
                  } 496
                  update_min_vruntime() {
                  } 527
                } 4580
                hrtick_update() {
                  hrtick_start_fair() {
                  } 488
                } 1489
              } 13700
            } 14949
          } 16016
          msecs_to_jiffies() {
          } 496
          put_prev_task_fair() {
          } 504
          pick_next_task_fair() {
          } 489
          pick_next_task_rt() {
          } 496
          pick_next_task_fair() {
          } 489
          pick_next_task_idle() {
          } 489

------------8<---------- thread 4 ------------8<----------

finish_task_switch() {
} 1203
do_softirq() {
  __do_softirq() {
    __local_bh_disable() {
    } 669
    rcu_process_callbacks() {
      __rcu_process_callbacks() {
        cpu_quiet() {
          rcu_start_batch() {
          } 503
        } 1647
      } 3128
      __rcu_process_callbacks() {
      } 542
    } 5362
    _local_bh_enable() {
    } 587
  } 8880
} 9986
kthread_should_stop() {
} 669
deactivate_task() {
  dequeue_task() {
    dequeue_task_fair() {
      dequeue_entity() {
        update_curr() {
          calc_delta_mine() {
          } 511
          update_min_vruntime() {
          } 511
        } 2813

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 arch/x86/kernel/ftrace.c              |   32 +++++++----
 include/linux/ftrace.h                |   25 ++++++--
 kernel/trace/ftrace.c                 |   30 +++++-----
 kernel/trace/trace.c                  |   67 +++++++++++++++++----
 kernel/trace/trace.h                  |   28 +++++----
 kernel/trace/trace_functions_graph.c  |  104 +++++++++++++++++++++++++--------
 kernel/trace/trace_functions_return.c |   98 -------------------------------
 7 files changed, 208 insertions(+), 176 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 3595a4c..26b2d92 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -347,7 +347,7 @@ void ftrace_nmi_exit(void)
 
 /* Add a function return address to the trace stack on thread info.*/
 static int push_return_trace(unsigned long ret, unsigned long long time,
-				unsigned long func)
+				unsigned long func, int *depth)
 {
 	int index;
 
@@ -365,21 +365,22 @@ static int push_return_trace(unsigned long ret, unsigned long long time,
 	current->ret_stack[index].ret = ret;
 	current->ret_stack[index].func = func;
 	current->ret_stack[index].calltime = time;
+	*depth = index;
 
 	return 0;
 }
 
 /* Retrieve a function return address to the trace stack on thread info.*/
-static void pop_return_trace(unsigned long *ret, unsigned long long *time,
-				unsigned long *func, unsigned long *overrun)
+static void pop_return_trace(struct ftrace_graph_ret *trace, unsigned long *ret)
 {
 	int index;
 
 	index = current->curr_ret_stack;
 	*ret = current->ret_stack[index].ret;
-	*func = current->ret_stack[index].func;
-	*time = current->ret_stack[index].calltime;
-	*overrun = atomic_read(&current->trace_overrun);
+	trace->func = current->ret_stack[index].func;
+	trace->calltime = current->ret_stack[index].calltime;
+	trace->overrun = atomic_read(&current->trace_overrun);
+	trace->depth = index;
 	current->curr_ret_stack--;
 }
 
@@ -390,12 +391,13 @@ static void pop_return_trace(unsigned long *ret, unsigned long long *time,
 unsigned long ftrace_return_to_handler(void)
 {
 	struct ftrace_graph_ret trace;
-	pop_return_trace(&trace.ret, &trace.calltime, &trace.func,
-			&trace.overrun);
+	unsigned long ret;
+
+	pop_return_trace(&trace, &ret);
 	trace.rettime = cpu_clock(raw_smp_processor_id());
-	ftrace_graph_function(&trace);
+	ftrace_graph_return(&trace);
 
-	return trace.ret;
+	return ret;
 }
 
 /*
@@ -407,6 +409,7 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 	unsigned long old;
 	unsigned long long calltime;
 	int faulted;
+	struct ftrace_graph_ent trace;
 	unsigned long return_hooker = (unsigned long)
 				&return_to_handler;
 
@@ -452,8 +455,15 @@ void prepare_ftrace_return(unsigned long *parent, unsigned long self_addr)
 
 	calltime = cpu_clock(raw_smp_processor_id());
 
-	if (push_return_trace(old, calltime, self_addr) == -EBUSY)
+	if (push_return_trace(old, calltime,
+				self_addr, &trace.depth) == -EBUSY) {
 		*parent = old;
+		return;
+	}
+
+	trace.func = self_addr;
+	ftrace_graph_entry(&trace);
+
 }
 
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index b4ac734..fc2d549 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -313,26 +313,39 @@ ftrace_init_module(struct module *mod,
 
 
 /*
+ * Structure that defines an entry function trace.
+ */
+struct ftrace_graph_ent {
+	unsigned long func; /* Current function */
+	int depth;
+};
+
+/*
  * Structure that defines a return function trace.
  */
 struct ftrace_graph_ret {
-	unsigned long ret; /* Return address */
 	unsigned long func; /* Current function */
 	unsigned long long calltime;
 	unsigned long long rettime;
 	/* Number of functions that overran the depth limit for current task */
 	unsigned long overrun;
+	int depth;
 };
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 #define FTRACE_RETFUNC_DEPTH 50
 #define FTRACE_RETSTACK_ALLOC_SIZE 32
-/* Type of a callback handler of tracing return function */
-typedef void (*trace_function_graph_t)(struct ftrace_graph_ret *);
+/* Type of the callback handlers for tracing function graph*/
+typedef void (*trace_func_graph_ret_t)(struct ftrace_graph_ret *); /* return */
+typedef void (*trace_func_graph_ent_t)(struct ftrace_graph_ent *); /* entry */
+
+extern int register_ftrace_graph(trace_func_graph_ret_t retfunc,
+				trace_func_graph_ent_t entryfunc);
+
+/* The current handlers in use */
+extern trace_func_graph_ret_t ftrace_graph_return;
+extern trace_func_graph_ent_t ftrace_graph_entry;
 
-extern int register_ftrace_graph(trace_function_graph_t func);
-/* The current handler in use */
-extern trace_function_graph_t ftrace_graph_function;
 extern void unregister_ftrace_graph(void);
 
 extern void ftrace_graph_init_task(struct task_struct *t);
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 9e19976..7e2d3b9 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1498,12 +1498,13 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
-static atomic_t ftrace_retfunc_active;
-
-/* The callback that hooks the return of a function */
-trace_function_graph_t ftrace_graph_function =
-			(trace_function_graph_t)ftrace_stub;
+static atomic_t ftrace_graph_active;
 
+/* The callbacks that hook a function */
+trace_func_graph_ret_t ftrace_graph_return =
+			(trace_func_graph_ret_t)ftrace_stub;
+trace_func_graph_ent_t ftrace_graph_entry =
+			(trace_func_graph_ent_t)ftrace_stub;
 
 /* Try to assign a return stack array on FTRACE_RETSTACK_ALLOC_SIZE tasks. */
 static int alloc_retstack_tasklist(struct ftrace_ret_stack **ret_stack_list)
@@ -1569,7 +1570,8 @@ static int start_graph_tracing(void)
 	return ret;
 }
 
-int register_ftrace_graph(trace_function_graph_t func)
+int register_ftrace_graph(trace_func_graph_ret_t retfunc,
+			trace_func_graph_ent_t entryfunc)
 {
 	int ret = 0;
 
@@ -1583,14 +1585,15 @@ int register_ftrace_graph(trace_function_graph_t func)
 		ret = -EBUSY;
 		goto out;
 	}
-	atomic_inc(&ftrace_retfunc_active);
+	atomic_inc(&ftrace_graph_active);
 	ret = start_graph_tracing();
 	if (ret) {
-		atomic_dec(&ftrace_retfunc_active);
+		atomic_dec(&ftrace_graph_active);
 		goto out;
 	}
 	ftrace_tracing_type = FTRACE_TYPE_RETURN;
-	ftrace_graph_function = func;
+	ftrace_graph_return = retfunc;
+	ftrace_graph_entry = entryfunc;
 	ftrace_startup();
 
 out:
@@ -1602,8 +1605,9 @@ void unregister_ftrace_graph(void)
 {
 	mutex_lock(&ftrace_sysctl_lock);
 
-	atomic_dec(&ftrace_retfunc_active);
-	ftrace_graph_function = (trace_function_graph_t)ftrace_stub;
+	atomic_dec(&ftrace_graph_active);
+	ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub;
+	ftrace_graph_entry = (trace_func_graph_ent_t)ftrace_stub;
 	ftrace_shutdown();
 	/* Restore normal tracing type */
 	ftrace_tracing_type = FTRACE_TYPE_ENTER;
@@ -1614,7 +1618,7 @@ void unregister_ftrace_graph(void)
 /* Allocate a return stack for newly created task */
 void ftrace_graph_init_task(struct task_struct *t)
 {
-	if (atomic_read(&ftrace_retfunc_active)) {
+	if (atomic_read(&ftrace_graph_active)) {
 		t->ret_stack = kmalloc(FTRACE_RETFUNC_DEPTH
 				* sizeof(struct ftrace_ret_stack),
 				GFP_KERNEL);
@@ -1638,5 +1642,3 @@ void ftrace_graph_exit_task(struct task_struct *t)
 }
 #endif
 
-
-
diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index f21ab2c..9d5f7c9 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -879,14 +879,38 @@ trace_function(struct trace_array *tr, struct trace_array_cpu *data,
 }
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-static void __trace_function_graph(struct trace_array *tr,
+static void __trace_graph_entry(struct trace_array *tr,
+				struct trace_array_cpu *data,
+				struct ftrace_graph_ent *trace,
+				unsigned long flags,
+				int pc)
+{
+	struct ring_buffer_event *event;
+	struct ftrace_graph_ent_entry *entry;
+	unsigned long irq_flags;
+
+	if (unlikely(local_read(&__get_cpu_var(ftrace_cpu_disabled))))
+		return;
+
+	event = ring_buffer_lock_reserve(global_trace.buffer, sizeof(*entry),
+					 &irq_flags);
+	if (!event)
+		return;
+	entry	= ring_buffer_event_data(event);
+	tracing_generic_entry_update(&entry->ent, flags, pc);
+	entry->ent.type			= TRACE_GRAPH_ENT;
+	entry->graph_ent			= *trace;
+	ring_buffer_unlock_commit(global_trace.buffer, event, irq_flags);
+}
+
+static void __trace_graph_return(struct trace_array *tr,
 				struct trace_array_cpu *data,
 				struct ftrace_graph_ret *trace,
 				unsigned long flags,
 				int pc)
 {
 	struct ring_buffer_event *event;
-	struct ftrace_graph_entry *entry;
+	struct ftrace_graph_ret_entry *entry;
 	unsigned long irq_flags;
 
 	if (unlikely(local_read(&__get_cpu_var(ftrace_cpu_disabled))))
@@ -898,12 +922,8 @@ static void __trace_function_graph(struct trace_array *tr,
 		return;
 	entry	= ring_buffer_event_data(event);
 	tracing_generic_entry_update(&entry->ent, flags, pc);
-	entry->ent.type			= TRACE_FN_RET;
-	entry->ip			= trace->func;
-	entry->parent_ip	= trace->ret;
-	entry->rettime		= trace->rettime;
-	entry->calltime		= trace->calltime;
-	entry->overrun		= trace->overrun;
+	entry->ent.type			= TRACE_GRAPH_RET;
+	entry->ret				= *trace;
 	ring_buffer_unlock_commit(global_trace.buffer, event, irq_flags);
 }
 #endif
@@ -1178,7 +1198,7 @@ function_trace_call(unsigned long ip, unsigned long parent_ip)
 }
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-void trace_function_graph(struct ftrace_graph_ret *trace)
+void trace_graph_entry(struct ftrace_graph_ent *trace)
 {
 	struct trace_array *tr = &global_trace;
 	struct trace_array_cpu *data;
@@ -1193,7 +1213,28 @@ void trace_function_graph(struct ftrace_graph_ret *trace)
 	disabled = atomic_inc_return(&data->disabled);
 	if (likely(disabled == 1)) {
 		pc = preempt_count();
-		__trace_function_graph(tr, data, trace, flags, pc);
+		__trace_graph_entry(tr, data, trace, flags, pc);
+	}
+	atomic_dec(&data->disabled);
+	raw_local_irq_restore(flags);
+}
+
+void trace_graph_return(struct ftrace_graph_ret *trace)
+{
+	struct trace_array *tr = &global_trace;
+	struct trace_array_cpu *data;
+	unsigned long flags;
+	long disabled;
+	int cpu;
+	int pc;
+
+	raw_local_irq_save(flags);
+	cpu = raw_smp_processor_id();
+	data = tr->data[cpu];
+	disabled = atomic_inc_return(&data->disabled);
+	if (likely(disabled == 1)) {
+		pc = preempt_count();
+		__trace_graph_return(tr, data, trace, flags, pc);
 	}
 	atomic_dec(&data->disabled);
 	raw_local_irq_restore(flags);
@@ -2000,9 +2041,11 @@ static enum print_line_t print_trace_fmt(struct trace_iterator *iter)
 			trace_seq_print_cont(s, iter);
 		break;
 	}
-	case TRACE_FN_RET: {
+	case TRACE_GRAPH_RET: {
+		return print_graph_function(iter);
+	}
+	case TRACE_GRAPH_ENT: {
 		return print_graph_function(iter);
-		break;
 	}
 	case TRACE_BRANCH: {
 		struct trace_branch *field;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 72b5ef8..ffe1bb1 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -25,7 +25,8 @@ enum trace_type {
 	TRACE_BRANCH,
 	TRACE_BOOT_CALL,
 	TRACE_BOOT_RET,
-	TRACE_FN_RET,
+	TRACE_GRAPH_RET,
+	TRACE_GRAPH_ENT,
 	TRACE_USER_STACK,
 	TRACE_BTS,
 
@@ -56,14 +57,16 @@ struct ftrace_entry {
 	unsigned long		parent_ip;
 };
 
+/* Function call entry */
+struct ftrace_graph_ent_entry {
+	struct trace_entry			ent;
+	struct ftrace_graph_ent		graph_ent;
+};
+
 /* Function return entry */
-struct ftrace_graph_entry {
-	struct trace_entry	ent;
-	unsigned long		ip;
-	unsigned long		parent_ip;
-	unsigned long long	calltime;
-	unsigned long long	rettime;
-	unsigned long		overrun;
+struct ftrace_graph_ret_entry {
+	struct trace_entry			ent;
+	struct ftrace_graph_ret		ret;
 };
 extern struct tracer boot_tracer;
 
@@ -264,7 +267,10 @@ extern void __ftrace_bad_type(void);
 		IF_ASSIGN(var, ent, struct trace_boot_call, TRACE_BOOT_CALL);\
 		IF_ASSIGN(var, ent, struct trace_boot_ret, TRACE_BOOT_RET);\
 		IF_ASSIGN(var, ent, struct trace_branch, TRACE_BRANCH); \
-		IF_ASSIGN(var, ent, struct ftrace_graph_entry, TRACE_FN_RET);\
+		IF_ASSIGN(var, ent, struct ftrace_graph_ent_entry,	\
+			  TRACE_GRAPH_ENT);		\
+		IF_ASSIGN(var, ent, struct ftrace_graph_ret_entry,	\
+			  TRACE_GRAPH_RET);		\
 		IF_ASSIGN(var, ent, struct bts_entry, TRACE_BTS);\
 		__ftrace_bad_type();					\
 	} while (0)
@@ -397,9 +403,9 @@ void trace_function(struct trace_array *tr,
 		    unsigned long ip,
 		    unsigned long parent_ip,
 		    unsigned long flags, int pc);
-void
-trace_function_graph(struct ftrace_graph_ret *trace);
 
+void trace_graph_return(struct ftrace_graph_ret *trace);
+void trace_graph_entry(struct ftrace_graph_ent *trace);
 void trace_bts(struct trace_array *tr,
 	       unsigned long from,
 	       unsigned long to);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index f5bad46..b6f0cc2 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -13,6 +13,7 @@
 
 #include "trace.h"
 
+#define TRACE_GRAPH_INDENT	2
 
 #define TRACE_GRAPH_PRINT_OVERRUN	0x1
 static struct tracer_opt trace_opts[] = {
@@ -26,6 +27,8 @@ static struct tracer_flags tracer_flags = {
 	.opts = trace_opts
 };
 
+/* pid on the last trace processed */
+static pid_t last_pid = -1;
 
 static int graph_trace_init(struct trace_array *tr)
 {
@@ -33,7 +36,8 @@ static int graph_trace_init(struct trace_array *tr)
 	for_each_online_cpu(cpu)
 		tracing_reset(tr, cpu);
 
-	return register_ftrace_graph(&trace_function_graph);
+	return register_ftrace_graph(&trace_graph_return,
+					&trace_graph_entry);
 }
 
 static void graph_trace_reset(struct trace_array *tr)
@@ -41,45 +45,97 @@ static void graph_trace_reset(struct trace_array *tr)
 		unregister_ftrace_graph();
 }
 
+/* If the pid changed since the last trace, output this event */
+static int verif_pid(struct trace_seq *s, pid_t pid)
+{
+	if (last_pid != -1 && last_pid == pid)
+		return 1;
 
-enum print_line_t
-print_graph_function(struct trace_iterator *iter)
+	last_pid = pid;
+	return trace_seq_printf(s, "\n------------8<---------- thread %d"
+				    " ------------8<----------\n\n",
+				  pid);
+}
+
+static enum print_line_t
+print_graph_entry(struct ftrace_graph_ent *call, struct trace_seq *s,
+		struct trace_entry *ent)
 {
-	struct trace_seq *s = &iter->seq;
-	struct trace_entry *entry = iter->ent;
-	struct ftrace_graph_entry *field;
+	int i;
 	int ret;
 
-	if (entry->type == TRACE_FN_RET) {
-		trace_assign_type(field, entry);
-		ret = trace_seq_printf(s, "%pF -> ", (void *)field->parent_ip);
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
+	if (!verif_pid(s, ent->pid))
+		return TRACE_TYPE_PARTIAL_LINE;
 
-		ret = seq_print_ip_sym(s, field->ip,
-					trace_flags & TRACE_ITER_SYM_MASK);
+	for (i = 0; i < call->depth * TRACE_GRAPH_INDENT; i++) {
+		ret = trace_seq_printf(s, " ");
 		if (!ret)
 			return TRACE_TYPE_PARTIAL_LINE;
+	}
+
+	ret = seq_print_ip_sym(s, call->func, 0);
+	if (!ret)
+		return TRACE_TYPE_PARTIAL_LINE;
+
+	ret = trace_seq_printf(s, "() {\n");
+	if (!ret)
+		return TRACE_TYPE_PARTIAL_LINE;
+	return TRACE_TYPE_HANDLED;
+}
+
+static enum print_line_t
+print_graph_return(struct ftrace_graph_ret *trace, struct trace_seq *s,
+		   struct trace_entry *ent)
+{
+	int i;
+	int ret;
+
+	if (!verif_pid(s, ent->pid))
+		return TRACE_TYPE_PARTIAL_LINE;
 
-		ret = trace_seq_printf(s, " (%llu ns)",
-					field->rettime - field->calltime);
+	for (i = 0; i < trace->depth * TRACE_GRAPH_INDENT; i++) {
+		ret = trace_seq_printf(s, " ");
 		if (!ret)
 			return TRACE_TYPE_PARTIAL_LINE;
+	}
+
+	ret = trace_seq_printf(s, "} ");
+	if (!ret)
+		return TRACE_TYPE_PARTIAL_LINE;
 
-		if (tracer_flags.val & TRACE_GRAPH_PRINT_OVERRUN) {
-			ret = trace_seq_printf(s, " (Overruns: %lu)",
-						field->overrun);
-			if (!ret)
-				return TRACE_TYPE_PARTIAL_LINE;
-		}
+	ret = trace_seq_printf(s, "%llu\n", trace->rettime - trace->calltime);
+	if (!ret)
+		return TRACE_TYPE_PARTIAL_LINE;
 
-		ret = trace_seq_printf(s, "\n");
+	if (tracer_flags.val & TRACE_GRAPH_PRINT_OVERRUN) {
+		ret = trace_seq_printf(s, " (Overruns: %lu)\n",
+					trace->overrun);
 		if (!ret)
 			return TRACE_TYPE_PARTIAL_LINE;
+	}
+	return TRACE_TYPE_HANDLED;
+}
+
+enum print_line_t
+print_graph_function(struct trace_iterator *iter)
+{
+	struct trace_seq *s = &iter->seq;
+	struct trace_entry *entry = iter->ent;
 
-		return TRACE_TYPE_HANDLED;
+	switch (entry->type) {
+	case TRACE_GRAPH_ENT: {
+		struct ftrace_graph_ent_entry *field;
+		trace_assign_type(field, entry);
+		return print_graph_entry(&field->graph_ent, s, entry);
+	}
+	case TRACE_GRAPH_RET: {
+		struct ftrace_graph_ret_entry *field;
+		trace_assign_type(field, entry);
+		return print_graph_return(&field->ret, s, entry);
+	}
+	default:
+		return TRACE_TYPE_UNHANDLED;
 	}
-	return TRACE_TYPE_UNHANDLED;
 }
 
 static struct tracer graph_trace __read_mostly = {
diff --git a/kernel/trace/trace_functions_return.c b/kernel/trace/trace_functions_return.c
deleted file mode 100644
index e00d645..0000000
--- a/kernel/trace/trace_functions_return.c
+++ /dev/null
@@ -1,98 +0,0 @@
-/*
- *
- * Function return tracer.
- * Copyright (c) 2008 Frederic Weisbecker <fweisbec@gmail.com>
- * Mostly borrowed from function tracer which
- * is Copyright (c) Steven Rostedt <srostedt@redhat.com>
- *
- */
-#include <linux/debugfs.h>
-#include <linux/uaccess.h>
-#include <linux/ftrace.h>
-#include <linux/fs.h>
-
-#include "trace.h"
-
-
-#define TRACE_RETURN_PRINT_OVERRUN	0x1
-static struct tracer_opt trace_opts[] = {
-	/* Display overruns or not */
-	{ TRACER_OPT(overrun, TRACE_RETURN_PRINT_OVERRUN) },
-	{ } /* Empty entry */
-};
-
-static struct tracer_flags tracer_flags = {
-	.val = 0, /* Don't display overruns by default */
-	.opts = trace_opts
-};
-
-
-static int return_trace_init(struct trace_array *tr)
-{
-	int cpu;
-	for_each_online_cpu(cpu)
-		tracing_reset(tr, cpu);
-
-	return register_ftrace_return(&trace_function_return);
-}
-
-static void return_trace_reset(struct trace_array *tr)
-{
-		unregister_ftrace_return();
-}
-
-
-enum print_line_t
-print_return_function(struct trace_iterator *iter)
-{
-	struct trace_seq *s = &iter->seq;
-	struct trace_entry *entry = iter->ent;
-	struct ftrace_ret_entry *field;
-	int ret;
-
-	if (entry->type == TRACE_FN_RET) {
-		trace_assign_type(field, entry);
-		ret = trace_seq_printf(s, "%pF -> ", (void *)field->parent_ip);
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
-
-		ret = seq_print_ip_sym(s, field->ip,
-					trace_flags & TRACE_ITER_SYM_MASK);
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
-
-		ret = trace_seq_printf(s, " (%llu ns)",
-					field->rettime - field->calltime);
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
-
-		if (tracer_flags.val & TRACE_RETURN_PRINT_OVERRUN) {
-			ret = trace_seq_printf(s, " (Overruns: %lu)",
-						field->overrun);
-			if (!ret)
-				return TRACE_TYPE_PARTIAL_LINE;
-		}
-
-		ret = trace_seq_printf(s, "\n");
-		if (!ret)
-			return TRACE_TYPE_PARTIAL_LINE;
-
-		return TRACE_TYPE_HANDLED;
-	}
-	return TRACE_TYPE_UNHANDLED;
-}
-
-static struct tracer return_trace __read_mostly = {
-	.name	     = "return",
-	.init	     = return_trace_init,
-	.reset	     = return_trace_reset,
-	.print_line = print_return_function,
-	.flags		= &tracer_flags,
-};
-
-static __init int init_return_trace(void)
-{
-	return register_tracer(&return_trace);
-}
-
-device_initcall(init_return_trace);
-- 
1.5.2.5

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Here is an example of trace:
> 
> sys_read() {
>   fget_light() {
>   } 526
>   vfs_read() {
>     rw_verify_area() {
>       security_file_permission() {
>         cap_file_permission() {
>         } 519
>       } 1564
>     } 2640
>     do_sync_read() {
>       pipe_read() {
>         __might_sleep() {
>         } 511
>         pipe_wait() {
>           prepare_to_wait() {
>           } 760
>           deactivate_task() {
>             dequeue_task() {
>               dequeue_task_fair() {
>                 dequeue_entity() {
>                   update_curr() {
>                     update_min_vruntime() {
>                     } 504
>                   } 1587
>                   clear_buddies() {
>                   } 512
>                   add_cfs_task_weight() {
>                   } 519
>                   update_min_vruntime() {
>                   } 511
>                 } 5602
>                 dequeue_entity() {
>                   update_curr() {
>                     update_min_vruntime() {
>                     } 496
>                   } 1631
>                   clear_buddies() {
>                   } 496
>                   update_min_vruntime() {
>                   } 527
>                 } 4580
>                 hrtick_update() {
>                   hrtick_start_fair() {
>                   } 488
>                 } 1489
>               } 13700
>             } 14949
>           } 16016
>           msecs_to_jiffies() {
>           } 496
>           put_prev_task_fair() {
>           } 504
>           pick_next_task_fair() {
>           } 489
>           pick_next_task_rt() {
>           } 496
>           pick_next_task_fair() {
>           } 489
>           pick_next_task_idle() {
>           } 489
> 
> ------------8<---------- thread 4 ------------8<----------

Wow, very nice output! :-)

I've applied them, will comment and test on them as well.

	Ingo

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> Wow, very nice output! :-)
> 
> I've applied them, will comment and test on them as well.

I kept playing with this for almost an hour and kept looking at pages 
and pages of tracer output - it really shows the kernel's flow in a 
completely different dimension!

Talked to Steve about it and we have a few minor suggestions regarding 
the default output:

I think we should optimize the visual qualities of the output some 
more.

Instead of:

 pick_next_task() {   
   pick_next_task_fair() {   
     wakeup_preempt_entity() { 
     } 301
     set_next_entity() {    
       update_stats_wait_end() { 
       } 281
       __dequeue_entity() { 
       } 319
     } 1990
     hrtick_start_fair() { 
     } 276
   } 4524                                 
 } 5341                 

We should print such traces [mockup]:

   pick_next_task() {                     -
     pick_next_task_fair() {                -
       wakeup_preempt_entity();               + 0.301 us
       set_next_entity() {                    -
         update_stats_wait_end();               + 0.281 us
         __dequeue_entity();                    + 0.319 us
       }                                      + 1.990 us
       hrtick_start_fair();                   + 0.276 us
 +   }                                      + 10.524 us
 ! }                                      + 100.341 us
   switch_mm();                           + 0.494 us

The changes would be:

1) Compression of non-nested calls into a single line.

   Implementing this probably necessiates some trickery with the
   ring-buffer: we'd have to look at the next entry as well and see 
   whether it closes the function call.

2) Adding a closing ';' semicolon to single-line calls. It's the C 
   syntax and i'm missing it :-)

3) The first column: single-character visual shortcuts for "overhead". 
   This is a concept we used in the -rt tracer and it still lives in 
   the latency tracer bits of ftrace and is quite useful:

    '+' means "overhead spike": overhead is above 10 usecs.
    '!' means "large overhead": overhead is above 100 usecs.

   These give at-a-glance hotspot analysis - hotspots are easier to 
   miss as pure numbers.

4) Printing out in units of microseconds, with a decimal point and by
   explicitly mentioning the 'us' unit.

   All of these are important properties: microseconds is still the 
   most common 'overhead' unit in kernel circles. It also gives rather 
   human single-digit or two-digit results. Nanoseconds is three or- 
   four-digit - which is too much. Repeating 'us' so many times is not 
   a problem - these lines will be grepped, quoted, etc - constant 
   reminder about the unit is just fine.

5) Nesting the overhead numbers aligned to the right end of the graph. 
 
   This cleans up the view of the source code printout, but not 
   cluttering it with numbers. It also structures the numbers some 
   more. Visual cues are given via '-' and '+' signs, to follow 
   nesting. This nesting aligns up with the function nesting levels on 
   the left side - so it's easy to pair up number and function name. 
   It also separates the numbers from the symbol length effects in the 
   single-line case. (which is the most common trace line)

What do you think? It's lots of work (sorry! :-), but it would produce 
even more stunning graph-tracer output IMO.

	Ingo

[PATCH] tracing: function graph tracer, fix

[Ingo Molnar]

small fixlet.

------------->
>From c2324b694fa8ffee382a124198c68754088e483c Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Wed, 26 Nov 2008 03:10:01 +0100
Subject: [PATCH] tracing: function graph tracer, fix

fix return-tracer => graph-tracer namespace rename fallout.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/entry_32.S |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 2b1f0f0..7def9fd 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -1189,7 +1189,7 @@ ENTRY(mcount)
 	cmpl $ftrace_stub, ftrace_trace_function
 	jnz trace
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
-	cmpl $ftrace_stub, ftrace_graph_function
+	cmpl $ftrace_stub, ftrace_graph_return
 	jnz ftrace_graph_caller
 #endif
 .globl ftrace_stub

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Steven Rostedt]

On Wed, 26 Nov 2008, Ingo Molnar wrote:
> 
> The changes would be:
> 
> 1) Compression of non-nested calls into a single line.
> 
>    Implementing this probably necessiates some trickery with the
>    ring-buffer: we'd have to look at the next entry as well and see 
>    whether it closes the function call.

The latency_trace file does this already:

You can look into the trace buffer without moving it with:

  entry = ring_buffer_iter_peek(iter->buffer_iter[iter->cpu], &ts);

where, ts will give you the time stamp, it can be NULL if you do not care.

When your print function is called, the item in the ring buffer has 
already been consumed. So the peek will give you the next item in the ring 
buffer.

> 
> 2) Adding a closing ';' semicolon to single-line calls. It's the C 
>    syntax and i'm missing it :-)
> 
> 3) The first column: single-character visual shortcuts for "overhead". 
>    This is a concept we used in the -rt tracer and it still lives in 
>    the latency tracer bits of ftrace and is quite useful:
> 
>     '+' means "overhead spike": overhead is above 10 usecs.
>     '!' means "large overhead": overhead is above 100 usecs.
> 
>    These give at-a-glance hotspot analysis - hotspots are easier to 
>    miss as pure numbers.

The latency_trace file has this too. And it uses the above peek to figure 
it out ;-)

> 
> 4) Printing out in units of microseconds, with a decimal point and by
>    explicitly mentioning the 'us' unit.
> 
>    All of these are important properties: microseconds is still the 
>    most common 'overhead' unit in kernel circles. It also gives rather 
>    human single-digit or two-digit results. Nanoseconds is three or- 
>    four-digit - which is too much. Repeating 'us' so many times is not 
>    a problem - these lines will be grepped, quoted, etc - constant 
>    reminder about the unit is just fine.
> 
> 5) Nesting the overhead numbers aligned to the right end of the graph. 
>  
>    This cleans up the view of the source code printout, but not 
>    cluttering it with numbers. It also structures the numbers some 
>    more. Visual cues are given via '-' and '+' signs, to follow 
>    nesting. This nesting aligns up with the function nesting levels on 
>    the left side - so it's easy to pair up number and function name. 
>    It also separates the numbers from the symbol length effects in the 
>    single-line case. (which is the most common trace line)
> 
> What do you think? It's lots of work (sorry! :-), but it would produce 
> even more stunning graph-tracer output IMO.

I like it, and I hope Frederic has enough time on his hands to do it ;)

-- Steve

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Frédéric Weisbecker]

2008/11/26 Steven Rostedt <rostedt@goodmis.org>:
>
> On Wed, 26 Nov 2008, Ingo Molnar wrote:
>>
>> The changes would be:
>>
>> 1) Compression of non-nested calls into a single line.
>>
>>    Implementing this probably necessiates some trickery with the
>>    ring-buffer: we'd have to look at the next entry as well and see
>>    whether it closes the function call.
>
> The latency_trace file does this already:
>
> You can look into the trace buffer without moving it with:
>
>  entry = ring_buffer_iter_peek(iter->buffer_iter[iter->cpu], &ts);
>
> where, ts will give you the time stamp, it can be NULL if you do not care.
>
> When your print function is called, the item in the ring buffer has
> already been consumed. So the peek will give you the next item in the ring
> buffer.
>>
>> 2) Adding a closing ';' semicolon to single-line calls. It's the C
>>    syntax and i'm missing it :-)
>>
>> 3) The first column: single-character visual shortcuts for "overhead".
>>    This is a concept we used in the -rt tracer and it still lives in
>>    the latency tracer bits of ftrace and is quite useful:
>>
>>     '+' means "overhead spike": overhead is above 10 usecs.
>>     '!' means "large overhead": overhead is above 100 usecs.
>>
>>    These give at-a-glance hotspot analysis - hotspots are easier to
>>    miss as pure numbers.
>
> The latency_trace file has this too. And it uses the above peek to figure
> it out ;-)


That's right, the latency_trace does it. But it is based on a
ring-buffer entry timestamp.
I wanted to use ring-buffer entry timestamp, but it would be hard to
calculate the duration
since the return trace of a function doesn't often follow immediately
its entry trace.
And I can't reuse lat_print_timestamp() because I need the nsec
remaining part....

This is too specific to reuse latency_trace...

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Steven Rostedt]

On Wed, 26 Nov 2008, Fr?d?ric Weisbecker wrote:

> >>
> >> 3) The first column: single-character visual shortcuts for "overhead".
> >>    This is a concept we used in the -rt tracer and it still lives in
> >>    the latency tracer bits of ftrace and is quite useful:
> >>
> >>     '+' means "overhead spike": overhead is above 10 usecs.
> >>     '!' means "large overhead": overhead is above 100 usecs.
> >>
> >>    These give at-a-glance hotspot analysis - hotspots are easier to
> >>    miss as pure numbers.
> >
> > The latency_trace file has this too. And it uses the above peek to figure
> > it out ;-)
> 
> 
> That's right, the latency_trace does it. But it is based on a
> ring-buffer entry timestamp.
> I wanted to use ring-buffer entry timestamp, but it would be hard to
> calculate the duration
> since the return trace of a function doesn't often follow immediately
> its entry trace.
> And I can't reuse lat_print_timestamp() because I need the nsec
> remaining part....
> 
> This is too specific to reuse latency_trace...

I'd suggest just looking at how those are implemented, and use your own. 
One note, the latency tracer looks at the next entry, even if it is on 
another CPU. This is incorrect, and will definitely be incorrect for you. 
Just use the ring_buffer_iter_peek, that will get you what you want. Well, 
for leaf functions.

-- Steve

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Frédéric Weisbecker]

2008/11/26 Steven Rostedt <rostedt@goodmis.org>:
> I'd suggest just looking at how those are implemented, and use your own.
> One note, the latency tracer looks at the next entry, even if it is on
> another CPU. This is incorrect, and will definitely be incorrect for you.
> Just use the ring_buffer_iter_peek, that will get you what you want. Well,
> for leaf functions.
>
> -- Steve
>
>


Ok, thanks for your tips!

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Frédéric Weisbecker]

2008/11/26 Ingo Molnar <mingo@elte.hu>:
> 1) Compression of non-nested calls into a single line.
>
>   Implementing this probably necessiates some trickery with the
>   ring-buffer: we'd have to look at the next entry as well and see
>   whether it closes the function call.


I wanted to do so. That's not so easy, I guess I will not only have to read
the next entry. I'll have to modify it to avoid the next entry to
print a closing brace.
But I don't think that would be a problem.
Note with this method that if there is a context switch or an
interrupt between the entry and
the return of a leaf function, it will not anymore be considered as a non-nested
function on its output.
But the case would be rare enough, so I will apply this.

> 2) Adding a closing ';' semicolon to single-line calls. It's the C
>   syntax and i'm missing it :-)


Yeah :-)


> 3) The first column: single-character visual shortcuts for "overhead".
>   This is a concept we used in the -rt tracer and it still lives in
>   the latency tracer bits of ftrace and is quite useful:
>
>    '+' means "overhead spike": overhead is above 10 usecs.
>    '!' means "large overhead": overhead is above 100 usecs.
>
>   These give at-a-glance hotspot analysis - hotspots are easier to
>   miss as pure numbers.
>
> 4) Printing out in units of microseconds, with a decimal point and by
>   explicitly mentioning the 'us' unit.
>
>   All of these are important properties: microseconds is still the
>   most common 'overhead' unit in kernel circles. It also gives rather
>   human single-digit or two-digit results. Nanoseconds is three or-
>   four-digit - which is too much. Repeating 'us' so many times is not
>   a problem - these lines will be grepped, quoted, etc - constant
>   reminder about the unit is just fine.
>
> 5) Nesting the overhead numbers aligned to the right end of the graph.
>
>   This cleans up the view of the source code printout, but not
>   cluttering it with numbers. It also structures the numbers some
>   more. Visual cues are given via '-' and '+' signs, to follow
>   nesting. This nesting aligns up with the function nesting levels on
>   the left side - so it's easy to pair up number and function name.
>   It also separates the numbers from the symbol length effects in the
>   single-line case. (which is the most common trace line)


Totally agree!


> What do you think? It's lots of work (sorry! :-), but it would produce
> even more stunning graph-tracer output IMO.

Yes, I knew this was only a first try when I submitted it :-)
I will apply these suggestions.

Thanks!

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Steven Rostedt]

On Wed, 26 Nov 2008, Fr?d?ric Weisbecker wrote:

> 2008/11/26 Ingo Molnar <mingo@elte.hu>:
> > 1) Compression of non-nested calls into a single line.
> >
> >   Implementing this probably necessiates some trickery with the
> >   ring-buffer: we'd have to look at the next entry as well and see
> >   whether it closes the function call.
> 
> 
> I wanted to do so. That's not so easy, I guess I will not only have to read
> the next entry. I'll have to modify it to avoid the next entry to
> print a closing brace.

You could run a ring_buffer_read(iter->buffer_iter[iter->cpu], NULL)
and that will consume the next entry, so it will not print.

> But I don't think that would be a problem.
> Note with this method that if there is a context switch or an
> interrupt between the entry and
> the return of a leaf function, it will not anymore be considered as a non-nested
> function on its output.
> But the case would be rare enough, so I will apply this.

Yeah, we would not care about that case. In fact it may be best to keep it 
as is.

-- Steve

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Ingo Molnar]

there's a small inconsistency here:

# cat available_tracers 
function-graph function sched_switch nop

note the '-' versus '_' separators. I wouldnt mind renaming the sched 
tracer to sched-switch. (and some of the other tracers as well)

	Ingo

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Frédéric Weisbecker]

2008/11/26 Ingo Molnar <mingo@elte.hu>:
>
> there's a small inconsistency here:
>
> # cat available_tracers
> function-graph function sched_switch nop
>
> note the '-' versus '_' separators. I wouldnt mind renaming the sched
> tracer to sched-switch. (and some of the other tracers as well)
>
>        Ingo
>

Will correct it too.

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Ingo Molnar]

the regex filter doesnt work well yet when used with the graph tracer:

[ 2994.079079] ------------[ cut here ]------------
[ 2994.079079] WARNING: at kernel/trace/ftrace.c:373 ftrace_bug+0x8c/0x1ce()
[ 2994.079079] Modules linked in:
[ 2994.079079] Pid: 12, comm: kstop/0 Not tainted 2.6.28-rc6-tip #2056
[ 2994.079079] Call Trace:
[ 2994.079079]  [<c02273dd>] warn_on_slowpath+0x46/0x60
[ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
[ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
[ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
[ 2994.079079]  [<c0357f66>] ? memcpy+0xe/0x31
[ 2994.079079]  [<c02633e6>] ? rb_commit+0x29/0x2d
[ 2994.079079]  [<c0264c8b>] ? ring_buffer_unlock_commit+0x15/0x1a
[ 2994.079079]  [<c0213ece>] ? ftrace_modify_code+0x21/0xa3
[ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
[ 2994.079079]  [<c026c3ce>] ? probe_kernel_read+0xe/0x5c
[ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
[ 2994.079079]  [<c0203926>] ? return_to_handler+0xa/0x14
[ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
[ 2994.079079]  [<c0213ee5>] ? ftrace_modify_code+0x38/0xa3
[ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
[ 2994.079079]  [<c0262749>] ftrace_bug+0x8c/0x1ce
[ 2994.079079]  [<c026299c>] ftrace_replace_code+0x111/0x13d
[ 2994.079079]  [<c0253b53>] ? stop_cpu+0x0/0xa5
[ 2994.079079]  [<c02629e6>] __ftrace_modify_code+0x1e/0x32
[ 2994.079079]  [<c0253bbe>] stop_cpu+0x6b/0xa5
[ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
[ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
[ 2994.079079]  [<c0237fc1>] ? autoremove_wake_function+0x0/0x35
[ 2994.079079]  [<c023584b>] ? worker_thread+0x0/0xc7
[ 2994.079079]  [<c0237eba>] kthread+0x40/0x66
[ 2994.079079]  [<c0237e7a>] ? kthread+0x0/0x66
[ 2994.079079]  [<c02038cb>] kernel_thread_helper+0x7/0x10
[ 2994.079079] ---[ end trace bff97d149b80ecde ]---
[ 2994.079079] ftrace failed to modify [<c0261ec7>] trace_selftest_dynamic_test_func+0x3/0xc
[ 2994.079079]  actual: e8:0c:1a:fa:ff

produced via:

 # cd /debug/tracing/
 # cat set_ftrace_filter
 # echo schedule > set_ftrace_filter
 # cat set_ftrace_filter
 cat: set_ftrace_filter: No such device

	Ingo

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Steven Rostedt]

On Wed, 26 Nov 2008, Ingo Molnar wrote:

> 
> the regex filter doesnt work well yet when used with the graph tracer:
> 
> [ 2994.079079] ------------[ cut here ]------------
> [ 2994.079079] WARNING: at kernel/trace/ftrace.c:373 ftrace_bug+0x8c/0x1ce()
> [ 2994.079079] Modules linked in:
> [ 2994.079079] Pid: 12, comm: kstop/0 Not tainted 2.6.28-rc6-tip #2056
> [ 2994.079079] Call Trace:
> [ 2994.079079]  [<c02273dd>] warn_on_slowpath+0x46/0x60
> [ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
> [ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
> [ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
> [ 2994.079079]  [<c0357f66>] ? memcpy+0xe/0x31
> [ 2994.079079]  [<c02633e6>] ? rb_commit+0x29/0x2d
> [ 2994.079079]  [<c0264c8b>] ? ring_buffer_unlock_commit+0x15/0x1a
> [ 2994.079079]  [<c0213ece>] ? ftrace_modify_code+0x21/0xa3
> [ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
> [ 2994.079079]  [<c026c3ce>] ? probe_kernel_read+0xe/0x5c
> [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> [ 2994.079079]  [<c0203926>] ? return_to_handler+0xa/0x14
> [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> [ 2994.079079]  [<c0213ee5>] ? ftrace_modify_code+0x38/0xa3
> [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> [ 2994.079079]  [<c0262749>] ftrace_bug+0x8c/0x1ce
> [ 2994.079079]  [<c026299c>] ftrace_replace_code+0x111/0x13d
> [ 2994.079079]  [<c0253b53>] ? stop_cpu+0x0/0xa5
> [ 2994.079079]  [<c02629e6>] __ftrace_modify_code+0x1e/0x32
> [ 2994.079079]  [<c0253bbe>] stop_cpu+0x6b/0xa5
> [ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
> [ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
> [ 2994.079079]  [<c0237fc1>] ? autoremove_wake_function+0x0/0x35
> [ 2994.079079]  [<c023584b>] ? worker_thread+0x0/0xc7
> [ 2994.079079]  [<c0237eba>] kthread+0x40/0x66
> [ 2994.079079]  [<c0237e7a>] ? kthread+0x0/0x66
> [ 2994.079079]  [<c02038cb>] kernel_thread_helper+0x7/0x10
> [ 2994.079079] ---[ end trace bff97d149b80ecde ]---
> [ 2994.079079] ftrace failed to modify [<c0261ec7>] trace_selftest_dynamic_test_func+0x3/0xc
> [ 2994.079079]  actual: e8:0c:1a:fa:ff
> 
> produced via:
> 
>  # cd /debug/tracing/
>  # cat set_ftrace_filter
>  # echo schedule > set_ftrace_filter
>  # cat set_ftrace_filter
>  cat: set_ftrace_filter: No such device

This is probably fixed with my patch set.

-- Steve

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> On Wed, 26 Nov 2008, Ingo Molnar wrote:
> 
> > 
> > the regex filter doesnt work well yet when used with the graph tracer:
> > 
> > [ 2994.079079] ------------[ cut here ]------------
> > [ 2994.079079] WARNING: at kernel/trace/ftrace.c:373 ftrace_bug+0x8c/0x1ce()
> > [ 2994.079079] Modules linked in:
> > [ 2994.079079] Pid: 12, comm: kstop/0 Not tainted 2.6.28-rc6-tip #2056
> > [ 2994.079079] Call Trace:
> > [ 2994.079079]  [<c02273dd>] warn_on_slowpath+0x46/0x60
> > [ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
> > [ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
> > [ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
> > [ 2994.079079]  [<c0357f66>] ? memcpy+0xe/0x31
> > [ 2994.079079]  [<c02633e6>] ? rb_commit+0x29/0x2d
> > [ 2994.079079]  [<c0264c8b>] ? ring_buffer_unlock_commit+0x15/0x1a
> > [ 2994.079079]  [<c0213ece>] ? ftrace_modify_code+0x21/0xa3
> > [ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
> > [ 2994.079079]  [<c026c3ce>] ? probe_kernel_read+0xe/0x5c
> > [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> > [ 2994.079079]  [<c0203926>] ? return_to_handler+0xa/0x14
> > [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> > [ 2994.079079]  [<c0213ee5>] ? ftrace_modify_code+0x38/0xa3
> > [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> > [ 2994.079079]  [<c0262749>] ftrace_bug+0x8c/0x1ce
> > [ 2994.079079]  [<c026299c>] ftrace_replace_code+0x111/0x13d
> > [ 2994.079079]  [<c0253b53>] ? stop_cpu+0x0/0xa5
> > [ 2994.079079]  [<c02629e6>] __ftrace_modify_code+0x1e/0x32
> > [ 2994.079079]  [<c0253bbe>] stop_cpu+0x6b/0xa5
> > [ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
> > [ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
> > [ 2994.079079]  [<c0237fc1>] ? autoremove_wake_function+0x0/0x35
> > [ 2994.079079]  [<c023584b>] ? worker_thread+0x0/0xc7
> > [ 2994.079079]  [<c0237eba>] kthread+0x40/0x66
> > [ 2994.079079]  [<c0237e7a>] ? kthread+0x0/0x66
> > [ 2994.079079]  [<c02038cb>] kernel_thread_helper+0x7/0x10
> > [ 2994.079079] ---[ end trace bff97d149b80ecde ]---
> > [ 2994.079079] ftrace failed to modify [<c0261ec7>] trace_selftest_dynamic_test_func+0x3/0xc
> > [ 2994.079079]  actual: e8:0c:1a:fa:ff
> > 
> > produced via:
> > 
> >  # cd /debug/tracing/
> >  # cat set_ftrace_filter
> >  # echo schedule > set_ftrace_filter
> >  # cat set_ftrace_filter
> >  cat: set_ftrace_filter: No such device
> 
> This is probably fixed with my patch set.

yeah - but note that the graph-tracer rename came in the way of them. 
Mind redoing your two patches ontop of latest tip/master?

	Ingo

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Steven Rostedt]

On Wed, 26 Nov 2008, Ingo Molnar wrote:
> > This is probably fixed with my patch set.
> 
> yeah - but note that the graph-tracer rename came in the way of them. 
> Mind redoing your two patches ontop of latest tip/master?

I expected this. I'll rebase them again.

-- Steve

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Frédéric Weisbecker]

2008/11/26 Ingo Molnar <mingo@elte.hu>:
>
> the regex filter doesnt work well yet when used with the graph tracer:
>
> [ 2994.079079] ------------[ cut here ]------------
> [ 2994.079079] WARNING: at kernel/trace/ftrace.c:373 ftrace_bug+0x8c/0x1ce()
> [ 2994.079079] Modules linked in:
> [ 2994.079079] Pid: 12, comm: kstop/0 Not tainted 2.6.28-rc6-tip #2056
> [ 2994.079079] Call Trace:
> [ 2994.079079]  [<c02273dd>] warn_on_slowpath+0x46/0x60
> [ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
> [ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
> [ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
> [ 2994.079079]  [<c0357f66>] ? memcpy+0xe/0x31
> [ 2994.079079]  [<c02633e6>] ? rb_commit+0x29/0x2d
> [ 2994.079079]  [<c0264c8b>] ? ring_buffer_unlock_commit+0x15/0x1a
> [ 2994.079079]  [<c0213ece>] ? ftrace_modify_code+0x21/0xa3
> [ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
> [ 2994.079079]  [<c026c3ce>] ? probe_kernel_read+0xe/0x5c
> [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> [ 2994.079079]  [<c0203926>] ? return_to_handler+0xa/0x14
> [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> [ 2994.079079]  [<c0213ee5>] ? ftrace_modify_code+0x38/0xa3
> [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> [ 2994.079079]  [<c0262749>] ftrace_bug+0x8c/0x1ce
> [ 2994.079079]  [<c026299c>] ftrace_replace_code+0x111/0x13d
> [ 2994.079079]  [<c0253b53>] ? stop_cpu+0x0/0xa5
> [ 2994.079079]  [<c02629e6>] __ftrace_modify_code+0x1e/0x32
> [ 2994.079079]  [<c0253bbe>] stop_cpu+0x6b/0xa5
> [ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
> [ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
> [ 2994.079079]  [<c0237fc1>] ? autoremove_wake_function+0x0/0x35
> [ 2994.079079]  [<c023584b>] ? worker_thread+0x0/0xc7
> [ 2994.079079]  [<c0237eba>] kthread+0x40/0x66
> [ 2994.079079]  [<c0237e7a>] ? kthread+0x0/0x66
> [ 2994.079079]  [<c02038cb>] kernel_thread_helper+0x7/0x10
> [ 2994.079079] ---[ end trace bff97d149b80ecde ]---
> [ 2994.079079] ftrace failed to modify [<c0261ec7>] trace_selftest_dynamic_test_func+0x3/0xc
> [ 2994.079079]  actual: e8:0c:1a:fa:ff
>
> produced via:
>
>  # cd /debug/tracing/
>  # cat set_ftrace_filter
>  # echo schedule > set_ftrace_filter
>  # cat set_ftrace_filter
>  cat: set_ftrace_filter: No such device


I didn't met this problem by the past with dynamic filtering on return
tracer. Did it occur only with schedule ?
Do the last patches from Steve correct the issue?

Re: [PATCH] tracing/function-return-tracer: set a more human readable output

[Ingo Molnar]

* Frédéric Weisbecker <fweisbec@gmail.com> wrote:

> 2008/11/26 Ingo Molnar <mingo@elte.hu>:
> >
> > the regex filter doesnt work well yet when used with the graph tracer:
> >
> > [ 2994.079079] ------------[ cut here ]------------
> > [ 2994.079079] WARNING: at kernel/trace/ftrace.c:373 ftrace_bug+0x8c/0x1ce()
> > [ 2994.079079] Modules linked in:
> > [ 2994.079079] Pid: 12, comm: kstop/0 Not tainted 2.6.28-rc6-tip #2056
> > [ 2994.079079] Call Trace:
> > [ 2994.079079]  [<c02273dd>] warn_on_slowpath+0x46/0x60
> > [ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
> > [ 2994.079079]  [<c0268308>] ? trace_graph_return+0x89/0xa7
> > [ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
> > [ 2994.079079]  [<c0357f66>] ? memcpy+0xe/0x31
> > [ 2994.079079]  [<c02633e6>] ? rb_commit+0x29/0x2d
> > [ 2994.079079]  [<c0264c8b>] ? ring_buffer_unlock_commit+0x15/0x1a
> > [ 2994.079079]  [<c0213ece>] ? ftrace_modify_code+0x21/0xa3
> > [ 2994.079079]  [<c0213e85>] ? ftrace_return_to_handler+0x6b/0x74
> > [ 2994.079079]  [<c026c3ce>] ? probe_kernel_read+0xe/0x5c
> > [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> > [ 2994.079079]  [<c0203926>] ? return_to_handler+0xa/0x14
> > [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> > [ 2994.079079]  [<c0213ee5>] ? ftrace_modify_code+0x38/0xa3
> > [ 2994.079079]  [<c0261ec7>] ? trace_selftest_dynamic_test_func+0x3/0xc
> > [ 2994.079079]  [<c0262749>] ftrace_bug+0x8c/0x1ce
> > [ 2994.079079]  [<c026299c>] ftrace_replace_code+0x111/0x13d
> > [ 2994.079079]  [<c0253b53>] ? stop_cpu+0x0/0xa5
> > [ 2994.079079]  [<c02629e6>] __ftrace_modify_code+0x1e/0x32
> > [ 2994.079079]  [<c0253bbe>] stop_cpu+0x6b/0xa5
> > [ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
> > [ 2994.079079]  [<c020391c>] return_to_handler+0x0/0x14
> > [ 2994.079079]  [<c0237fc1>] ? autoremove_wake_function+0x0/0x35
> > [ 2994.079079]  [<c023584b>] ? worker_thread+0x0/0xc7
> > [ 2994.079079]  [<c0237eba>] kthread+0x40/0x66
> > [ 2994.079079]  [<c0237e7a>] ? kthread+0x0/0x66
> > [ 2994.079079]  [<c02038cb>] kernel_thread_helper+0x7/0x10
> > [ 2994.079079] ---[ end trace bff97d149b80ecde ]---
> > [ 2994.079079] ftrace failed to modify [<c0261ec7>] trace_selftest_dynamic_test_func+0x3/0xc
> > [ 2994.079079]  actual: e8:0c:1a:fa:ff
> >
> > produced via:
> >
> >  # cd /debug/tracing/
> >  # cat set_ftrace_filter
> >  # echo schedule > set_ftrace_filter
> >  # cat set_ftrace_filter
> >  cat: set_ftrace_filter: No such device
> 
> 
> I didn't met this problem by the past with dynamic filtering on 
> return tracer. Did it occur only with schedule ? Do the last patches 
> from Steve correct the issue?

yep. Will re-report it if it occurs again.

	Ingo