Skip to contentSkip to conversation

tracing: Add usecase of synchronize_rcu_tasks() and stack_tracer_disable()

PATCH Superseded Steven Rostedt 3 participants 16 messages Apr 6, 2017 – Apr 7, 2017

Subsystems (3)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

cover 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(-)
author maintainer
#1 lore raw

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

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

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.

---
 kernel/trace/Kconfig  |  3 ++-
 kernel/trace/ftrace.c | 42 ++++++++++++++++++------------------------
 2 files changed, 20 insertions(+), 25 deletions(-)
kernel/trace/Kconfig +2 -1
index d4a06e714645..67b463b4f169 100644
@@ -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
kernel/trace/ftrace.c +18 -24

58 lines not on this page · raw message

Signature · 1 line
2.10.2
Acked-by on v1 1/4
#2 lore raw

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

Quoted 20 lines · Steven Rostedt

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

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.

Quoted 1 line
Cc "Paul E. McKenney" <[email protected]>

One question below. Other than that:

Quoted 11 lines
---
 kernel/trace/Kconfig  |  3 ++-
 kernel/trace/ftrace.c | 42 ++++++++++++++++++------------------------
 2 files changed, 20 insertions(+), 25 deletions(-)
kernel/trace/Kconfig +0 -0
index d4a06e714645..67b463b4f169 100644

kernel/trace/Kconfig config FUNCTION_TRACER · line 137

Quoted 5 lines
kernel/trace/Kconfig +1 -1
@@ -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.

Quoted 72 lines
kernel/trace/Kconfig +1 -0
+	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
kernel/trace/ftrace.c +18 -24
index 8efd9fe7aec0..34f63e78d661 100644
@@ -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);
 }
 
Signature · 1 line
2.10.2
author maintainer
#3 lore raw

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

Quoted 21 lines · On Thu, 6 Apr 2017 11:05:53 -0700 "Paul E. McKenney"…
Quoted 20 lines · Steven Rostedt

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

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.

Quoted 5 lines
Quoted 1 line
Cc "Paul E. McKenney" <[email protected]>

One question below. Other than that:

Thanks!

Quoted 7 lines
Quoted 7 lines
---
 kernel/trace/Kconfig  |  3 ++-
 kernel/trace/ftrace.c | 42 ++++++++++++++++++------------------------
 2 files changed, 20 insertions(+), 25 deletions(-)
Quoted 13 lines

kernel/trace/Kconfig config FUNCTION_TRACER · line 137

Quoted 10 lines
kernel/trace/Kconfig +1 -1
index d4a06e714645..67b463b4f169 100644
@@ -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

Quoted 5 lines
Quoted 5 lines
kernel/trace/Kconfig +1 -0
+	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
#4 lore raw

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

Quoted 75 lines · Steven Rostedt
Quoted 21 lines · On Thu, 6 Apr 2017 11:05:53 -0700 "Paul E. McKenney"…
Quoted 20 lines · Steven Rostedt

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

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.

Quoted 5 lines
Quoted 1 line
Cc "Paul E. McKenney" <[email protected]>

One question below. Other than that:

Thanks!

Quoted 7 lines
Quoted 7 lines
---
 kernel/trace/Kconfig  |  3 ++-
 kernel/trace/ftrace.c | 42 ++++++++++++++++++------------------------
 2 files changed, 20 insertions(+), 25 deletions(-)
Quoted 13 lines

kernel/trace/Kconfig config FUNCTION_TRACER · line 137

Quoted 10 lines
kernel/trace/Kconfig +1 -1
index d4a06e714645..67b463b4f169 100644
@@ -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

Quoted 3 lines

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

Quoted 9 lines

-- Steve

Quoted 5 lines
Quoted 5 lines
kernel/trace/Kconfig +1 -0
+	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
author maintainer
#5 lore raw

PATCH 2/4 tracing: Replace the per_cpu() with this_cpu() in trace_stack.c

In reply to Steven Rostedt

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

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.

---
 kernel/trace/trace_stack.c | 23 +++++++----------------
 1 file changed, 7 insertions(+), 16 deletions(-)
