tracing: Add usecase of synchronize_rcu_tasks() and stack_tracer_disable()
Steven Rostedt
maintainercover 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(-)
Steven Rostedt
maintainerPATCH 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
@@ -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
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 whenCONFIG_PREEMPTwas enabled. That's because a task could be indefinitely preempted while sitting on the trampoline. Now withsynchronize_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 withCONFIG_PREEMPTset.
Quoted 1 line
Cc "Paul E. McKenney" <[email protected]>
One question below. Other than that:
Quoted 11 lines
Signed-off-by Steven Rostedt (VMware) <[email protected]>--- kernel/trace/Kconfig | 3 ++- kernel/trace/ftrace.c | 42 ++++++++++++++++++------------------------ 2 files changed, 20 insertions(+), 25 deletions(-)kernel/trace/Kconfig +0
-0
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-Operationkernel/trace/ftrace.c +18
-24@@ -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
Steven Rostedt
maintainerRe: [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 whenCONFIG_PREEMPTwas enabled. That's because a task could be indefinitely preempted while sitting on the trampoline. Now withsynchronize_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 withCONFIG_PREEMPTset.
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
Signed-off-by Steven Rostedt (VMware) <[email protected]>--- 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@@ -134,7 +134,8 @@ config FUNCTION_TRACER select KALLSYMS select GENERIC_TRACER select CONTEXT_SWITCH_TRACER - select GLOB + select GLOBDoes 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_taskssynchronize_srcusynchronize_srcu_expeditedsync_rcu_exp_select_cpussync_rcu_exp_handler
synchronize_rcu_bh.part.65synchronize_rcu_expedited
synchronize_rcu.part.67synchronize_rcusynchronize_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
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 whenCONFIG_PREEMPTwas enabled. That's because a task could be indefinitely preempted while sitting on the trampoline. Now withsynchronize_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 withCONFIG_PREEMPTset.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
Signed-off-by Steven Rostedt (VMware) <[email protected]>--- 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@@ -134,7 +134,8 @@ config FUNCTION_TRACER select KALLSYMS select GENERIC_TRACER select CONTEXT_SWITCH_TRACER - select GLOB + select GLOBDoes 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_TRACERdepends on glob. It's what is used for the function filters. You can do:echo '*sync*rcu*' >
/sys/kernel/tracing/set_ftrace_filterAnd get:
synchronize_rcu_taskssynchronize_srcusynchronize_srcu_expeditedsync_rcu_exp_select_cpussync_rcu_exp_handler
synchronize_rcu_bh.part.65synchronize_rcu_expedited
synchronize_rcu.part.67synchronize_rcusynchronize_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
Steven Rostedt
maintainerPATCH 2/4 tracing: Replace the per_cpu() with this_cpu() in trace_stack.c
In reply to Steven RostedtFrom: "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
@@ -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
Steven Rostedt
maintainerPATCH 3/4 tracing: Add stack_tracer_disable/enable() functions
In reply to Steven RostedtFrom: "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
@@ -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
@@ -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
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()andstack_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
Signed-off-by Steven Rostedt (VMware) <[email protected]>
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@@ -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@@ -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
Steven Rostedt
maintainerRe: [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()andstack_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
Signed-off-by Steven Rostedt (VMware) <[email protected]>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@@ -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@@ -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
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()andstack_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
Signed-off-by Steven Rostedt (VMware) <[email protected]>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@@ -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@@ -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
Steven Rostedt
maintainerRe: [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()andstack_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
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()andstack_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
Steven Rostedt
maintainerRe: [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()andstack_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.hfile 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
Steven Rostedt
maintainerPATCH 4/4 rcu: Fix dyntick-idle tracing
In reply to Steven RostedtFrom: "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
@@ -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
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()andrcu_irq_exit()(with my blessing) to allow the current_rcuidlealternative 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
Reported-by Steven Rostedt <[email protected]>Signed-off-by Paul E. McKenney <[email protected]>Signed-off-by Steven Rostedt (VMware) <[email protected]>
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@@ -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
kbuild test robot
build botRe: [PATCH 4/4] rcu: Fix dyntick-idle tracing
In reply to Steven Rostedturl: 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