On Thu, Feb 06, 2014 at 03:09:56PM -0800, Greg Kroah-Hartman wrote: > > This is a note to let you know that I've just added the patch titled > > ftrace: Have function graph only trace based on global_ops filters > > to the 3.10-stable tree which can be found at: > http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary > > The filename of the patch is: > ftrace-have-function-graph-only-trace-based-on-global_ops-filters.patch > and it can be found in the queue-3.10 subdirectory. > > If you, or anyone else, feels it should not be added to the stable tree, > please let <stable@xxxxxxxxxxxxxxx> know about it. > > > From 23a8e8441a0a74dd612edf81dc89d1600bc0a3d1 Mon Sep 17 00:00:00 2001 > From: "Steven Rostedt (Red Hat)" <rostedt@xxxxxxxxxxx> > Date: Mon, 13 Jan 2014 10:30:23 -0500 > Subject: ftrace: Have function graph only trace based on global_ops filters > > From: "Steven Rostedt (Red Hat)" <rostedt@xxxxxxxxxxx> > > commit 23a8e8441a0a74dd612edf81dc89d1600bc0a3d1 upstream. > > Doing some different tests, I discovered that function graph tracing, when > filtered via the set_ftrace_filter and set_ftrace_notrace files, does > not always keep with them if another function ftrace_ops is registered > to trace functions. > > The reason is that function graph just happens to trace all functions > that the function tracer enables. When there was only one user of > function tracing, the function graph tracer did not need to worry about > being called by functions that it did not want to trace. But now that there > are other users, this becomes a problem. > > For example, one just needs to do the following: > > # cd /sys/kernel/debug/tracing > # echo schedule > set_ftrace_filter > # echo function_graph > current_tracer > # cat trace > [..] > 0) | schedule() { > ------------------------------------------ > 0) <idle>-0 => rcu_pre-7 > ------------------------------------------ > > 0) ! 2980.314 us | } > 0) | schedule() { > ------------------------------------------ > 0) rcu_pre-7 => <idle>-0 > ------------------------------------------ > > 0) + 20.701 us | } > > # echo 1 > /proc/sys/kernel/stack_tracer_enabled > # cat trace > [..] > 1) + 20.825 us | } > 1) + 21.651 us | } > 1) + 30.924 us | } /* SyS_ioctl */ > 1) | do_page_fault() { > 1) | __do_page_fault() { > 1) 0.274 us | down_read_trylock(); > 1) 0.098 us | find_vma(); > 1) | handle_mm_fault() { > 1) | _raw_spin_lock() { > 1) 0.102 us | preempt_count_add(); > 1) 0.097 us | do_raw_spin_lock(); > 1) 2.173 us | } > 1) | do_wp_page() { > 1) 0.079 us | vm_normal_page(); > 1) 0.086 us | reuse_swap_page(); > 1) 0.076 us | page_move_anon_rmap(); > 1) | unlock_page() { > 1) 0.082 us | page_waitqueue(); > 1) 0.086 us | __wake_up_bit(); > 1) 1.801 us | } > 1) 0.075 us | ptep_set_access_flags(); > 1) | _raw_spin_unlock() { > 1) 0.098 us | do_raw_spin_unlock(); > 1) 0.105 us | preempt_count_sub(); > 1) 1.884 us | } > 1) 9.149 us | } > 1) + 13.083 us | } > 1) 0.146 us | up_read(); > > When the stack tracer was enabled, it enabled all functions to be traced, which > now the function graph tracer also traces. This is a side effect that should > not occur. > > To fix this a test is added when the function tracing is changed, as well as when > the graph tracer is enabled, to see if anything other than the ftrace global_ops > function tracer is enabled. If so, then the graph tracer calls a test trampoline > that will look at the function that is being traced and compare it with the > filters defined by the global_ops. > > As an optimization, if there's no other function tracers registered, or if > the only registered function tracers also use the global ops, the function > graph infrastructure will call the registered function graph callback directly > and not go through the test trampoline. > > Fixes: d2d45c7a03a2 "tracing: Have stack_tracer use a separate list of functions" > Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx> > Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > > --- > kernel/trace/ftrace.c | 45 ++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 44 insertions(+), 1 deletion(-) > > --- a/kernel/trace/ftrace.c > +++ b/kernel/trace/ftrace.c > @@ -278,6 +278,12 @@ static void update_global_ops(void) > global_ops.func = func; > } > > +#ifdef CONFIG_FUNCTION_GRAPH_TRACER > +static void update_function_graph_func(void); > +#else > +static inline void update_function_graph_func(void) { } > +#endif > + > static void update_ftrace_function(void) > { > ftrace_func_t func; > @@ -325,6 +331,8 @@ static int remove_ftrace_ops(struct ftra > { > struct ftrace_ops **p; > > + update_function_graph_func(); > + Hi Greg, This change doesn't look correct to me. The original commit changes function update_ftrace_function(), not remove_ftrace_ops(). Cheers, -- Luis > /* > * If we are removing the last function, then simply point > * to the ftrace_stub. > @@ -4728,6 +4736,7 @@ int ftrace_graph_entry_stub(struct ftrac > trace_func_graph_ret_t ftrace_graph_return = > (trace_func_graph_ret_t)ftrace_stub; > trace_func_graph_ent_t ftrace_graph_entry = ftrace_graph_entry_stub; > +static trace_func_graph_ent_t __ftrace_graph_entry = ftrace_graph_entry_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) > @@ -4869,6 +4878,30 @@ static struct ftrace_ops fgraph_ops __re > FTRACE_OPS_FL_RECURSION_SAFE, > }; > > +static int ftrace_graph_entry_test(struct ftrace_graph_ent *trace) > +{ > + if (!ftrace_ops_test(&global_ops, trace->func, NULL)) > + return 0; > + return __ftrace_graph_entry(trace); > +} > + > +/* > + * The function graph tracer should only trace the functions defined > + * by set_ftrace_filter and set_ftrace_notrace. If another function > + * tracer ops is registered, the graph tracer requires testing the > + * function against the global ops, and not just trace any function > + * that any ftrace_ops registered. > + */ > +static void update_function_graph_func(void) > +{ > + if (ftrace_ops_list == &ftrace_list_end || > + (ftrace_ops_list == &global_ops && > + global_ops.next == &ftrace_list_end)) > + ftrace_graph_entry = __ftrace_graph_entry; > + else > + ftrace_graph_entry = ftrace_graph_entry_test; > +} > + > int register_ftrace_graph(trace_func_graph_ret_t retfunc, > trace_func_graph_ent_t entryfunc) > { > @@ -4893,7 +4926,16 @@ int register_ftrace_graph(trace_func_gra > } > > ftrace_graph_return = retfunc; > - ftrace_graph_entry = entryfunc; > + > + /* > + * Update the indirect function to the entryfunc, and the > + * function that gets called to the entry_test first. Then > + * call the update fgraph entry function to determine if > + * the entryfunc should be called directly or not. > + */ > + __ftrace_graph_entry = entryfunc; > + ftrace_graph_entry = ftrace_graph_entry_test; > + update_function_graph_func(); > > ret = ftrace_startup(&fgraph_ops, FTRACE_START_FUNC_RET); > > @@ -4912,6 +4954,7 @@ void unregister_ftrace_graph(void) > ftrace_graph_active--; > ftrace_graph_return = (trace_func_graph_ret_t)ftrace_stub; > ftrace_graph_entry = ftrace_graph_entry_stub; > + __ftrace_graph_entry = ftrace_graph_entry_stub; > ftrace_shutdown(&fgraph_ops, FTRACE_STOP_FUNC_RET); > unregister_pm_notifier(&ftrace_suspend_notifier); > unregister_trace_sched_switch(ftrace_graph_probe_sched_switch, NULL); > > > Patches currently in stable-queue which might be from rostedt@xxxxxxxxxxx are > > queue-3.10/ftrace-have-function-graph-only-trace-based-on-global_ops-filters.patch > queue-3.10/tracing-check-if-tracing-is-enabled-in-trace_puts.patch > queue-3.10/tracing-have-trace-buffer-point-back-to-trace_array.patch > -- > To unsubscribe from this list: send the line "unsubscribe stable" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html