Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1618177 > unrolled thread

[PATCH 0/4] tracing: Add usecase of synchronize_rcu_tasks() and stack_tracer_disable()

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] tracing: Add usecase of synchronize_rcu_tasks() and stack_tracer_disable() Steven Rostedt <rostedt@goodmis.org> - 2017-04-06 18:50 +0200
    [PATCH 1/4] ftrace: Add use of synchronize_rcu_tasks() with dynamic trampolines Steven Rostedt <rostedt@goodmis.org> - 2017-04-06 18:50 +0200
      Re: [PATCH 1/4] ftrace: Add use of synchronize_rcu_tasks() with  dynamic trampolines "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-06 20:10 +0200
        Re: [PATCH 1/4] ftrace: Add use of synchronize_rcu_tasks() with  dynamic trampolines Steven Rostedt <rostedt@goodmis.org> - 2017-04-06 20:20 +0200
          Re: [PATCH 1/4] ftrace: Add use of synchronize_rcu_tasks() with  dynamic trampolines "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2017-04-06 22:30 +0200
    [PATCH 2/4] tracing: Replace the per_cpu() with this_cpu() in trace_stack.c Steven Rostedt <rostedt@goodmis.org> - 2017-04-06 18:50 +0200

#1618177 — [PATCH 0/4] tracing: Add usecase of synchronize_rcu_tasks() and stack_tracer_disable()

Paul, can you take a quick look at these patches. Namely patch 1, 3 and 4.

I modified your patch to use the updated name for stack_tracer_disable().

If all looks well, can you give them your ack.

Thanks!

-- Steve



Paul E. McKenney (1):
      rcu: Fix dyntick-idle tracing

Steven Rostedt (VMware) (3):
      ftrace: Add use of synchronize_rcu_tasks() with dynamic trampolines
      tracing: Replace the per_cpu() with this_cpu() in trace_stack.c
      tracing: Add stack_tracer_disable/enable() functions

----
 include/linux/ftrace.h     |  6 ++++++
 kernel/rcu/tree.c          | 48 +++++++++++++++++++++----------------------
 kernel/trace/Kconfig       |  3 ++-
 kernel/trace/ftrace.c      | 42 ++++++++++++++++----------------------
 kernel/trace/trace_stack.c | 51 +++++++++++++++++++++++++++++++---------------
 5 files changed, 84 insertions(+), 66 deletions(-)

[toc] | [next] | [standalone]

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

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>
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
+	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
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 8efd9fe7aec0..34f63e78d661 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2808,18 +2808,28 @@ static int ftrace_shutdown(struct ftrace_ops *ops, int command)
 	 * callers are done before leaving this function.
 	 * The same goes for freeing the per_cpu data of the per_cpu
 	 * ops.