kernel/trace/trace_stack.c +7 -16
index 5fb1f2c87e6b..05ad2b86461e 100644
@@ -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();
 }
Signature · 1 line
2.10.2
author maintainer
#6 lore raw

PATCH 3/4 tracing: Add stack_tracer_disable/enable() functions

In reply to Steven Rostedt

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

There are certain parts of the kernel that can not let stack tracing proceed (namely in RCU), because the stack tracer uses RCU, and parts of RCU internals can not handle having RCU read side locks taken.

Add stack_tracer_disable() and stack_tracer_enable() functions to let RCU stop stack tracing on the current CPU as it is in those critical sections.

---
 include/linux/ftrace.h     |  6 ++++++
 kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)
include/linux/ftrace.h +6 -0
index ef7123219f14..40afee35565a 100644
@@ -286,6 +286,12 @@ int
 stack_trace_sysctl(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp,
 		   loff_t *ppos);
+
+void stack_tracer_disable(void);
+void stack_tracer_enable(void);
+#else
+static inline void stack_tracer_disable(void) { }
+static inline void stack_tracer_enabe(void) { }
 #endif
 
 struct ftrace_func_command {
kernel/trace/trace_stack.c +28 -0
index 05ad2b86461e..5adbb73ec2ec 100644
@@ -41,6 +41,34 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
 int stack_tracer_enabled;
 static int last_stack_tracer_enabled;
 
+/**
+ * stack_tracer_disable - temporarily disable the stack tracer
+ *
+ * There's a few locations (namely in RCU) where stack tracing
+ * can not be executed. This function is used to disable stack
+ * tracing during those critical sections.
+ *
+ * This function will disable preemption. stack_tracer_enable()
+ * must be called shortly after this is called.
+ */
+void stack_tracer_disable(void)
+{
+	preempt_disable_notrace();
+	this_cpu_inc(trace_active);
+}
+
+/**
+ * stack_tracer_enable - re-enable the stack tracer
+ *
+ * After stack_tracer_disable() is called, stack_tracer_enable()
+ * must shortly be called afterward.
+ */
+void stack_tracer_enable(void)
+{
+	this_cpu_dec(trace_active);
+	preempt_enable_notrace();
+}
+
 void stack_trace_print(void)
 {
 	long i;
Signature · 1 line
2.10.2
#7 lore raw

Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

Quoted 8 lines · Steven Rostedt

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

There are certain parts of the kernel that can not let stack tracing proceed (namely in RCU), because the stack tracer uses RCU, and parts of RCU internals can not handle having RCU read side locks taken.

Add stack_tracer_disable() and stack_tracer_enable() functions to let RCU stop stack tracing on the current CPU as it is in those critical sections.

s/as it is in/when it is in/?

Quoted 1 line

One quibble above, one objection below.

Thanx, Paul

Quoted 33 lines
---
 include/linux/ftrace.h     |  6 ++++++
 kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)
include/linux/ftrace.h +6 -0
index ef7123219f14..40afee35565a 100644
@@ -286,6 +286,12 @@ int
 stack_trace_sysctl(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp,
 		   loff_t *ppos);
+
+void stack_tracer_disable(void);
+void stack_tracer_enable(void);
+#else
+static inline void stack_tracer_disable(void) { }
+static inline void stack_tracer_enabe(void) { }
 #endif
 
 struct ftrace_func_command {
kernel/trace/trace_stack.c +3 -0
index 05ad2b86461e..5adbb73ec2ec 100644
@@ -41,6 +41,34 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
 int stack_tracer_enabled;
 static int last_stack_tracer_enabled;
 
+/**
+ * stack_tracer_disable - temporarily disable the stack tracer
+ *

kernel/trace/trace_stack.c static DEFINE_MUTEX(stack_sysctl_mutex); · line 56

Quoted 10 lines
kernel/trace/trace_stack.c +10 -0
+ * There's a few locations (namely in RCU) where stack tracing
+ * can not be executed. This function is used to disable stack
+ * tracing during those critical sections.
+ *
+ * This function will disable preemption. stack_tracer_enable()
+ * must be called shortly after this is called.
+ */
+void stack_tracer_disable(void)
+{
+	preempt_disable_notrace();

Interrupts are disabled in all current call points, so you don't really need to disable preemption. I would normally not worry, given the ease-of-use improvements, but some people get annoyed about even slight increases in idle-entry overhead.

kernel/trace/trace_stack.c static DEFINE_MUTEX(stack_sysctl_mutex); · line 69

Quoted 13 lines
kernel/trace/trace_stack.c +13 -0
+	this_cpu_inc(trace_active);
+}
+
+/**
+ * stack_tracer_enable - re-enable the stack tracer
+ *
+ * After stack_tracer_disable() is called, stack_tracer_enable()
+ * must shortly be called afterward.
+ */
+void stack_tracer_enable(void)
+{
+	this_cpu_dec(trace_active);
+	preempt_enable_notrace();

Ditto...

Quoted 9 lines
kernel/trace/trace_stack.c +2 -0
+}
+
 void stack_trace_print(void)
 {
 	long i;
Signature · 1 line
2.10.2
author maintainer
#8 lore raw

Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

Quoted 58 lines · On Thu, 6 Apr 2017 11:12:22 -0700 "Paul E. McKenney"…
Quoted 8 lines · Steven Rostedt

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

There are certain parts of the kernel that can not let stack tracing proceed (namely in RCU), because the stack tracer uses RCU, and parts of RCU internals can not handle having RCU read side locks taken.

Add stack_tracer_disable() and stack_tracer_enable() functions to let RCU stop stack tracing on the current CPU as it is in those critical sections.

s/as it is in/when it is in/?

Quoted 1 line

One quibble above, one objection below.

Thanx, Paul

Quoted 40 lines
---
 include/linux/ftrace.h     |  6 ++++++
 kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)
include/linux/ftrace.h +6 -0
index ef7123219f14..40afee35565a 100644
@@ -286,6 +286,12 @@ int
 stack_trace_sysctl(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp,
 		   loff_t *ppos);
+
+void stack_tracer_disable(void);
+void stack_tracer_enable(void);
+#else
+static inline void stack_tracer_disable(void) { }
+static inline void stack_tracer_enabe(void) { }
 #endif
 
 struct ftrace_func_command {
kernel/trace/trace_stack.c +10 -0
index 05ad2b86461e..5adbb73ec2ec 100644
@@ -41,6 +41,34 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
 int stack_tracer_enabled;
 static int last_stack_tracer_enabled;
 
+/**
+ * stack_tracer_disable - temporarily disable the stack tracer
+ *
+ * There's a few locations (namely in RCU) where stack tracing
+ * can not be executed. This function is used to disable stack
+ * tracing during those critical sections.
+ *
+ * This function will disable preemption. stack_tracer_enable()
+ * must be called shortly after this is called.
+ */
Quoted 8 lines

kernel/trace/trace_stack.c static DEFINE_MUTEX(stack_sysctl_mutex); · line 56

Quoted 3 lines
kernel/trace/trace_stack.c +3 -0
+void stack_tracer_disable(void)
+{
+	preempt_disable_notrace();  

Interrupts are disabled in all current call points, so you don't really need to disable preemption. I would normally not worry, given the ease-of-use improvements, but some people get annoyed about even slight increases in idle-entry overhead.

My worry is that we add another caller that doesn't disable interrupts or preemption.

I could add a __stack_trace_disable() that skips the disabling of preemption, as the "__" usually denotes the call is "special".

-- Steve

Quoted 26 lines
Quoted 4 lines
kernel/trace/trace_stack.c +3 -0
+	this_cpu_inc(trace_active);
+}
+

kernel/trace/trace_stack.c static DEFINE_MUTEX(stack_sysctl_mutex); · line 69

Quoted 10 lines
kernel/trace/trace_stack.c +10 -0
+/**
+ * stack_tracer_enable - re-enable the stack tracer
+ *
+ * After stack_tracer_disable() is called, stack_tracer_enable()
+ * must shortly be called afterward.
+ */
+void stack_tracer_enable(void)
+{
+	this_cpu_dec(trace_active);
+	preempt_enable_notrace();  

Ditto...

Quoted 9 lines
kernel/trace/trace_stack.c +2 -0
+}
+
 void stack_trace_print(void)
 {
 	long i;
Signature · 1 line
2.10.2
#9 lore raw

Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

Quoted 70 lines · Steven Rostedt
Quoted 58 lines · On Thu, 6 Apr 2017 11:12:22 -0700 "Paul E. McKenney"…
Quoted 8 lines · Steven Rostedt

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

There are certain parts of the kernel that can not let stack tracing proceed (namely in RCU), because the stack tracer uses RCU, and parts of RCU internals can not handle having RCU read side locks taken.

Add stack_tracer_disable() and stack_tracer_enable() functions to let RCU stop stack tracing on the current CPU as it is in those critical sections.

s/as it is in/when it is in/?

Quoted 1 line

One quibble above, one objection below.

Thanx, Paul

Quoted 40 lines
---
 include/linux/ftrace.h     |  6 ++++++
 kernel/trace/trace_stack.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)
include/linux/ftrace.h +6 -0
index ef7123219f14..40afee35565a 100644
@@ -286,6 +286,12 @@ int
 stack_trace_sysctl(struct ctl_table *table, int write,
 		   void __user *buffer, size_t *lenp,
 		   loff_t *ppos);
+
+void stack_tracer_disable(void);
+void stack_tracer_enable(void);
+#else
+static inline void stack_tracer_disable(void) { }
+static inline void stack_tracer_enabe(void) { }
 #endif
 
 struct ftrace_func_command {
kernel/trace/trace_stack.c +10 -0
index 05ad2b86461e..5adbb73ec2ec 100644
@@ -41,6 +41,34 @@ static DEFINE_MUTEX(stack_sysctl_mutex);
 int stack_tracer_enabled;
 static int last_stack_tracer_enabled;
 
+/**
+ * stack_tracer_disable - temporarily disable the stack tracer
+ *
+ * There's a few locations (namely in RCU) where stack tracing
+ * can not be executed. This function is used to disable stack
+ * tracing during those critical sections.
+ *
+ * This function will disable preemption. stack_tracer_enable()
+ * must be called shortly after this is called.
+ */
Quoted 8 lines

kernel/trace/trace_stack.c static DEFINE_MUTEX(stack_sysctl_mutex); · line 56

Quoted 3 lines
kernel/trace/trace_stack.c +3 -0
+void stack_tracer_disable(void)
+{
+	preempt_disable_notrace();  

Interrupts are disabled in all current call points, so you don't really need to disable preemption. I would normally not worry, given the ease-of-use improvements, but some people get annoyed about even slight increases in idle-entry overhead.

Quoted 5 lines

My worry is that we add another caller that doesn't disable interrupts or preemption.

I could add a __stack_trace_disable() that skips the disabling of preemption, as the "__" usually denotes the call is "special".

Given that interrupts are disabled at that point, and given also that NMI skips stack tracing if growth is required, could we just leave out the stack_tracer_disable() and stack_tracer_enable()?

Thanx, Paul

Quoted 29 lines

-- Steve

Quoted 26 lines
Quoted 4 lines
kernel/trace/trace_stack.c +3 -0
+	this_cpu_inc(trace_active);
+}
+

kernel/trace/trace_stack.c static DEFINE_MUTEX(stack_sysctl_mutex); · line 69

Quoted 10 lines
kernel/trace/trace_stack.c +10 -0
+/**
+ * stack_tracer_enable - re-enable the stack tracer
+ *
+ * After stack_tracer_disable() is called, stack_tracer_enable()
+ * must shortly be called afterward.
+ */
+void stack_tracer_enable(void)
+{
+	this_cpu_dec(trace_active);
+	preempt_enable_notrace();  

Ditto...

Quoted 9 lines
kernel/trace/trace_stack.c +2 -0
+}
+
 void stack_trace_print(void)
 {
 	long i;
Signature · 1 line
2.10.2
author maintainer
#10 lore raw

Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

Quoted 9 lines · On Thu, 6 Apr 2017 13:21:17 -0700 "Paul E. McKenney"…
Quoted 5 lines

My worry is that we add another caller that doesn't disable interrupts or preemption.

I could add a __stack_trace_disable() that skips the disabling of preemption, as the "__" usually denotes the call is "special".

Given that interrupts are disabled at that point, and given also that NMI skips stack tracing if growth is required, could we just leave out the stack_tracer_disable() and stack_tracer_enable()?

There may be other use cases. Hmm, maybe I'll just have it do a check to make sure preemption is disabled. Something like:

	if (IS_ENABLED(CONFIG_DEBUG_PREEMPT))
		WARN_ON_ONCE(!preempt_count());

-- Steve

#11 lore raw

Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

Quoted 13 lines · Steven Rostedt
Quoted 9 lines · On Thu, 6 Apr 2017 13:21:17 -0700 "Paul E. McKenney"…
Quoted 5 lines

My worry is that we add another caller that doesn't disable interrupts or preemption.

I could add a __stack_trace_disable() that skips the disabling of preemption, as the "__" usually denotes the call is "special".

Given that interrupts are disabled at that point, and given also that NMI skips stack tracing if growth is required, could we just leave out the stack_tracer_disable() and stack_tracer_enable()?

Quoted 5 lines

There may be other use cases. Hmm, maybe I'll just have it do a check to make sure preemption is disabled. Something like:

	if (IS_ENABLED(CONFIG_DEBUG_PREEMPT))
		WARN_ON_ONCE(!preempt_count());

This in the include/linux/ftrace.h file so that it can be inlined? That makes sense to me.

Thanx, Paul

author maintainer
#12 lore raw

Re: [PATCH 3/4] tracing: Add stack_tracer_disable/enable() functions

Quoted 14 lines · On Thu, 6 Apr 2017 15:08:21 -0700 "Paul E. McKenney"…
Quoted 13 lines · Steven Rostedt
Quoted 9 lines · On Thu, 6 Apr 2017 13:21:17 -0700 "Paul E. McKenney"…
Quoted 5 lines

My worry is that we add another caller that doesn't disable interrupts or preemption.

I could add a __stack_trace_disable() that skips the disabling of preemption, as the "__" usually denotes the call is "special".

Given that interrupts are disabled at that point, and given also that NMI skips stack tracing if growth is required, could we just leave out the stack_tracer_disable() and stack_tracer_enable()?

Quoted 9 lines
Quoted 5 lines

There may be other use cases. Hmm, maybe I'll just have it do a check to make sure preemption is disabled. Something like:

	if (IS_ENABLED(CONFIG_DEBUG_PREEMPT))
		WARN_ON_ONCE(!preempt_count());  

This in the include/linux/ftrace.h file so that it can be inlined? That makes sense to me.

Hah, I already had that part (inlining) written. It's a separate patch though. I'll post another series tomorrow.

-- Steve

author maintainer
#13 lore raw

PATCH 4/4 rcu: Fix dyntick-idle tracing

In reply to Steven Rostedt

From: "Paul E. McKenney" <[email protected]>

The tracing subsystem started using rcu_irq_entry() and rcu_irq_exit() (with my blessing) to allow the current _rcuidle alternative tracepoint name to be dispensed with while still maintaining good performance. Unfortunately, this causes RCU's dyntick-idle entry code's tracing to appear to RCU like an interrupt that occurs where RCU is not designed to handle interrupts.

This commit fixes this problem by moving the zeroing of ->dynticks_nesting after the offending trace_rcu_dyntick() statement, which narrows the window of vulnerability to a pair of adjacent statements that are now marked with comments to that effect.

---
 kernel/rcu/tree.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)
kernel/rcu/tree.c +23 -25
index 50fee7689e71..8b4d273331e4 100644
@@ -57,6 +57,7 @@
 #include <linux/random.h>
 #include <linux/trace_events.h>
 #include <linux/suspend.h>
+#include <linux/ftrace.h>
 
 #include "tree.h"
 #include "rcu.h"
@@ -771,25 +772,24 @@ cpu_needs_another_gp(struct rcu_state *rsp, struct rcu_data *rdp)
 }
 
 /*
- * rcu_eqs_enter_common - current CPU is moving towards extended quiescent state
+ * rcu_eqs_enter_common - current CPU is entering an extended quiescent state
  *
- * If the new value of the ->dynticks_nesting counter now is zero,
- * we really have entered idle, and must do the appropriate accounting.
- * The caller must have disabled interrupts.
+ * Enter idle, doing appropriate accounting.  The caller must have
+ * disabled interrupts.
  */
-static void rcu_eqs_enter_common(long long oldval, bool user)
+static void rcu_eqs_enter_common(bool user)
 {
 	struct rcu_state *rsp;
 	struct rcu_data *rdp;
-	RCU_TRACE(struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);)
+	struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
 
-	trace_rcu_dyntick(TPS("Start"), oldval, rdtp->dynticks_nesting);
+	trace_rcu_dyntick(TPS("Start"), rdtp->dynticks_nesting, 0);
 	if (IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
 	    !user && !is_idle_task(current)) {
 		struct task_struct *idle __maybe_unused =
 			idle_task(smp_processor_id());
 
-		trace_rcu_dyntick(TPS("Error on entry: not idle task"), oldval, 0);
+		trace_rcu_dyntick(TPS("Error on entry: not idle task"), rdtp->dynticks_nesting, 0);
 		rcu_ftrace_dump(DUMP_ORIG);
 		WARN_ONCE(1, "Current pid: %d comm: %s / Idle pid: %d comm: %s",
 			  current->pid, current->comm,
@@ -800,7 +800,10 @@ static void rcu_eqs_enter_common(long long oldval, bool user)
 		do_nocb_deferred_wakeup(rdp);
 	}
 	rcu_prepare_for_idle();
-	rcu_dynticks_eqs_enter();
+	stack_tracer_disable();
+	rdtp->dynticks_nesting = 0; /* Breaks tracing momentarily. */
+	rcu_dynticks_eqs_enter(); /* After this, tracing works again. */
+	stack_tracer_enable();
 	rcu_dynticks_task_enter();
 
 	/*
@@ -821,19 +824,15 @@ static void rcu_eqs_enter_common(long long oldval, bool user)
  */
 static void rcu_eqs_enter(bool user)
 {
-	long long oldval;
 	struct rcu_dynticks *rdtp;
 
 	rdtp = this_cpu_ptr(&rcu_dynticks);
-	oldval = rdtp->dynticks_nesting;
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
-		     (oldval & DYNTICK_TASK_NEST_MASK) == 0);
-	if ((oldval & DYNTICK_TASK_NEST_MASK) == DYNTICK_TASK_NEST_VALUE) {
-		rdtp->dynticks_nesting = 0;
-		rcu_eqs_enter_common(oldval, user);
-	} else {
+		     (rdtp->dynticks_nesting & DYNTICK_TASK_NEST_MASK) == 0);
+	if ((rdtp->dynticks_nesting & DYNTICK_TASK_NEST_MASK) == DYNTICK_TASK_NEST_VALUE)
+		rcu_eqs_enter_common(user);
+	else
 		rdtp->dynticks_nesting -= DYNTICK_TASK_NEST_VALUE;
-	}
 }
 
 /**
@@ -892,19 +891,18 @@ void rcu_user_enter(void)
  */
 void rcu_irq_exit(void)
 {
-	long long oldval;
 	struct rcu_dynticks *rdtp;
 
 	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs enabled!!!");
 	rdtp = this_cpu_ptr(&rcu_dynticks);
-	oldval = rdtp->dynticks_nesting;
-	rdtp->dynticks_nesting--;
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
-		     rdtp->dynticks_nesting < 0);
-	if (rdtp->dynticks_nesting)
-		trace_rcu_dyntick(TPS("--="), oldval, rdtp->dynticks_nesting);
-	else
-		rcu_eqs_enter_common(oldval, true);
+		     rdtp->dynticks_nesting < 1);
+	if (rdtp->dynticks_nesting <= 1) {
+		rcu_eqs_enter_common(true);
+	} else {
+		trace_rcu_dyntick(TPS("--="), rdtp->dynticks_nesting, rdtp->dynticks_nesting - 1);
+		rdtp->dynticks_nesting--;
+	}
 	rcu_sysidle_enter(1);
 }
 
