tracing vs world

Since there are already a number of sites (ARM64, PowerPC) that effectively nest nmi_enter(), lets make the primitive support this before adding even more.

void SMIException(struct pt_regs *regs)

include/linux/hardirq.h +1 -1

@@ -71,7 +71,7 @@ extern void irq_exit(void);
 		printk_nmi_enter();				\
 		lockdep_off();					\
 		ftrace_nmi_enter();				\
-		BUG_ON(in_nmi());				\
+		BUG_ON(in_nmi() == NMI_MASK);			\
 		preempt_count_add(NMI_OFFSET + HARDIRQ_OFFSET);	\
 		rcu_nmi_enter();				\
 		trace_hardirq_enter();				\

include/linux/preempt.h +0 -0

@@ -26,13 +26,13 @@
  *         PREEMPT_MASK:	0x000000ff
  *         SOFTIRQ_MASK:	0x0000ff00
  *         HARDIRQ_MASK:	0x000f0000

Probably should document somewhere (in a comment above nmi_enter()?) that we allow nmi_enter() to nest up to 15 times.

Quoted 2 lines · Steven Rostedt

Probably should document somewhere (in a comment above nmi_enter()?) that we allow nmi_enter() to nest up to 15 times.

Yah, and can we make the BUG_ON() WARN_ON or so instead, so that there's at least a chance to be able to catch it for debugging. Or is the box going to be irreparably wedged after the 4 bits overflow?

Since there are already a number of sites (ARM64, PowerPC) that effectively nest nmi_enter(), lets make the primitive support this before adding even more.

Cc Will Deacon <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc Petr Mladek <[email protected]>

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

---
 arch/arm64/include/asm/hardirq.h |    4 ++--
 arch/arm64/kernel/sdei.c         |   14 ++------------
 arch/arm64/kernel/traps.c        |    8 ++------

Since there are already a number of sites (ARM64, PowerPC) that effectively nest nmi_enter(), lets make the primitive support this before adding even more.

Cc Will Deacon <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Michael Ellerman <[email protected]>

Cc Petr Mladek <[email protected]>

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

Since there are already a number of sites (ARM64, PowerPC) that effectively nest nmi_enter(), lets make the primitive support this before adding even more.

Subject: Re: [PATCH v3 02/22] x86,mce: Delete ist_begin_non_atomic()

It is an abomination; and in prepration of removing the whole ist_enter() thing, it needs to go.

Convert #MC over to using task_work_add() instead; it will run the same code slightly later, on the return to user path of the same exception.

Quoted 1 line · Peter Zijlstra

Subject: Re: [PATCH v3 02/22] x86,mce: Delete ist_begin_non_atomic()

x86/mce: ...

Quoted 6 lines

It is an abomination; and in prepration of removing the whole ist_enter() thing, it needs to go.

Convert #MC over to using task_work_add() instead; it will run the same code slightly later, on the return to user path of the same exception.

That's fine because the error happened in userspace.

Quoted 13 lines · Borislav Petkov

Quoted 1 line · Peter Zijlstra

Subject: Re: [PATCH v3 02/22] x86,mce: Delete ist_begin_non_atomic()

x86/mce: ...

Quoted 6 lines

It is an abomination; and in prepration of removing the whole ist_enter() thing, it needs to go.

Convert #MC over to using task_work_add() instead; it will run the same code slightly later, on the return to user path of the same exception.

Quoted 1 line

That's fine because the error happened in userspace.

Unless there is a signal pending and the signal setup code is about to hit the same failed memory. I suppose we can just treat cases like this as "oh well, time to kill the whole system".

But we should genuinely agree that we're okay with deferring this handling.

Quoted 20 lines · Andy Lutomirski

Quoted 6 lines · Borislav Petkov

Quoted 1 line · Peter Zijlstra

Subject: Re: [PATCH v3 02/22] x86,mce: Delete ist_begin_non_atomic()

x86/mce: ...

Quoted 8 lines

Quoted 6 lines

It is an abomination; and in prepration of removing the whole ist_enter() thing, it needs to go.

Convert #MC over to using task_work_add() instead; it will run the same code slightly later, on the return to user path of the same exception.

That's fine because the error happened in userspace.

Unless there is a signal pending and the signal setup code is about to hit the same failed memory. I suppose we can just treat cases like this as "oh well, time to kill the whole system".

Quoted 1 line

But we should genuinely agree that we're okay with deferring this handling.

It doesn't delay much. The moment it does that local_irq_enable() it's subject to preemption, just like it is on the return to user path.

Do you really want to create code that unwinds enough of nmi_enter() to get you to a preemptible context? *shudder*

One big question here: are memory failure #MC exceptions synchronous
or can they be delayed? If we get a memory failure, is it possible
that the #MC hits some random context and not the actual context where the error occurred?



Quoted 5 lines

One big question here: are memory failure #MC exceptions synchronous
or can they be delayed? If we get a memory failure, is it possible
that the #MC hits some random context and not the actual context where the error occurred?

There are a few cases:
1) SRAO (Software recoverable action optional) [Patrol scrub or L3 cache eviction] These aren't synchronous with any core execution. Using machine check to signal was probably a mistake - compounded by it being broadcast :-( Could pick any CPU to handle (actually choose the first to arrive in do_machine_check()). That guy should arrange to soft offline the affected page. Every CPU can return to what they were doing before.

2) SRAR (Software recoverable action required)
These are synchronous. Starting with Skylake they may be signaled just to the thread that hit the poison. Earlier generations broadcast.

2a) Hit in ring3 code ... we want to offline the page and SIGBUS the task(s)
2b) Memcpy_mcsafe() ... kernel has a recovery path. "Return" to the recovery code instead of to the original RIP. 2c) copy_from_user ... not implemented yet. We are in kernel, but would like to treat this like case 2a

3) Fatal
Always broadcast. Some bank has MCi_STATUS.PCC==1. System must be shutdown.

-Tony

Quoted 2 lines · Peter Zijlstra

Do you really want to create code that unwinds enough of nmi_enter() to get you to a preemptible context? *shudder*

Well, there's another way to approach this:

void notrace nonothing do_machine_check(struct pt_regs *regs) {
if (user_mode(regs))

    do_sane_machine_check(regs);
  else
    do_awful_machine_check(regs);
}

void do_sane_machine_check(regs)
{
nothing special here. just a regular exception, more or less.
}