-	 *
-	 * Again, normal synchronize_sched() is not good enough.
-	 * We need to do a hard force of sched synchronization.
-	 * This is because we use preempt_disable() to do RCU, but
-	 * the function tracers can be called where RCU is not watching
-	 * (like before user_exit()). We can not rely on the RCU
-	 * infrastructure to do the synchronization, thus we must do it
-	 * ourselves.
 	 */
 	if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_PER_CPU)) {
+		/*
+		 * We need to do a hard force of sched synchronization.
+		 * This is because we use preempt_disable() to do RCU, but
+		 * the function tracers can be called where RCU is not watching
+		 * (like before user_exit()). We can not rely on the RCU
+		 * infrastructure to do the synchronization, thus we must do it
+		 * ourselves.
+		 */
 		schedule_on_each_cpu(ftrace_sync);
 
+		/*
+		 * When the kernel is preeptive, tasks can be preempted
+		 * while on a ftrace trampoline. Just scheduling a task on
+		 * a CPU is not good enough to flush them. Calling
+		 * synchornize_rcu_tasks() will wait for those tasks to
+		 * execute and either schedule voluntarily or enter user space.
+		 */
+		if (IS_ENABLED(CONFIG_PREEMPT))
+			synchronize_rcu_tasks();
+
 		arch_ftrace_trampoline_free(ops);
 
 		if (ops->flags & FTRACE_OPS_FL_PER_CPU)
@@ -5366,22 +5376,6 @@ void __weak arch_ftrace_update_trampoline(struct ftrace_ops *ops)
 
 static void ftrace_update_trampoline(struct ftrace_ops *ops)
 {
-
-/*
- * Currently there's no safe way to free a trampoline when the kernel
- * is configured with PREEMPT. That is because a task could be preempted
- * when it jumped to the trampoline, it may be preempted for a long time
- * depending on the system load, and currently there's no way to know
- * when it will be off the trampoline. If the trampoline is freed
- * too early, when the task runs again, it will be executing on freed
- * memory and crash.
- */
-#ifdef CONFIG_PREEMPT
-	/* Currently, only non dynamic ops can have a trampoline */
-	if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
-		return;
-#endif
-
 	arch_ftrace_update_trampoline(ops);
 }
 
-- 
2.10.2

[toc] | [prev] | [next] | [standalone]

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

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>

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

> +	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
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 8efd9fe7aec0..34f63e78d661 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -2808,18 +2808,28 @@ static int ftrace_shutdown(struct ftrace_ops *ops, int command)
>  	 * callers are done before leaving this function.
>  	 * The same goes for freeing the per_cpu data of the per_cpu
>  	 * ops.
> -	 *
> -	 * Again, normal synchronize_sched() is not good enough.
> -	 * We need to do a hard force of sched synchronization.
> -	 * This is because we use preempt_disable() to do RCU, but
> -	 * the function tracers can be called where RCU is not watching
> -	 * (like before user_exit()). We can not rely on the RCU
> -	 * infrastructure to do the synchronization, thus we must do it
> -	 * ourselves.
>  	 */
>  	if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_PER_CPU)) {
> +		/*
> +		 * We need to do a hard force of sched synchronization.
> +		 * This is because we use preempt_disable() to do RCU, but
> +		 * the function tracers can be called where RCU is not watching
> +		 * (like before user_exit()). We can not rely on the RCU
> +		 * infrastructure to do the synchronization, thus we must do it
> +		 * ourselves.
> +		 */
>  		schedule_on_each_cpu(ftrace_sync);
> 
> +		/*
> +		 * When the kernel is preeptive, tasks can be preempted
> +		 * while on a ftrace trampoline. Just scheduling a task on
> +		 * a CPU is not good enough to flush them. Calling
> +		 * synchornize_rcu_tasks() will wait for those tasks to
> +		 * execute and either schedule voluntarily or enter user space.
> +		 */
> +		if (IS_ENABLED(CONFIG_PREEMPT))
> +			synchronize_rcu_tasks();
> +
>  		arch_ftrace_trampoline_free(ops);
> 
>  		if (ops->flags & FTRACE_OPS_FL_PER_CPU)
> @@ -5366,22 +5376,6 @@ void __weak arch_ftrace_update_trampoline(struct ftrace_ops *ops)
> 
>  static void ftrace_update_trampoline(struct ftrace_ops *ops)
>  {
> -
> -/*
> - * Currently there's no safe way to free a trampoline when the kernel
> - * is configured with PREEMPT. That is because a task could be preempted
> - * when it jumped to the trampoline, it may be preempted for a long time
> - * depending on the system load, and currently there's no way to know
> - * when it will be off the trampoline. If the trampoline is freed
> - * too early, when the task runs again, it will be executing on freed
> - * memory and crash.
> - */
> -#ifdef CONFIG_PREEMPT
> -	/* Currently, only non dynamic ops can have a trampoline */
> -	if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
> -		return;
> -#endif
> -
>  	arch_ftrace_update_trampoline(ops);
>  }
> 
> -- 
> 2.10.2
> 
> 

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

On Thu, Apr 06, 2017 at 02:13:29PM -0400, Steven Rostedt wrote:
> 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.

Now that you mention it, that does seem like it might be important.  ;-)