Signature · 1 line
2.10.2
Acked-by on v1 4/4
#14 lore raw

Re: [PATCH 4/4] rcu: Fix dyntick-idle tracing

Quoted 14 lines · Steven Rostedt

From: "Paul E. McKenney" <[email protected]>

The tracing subsystem started using rcu_irq_entry() and rcu_irq_exit() (with my blessing) to allow the current _rcuidle alternative tracepoint name to be dispensed with while still maintaining good performance. Unfortunately, this causes RCU's dyntick-idle entry code's tracing to appear to RCU like an interrupt that occurs where RCU is not designed to handle interrupts.

This commit fixes this problem by moving the zeroing of ->dynticks_nesting after the offending trace_rcu_dyntick() statement, which narrows the window of vulnerability to a pair of adjacent statements that are now marked with comments to that effect.

Quoted 5 lines

Really confirming my Signed-off-by given Steven's changes, but whatever. ;-)

Thanx, Paul

Quoted 116 lines
---
 kernel/rcu/tree.c | 48 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)
kernel/rcu/tree.c +23 -25
index 50fee7689e71..8b4d273331e4 100644
@@ -57,6 +57,7 @@
 #include <linux/random.h>
 #include <linux/trace_events.h>
 #include <linux/suspend.h>
+#include <linux/ftrace.h>
 
 #include "tree.h"
 #include "rcu.h"
