Re: [PATCH 1/4] ftrace: Add use of synchronize_rcu_tasks() with dynamic trampolines

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 6 Apr 2017 11:05:53 -0700
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> On Thu, Apr 06, 2017 at 12:42:38PM -0400, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> > 
> > The function tracer needs to be more careful than other subsystems when it
> > comes to freeing data. Especially if that data is actually executable code.
> > When a single function is traced, a trampoline can be dynamically allocated
> > which is called to jump to the function trace callback. When the callback is
> > no longer needed, the dynamic allocated trampoline needs to be freed. This
> > is where the issues arise. The dynamically allocated trampoline must not be
> > used again. As function tracing can trace all subsystems, including
> > subsystems that are used to serialize aspects of freeing (namely RCU), it
> > must take extra care when doing the freeing.
> > 
> > Before synchronize_rcu_tasks() was around, there was no way for the function
> > tracer to know that nothing was using the dynamically allocated trampoline
> > when CONFIG_PREEMPT was enabled. That's because a task could be indefinitely
> > preempted while sitting on the trampoline. Now with synchronize_rcu_tasks(),
> > it will wait till all tasks have either voluntarily scheduled (not on the
> > trampoline) or goes into userspace (not on the trampoline). Then it is safe
> > to free the trampoline even with CONFIG_PREEMPT set.
> > 
> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>  
> 
> One question below.  Other than that:
> 
> Acked-by: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>

Thanks!

> 
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> >  kernel/trace/Kconfig  |  3 ++-
> >  kernel/trace/ftrace.c | 42 ++++++++++++++++++------------------------
> >  2 files changed, 20 insertions(+), 25 deletions(-)
> > 
> > diff --git a/kernel/trace/Kconfig b/kernel/trace/Kconfig
> > index d4a06e714645..67b463b4f169 100644
> > --- a/kernel/trace/Kconfig
> > +++ b/kernel/trace/Kconfig
> > @@ -134,7 +134,8 @@ config FUNCTION_TRACER
> >  	select KALLSYMS
> >  	select GENERIC_TRACER
> >  	select CONTEXT_SWITCH_TRACER
> > -        select GLOB
> > +	select GLOB  
> 
> Does GLOB really want to be selected in production environments?
> I could understand "select GLOB if WE_ARE_TESTING" or some such.

Note, this patch just fixes the whitespace issue for the "select GLOB".
All the selects had a tab in front of them, where as GLOB had all
spaces.

As for your question, FUNCTION_TRACER depends on glob. It's what is
used for the function filters. You can do:

	echo '*sync*rcu*' > /sys/kernel/tracing/set_ftrace_filter

And get:

synchronize_rcu_tasks
synchronize_srcu
synchronize_srcu_expedited
sync_rcu_exp_select_cpus
sync_rcu_exp_handler
synchronize_rcu_bh.part.65
synchronize_rcu_expedited
synchronize_rcu.part.67
synchronize_rcu
synchronize_rcu_bh

selected.

So yes, production environments do want it selected.

-- Steve


> 
> > +	select TASKS_RCU if PREEMPT
> >  	help
> >  	  Enable the kernel to trace every kernel function. This is done
> >  	  by using a compiler feature to insert a small, 5-byte No-Operation