Ah, I was mistakenly looking at GLOB_SELFTEST instead of just plain
GLOB -- sorry for the noise!

							Thanx, Paul

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

[toc] | [prev] | [next] | [standalone]

#1618179 — [PATCH 2/4] tracing: Replace the per_cpu() with this_cpu() in trace_stack.c

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

The updates to the trace_active per cpu variable can be updated with the
this_cpu_*() functions as it only gets updated on the CPU that the variable
is on.

Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 kernel/trace/trace_stack.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)

diff --git a/kernel/trace/trace_stack.c b/kernel/trace/trace_stack.c
index 5fb1f2c87e6b..05ad2b86461e 100644
--- a/kernel/trace/trace_stack.c
+++ b/kernel/trace/trace_stack.c
@@ -207,13 +207,12 @@ stack_trace_call(unsigned long ip, unsigned long parent_ip,
 		 struct ftrace_ops *op, struct pt_regs *pt_regs)
 {
 	unsigned long stack;
-	int cpu;
 
 	preempt_disable_notrace();
 
-	cpu = raw_smp_processor_id();
 	/* no atomic needed, we only modify this variable by this cpu */
-	if (per_cpu(trace_active, cpu)++ != 0)
+	this_cpu_inc(trace_active);
+	if (this_cpu_read(trace_active) != 1)
 		goto out;
 
 	ip += MCOUNT_INSN_SIZE;
@@ -221,7 +220,7 @@ stack_trace_call(unsigned long ip, unsigned long parent_ip,
 	check_stack(ip, &stack);
 
  out:
-	per_cpu(trace_active, cpu)--;
+	this_cpu_dec(trace_active);
 	/* prevent recursion in schedule */
 	preempt_enable_notrace();
 }
@@ -253,7 +252,6 @@ stack_max_size_write(struct file *filp, const char __user *ubuf,
 	long *ptr = filp->private_data;
 	unsigned long val, flags;
 	int ret;
-	int cpu;
 
 	ret = kstrtoul_from_user(ubuf, count, 10, &val);
 	if (ret)
@@ -266,14 +264,13 @@ stack_max_size_write(struct file *filp, const char __user *ubuf,
 	 * we will cause circular lock, so we also need to increase
 	 * the percpu trace_active here.
 	 */
-	cpu = smp_processor_id();
-	per_cpu(trace_active, cpu)++;
+	this_cpu_inc(trace_active);
 
 	arch_spin_lock(&stack_trace_max_lock);
 	*ptr = val;
 	arch_spin_unlock(&stack_trace_max_lock);
 
-	per_cpu(trace_active, cpu)--;
+	this_cpu_dec(trace_active);
 	local_irq_restore(flags);
 
 	return count;
@@ -307,12 +304,9 @@ t_next(struct seq_file *m, void *v, loff_t *pos)
 
 static void *t_start(struct seq_file *m, loff_t *pos)
 {
-	int cpu;
-
 	local_irq_disable();
 
-	cpu = smp_processor_id();
-	per_cpu(trace_active, cpu)++;
+	this_cpu_inc(trace_active);
 
 	arch_spin_lock(&stack_trace_max_lock);
 
@@ -324,12 +318,9 @@ static void *t_start(struct seq_file *m, loff_t *pos)
 
 static void t_stop(struct seq_file *m, void *p)
 {
-	int cpu;
-
 	arch_spin_unlock(&stack_trace_max_lock);
 
-	cpu = smp_processor_id();
-	per_cpu(trace_active, cpu)--;
+	this_cpu_dec(trace_active);
 
 	local_irq_enable();
 }
-- 
2.10.2

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web