@@ -771,25 +772,24 @@ cpu_needs_another_gp(struct rcu_state *rsp, struct rcu_data *rdp)
 }
 
 /*
- * rcu_eqs_enter_common - current CPU is moving towards extended quiescent state
+ * rcu_eqs_enter_common - current CPU is entering an extended quiescent state
  *
- * If the new value of the ->dynticks_nesting counter now is zero,
- * we really have entered idle, and must do the appropriate accounting.
- * The caller must have disabled interrupts.
+ * Enter idle, doing appropriate accounting.  The caller must have
+ * disabled interrupts.
  */
-static void rcu_eqs_enter_common(long long oldval, bool user)
+static void rcu_eqs_enter_common(bool user)
 {
 	struct rcu_state *rsp;
 	struct rcu_data *rdp;
-	RCU_TRACE(struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);)
+	struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
 
-	trace_rcu_dyntick(TPS("Start"), oldval, rdtp->dynticks_nesting);
+	trace_rcu_dyntick(TPS("Start"), rdtp->dynticks_nesting, 0);
 	if (IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
 	    !user && !is_idle_task(current)) {
 		struct task_struct *idle __maybe_unused =
 			idle_task(smp_processor_id());
 
-		trace_rcu_dyntick(TPS("Error on entry: not idle task"), oldval, 0);
+		trace_rcu_dyntick(TPS("Error on entry: not idle task"), rdtp->dynticks_nesting, 0);
 		rcu_ftrace_dump(DUMP_ORIG);
 		WARN_ONCE(1, "Current pid: %d comm: %s / Idle pid: %d comm: %s",
 			  current->pid, current->comm,
@@ -800,7 +800,10 @@ static void rcu_eqs_enter_common(long long oldval, bool user)
 		do_nocb_deferred_wakeup(rdp);
 	}
 	rcu_prepare_for_idle();
-	rcu_dynticks_eqs_enter();
+	stack_tracer_disable();
+	rdtp->dynticks_nesting = 0; /* Breaks tracing momentarily. */
+	rcu_dynticks_eqs_enter(); /* After this, tracing works again. */
+	stack_tracer_enable();
 	rcu_dynticks_task_enter();
 
 	/*
@@ -821,19 +824,15 @@ static void rcu_eqs_enter_common(long long oldval, bool user)
  */
 static void rcu_eqs_enter(bool user)
 {
-	long long oldval;
 	struct rcu_dynticks *rdtp;
 
 	rdtp = this_cpu_ptr(&rcu_dynticks);
-	oldval = rdtp->dynticks_nesting;
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
-		     (oldval & DYNTICK_TASK_NEST_MASK) == 0);
-	if ((oldval & DYNTICK_TASK_NEST_MASK) == DYNTICK_TASK_NEST_VALUE) {
-		rdtp->dynticks_nesting = 0;
-		rcu_eqs_enter_common(oldval, user);
-	} else {
+		     (rdtp->dynticks_nesting & DYNTICK_TASK_NEST_MASK) == 0);
+	if ((rdtp->dynticks_nesting & DYNTICK_TASK_NEST_MASK) == DYNTICK_TASK_NEST_VALUE)
+		rcu_eqs_enter_common(user);
+	else
 		rdtp->dynticks_nesting -= DYNTICK_TASK_NEST_VALUE;
-	}
 }
 
 /**
@@ -892,19 +891,18 @@ void rcu_user_enter(void)
  */
 void rcu_irq_exit(void)
 {
-	long long oldval;
 	struct rcu_dynticks *rdtp;
 
 	RCU_LOCKDEP_WARN(!irqs_disabled(), "rcu_irq_exit() invoked with irqs enabled!!!");
 	rdtp = this_cpu_ptr(&rcu_dynticks);
-	oldval = rdtp->dynticks_nesting;
-	rdtp->dynticks_nesting--;
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
-		     rdtp->dynticks_nesting < 0);
-	if (rdtp->dynticks_nesting)
-		trace_rcu_dyntick(TPS("--="), oldval, rdtp->dynticks_nesting);
-	else
-		rcu_eqs_enter_common(oldval, true);
+		     rdtp->dynticks_nesting < 1);
+	if (rdtp->dynticks_nesting <= 1) {
+		rcu_eqs_enter_common(true);
+	} else {
+		trace_rcu_dyntick(TPS("--="), rdtp->dynticks_nesting, rdtp->dynticks_nesting - 1);
+		rdtp->dynticks_nesting--;
+	}
 	rcu_sysidle_enter(1);
 }
 