void do_awful_macine_check(regs)
{
basically an NMI. No funny business, no recovery possible.
task_work_add() not allowed.
}

I suppose the general consideration I'm trying to get at is: is task_work_add() actually useful at all here? For the case when a kernel thread does memcpy_mcsafe() or similar, task work registered using task_work_add() will never run.

Unless there is a signal pending and the signal setup code is about to hit the same failed memory. I suppose we can just treat cases like this as "oh well, time to kill the whole system".

But we should genuinely agree that we're okay with deferring this handling.

Quoted 5 lines · Andy Lutomirski

Unless there is a signal pending and the signal setup code is about to hit the same failed memory. I suppose we can just treat cases like this as "oh well, time to kill the whole system".

But we should genuinely agree that we're okay with deferring this handling.

Good catch!

static void exit_to_usermode_loop(struct pt_regs *regs, u32 cached_flags) {

	...

		/* deal with pending signal delivery */
                if (cached_flags & _TIF_SIGPENDING)
                        do_signal(regs);

                if (cached_flags & _TIF_NOTIFY_RESUME) {
                        clear_thread_flag(TIF_NOTIFY_RESUME);
                        tracehook_notify_resume(regs);
                        rseq_handle_notify_resume(NULL, regs);
                }

Err, can we make task_work run before we handle signals? Or there's a reason it is run in this order?

Comment over task_work_add() says:

* This is like the signal handler which runs in kernel mode, but it doesn't
* try to wake up the @task.

which sounds to me like this should really run before the signal handlers...

arch/x86/kernel/cpu/mce/core.c +0 -0

 {
 	DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);

@@ -1254,10 +1254,10 @@ void do_machine_check(struct pt_regs *re
 	 */
 	int lmce = 1;
 

arch/x86/kernel/cpu/mce/core.c +1 -1

@@ -1346,7 +1346,7 @@ void do_machine_check(struct pt_regs *re
 	sync_core();
 
 	if (worst != MCE_AR_SEVERITY && !kill_it)
-		goto out_ist;
+		goto out;
 
 	/* Fault was in user mode and we need to take some action */
 	if ((m.cs & 3) == 3) {

arch/x86/kernel/cpu/mce/core.c static void mce_kill_me_maybe(struct cal · line 1223

Quoted 6 lines · Peter Zijlstra

arch/x86/kernel/cpu/mce/core.c +1 -1

@@ -1220,7 +1220,7 @@ static void mce_kill_me_maybe(struct cal
  * MCE broadcast. However some CPUs might be broken beyond repair,
  * so be always careful when synchronizing with others.
  */
-void do_machine_check(struct pt_regs *regs, long error_code)
+notrace void do_machine_check(struct pt_regs *regs, long error_code)

Is there a convention where the notrace marker should come in the function signature? I see all possible combinations while grepping...

Quoted 15 lines

arch/x86/kernel/cpu/mce/core.c +3 -3

 {
 	DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
 	DECLARE_BITMAP(toclear, MAX_NR_BANKS);

@@ -1254,10 +1254,10 @@ void do_machine_check(struct pt_regs *re
 	 */
 	int lmce = 1;
 
-	if (__mc_check_crashing_cpu(cpu))
-		return;
+	nmi_enter();
 
-	ist_enter(regs);
+	if (__mc_check_crashing_cpu(cpu))
+		goto out;
 

arch/x86/kernel/cpu/mce/core.c void do_machine_check(struct pt_regs *re · line 1263

Quoted 2 lines

arch/x86/kernel/cpu/mce/core.c +0 -0

 	this_cpu_inc(mce_exception_count);
 

Should that __mc_check_crashing_cpu() happen before nmi_enter? The function is doing only a bunch of checks and clearing MSRs for bystander CPUs...

arch/x86/lib/memcpy_32.c +1 -1

@@ -21,7 +21,7 @@ __visible void *memset(void *s, int c, s
 }
 EXPORT_SYMBOL(memset);
 
-__visible void *memmove(void *dest, const void *src, size_t n)
+__visible notrace void *memmove(void *dest, const void *src, size_t n)
 {
 	int d0,d1,d2,d3,d4,d5;
 	char *ret = dest;

Quoted 13 lines · On Wed, 19 Feb 2020 15:47:28 +0100 Peter Zijlstra <[email protected]> wrote:

arch/x86/lib/memcpy_32.c +1 -1

@@ -21,7 +21,7 @@ __visible void *memset(void *s, int c, s
 }
 EXPORT_SYMBOL(memset);
 
-__visible void *memmove(void *dest, const void *src, size_t n)
+__visible notrace void *memmove(void *dest, const void *src, size_t n)
 {
 	int d0,d1,d2,d3,d4,d5;
 	char *ret = dest;

@@ -207,3 +207,8 @@ __visible void *memmove(void *dest, cons
 

arch/x86/lib/memcpy_32.c __visible void *memmove(void *dest, cons · line 209

Quoted 2 lines

arch/x86/lib/memcpy_32.c +0 -0

 }
 EXPORT_SYMBOL(memmove);

Hmm, for things like this, which is adding notrace because of a single instance of it (although it is fine to trace in any other instance), it would be nice to have a gcc helper that could call "memmove+5" which would skip the tracing portion.

Quoted 4 lines

Hmm, for things like this, which is adding notrace because of a single instance of it (although it is fine to trace in any other instance), it would be nice to have a gcc helper that could call "memmove+5" which would skip the tracing portion.

Or just open-code the memmove() in do_double_fault() I suppose. I don't think we care about super optimized code there. It's the bloody ESPFIX trainwreck.

Quoted 16 lines · Steven Rostedt

Quoted 13 lines · On Wed, 19 Feb 2020 15:47:28 +0100 Peter Zijlstra <[email protected]> wrote:

arch/x86/lib/memcpy_32.c +1 -1

@@ -21,7 +21,7 @@ __visible void *memset(void *s, int c, s
 }
 EXPORT_SYMBOL(memset);
 
-__visible void *memmove(void *dest, const void *src, size_t n)
+__visible notrace void *memmove(void *dest, const void *src, size_t n)
 {
 	int d0,d1,d2,d3,d4,d5;
 	char *ret = dest;

@@ -207,3 +207,8 @@ __visible void *memmove(void *dest, cons
 

Quoted 7 lines

arch/x86/lib/memcpy_32.c __visible void *memmove(void *dest, cons · line 209

Quoted 2 lines

arch/x86/lib/memcpy_32.c +0 -0

 }
 EXPORT_SYMBOL(memmove);

Hmm, for things like this, which is adding notrace because of a single instance of it (although it is fine to trace in any other instance), it would be nice to have a gcc helper that could call "memmove+5" which would skip the tracing portion.

Or just open-code the memmove() in do_double_fault() I suppose. I don't think we care about super optimized code there. It's the bloody ESPFIX trainwreck.

Something like so, I suppose...

arch/x86/kernel/traps.c +3 -0

index 6ef00eb6fbb9..543de932dc7c 100644
@@ -350,14 +350,20 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code, unsign
 		regs->ip == (unsigned long)native_irq_return_iret)
 	{
 		struct pt_regs *gpregs = (struct pt_regs *)this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
+		unsigned long *dst = &gpregs->ip;
+		unsigned long *src = (void *)regs->dp;
+		int i, count = 5;

arch/x86/kernel/traps.c +3 -0

+			dst[idx] = src[idx];
+		}
+
 		gpregs->orig_ax = 0;  /* Missing (lost) #GP error code */
 
 		/*

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code, unsign · line 363

Quoted 3 lines

arch/x86/kernel/traps.c +2 -1

-		memmove(&gpregs->ip, (void *)regs->sp, 5*8);
+		for (i = 0; i < count; i++) {
+			int idx = (dst <= src) ? i : count - i;  

That's an off-by-one for going backward; 'count - 1 - i' should work better, or I should just stop typing for today ;-)

Quoted 6 lines · On Wed, 19 Feb 2020 17:04:42 +0100 Peter Zijlstra <[email protected]> wrote:

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code, unsign · line 363

Quoted 3 lines

arch/x86/kernel/traps.c +2 -1

-		memmove(&gpregs->ip, (void *)regs->sp, 5*8);
+		for (i = 0; i < count; i++) {
+			int idx = (dst <= src) ? i : count - i;  

That's an off-by-one for going backward; 'count - 1 - i' should work better, or I should just stop typing for today ;-)

Or, we could just cut and paste the current memmove and make a notrace version too. Then we don't need to worry bout bugs like this.

Quoted 10 lines · Steven Rostedt

Quoted 6 lines · On Wed, 19 Feb 2020 17:04:42 +0100 Peter Zijlstra <[email protected]> wrote:

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code, unsign · line 363

Quoted 3 lines

arch/x86/kernel/traps.c +2 -1

-		memmove(&gpregs->ip, (void *)regs->sp, 5*8);
+		for (i = 0; i < count; i++) {
+			int idx = (dst <= src) ? i : count - i;  

That's an off-by-one for going backward; 'count - 1 - i' should work better, or I should just stop typing for today ;-)

Quoted 2 lines

Or, we could just cut and paste the current memmove and make a notrace version too. Then we don't need to worry bout bugs like this.

OK, I will bite...

Can we just make the core be an inline function and make a notrace and a trace caller? Possibly going one step further and having one call the other? (Presumably the traceable version invoking the notrace version, but it has been one good long time since I have looked at function preambles.)

Quoted 14 lines · Paul E. McKenney

Quoted 10 lines · Steven Rostedt

Quoted 6 lines · On Wed, 19 Feb 2020 17:04:42 +0100 Peter Zijlstra <[email protected]> wrote:

arch/x86/kernel/traps.c dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code, unsign · line 363

Quoted 3 lines

arch/x86/kernel/traps.c +2 -1

-		memmove(&gpregs->ip, (void *)regs->sp, 5*8);
+		for (i = 0; i < count; i++) {
+			int idx = (dst <= src) ? i : count - i;  

That's an off-by-one for going backward; 'count - 1 - i' should work better, or I should just stop typing for today ;-)

Quoted 2 lines

Or, we could just cut and paste the current memmove and make a notrace version too. Then we don't need to worry bout bugs like this.

Quoted 7 lines

OK, I will bite...

Can we just make the core be an inline function and make a notrace and a trace caller? Possibly going one step further and having one call the other? (Presumably the traceable version invoking the notrace version, but it has been one good long time since I have looked at function preambles.)

One complication is that GCC (and others) are prone to stick their own implementation of memmove() (and other string functions) in at 'random'. That is, it is up to the compiler's discretion wether or not to put a call to memmove() in or just emit some random giberish they feel has the same effect.

So if we go play silly games like that, we need be careful (or just call __memmove I suppose, which is supposed to avoid that IIRC).

Quoted 2 lines

Or, we could just cut and paste the current memmove and make a notrace version too. Then we don't need to worry bout bugs like this.

OK, I will bite...

Can we just make the core be an inline function and make a notrace and a trace caller? Possibly going one step further and having one call the other? (Presumably the traceable version invoking the notrace version, but it has been one good long time since I have looked at function preambles.)

Review

Quoted 5 lines · Peter Zijlstra

(quoted diff) +4 -1

-		memmove(&gpregs->ip, (void *)regs->sp, 5*8);
+		for (i = 0; i < count; i++) {
+			int idx = (dst <= src) ? i : count - i;
+			dst[idx] = src[idx];
+		}

Or, you can actually unroll it. This way it even documents clearly what it does:

arch/x86/kernel/traps.c +1 -0

index fe38015ed50a..2b790a574ba5 100644
@@ -298,6 +298,7 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code, unsign
 		regs->ip == (unsigned long)native_irq_return_iret)
 	{
 		struct pt_regs *gpregs = (struct pt_regs *)this_cpu_read(cpu_tss_rw.x86_tss.sp0) - 1;
+		unsigned long *p = (unsigned long *)regs->sp;
 
 		/*
 		 * regs->sp points to the failing IRET frame on the

@@ -305,7 +306,11 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code, unsign
 		 * in gpregs->ss through gpregs->ip.
 		 *

Hitting the tracer or a kprobes from #DF is 'interesting', lets avoid that.

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

---

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

The rcu_nmi_enter_common() and rcu_nmi_exit_common() functions take an "irq" parameter that indicates whether these functions are invoked from an irq handler (irq==true) or an NMI handler (irq==false). However, recent changes have applied notrace to a few critical functions such that rcu_nmi_enter_common() and rcu_nmi_exit_common() many now rely on in_nmi(). Note that in_nmi() works no differently than before, but rather that tracing is now prohibited in code regions where in_nmi() would incorrectly report NMI state.

This commit therefore removes the "irq" parameter and inlines rcu_nmi_enter_common() and rcu_nmi_exit_common() into rcu_nmi_enter() and rcu_nmi_exit(), respectively.

Signed-off-by Paul E. McKenney <[email protected]>

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

---
 kernel/rcu/tree.c |   45 ++++++++++++++-------------------------------
 1 file changed, 14 insertions(+), 31 deletions(-)

kernel/rcu/tree.c +14 -31

@@ -614,16 +614,18 @@ void rcu_user_enter(void)
 }
 #endif /* CONFIG_NO_HZ_FULL */
 
-/*
+/**
+ * rcu_nmi_exit - inform RCU of exit from NMI context
+ *
  * If we are returning from the outermost NMI handler that interrupted an
  * RCU-idle period, update rdp->dynticks and rdp->dynticks_nmi_nesting
  * to let the RCU grace-period handling know that the CPU is back to
  * being RCU-idle.
  *
- * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_exit(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_exit_common(bool irq)
+void rcu_nmi_exit(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

@@ -651,27 +653,16 @@ static __always_inline void rcu_nmi_exit
 	trace_rcu_dyntick(TPS("Startirq"), rdp->dynticks_nmi_nesting, 0, atomic_read(&rdp->dynticks));
 	WRITE_ONCE(rdp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
 
-	if (irq)
+	if (!in_nmi())
 		rcu_prepare_for_idle();
 
 	rcu_dynticks_eqs_enter();
 
-	if (irq)
+	if (!in_nmi())
 		rcu_dynticks_task_enter();
 }
 
 /**
- * rcu_nmi_exit - inform RCU of exit from NMI context
- *
- * If you add or remove a call to rcu_nmi_exit(), be sure to test
- * with CONFIG_RCU_EQS_DEBUG=y.
- */
-void rcu_nmi_exit(void)
-{
-	rcu_nmi_exit_common(false);
-}
-
-/**
  * rcu_irq_exit - inform RCU that current CPU is exiting irq towards idle
  *
  * Exit from an interrupt handler, which might possibly result in entering

@@ -693,7 +684,7 @@ void rcu_nmi_exit(void)
 void rcu_irq_exit(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_exit_common(true);
+	rcu_nmi_exit();
 }
 
 /*

@@ -777,7 +768,7 @@ void rcu_user_exit(void)
 #endif /* CONFIG_NO_HZ_FULL */
 
 /**
- * rcu_nmi_enter_common - inform RCU of entry to NMI context
+ * rcu_nmi_enter - inform RCU of entry to NMI context
  * @irq: Is this call from rcu_irq_enter?
  *
  * If the CPU was idle from RCU's viewpoint, update rdp->dynticks and

@@ -786,10 +777,10 @@ void rcu_user_exit(void)
  * long as the nesting level does not overflow an int.  (You will probably
  * run out of stack space first.)
  *
- * If you add or remove a call to rcu_nmi_enter_common(), be sure to test
+ * If you add or remove a call to rcu_nmi_enter(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-static __always_inline void rcu_nmi_enter_common(bool irq)
+void rcu_nmi_enter(void)
 {
 	long incby = 2;
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);

@@ -807,12 +798,12 @@ static __always_inline void rcu_nmi_ente
 	 */
 	if (rcu_dynticks_curr_cpu_in_eqs()) {
 
-		if (irq)
+		if (!in_nmi())
 			rcu_dynticks_task_exit();
 
 		rcu_dynticks_eqs_exit();
 
-		if (irq)
+		if (!in_nmi())
 			rcu_cleanup_after_idle();
 
 		incby = 1;

@@ -834,14 +825,6 @@ static __always_inline void rcu_nmi_ente
 		   rdp->dynticks_nmi_nesting + incby);
 	barrier();
 }
-
-/**
- * rcu_nmi_enter - inform RCU of entry to NMI context
- */
-void rcu_nmi_enter(void)
-{
-	rcu_nmi_enter_common(false);
-}
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

@@ -869,7 +852,7 @@ NOKPROBE_SYMBOL(rcu_nmi_enter);
 void rcu_irq_enter(void)
 {
 	lockdep_assert_irqs_disabled();
-	rcu_nmi_enter_common(true);
+	rcu_nmi_enter();
 }
 
 /*

Quoted 11 lines · Peter Zijlstra

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

The rcu_nmi_enter_common() and rcu_nmi_exit_common() functions take an "irq" parameter that indicates whether these functions are invoked from an irq handler (irq==true) or an NMI handler (irq==false). However, recent changes have applied notrace to a few critical functions such that rcu_nmi_enter_common() and rcu_nmi_exit_common() many now rely on in_nmi(). Note that in_nmi() works no differently than before, but rather that tracing is now prohibited in code regions where in_nmi() would incorrectly report NMI state.

Quoted 6 lines

This commit therefore removes the "irq" parameter and inlines rcu_nmi_enter_common() and rcu_nmi_exit_common() into rcu_nmi_enter() and rcu_nmi_exit(), respectively.

Signed-off-by Paul E. McKenney <[email protected]>

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

Again, thank you.

Would you like to also take the added comment for NOKPROBE_SYMBOL(), or would you prefer that I carry that separately? (I dropped it for now to avoid the conflict with the patch below.)

Here is the latest version of that comment, posted by Steve Rostedt.

Thanx, Paul

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Quoted 29 lines · Paul E. McKenney

Quoted 11 lines · Peter Zijlstra

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

The rcu_nmi_enter_common() and rcu_nmi_exit_common() functions take an "irq" parameter that indicates whether these functions are invoked from an irq handler (irq==true) or an NMI handler (irq==false). However, recent changes have applied notrace to a few critical functions such that rcu_nmi_enter_common() and rcu_nmi_exit_common() many now rely on in_nmi(). Note that in_nmi() works no differently than before, but rather that tracing is now prohibited in code regions where in_nmi() would incorrectly report NMI state.

Quoted 6 lines

This commit therefore removes the "irq" parameter and inlines rcu_nmi_enter_common() and rcu_nmi_exit_common() into rcu_nmi_enter() and rcu_nmi_exit(), respectively.

Signed-off-by Paul E. McKenney <[email protected]>

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

Again, thank you.

Would you like to also take the added comment for NOKPROBE_SYMBOL(), or would you prefer that I carry that separately? (I dropped it for now to avoid the conflict with the patch below.)

Here is the latest version of that comment, posted by Steve Rostedt.

Thanx, Paul

Quoted 8 lines

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Oh right, let me stick that in a separate patch. Best we not loose that I suppose ;-)

Quoted 4 lines · Paul E. McKenney

Here is the latest version of that comment, posted by Steve Rostedt.

Thanx, Paul

Quoted 8 lines

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Oh right, let me stick that in a separate patch. Best we not loose that I suppose ;-)

Quoted 15 lines · Peter Zijlstra

Quoted 12 lines · Paul E. McKenney

Here is the latest version of that comment, posted by Steve Rostedt.

Thanx, Paul

/*
* All functions called in the breakpoint trap handler (e.g. do_int3()
* on x86), must not allow kprobes until the kprobe breakpoint handler
* is called, otherwise it can cause an infinite recursion.
* On some archs, rcu_nmi_enter() is called in the breakpoint handler
* before the kprobe breakpoint handler is called, thus it must be
* marked as NOKPROBE.
*/

Quoted 2 lines

Oh right, let me stick that in a separate patch. Best we not loose that I suppose ;-)

Having gone over the old thread, I ended up with the below. Anyone holler if I got it wrong somehow.

---
Subject: rcu: Provide comment for NOKPROBE() on rcu_nmi_enter()
From: Steven Rostedt <[email protected]>

From: Steven Rostedt <[email protected]>

The rcu_nmi_enter() function was marked NOKPROBE() by commit c13324a505c77 ("x86/kprobes: Prohibit probing on functions before kprobe_int3_handler()") because the do_int3() call kprobe code must not be invoked before kprobe_int3_handler() is called. It turns out that ist_enter() (in do_int3()) calls rcu_nmi_enter(), hence the marking NOKPROBE() being added to rcu_nmi_enter().

This commit therefore adds a comment documenting this line of reasoning.

Signed-off-by Steven Rostedt <[email protected]>

Reviewed-by Masami Hiramatsu <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

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

---
 kernel/rcu/tree.c |    8 ++++++++
 1 file changed, 8 insertions(+)

kernel/rcu/tree.c +8 -0

@@ -842,6 +842,14 @@ void rcu_nmi_enter(void)
 {
 	rcu_nmi_enter_common(false);
 }
+/*
+ * All functions called in the breakpoint trap handler (e.g. do_int3()
+ * on x86), must not allow kprobes until the kprobe breakpoint handler
+ * is called, otherwise it can cause an infinite recursion.
+ * On some archs, rcu_nmi_enter() is called in the breakpoint handler
+ * before the kprobe breakpoint handler is called, thus it must be
+ * marked as NOKPROBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

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

It's confusing that rcu_nmi_enter() is marked NOKPROBE and rcu_nmi_exit() is not. One may think that the exit needs to be marked for the same reason the enter is, as rcu_nmi_exit() reverts the RCU state back to what it was before rcu_nmi_enter(). But the reason has nothing to do with the state of RCU.

The breakpoint handler (int3 on x86) must not have any kprobe on it until the kprobe handler is called. Otherwise, it can cause an infinite recursion and crash the machine. It just so happens that rcu_nmi_enter() is called by the int3 handler before the kprobe handler can run, and therefore needs to be marked as NOKPROBE.

Comment this to remove the confusion to why rcu_nmi_enter() is marked NOKPROBE but rcu_nmi_exit() is not.

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

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

---

kernel/rcu/tree.c +8 -0

index 1694a6b57ad8..ada7b2b638fb 100644
@@ -846,6 +846,14 @@ void rcu_nmi_enter(void)
 {
        rcu_nmi_enter_common(false);
 }
+/*
+ * All functions called in the breakpoint trap handler (e.g. do_int3()
+ * on x86), must not allow kprobes until the kprobe breakpoint handler
+ * is called, otherwise it can cause an infinite recursion.
+ * On some archs, rcu_nmi_enter() is called in the breakpoint handler
+ * before the kprobe breakpoint handler is called, thus it must be
+ * marked as NOKPROBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

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

It's confusing that rcu_nmi_enter() is marked NOKPROBE and rcu_nmi_exit() is not. One may think that the exit needs to be marked for the same reason the enter is, as rcu_nmi_exit() reverts the RCU state back to what it was before rcu_nmi_enter(). But the reason has nothing to do with the state of RCU.

The breakpoint handler (int3 on x86) must not have any kprobe on it until the kprobe handler is called. Otherwise, it can cause an infinite recursion and crash the machine. It just so happens that rcu_nmi_enter() is called by the int3 handler before the kprobe handler can run, and therefore needs to be marked as NOKPROBE.

Comment this to remove the confusion to why rcu_nmi_enter() is marked NOKPROBE but rcu_nmi_exit() is not.

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

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

---

kernel/rcu/tree.c +8 -0

index 1694a6b57ad8..ada7b2b638fb 100644
@@ -846,6 +846,14 @@ void rcu_nmi_enter(void)
 {
 	rcu_nmi_enter_common(false);
 }
+/*
+ * All functions called in the breakpoint trap handler (e.g. do_int3()
+ * on x86), must not allow kprobes until the kprobe breakpoint handler
+ * is called, otherwise it can cause an infinite recursion.
+ * On some archs, rcu_nmi_enter() is called in the breakpoint handler
+ * before the kprobe breakpoint handler is called, thus it must be
+ * marked as NOKPROBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

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

It's confusing that rcu_nmi_enter() is marked NOKPROBE and rcu_nmi_exit() is not. One may think that the exit needs to be marked for the same reason the enter is, as rcu_nmi_exit() reverts the RCU state back to what it was before rcu_nmi_enter(). But the reason has nothing to do with the state of RCU.

The breakpoint handler (int3 on x86) must not have any kprobe on it until the kprobe handler is called. Otherwise, it can cause an infinite recursion and crash the machine. It just so happens that rcu_nmi_enter() is called by the int3 handler before the kprobe handler can run, and therefore needs to be marked as NOKPROBE.

Comment this to remove the confusion to why rcu_nmi_enter() is marked NOKPROBE but rcu_nmi_exit() is not.

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

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

---

kernel/rcu/tree.c +8 -0

index 1694a6b57ad8..ada7b2b638fb 100644
@@ -846,6 +846,14 @@ void rcu_nmi_enter(void)
 {
 	rcu_nmi_enter_common(false);
 }
+/*
+ * All functions called in the breakpoint trap handler (e.g. do_int3()
+ * on x86), must not allow kprobes until the kprobe breakpoint handler
+ * is called, otherwise it can cause an infinite recursion.
+ * On some archs, rcu_nmi_enter() is called in the breakpoint handler
+ * before the kprobe breakpoint handler is called, thus it must be
+ * marked as NOKPROBE.
+ */
 NOKPROBE_SYMBOL(rcu_nmi_enter);
 
 /**

The functions do in fact use local_irq_{save,restore}() and can therefore be used when IRQs are in fact disabled. Worse, they are already used in places where IRQs are disabled, leading to great confusion when reading the code.

Rename them to fix this confusion.

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

---
 include/linux/rcupdate.h   |    4 ++--
 include/linux/rcutiny.h    |    4 ++--
 include/linux/rcutree.h    |    4 ++--
 include/linux/tracepoint.h |    4 ++--
 kernel/cpu_pm.c            |    4 ++--
 kernel/rcu/tree.c          |    8 ++++----
 kernel/trace/trace.c       |    4 ++--
 7 files changed, 16 insertions(+), 16 deletions(-)

include/linux/rcupdate.h +2 -2

@@ -120,9 +120,9 @@ static inline void rcu_init_nohz(void) {
  */
 #define RCU_NONIDLE(a) \
 	do { \
-		rcu_irq_enter_irqson(); \
+		rcu_irq_enter_irqsave(); \
 		do { a; } while (0); \
-		rcu_irq_exit_irqson(); \
+		rcu_irq_exit_irqsave(); \
 	} while (0)
 
 /*

include/linux/rcutiny.h +2 -2

@@ -68,8 +68,8 @@ static inline int rcu_jiffies_till_stall
 static inline void rcu_idle_enter(void) { }
 static inline void rcu_idle_exit(void) { }
 static inline void rcu_irq_enter(void) { }
-static inline void rcu_irq_exit_irqson(void) { }
-static inline void rcu_irq_enter_irqson(void) { }
+static inline void rcu_irq_exit_irqsave(void) { }
+static inline void rcu_irq_enter_irqsave(void) { }
 static inline void rcu_irq_exit(void) { }
 static inline void exit_rcu(void) { }
 static inline bool rcu_preempt_need_deferred_qs(struct task_struct *t)

include/linux/rcutree.h +2 -2

@@ -46,8 +46,8 @@ void rcu_idle_enter(void);
 void rcu_idle_exit(void);
 void rcu_irq_enter(void);
 void rcu_irq_exit(void);
-void rcu_irq_enter_irqson(void);
-void rcu_irq_exit_irqson(void);
+void rcu_irq_enter_irqsave(void);
+void rcu_irq_exit_irqsave(void);
 
 void exit_rcu(void);
 

include/linux/tracepoint.h +2 -2

@@ -181,7 +181,7 @@ static inline struct tracepoint *tracepo
 		 */							\
 		if (rcuidle) {						\
 			__idx = srcu_read_lock_notrace(&tracepoint_srcu);\
-			rcu_irq_enter_irqson();				\
+			rcu_irq_enter_irqsave();			\
 		}							\
 									\
 		it_func_ptr = rcu_dereference_raw((tp)->funcs);		\

@@ -195,7 +195,7 @@ static inline struct tracepoint *tracepo
 		}							\
 									\
 		if (rcuidle) {						\
-			rcu_irq_exit_irqson();				\
+			rcu_irq_exit_irqsave();				\
 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
 		}							\
 									\

kernel/cpu_pm.c +2 -2

@@ -24,10 +24,10 @@ static int cpu_pm_notify(enum cpu_pm_eve
 	 * could be disfunctional in cpu idle. Copy RCU_NONIDLE code to let
 	 * RCU know this.
 	 */
-	rcu_irq_enter_irqson();
+	rcu_irq_enter_irqsave();
 	ret = __atomic_notifier_call_chain(&cpu_pm_notifier_chain, event, NULL,
 		nr_to_call, nr_calls);
-	rcu_irq_exit_irqson();
+	rcu_irq_exit_irqsave();
 
 	return notifier_to_errno(ret);
 }

kernel/rcu/tree.c +4 -4

@@ -699,10 +699,10 @@ void rcu_irq_exit(void)
 /*
  * Wrapper for rcu_irq_exit() where interrupts are enabled.
  *
- * If you add or remove a call to rcu_irq_exit_irqson(), be sure to test
+ * If you add or remove a call to rcu_irq_exit_irqsave(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-void rcu_irq_exit_irqson(void)
+void rcu_irq_exit_irqsave(void)
 {
 	unsigned long flags;
 

@@ -875,10 +875,10 @@ void rcu_irq_enter(void)
 /*
  * Wrapper for rcu_irq_enter() where interrupts are enabled.
  *
- * If you add or remove a call to rcu_irq_enter_irqson(), be sure to test
+ * If you add or remove a call to rcu_irq_enter_irqsave(), be sure to test
  * with CONFIG_RCU_EQS_DEBUG=y.
  */
-void rcu_irq_enter_irqson(void)
+void rcu_irq_enter_irqsave(void)
 {
 	unsigned long flags;
 

kernel/trace/trace.c +2 -2

@@ -3004,9 +3004,9 @@ void __trace_stack(struct trace_array *t
 	if (unlikely(in_nmi()))
 		return;
 
-	rcu_irq_enter_irqson();
+	rcu_irq_enter_irqsave();
 	__ftrace_trace_stack(buffer, flags, skip, pc, NULL);
-	rcu_irq_exit_irqson();
+	rcu_irq_exit_irqsave();
 }
 
 /**

Since rcu_is_watching() is notrace (and needs to be, as it can be called from the tracers), make sure everything it in turn calls is notrace too.

To that effect, mark rcu_dynticks_curr_cpu_in_eqs() inline, which implies notrace, as the function is tiny.

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

---
 kernel/rcu/tree.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/rcu/tree.c +1 -1

@@ -294,7 +294,7 @@ static void rcu_dynticks_eqs_online(void
  *
  * No ordering, as we are sampling CPU-local information.
  */
-static bool rcu_dynticks_curr_cpu_in_eqs(void)
+static inline bool rcu_dynticks_curr_cpu_in_eqs(void)
 {
 	struct rcu_data *rdp = this_cpu_ptr(&rcu_data);
 

There was some controversy over inline vs. notrace, leading me to ask whether we should use both inline and notrace here. ;-)

To facilitate tracers that need RCU, add some helpers to wrap the magic required.

The problem is that we can call into tracers (trace events and function tracing) while RCU isn't watching and this can happen from any context, including NMI.

It is this latter that is causing most of the trouble; we must make sure in_nmi() returns true before we land in anything tracing, otherwise we cannot recover.

These helpers are macros because of header-hell; they're placed here because of the proximity to nmi_{enter,exit{().

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

---
 include/linux/hardirq.h |   32 ++++++++++++++++++++++++++++++++

include/linux/hardirq.h +22 -0

+/*
+ * Tracing vs RCU
+ * --------------
+ *
+ * tracepoints and function-tracing can happen when RCU isn't watching (idle,
+ * or early IRQ/NMI entry).
+ *
+ * When it happens during idle or early during IRQ entry, tracing will have
+ * to inform RCU that it ought to pay attention, this is done by calling
+ * rcu_irq_enter_irqsave().
+ *
+ * On NMI entry, we must be very careful that tracing only happens after we've
+ * incremented preempt_count(), otherwise we cannot tell we're in NMI and take
+ * the special path.
+ */
+
+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		rcu_irq_enter_irqsave();			\
+		state = 1;					\

#endif /* LINUX_HARDIRQ_H */

Quoted 2 lines · On Wed, 19 Feb 2020 15:47:32 +0100 Peter Zijlstra <[email protected]> wrote:

These helpers are macros because of header-hell; they're placed here because of the proximity to nmi_{enter,exit{().

Quoted 10 lines

include/linux/hardirq.h +10 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		rcu_irq_enter_irqsave();			\
+		state = 1;					\
+	}							\
+	state;							\
+})
+

include/linux/hardirq.h extern void irq_exit(void); · line 123

Quoted 6 lines

include/linux/hardirq.h +6 -0

+#define trace_rcu_exit(state)					\
+do {								\
+	if (state)						\
+		rcu_irq_exit_irqsave();				\
+} while (0)
+

Is there a reason that these can't be static __always_inline functions?

Quoted 5 lines · Steven Rostedt

Quoted 2 lines · On Wed, 19 Feb 2020 15:47:32 +0100 Peter Zijlstra <[email protected]> wrote:

These helpers are macros because of header-hell; they're placed here because of the proximity to nmi_{enter,exit{().

^^^^

Quoted 11 lines

Quoted 11 lines

include/linux/hardirq.h +10 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		rcu_irq_enter_irqsave();			\
+		state = 1;					\
+	}							\
+	state;							\
+})
+

Quoted 8 lines

include/linux/hardirq.h extern void irq_exit(void); · line 123

Quoted 6 lines

include/linux/hardirq.h +6 -0

+#define trace_rcu_exit(state)					\
+do {								\
+	if (state)						\
+		rcu_irq_exit_irqsave();				\
+} while (0)
+  

Is there a reason that these can't be static __always_inline functions?

It can be done, but then we need fwd declarations of those RCU functions somewhere outside of rcupdate.h. It's all a bit of a mess.

Quoted 8 lines · On Wed, 19 Feb 2020 16:58:28 +0100 Peter Zijlstra <[email protected]> wrote:

Quoted 5 lines · Steven Rostedt

Quoted 2 lines · On Wed, 19 Feb 2020 15:47:32 +0100 Peter Zijlstra <[email protected]> wrote:

These helpers are macros because of header-hell; they're placed here because of the proximity to nmi_{enter,exit{().

^^^^

Bah I can't read, because I even went looking for this!

Quoted 20 lines

Quoted 19 lines

Quoted 7 lines

include/linux/hardirq.h +6 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		rcu_irq_enter_irqsave();			\
+		state = 1;					\

include/linux/hardirq.h extern void irq_exit(void); · line 123

Quoted 10 lines

include/linux/hardirq.h +10 -0

+	}							\
+	state;							\
+})
+
+#define trace_rcu_exit(state)					\
+do {								\
+	if (state)						\
+		rcu_irq_exit_irqsave();				\
+} while (0)
+  

Is there a reason that these can't be static __always_inline functions?

Quoted 2 lines

It can be done, but then we need fwd declarations of those RCU functions somewhere outside of rcupdate.h. It's all a bit of a mess.

Maybe this belongs in the rcupdate.h file then?

Quoted 37 lines · Steven Rostedt

Quoted 8 lines · On Wed, 19 Feb 2020 16:58:28 +0100 Peter Zijlstra <[email protected]> wrote:

Quoted 5 lines · Steven Rostedt

Quoted 2 lines · On Wed, 19 Feb 2020 15:47:32 +0100 Peter Zijlstra <[email protected]> wrote:

These helpers are macros because of header-hell; they're placed here because of the proximity to nmi_{enter,exit{().

^^^^

Bah I can't read, because I even went looking for this!

Quoted 22 lines

Quoted 11 lines

Quoted 11 lines

include/linux/hardirq.h +10 -0

+#define trace_rcu_enter()					\
+({								\
+	unsigned long state = 0;				\
+	if (!rcu_is_watching())	{				\
+		rcu_irq_enter_irqsave();			\
+		state = 1;					\
+	}							\
+	state;							\
+})
+

Quoted 8 lines

include/linux/hardirq.h extern void irq_exit(void); · line 123

Quoted 6 lines

include/linux/hardirq.h +6 -0

+#define trace_rcu_exit(state)					\
+do {								\
+	if (state)						\
+		rcu_irq_exit_irqsave();				\
+} while (0)
+  

Is there a reason that these can't be static __always_inline functions?

It can be done, but then we need fwd declarations of those RCU functions somewhere outside of rcupdate.h. It's all a bit of a mess.

Quoted 1 line

Maybe this belongs in the rcupdate.h file then?

Possibly, and I suppose the current version is less obviously dependent on the in_nmi() functionality as was the previous, seeing how Paul frobbed that all the way into the rcu_irq_enter*() implementation.

So sure, I can go move it I suppose.

Quoted 5 lines · Peter Zijlstra

Possibly, and I suppose the current version is less obviously dependent on the in_nmi() functionality as was the previous, seeing how Paul frobbed that all the way into the rcu_irq_enter*() implementation.

So sure, I can go move it I suppose.

No objections here.

Quoted 9 lines · Paul E. McKenney

Quoted 5 lines · Peter Zijlstra

Possibly, and I suppose the current version is less obviously dependent on the in_nmi() functionality as was the previous, seeing how Paul frobbed that all the way into the rcu_irq_enter*() implementation.

So sure, I can go move it I suppose.

No objections here.

It now looks like so:

---
Subject: rcu,tracing: Create trace_rcu_{enter,exit}()
From: Peter Zijlstra <[email protected]>
Date: Wed Feb 12 09:18:57 CET 2020

To facilitate tracers that need RCU, add some helpers to wrap the magic required.

The problem is that we can call into tracers (trace events and function tracing) while RCU isn't watching and this can happen from any context, including NMI.

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

---
 include/linux/rcupdate.h |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

include/linux/rcupdate.h +29 -0

@@ -175,6 +175,35 @@ do { \
 #error "Unknown RCU implementation specified to kernel configuration"
 #endif
 
+/**
+ * trace_rcu_enter - Force RCU to be active, for code that needs RCU readers
+ *
+ * Very similar to RCU_NONIDLE() above.
+ *
+ * Tracing can happen while RCU isn't active yet, for instance in the idle loop
+ * between rcu_idle_enter() and rcu_idle_exit(), or early in exception entry.
+ * RCU will happily ignore any read-side critical sections in this case.
+ *
+ * This function ensures that RCU is aware hereafter and the code can readily
+ * rely on RCU read-side critical sections working as expected.
+ *
+ * This function is NMI safe -- provided in_nmi() is correct and will nest up-to
+ * INT_MAX/2 times.
+ */
+static inline int trace_rcu_enter(void)
+{
+	int state = !rcu_is_watching();
+	if (state)
+		rcu_irq_enter_irqsave();
+	return state;
+}
+
+static inline void trace_rcu_exit(int state)
+{
+	if (state)
+		rcu_irq_exit_irqsave();
+}
+
 /*
  * The init_rcu_head_on_stack() and destroy_rcu_head_on_stack() calls
  * are needed for dynamic initialization and destruction of rcu_head

If we call into a tracer before in_nmi() becomes true, the tracer can no longer detect it is called from NMI context and behave correctly.

Therefore change nmi_{enter,exit}() to use __preempt_count_{add,sub}() as the normal preempt_count_{add,sub}() have a (desired) function trace entry.

This fixes a potential issue with current code; AFAICT when the function-tracer has stack-tracing enabled __trace_stack() will malfunction when it hits the preempt_count_add() function entry from NMI context.

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

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

---

If we call into a tracer before in_nmi() becomes true, the tracer can no longer detect it is called from NMI context and behave correctly.

Therefore change nmi_{enter,exit}() to use __preempt_count_{add,sub}() as the normal preempt_count_{add,sub}() have a (desired) function trace entry.

This fixes a potential issue with current code; AFAICT when the function-tracer has stack-tracing enabled __trace_stack() will malfunction when it hits the preempt_count_add() function entry from NMI context.

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

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

---

Add a few comments to do_nmi() as a result of the audit.

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

---

Replace the opencoded (and incomplete) RCU manipulations with the new helpers to ensure a regular RCU context when calling into __ftrace_trace_stack().

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

---

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 +2 -6

 		 * 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);		\
 		}							\
 									\
-		if (rcuidle) {						\
-			rcu_irq_exit_irqsave();				\
+		if (rcuidle)						\
 			srcu_read_unlock_notrace(&tracepoint_srcu, __idx);\
-		}							\
 									\
 		preempt_enable_notrace();				\
 	} while (0)

Quoted 9 lines · Peter Zijlstra

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 static inline struct tracepoint *tracepo

Quoted 3 lines

include/linux/tracepoint.h +0 -0

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo

Shouldn't we also get rid of this line above?

		int __maybe_unused __idx = 0;				\

Quoted 16 lines · Paul E. McKenney

include/linux/tracepoint.h static inline struct tracepoint *tracepo

Quoted 12 lines · Peter Zijlstra

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

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo

Shouldn't we also get rid of this line above?

Quoted 2 lines

		int __maybe_unused __idx = 0;				\

Probably makes a lot of sense, lemme fix that!

Quoted 17 lines · Peter Zijlstra

Quoted 16 lines · Paul E. McKenney

include/linux/tracepoint.h static inline struct tracepoint *tracepo

Quoted 12 lines · Peter Zijlstra

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

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo  

Shouldn't we also get rid of this line above?

Quoted 4 lines

Quoted 2 lines

		int __maybe_unused __idx = 0;				\

Probably makes a lot of sense, lemme fix that!

Oh wait, no! SRCU is the one that remains !

Quoted 21 lines · On Wed, 19 Feb 2020 18:05:07 +0100 Peter Zijlstra <[email protected]> wrote:

Quoted 20 lines · Peter Zijlstra

Quoted 18 lines · Paul E. McKenney

include/linux/tracepoint.h static inline struct tracepoint *tracepo

Quoted 12 lines · Peter Zijlstra

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

@@ -179,10 +179,8 @@ static inline struct tracepoint *tracepo  

Shouldn't we also get rid of this line above?

		int __maybe_unused __idx = 0;				\

Quoted 3 lines

Quoted 1 line

Probably makes a lot of sense, lemme fix that!

Oh wait, no! SRCU is the one that remains !

Correct, and if rcuidle is not set, and this is a macro, the SRCU portion is compiled out.

Quoted 2 lines

Correct, and if rcuidle is not set, and this is a macro, the SRCU portion is compiled out.

Sigh! Apologies for the noise!

If we are using SRCU, we don't care whether or not RCU is watching. OK, maybe finally catching up -- the whole point was use of RCU in other tracing code, wasn't it?

Quoted 3 lines · On Wed, 19 Feb 2020 09:40:25 -0800 "Paul E. McKenney" <[email protected]>…

Quoted 2 lines

Correct, and if rcuidle is not set, and this is a macro, the SRCU portion is compiled out.

Quoted 5 lines

Sigh! Apologies for the noise!

If we are using SRCU, we don't care whether or not RCU is watching. OK, maybe finally catching up -- the whole point was use of RCU in other tracing code, wasn't it?

Some callbacks (namely perf) might use RCU, but then the callbacks need to make sure rcu is watching.