tracing vs rcu vs nmi

Because of the requirement that no tracing happens until after we've incremented preempt_count, see nmi_enter() / trace_rcu_enter(), mark these functions as notrace.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/sched/core.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

kernel/sched/core.c +2 -2

@@ -3781,7 +3781,7 @@ static inline void preempt_latency_start
 	}
 }
 
-void preempt_count_add(int val)
+void notrace preempt_count_add(int val)
 {
 #ifdef CONFIG_DEBUG_PREEMPT
 	/*

@@ -3813,7 +3813,7 @@ static inline void preempt_latency_stop(
 		trace_preempt_on(CALLER_ADDR0, get_lock_parent_ip());
 }
 
-void preempt_count_sub(int val)
+void notrace preempt_count_sub(int val)
 {
 #ifdef CONFIG_DEBUG_PREEMPT
 	/*

Quoted 3 lines · On Wed, 12 Feb 2020 10:32:14 +0100 Peter Zijlstra <[email protected]> wrote:

Because of the requirement that no tracing happens until after we've incremented preempt_count, see nmi_enter() / trace_rcu_enter(), mark these functions as notrace.

I actually depend on these function being traced.

We do have
"preempt_enable_notrace()" and "preempt_disable_notrace()" for places that shouldn't be traced. Can't we use those? (or simply __preempt_count_add()) in the nmi_enter() code instead? (perhaps create a preempt_count_add_notrace()).

Quoted 8 lines · Steven Rostedt

Quoted 3 lines · On Wed, 12 Feb 2020 10:32:14 +0100 Peter Zijlstra <[email protected]> wrote:

Because of the requirement that no tracing happens until after we've incremented preempt_count, see nmi_enter() / trace_rcu_enter(), mark these functions as notrace.

I actually depend on these function being traced.

Why? They already have a tracepoint inside.

Quoted 6 lines

We do have
"preempt_enable_notrace()" and "preempt_disable_notrace()" for places that shouldn't be traced. Can't we use those? (or simply __preempt_count_add()) in the nmi_enter() code instead? (perhaps create a preempt_count_add_notrace()).

My initial patch has __preempt_count_add/sub() in, but then I figured someone would go complain the tracepoint would go missing.

Quoted 2 lines

My initial patch has __preempt_count_add/sub() in, but then I figured someone would go complain the tracepoint would go missing.

Fine, but what bug are you trying to fix? I haven't seen one mentioned yet. Function tracing has recursion protection, and tracing preempt_count in nmi_enter() causes no problems. What's the problem you are trying to solve?

arch/x86/kernel/cpu/common.c +2 -2

@@ -1690,14 +1690,14 @@ void syscall_init(void)
 DEFINE_PER_CPU(int, debug_stack_usage);
 DEFINE_PER_CPU(u32, debug_idt_ctr);
 
-void debug_stack_set_zero(void)
+void notrace debug_stack_set_zero(void)
 {
 	this_cpu_inc(debug_idt_ctr);
 	load_current_idt();
 }
 NOKPROBE_SYMBOL(debug_stack_set_zero);
 
-void debug_stack_reset(void)
+void notrace debug_stack_reset(void)
 {
 	if (WARN_ON(!this_cpu_read(debug_idt_ctr)))
 		return;

arch/x86/kernel/nmi.c +6 -0

@@ -534,6 +534,9 @@ do_nmi(struct pt_regs *regs, long error_
 	}
 #endif
 
+	/*
+	 * It is important that no tracing happens before nmi_enter()!
+	 */
 	nmi_enter();
 
 	inc_irq_stat(__nmi_count);

@@ -542,6 +545,9 @@ do_nmi(struct pt_regs *regs, long error_
 		default_do_nmi(regs);
 
 	nmi_exit();
+	/*
+	 * No tracing after nmi_exit()!
+	 */
 
 #ifdef CONFIG_X86_64
 	if (unlikely(this_cpu_read(update_debug_stack))) {

Quoted 9 lines · On Wed, 12 Feb 2020 10:32:15 +0100 Peter Zijlstra <[email protected]> wrote:

Because do_nmi() must not call into tracing outside of nmi_enter()/nmi_exit(), these functions must be notrace.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/x86/kernel/cpu/common.c |    4 ++--
 arch/x86/kernel/nmi.c        |    6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

arch/x86/kernel/cpu/common.c

Quoted 2 lines

arch/x86/kernel/cpu/common.c +0 -0

This entire file is notrace:

arch/x86/kernel/cpu/Makefile:

CFLAGS_REMOVE_common.o = -pg

Quoted 4 lines

This entire file is notrace:

arch/x86/kernel/cpu/Makefile:

Quoted 1 line

CFLAGS_REMOVE_common.o = -pg

Urgh, I hate it that that annotation is so hard to find :/ Also, there seem to be various flavours of that Makefile magic around.

  CFLAGS_REMOVE_lockdep.o = $(CC_FLAGS_FTRACE)

is another variant I encountered.

Quoted 9 lines · On Wed, 12 Feb 2020 16:04:40 +0100 Peter Zijlstra <[email protected]> wrote:

Quoted 5 lines

This entire file is notrace:

arch/x86/kernel/cpu/Makefile:

CFLAGS_REMOVE_common.o = -pg

Urgh, I hate it that that annotation is so hard to find :/ Also, there seem to be various flavours of that Makefile magic around.

Quoted 3 lines

  CFLAGS_REMOVE_lockdep.o = $(CC_FLAGS_FTRACE)

is another variant I encountered.

Actually, that should be the only variant. That was added for various archs, and should be used consistently throughout.

There's a clean up series for you ;-) (or whoever)

The tracepoint interface will stop providing regular RCU context; make sure we do it ourselves, since perf makes use of regular RCU protected data.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/events/core.c |    5 +++++
 1 file changed, 5 insertions(+)

kernel/events/core.c +5 -0

@@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 c
 {
 	struct perf_sample_data data;
 	struct perf_event *event;
+	unsigned long rcu_flags;
 
 	struct perf_raw_record raw = {
 		.frag = {

@@ -8961,6 +8962,8 @@ void perf_tp_event(u16 event_type, u64 c
 	perf_sample_data_init(&data, 0, 0);
 	data.raw = &raw;
 
+	rcu_flags = trace_rcu_enter();
+
 	perf_trace_buf_update(record, event_type);
 
 	hlist_for_each_entry_rcu(event, head, hlist_entry) {

@@ -8996,6 +8999,8 @@ void perf_tp_event(u16 event_type, u64 c
 	}
 
 	perf_swevent_put_recursion_context(rctx);
+
+	trace_rcu_exit(rcu_flags);
 }
 EXPORT_SYMBOL_GPL(perf_tp_event);
 

Quoted 4 lines · On Wed, 12 Feb 2020 10:32:16 +0100 Peter Zijlstra <[email protected]> wrote:

The tracepoint interface will stop providing regular RCU context; make sure we do it ourselves, since perf makes use of regular RCU protected data.

Suggested-by Steven Rostedt (VMware) <[email protected]>

;-)

-- Steve

Quoted 33 lines

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 kernel/events/core.c |    5 +++++
 1 file changed, 5 insertions(+)

kernel/events/core.c +5 -0

@@ -8950,6 +8950,7 @@ void perf_tp_event(u16 event_type, u64 c
 {
 	struct perf_sample_data data;
 	struct perf_event *event;
+	unsigned long rcu_flags;
 
 	struct perf_raw_record raw = {
 		.frag = {

@@ -8961,6 +8962,8 @@ void perf_tp_event(u16 event_type, u64 c
 	perf_sample_data_init(&data, 0, 0);
 	data.raw = &raw;
 
+	rcu_flags = trace_rcu_enter();
+
 	perf_trace_buf_update(record, event_type);
 
 	hlist_for_each_entry_rcu(event, head, hlist_entry) {

@@ -8996,6 +8999,8 @@ void perf_tp_event(u16 event_type, u64 c
 	}
 
 	perf_swevent_put_recursion_context(rctx);
+
+	trace_rcu_exit(rcu_flags);
 }
 EXPORT_SYMBOL_GPL(perf_tp_event);
 

Effectively revert commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") now that we've taught perf how to deal with not having an RCU context provided.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/tracepoint.h |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

include/linux/tracepoint.h +1 -3

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo
 		 * For rcuidle callers, use srcu since sched-rcu	\
 		 * doesn't work from the idle path.			\
 		 */							\
-		if (rcuidle) {						\
+		if (rcuidle)						\
 			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-			rcu_irq_enter_irqsave();			\
-		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
 									\

@@ -194,10 +192,8 @@ static inline struct tracepoint *tracepo
 			} while ((++it_func_ptr)->func);		\

Quoted 26 lines · On Wed, 12 Feb 2020 10:32:18 +0100 Peter Zijlstra <[email protected]> wrote:

Effectively revert commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") now that we've taught perf how to deal with not having an RCU context provided.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/linux/tracepoint.h |    8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

include/linux/tracepoint.h +1 -3

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo
 		 * For rcuidle callers, use srcu since sched-rcu	\
 		 * doesn't work from the idle path.			\
 		 */							\
-		if (rcuidle) {						\
+		if (rcuidle)						\
 			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-			rcu_irq_enter_irqsave();			\
-		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\
 									\

@@ -194,10 +192,8 @@ static inline struct tracepoint *tracepo
 			} while ((++it_func_ptr)->func);		\

include/linux/tracepoint.h static inline struct tracepoint *tracepo · line 199

Quoted 10 lines

include/linux/tracepoint.h +1 -3

 		}							\
 									\
-		if (rcuidle) {						\
-			rcu_irq_exit_irqsave();				\
+		if (rcuidle)						\
 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
-		}							\
 									\
 		preempt_enable_notrace();				\
 	} while (0)

Which looks basically the same as this patch...

https://lore.kernel.org/r/[email protected]

Hi all,

These here patches are the result of Mathieu and Steve trying to get commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") reverted again.

One of the things discovered is that tracing MUST NOT happen before nmi_enter() or after nmi_exit(). I've only fixed x86, but quickly gone through other architectures and there is definitely more stuff to be fixed (simply grep for nmi_enter in your arch).

Quoted 10 lines · Peter Zijlstra

Hi all,

These here patches are the result of Mathieu and Steve trying to get commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") reverted again.

One of the things discovered is that tracing MUST NOT happen before nmi_enter() or after nmi_exit(). I've only fixed x86, but quickly gone through other architectures and there is definitely more stuff to be fixed (simply grep for nmi_enter in your arch).

For ARM64:

- apei_claim_sea()
- __sdei_handler()
- do_serror()
- debug_exception_enter() / do_debug_exception()

all look dodgy.

Quoted 19 lines · Peter Zijlstra

Quoted 10 lines · Peter Zijlstra

Hi all,

These here patches are the result of Mathieu and Steve trying to get commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") reverted again.

One of the things discovered is that tracing MUST NOT happen before nmi_enter() or after nmi_exit(). I've only fixed x86, but quickly gone through other architectures and there is definitely more stuff to be fixed (simply grep for nmi_enter in your arch).

For ARM64:

- apei_claim_sea()
- __sdei_handler()
- do_serror()
- debug_exception_enter() / do_debug_exception()

Quoted 1 line

all look dodgy.

Hmm, so looks like we need to spinkle some 'notrace' annotations around these. Are there are scenarios where you would want NOKPROBE_SYMBOL() but *not* 'notrace'? We've already got the former for the debug exception handlers and we probably (?) want it for the SDEI stuff too...

Hmm, so looks like we need to spinkle some 'notrace' annotations around these. Are there are scenarios where you would want NOKPROBE_SYMBOL() but *not* 'notrace'? We've already got the former for the debug exception handlers and we probably (?) want it for the SDEI stuff too...

I've been trying to get rid of notrace around the kernel, not add more.

Hi all,

These here patches are the result of Mathieu and Steve trying to get commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") reverted again.

One of the things discovered is that tracing MUST NOT happen before nmi_enter() or after nmi_exit(). I've only fixed x86, but quickly gone through other architectures and there is definitely more stuff to be fixed (simply grep for nmi_enter in your arch).

Hi all,

These here patches are the result of Mathieu and Steve trying to get commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") reverted again.

One of the things discovered is that tracing MUST NOT happen before nmi_enter() or after nmi_exit(). I've only fixed x86, but quickly gone through other architectures and there is definitely more stuff to be fixed (simply grep for nmi_enter in your arch).

Hi all,

These here patches are the result of Mathieu and Steve trying to get commit 865e63b04e9b2 ("tracing: Add back in rcu_irq_enter/exit_irqson() for rcuidle tracepoints") reverted again.

One of the things discovered is that tracing MUST NOT happen before nmi_enter() or after nmi_exit(). I've only fixed x86, but quickly gone through other architectures and there is definitely more stuff to be fixed (simply grep for nmi_enter in your arch).