Signature · 1 line
2.10.2
#15 lore raw

Re: [PATCH 4/4] rcu: Fix dyntick-idle tracing

In reply to Steven Rostedt
url: https://github.com/0day-ci/linux/commits/Steven-Rostedt/tracing-Add-usecase-of-synchronize_rcu_tasks-and-stack_tracer_disable/20170407-122352 config: x86_64-randconfig-x009-201714 (attached as…

Hi Paul,

[auto build test ERROR on tip/perf/core]
[also build test ERROR on v4.11-rc5 next-20170406]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url: https://github.com/0day-ci/linux/commits/Steven-Rostedt/tracing-Add-usecase-of-synchronize_rcu_tasks-and-stack_tracer_disable/20170407-122352
config: x86_64-randconfig-x009-201714 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce:

        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

kernel/rcu/tree.c: In function 'rcu_eqs_enter_common':

>> kernel/rcu/tree.c:806:2: error: implicit declaration of function 'stack_tracer_enable' [-Werror=implicit-function-declaration]
     stack_tracer_enable();
     ^~~~~~~~~~~~~~~~~~~

cc1: some warnings being treated as errors

vim +/stack_tracer_enable +806 kernel/rcu/tree.c

800 do_nocb_deferred_wakeup(rdp);
801 }
802 rcu_prepare_for_idle();
803 stack_tracer_disable();
804 rdtp->dynticks_nesting = 0; /* Breaks tracing momentarily. */
805 rcu_dynticks_eqs_enter(); /* After this, tracing works again. */

 > 806		stack_tracer_enable();

807 rcu_dynticks_task_enter();
808
809 /*

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation