x86: Rework IST interrupts

x86 call do_notify_resume on paranoid returns if TIF_UPROBE is set but not on non-paranoid returns. I suspect that this is a mistake and that the code only works because int3 is paranoid.

Setting _TIF_NOTIFY_RESUME in the uprobe code was probably a workaround for the x86 bug. With that bug fixed, we can remove _TIF_NOTIFY_RESUME from the uprobes code.

Acked-by Srikar Dronamraju <[email protected]>

Reported-by Oleg Nesterov <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Quoted 8 lines · Andy Lutomirski

x86 call do_notify_resume on paranoid returns if TIF_UPROBE is set but not on non-paranoid returns. I suspect that this is a mistake and that the code only works because int3 is paranoid.

Setting _TIF_NOTIFY_RESUME in the uprobe code was probably a workaround for the x86 bug. With that bug fixed, we can remove _TIF_NOTIFY_RESUME from the uprobes code.

Quoted 3 lines

Acked-by Srikar Dronamraju <[email protected]>

Reported-by Oleg Nesterov <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

Acked-by Borislav Petkov <[email protected]>

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine. --

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

This fixes two issues:

- Scheduling from an IST interrupt handler will now warn. It would previously appear to work as long as we got lucky and nothing overwrote the stack frame. (I don't know of any bugs in this that would trigger the warning, but it's good to be on the safe side.)

- RCU handling in IST context was dangerous. As far as I know, only machine checks were likely to trigger this, but it's good to be on the safe side.

Note that the machine check handlers appears to have been missing any context tracking at all before this patch.

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

Cc Josh Triplett <[email protected]>

Cc Frédéric Weisbecker <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/traps.h         |  4 +++
 arch/x86/kernel/cpu/mcheck/mce.c     |  5 ++++
 arch/x86/kernel/cpu/mcheck/p5.c      |  6 +++++
 arch/x86/kernel/cpu/mcheck/winchip.c |  5 ++++
 arch/x86/kernel/traps.c              | 49 ++++++++++++++++++++++++++++++------
 5 files changed, 61 insertions(+), 8 deletions(-)

arch/x86/include/asm/traps.h +4 -0

index bc8352e7010a..eb16a61bfd06 100644
@@ -1,6 +1,7 @@
 #ifndef _ASM_X86_TRAPS_H
 #define _ASM_X86_TRAPS_H
 
+#include <linux/context_tracking_state.h>
 #include <linux/kprobes.h>
 
 #include <asm/debugreg.h>

@@ -109,6 +110,9 @@ asmlinkage void smp_thermal_interrupt(void);
 asmlinkage void mce_threshold_interrupt(void);
 #endif
 
+extern enum ctx_state ist_enter(struct pt_regs *regs);
+extern void ist_exit(struct pt_regs *regs, enum ctx_state prev_state);
+
 /* Interrupts/Exceptions */
 enum {
        X86_TRAP_DE = 0,        /*  0, Divide-by-zero */

arch/x86/kernel/cpu/mcheck/mce.c +5 -0

index 61a9668cebfd..b72509d77337 100644
@@ -43,6 +43,7 @@
 #include <linux/export.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 

@@ -1016,6 +1017,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 {
        struct mca_config *cfg = &mca_cfg;
        struct mce m, *final;
+       enum ctx_state prev_state;
        int i;
        int worst = 0;
        int severity;

@@ -1038,6 +1040,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
        DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
        char *msg = "Unknown";
 
+       prev_state = ist_enter(regs);
+
        this_cpu_inc(mce_exception_count);
 
        if (!cfg->banks)

@@ -1168,6 +1172,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
        mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
 out:
        sync_core();
+       ist_exit(regs, prev_state);
 }
 EXPORT_SYMBOL_GPL(do_machine_check);
 

arch/x86/kernel/cpu/mcheck/p5.c +6 -0

index a3042989398c..ec2663a708e4 100644
@@ -8,6 +8,7 @@
 #include <linux/smp.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 

@@ -17,8 +18,11 @@ int mce_p5_enabled __read_mostly;
 /* Machine check handler for Pentium class Intel CPUs: */
 static void pentium_machine_check(struct pt_regs *regs, long error_code)
 {
+       enum ctx_state prev_state;
        u32 loaddr, hi, lotype;
 
+       prev_state = ist_enter(regs);
+
        rdmsr(MSR_IA32_P5_MC_ADDR, loaddr, hi);
        rdmsr(MSR_IA32_P5_MC_TYPE, lotype, hi);
 

@@ -33,6 +37,8 @@ static void pentium_machine_check(struct pt_regs *regs, long error_code)
        }
 
        add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
+
+       ist_exit(regs, prev_state);
 }
 
 /* Set up machine check reporting for processors with Intel style MCE: */

arch/x86/kernel/cpu/mcheck/winchip.c +5 -0

index 7dc5564d0cdf..bd5d46a32210 100644
@@ -7,14 +7,19 @@
 #include <linux/types.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 
 /* Machine check handler for WinChip C6: */
 static void winchip_machine_check(struct pt_regs *regs, long error_code)
 {
+       enum ctx_state prev_state = ist_enter(regs);
+
        printk(KERN_EMERG "CPU0: Machine Check Exception.\n");
        add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
+
+       ist_exit(regs, prev_state);
 }
 
 /* Set up machine check reporting on the Winchip C6 series */

arch/x86/kernel/traps.c +41 -9

index 0d0e922fafc1..f5c4b8813774 100644
@@ -107,6 +107,39 @@ static inline void preempt_conditional_cli(struct pt_regs *regs)
        preempt_count_dec();
 }
 
+enum ctx_state ist_enter(struct pt_regs *regs)
+{
+       /*
+        * We are atomic because we're on the IST stack (or we're on x86_32,
+        * in which case we still shouldn't schedule.
+        */
+       preempt_count_add(HARDIRQ_OFFSET);
+
+       if (user_mode_vm(regs)) {
+               /* Other than that, we're just an exception. */
+               return exception_enter();
+       } else {
+               /*
+                * We might have interrupted pretty much anything.  In
+                * fact, if we're a machine check, we can even interrupt
+                * NMI processing.  We don't want in_nmi() to return true,
+                * but we need to notify RCU.
+                */
+               rcu_nmi_enter();
+               return IN_KERNEL;  /* the value is irrelevant. */
+       }
+}
+
+void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
+{
+       preempt_count_sub(HARDIRQ_OFFSET);
+
+       if (user_mode_vm(regs))
+               return exception_exit(prev_state);
+       else
+               rcu_nmi_exit();
+}
+
 static nokprobe_inline int
 do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
                  struct pt_regs *regs, long error_code)

@@ -244,14 +277,14 @@ dotraplinkage void do_stack_segment(struct pt_regs *regs, long error_code)
 {
        enum ctx_state prev_state;
 
-       prev_state = exception_enter();
+       prev_state = ist_enter(regs);
        if (notify_die(DIE_TRAP, "stack segment", regs, error_code,
                       X86_TRAP_SS, SIGBUS) != NOTIFY_STOP) {
                preempt_conditional_sti(regs);
                do_trap(X86_TRAP_SS, SIGBUS, "stack segment", regs, error_code, NULL);
                preempt_conditional_cli(regs);
        }
-       exception_exit(prev_state);
+       ist_exit(regs, prev_state);
 }
 
 dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)

@@ -259,8 +292,8 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        static const char str[] = "double fault";
        struct task_struct *tsk = current;
 
-       exception_enter();
-       /* Return not checked because double check cannot be ignored */
+       ist_enter(regs);
+       /* Return not checked because double fault cannot be ignored */
        notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_DF, SIGSEGV);
 
        tsk->thread.error_code = error_code;

@@ -343,7 +376,7 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
        if (poke_int3_handler(regs))
                return;
 
-       prev_state = exception_enter();
+       prev_state = ist_enter(regs);
 #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
        if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP,
                                SIGTRAP) == NOTIFY_STOP)

@@ -369,7 +402,7 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
        preempt_conditional_cli(regs);
        debug_stack_usage_dec();
 exit:
-       exception_exit(prev_state);
+       ist_exit(regs, prev_state);
 }
 NOKPROBE_SYMBOL(do_int3);
 

@@ -433,7 +466,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
        unsigned long dr6;
        int si_code;
 
-       prev_state = exception_enter();
+       prev_state = ist_enter(regs);
 
        get_debugreg(dr6, 6);
 

@@ -508,7 +541,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
        debug_stack_usage_dec();
 
 exit:
-       exception_exit(prev_state);
+       ist_exit(regs, prev_state);
 }
 NOKPROBE_SYMBOL(do_debug);
 
--

1.9.3

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

--Andy

Quoted 255 lines

This fixes two issues:

- Scheduling from an IST interrupt handler will now warn. It would previously appear to work as long as we got lucky and nothing overwrote the stack frame. (I don't know of any bugs in this that would trigger the warning, but it's good to be on the safe side.)

- RCU handling in IST context was dangerous. As far as I know, only machine checks were likely to trigger this, but it's good to be on the safe side.

Note that the machine check handlers appears to have been missing any context tracking at all before this patch.

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

Cc Josh Triplett <[email protected]>

Cc Frédéric Weisbecker <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/traps.h         |  4 +++
 arch/x86/kernel/cpu/mcheck/mce.c     |  5 ++++
 arch/x86/kernel/cpu/mcheck/p5.c      |  6 +++++
 arch/x86/kernel/cpu/mcheck/winchip.c |  5 ++++
 arch/x86/kernel/traps.c              | 49 ++++++++++++++++++++++++++++++------
 5 files changed, 61 insertions(+), 8 deletions(-)

arch/x86/include/asm/traps.h +4 -0

index bc8352e7010a..eb16a61bfd06 100644
@@ -1,6 +1,7 @@
 #ifndef _ASM_X86_TRAPS_H
 #define _ASM_X86_TRAPS_H
 
+#include <linux/context_tracking_state.h>
 #include <linux/kprobes.h>
 
 #include <asm/debugreg.h>

@@ -109,6 +110,9 @@ asmlinkage void smp_thermal_interrupt(void);
 asmlinkage void mce_threshold_interrupt(void);
 #endif
 
+extern enum ctx_state ist_enter(struct pt_regs *regs);
+extern void ist_exit(struct pt_regs *regs, enum ctx_state prev_state);
+
 /* Interrupts/Exceptions */
 enum {
        X86_TRAP_DE = 0,        /*  0, Divide-by-zero */

arch/x86/kernel/cpu/mcheck/mce.c +5 -0

index 61a9668cebfd..b72509d77337 100644
@@ -43,6 +43,7 @@
 #include <linux/export.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 

@@ -1016,6 +1017,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 {
        struct mca_config *cfg = &mca_cfg;
        struct mce m, *final;
+       enum ctx_state prev_state;
        int i;
        int worst = 0;
        int severity;

@@ -1038,6 +1040,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
        DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
        char *msg = "Unknown";
 
+       prev_state = ist_enter(regs);
+
        this_cpu_inc(mce_exception_count);
 
        if (!cfg->banks)

@@ -1168,6 +1172,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
        mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
 out:
        sync_core();
+       ist_exit(regs, prev_state);
 }
 EXPORT_SYMBOL_GPL(do_machine_check);
 

arch/x86/kernel/cpu/mcheck/p5.c +6 -0

index a3042989398c..ec2663a708e4 100644
@@ -8,6 +8,7 @@
 #include <linux/smp.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 

@@ -17,8 +18,11 @@ int mce_p5_enabled __read_mostly;
 /* Machine check handler for Pentium class Intel CPUs: */
 static void pentium_machine_check(struct pt_regs *regs, long error_code)
 {
+       enum ctx_state prev_state;
        u32 loaddr, hi, lotype;
 
+       prev_state = ist_enter(regs);
+
        rdmsr(MSR_IA32_P5_MC_ADDR, loaddr, hi);
        rdmsr(MSR_IA32_P5_MC_TYPE, lotype, hi);
 

@@ -33,6 +37,8 @@ static void pentium_machine_check(struct pt_regs *regs, long error_code)
        }
 
        add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
+
+       ist_exit(regs, prev_state);
 }
 
 /* Set up machine check reporting for processors with Intel style MCE: */

arch/x86/kernel/cpu/mcheck/winchip.c +5 -0

index 7dc5564d0cdf..bd5d46a32210 100644
@@ -7,14 +7,19 @@
 #include <linux/types.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 
 /* Machine check handler for WinChip C6: */
 static void winchip_machine_check(struct pt_regs *regs, long error_code)
 {
+       enum ctx_state prev_state = ist_enter(regs);
+
        printk(KERN_EMERG "CPU0: Machine Check Exception.\n");
        add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
+
+       ist_exit(regs, prev_state);
 }
 
 /* Set up machine check reporting on the Winchip C6 series */

arch/x86/kernel/traps.c +41 -9

index 0d0e922fafc1..f5c4b8813774 100644
@@ -107,6 +107,39 @@ static inline void preempt_conditional_cli(struct pt_regs *regs)
        preempt_count_dec();
 }
 
+enum ctx_state ist_enter(struct pt_regs *regs)
+{
+       /*
+        * We are atomic because we're on the IST stack (or we're on x86_32,
+        * in which case we still shouldn't schedule.
+        */
+       preempt_count_add(HARDIRQ_OFFSET);
+
+       if (user_mode_vm(regs)) {
+               /* Other than that, we're just an exception. */
+               return exception_enter();
+       } else {
+               /*
+                * We might have interrupted pretty much anything.  In
+                * fact, if we're a machine check, we can even interrupt
+                * NMI processing.  We don't want in_nmi() to return true,
+                * but we need to notify RCU.
+                */
+               rcu_nmi_enter();
+               return IN_KERNEL;  /* the value is irrelevant. */
+       }
+}
+
+void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
+{
+       preempt_count_sub(HARDIRQ_OFFSET);
+
+       if (user_mode_vm(regs))
+               return exception_exit(prev_state);
+       else
+               rcu_nmi_exit();
+}
+
 static nokprobe_inline int
 do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
                  struct pt_regs *regs, long error_code)

@@ -244,14 +277,14 @@ dotraplinkage void do_stack_segment(struct pt_regs *regs, long error_code)
 {
        enum ctx_state prev_state;
 
-       prev_state = exception_enter();
+       prev_state = ist_enter(regs);
        if (notify_die(DIE_TRAP, "stack segment", regs, error_code,
                       X86_TRAP_SS, SIGBUS) != NOTIFY_STOP) {
                preempt_conditional_sti(regs);
                do_trap(X86_TRAP_SS, SIGBUS, "stack segment", regs, error_code, NULL);
                preempt_conditional_cli(regs);
        }
-       exception_exit(prev_state);
+       ist_exit(regs, prev_state);
 }
 
 dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)

@@ -259,8 +292,8 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
        static const char str[] = "double fault";
        struct task_struct *tsk = current;
 
-       exception_enter();
-       /* Return not checked because double check cannot be ignored */
+       ist_enter(regs);
+       /* Return not checked because double fault cannot be ignored */
        notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_DF, SIGSEGV);
 
        tsk->thread.error_code = error_code;

@@ -343,7 +376,7 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
        if (poke_int3_handler(regs))
                return;
 
-       prev_state = exception_enter();
+       prev_state = ist_enter(regs);
 #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
        if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP,
                                SIGTRAP) == NOTIFY_STOP)

@@ -369,7 +402,7 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
        preempt_conditional_cli(regs);
        debug_stack_usage_dec();
 exit:
-       exception_exit(prev_state);
+       ist_exit(regs, prev_state);
 }
 NOKPROBE_SYMBOL(do_int3);
 

@@ -433,7 +466,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
        unsigned long dr6;
        int si_code;
 
-       prev_state = exception_enter();
+       prev_state = ist_enter(regs);
 
        get_debugreg(dr6, 6);
 

@@ -508,7 +541,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
        debug_stack_usage_dec();
 
 exit:
-       exception_exit(prev_state);
+       ist_exit(regs, prev_state);
 }
 NOKPROBE_SYMBOL(do_debug);
 
--

1.9.3

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

Just out of curiosity... Can an NMI occur in IST context? If it can, I need to make rcu_nmi_enter() and rcu_nmi_exit() deal properly with nested calls.

Quoted 34 lines · Paul E. McKenney

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

Quoted 4 lines

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

I guess so, as long as there are really no page faults in the idle loop.

There are, however, machine checks in the idle loop, and maybe kprobes (haven't checked), so I think this patch might fix real bugs.

Quoted 3 lines

Just out of curiosity... Can an NMI occur in IST context? If it can, I need to make rcu_nmi_enter() and rcu_nmi_exit() deal properly with nested calls.

Yes, and vice versa. That code looked like it handled nesting correctly, but I wasn't entirely sure.

Also, just to make sure: are we okay if rcu_nmi_enter() is called before exception_enter if context tracking is on and we came directly from userspace?

Quoted 36 lines · Andy Lutomirski

Quoted 34 lines · Paul E. McKenney

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

Quoted 6 lines

Quoted 4 lines

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

I guess so, as long as there are really no page faults in the idle loop.

As far as I know, there are not. If there are, someone needs to let me know! ;-)

Quoted 2 lines

There are, however, machine checks in the idle loop, and maybe kprobes (haven't checked), so I think this patch might fix real bugs.

If you can get ISTs from the idle loop, then the patch is needed.

Quoted 4 lines

Quoted 3 lines

Just out of curiosity... Can an NMI occur in IST context? If it can, I need to make rcu_nmi_enter() and rcu_nmi_exit() deal properly with nested calls.

Quoted 2 lines

Yes, and vice versa. That code looked like it handled nesting correctly, but I wasn't entirely sure.

It currently does not, please see below patch. Are you able to test nesting? It would be really cool if you could do so -- I have no way to test this patch.

Quoted 4 lines

Also, just to make sure: are we okay if rcu_nmi_enter() is called before exception_enter if context tracking is on and we came directly from userspace?

If I understand correctly, this will result in context tracking invoking rcu_user_enter(), which will result in the rcu_dynticks counter having an odd value. In that case, rcu_nmi_enter() will notice that RCU is already paying attention to this CPU via its check of atomic_read(&rdtp->dynticks) & 0x1), and will thus just return. The matching rcu_nmi_exit() will notice that the nesting count is zero, and will also just return.

Thus, everything works in that case.

In contrast, if rcu_nmi_enter() was invoked from the idle loop, it would see that RCU is not paying attention to this CPU and that the NMI nesting depth (which rcu_nmi_enter() increments) used to be zero. It would then atomically increment rtdp->dynticks, forcing RCU to start paying attention to this CPU. The matching rcu_nmi_exit() will see that the nesting count was non-zero, but became zero when decremented. This will cause rcu_nmi_exit() to atomically increment rtdp->dynticks, which will tell RCU to stop paying attention to this CPU.

Thanx, Paul

------------------------------------------------------------------------

rcu: Make rcu_nmi_enter() handle nesting

Andy Lutomirski is introducing ISTs into x86, which from RCU's viewpoint are NMIs. Because ISTs and NMIs can nest, rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting. As luck would have it, rcu_nmi_exit() handles nesting but rcu_nmi_enter() does not. This patch therefore makes rcu_nmi_enter() handle nesting.

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

kernel/rcu/tree.c +2 -1

index 8749f43f3f05..875421aff6e3 100644
@@ -770,7 +770,8 @@ void rcu_nmi_enter(void)
        if (rdtp->dynticks_nmi_nesting == 0 &&
            (atomic_read(&rdtp->dynticks) & 0x1))
                return;
-       rdtp->dynticks_nmi_nesting++;
+       if (rdtp->dynticks_nmi_nesting++ != 0)
+               return; /* Nested NMI/IST/whatever. */
        smp_mb__before_atomic();  /* Force delay from prior write. */
        atomic_inc(&rdtp->dynticks);
        /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */

Quoted 56 lines · Paul E. McKenney

Quoted 36 lines · Andy Lutomirski

Quoted 34 lines · Paul E. McKenney

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

Quoted 6 lines

Quoted 4 lines

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

I guess so, as long as there are really no page faults in the idle loop.

As far as I know, there are not. If there are, someone needs to let me know! ;-)

Quoted 2 lines

There are, however, machine checks in the idle loop, and maybe kprobes (haven't checked), so I think this patch might fix real bugs.

If you can get ISTs from the idle loop, then the patch is needed.

Quoted 4 lines

Quoted 3 lines

Just out of curiosity... Can an NMI occur in IST context? If it can, I need to make rcu_nmi_enter() and rcu_nmi_exit() deal properly with nested calls.

Quoted 6 lines

Quoted 2 lines

Yes, and vice versa. That code looked like it handled nesting correctly, but I wasn't entirely sure.

It currently does not, please see below patch. Are you able to test nesting? It would be really cool if you could do so -- I have no way to test this patch.

I can try. It's sort of easy -- I'll put an int3 into do_nmi and add a fixup to avoid crashing.

What should I look for? Should I try to force full nohz on and assert something? I don't really know how to make full nohz work.

Quoted 26 lines

Quoted 3 lines

Also, just to make sure: are we okay if rcu_nmi_enter() is called before exception_enter if context tracking is on and we came directly from userspace?

If I understand correctly, this will result in context tracking invoking rcu_user_enter(), which will result in the rcu_dynticks counter having an odd value. In that case, rcu_nmi_enter() will notice that RCU is already paying attention to this CPU via its check of atomic_read(&rdtp->dynticks) & 0x1), and will thus just return. The matching rcu_nmi_exit() will notice that the nesting count is zero, and will also just return.

Thus, everything works in that case.

In contrast, if rcu_nmi_enter() was invoked from the idle loop, it would see that RCU is not paying attention to this CPU and that the NMI nesting depth (which rcu_nmi_enter() increments) used to be zero. It would then atomically increment rtdp->dynticks, forcing RCU to start paying attention to this CPU. The matching rcu_nmi_exit() will see that the nesting count was non-zero, but became zero when decremented. This will cause rcu_nmi_exit() to atomically increment rtdp->dynticks, which will tell RCU to stop paying attention to this CPU.

Thanx, Paul

------------------------------------------------------------------------

Quoted 7 lines

rcu: Make rcu_nmi_enter() handle nesting

Andy Lutomirski is introducing ISTs into x86, which from RCU's viewpoint are NMIs. Because ISTs and NMIs can nest, rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting. As luck would have it, rcu_nmi_exit() handles nesting but rcu_nmi_enter() does not. This patch therefore makes rcu_nmi_enter() handle nesting.

Thanks. Should I add this to v5 of my series?

--Andy

Quoted 18 lines

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

kernel/rcu/tree.c +2 -1

index 8749f43f3f05..875421aff6e3 100644
@@ -770,7 +770,8 @@ void rcu_nmi_enter(void)
        if (rdtp->dynticks_nmi_nesting == 0 &&
            (atomic_read(&rdtp->dynticks) & 0x1))
                return;
-       rdtp->dynticks_nmi_nesting++;
+       if (rdtp->dynticks_nmi_nesting++ != 0)
+               return; /* Nested NMI/IST/whatever. */
        smp_mb__before_atomic();  /* Force delay from prior write. */
        atomic_inc(&rdtp->dynticks);
        /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 65 lines · Andy Lutomirski

Quoted 62 lines · Paul E. McKenney

Quoted 36 lines · Andy Lutomirski

Quoted 34 lines · Paul E. McKenney

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

Quoted 6 lines

Quoted 4 lines

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

I guess so, as long as there are really no page faults in the idle loop.

As far as I know, there are not. If there are, someone needs to let me know! ;-)

Quoted 2 lines

There are, however, machine checks in the idle loop, and maybe kprobes (haven't checked), so I think this patch might fix real bugs.

If you can get ISTs from the idle loop, then the patch is needed.

Quoted 6 lines

Quoted 3 lines

Just out of curiosity... Can an NMI occur in IST context? If it can, I need to make rcu_nmi_enter() and rcu_nmi_exit() deal properly with nested calls.

Yes, and vice versa. That code looked like it handled nesting correctly, but I wasn't entirely sure.

It currently does not, please see below patch. Are you able to test nesting? It would be really cool if you could do so -- I have no way to test this patch.

Quoted 5 lines

I can try. It's sort of easy -- I'll put an int3 into do_nmi and add a fixup to avoid crashing.

What should I look for? Should I try to force full nohz on and assert something? I don't really know how to make full nohz work.

You should look for the WARN_ON_ONCE() calls in rcu_nmi_enter() and rcu_nmi_exit() to fire.

Thanx, Paul

Quoted 63 lines

Quoted 26 lines

Quoted 3 lines

Also, just to make sure: are we okay if rcu_nmi_enter() is called before exception_enter if context tracking is on and we came directly from userspace?

If I understand correctly, this will result in context tracking invoking rcu_user_enter(), which will result in the rcu_dynticks counter having an odd value. In that case, rcu_nmi_enter() will notice that RCU is already paying attention to this CPU via its check of atomic_read(&rdtp->dynticks) & 0x1), and will thus just return. The matching rcu_nmi_exit() will notice that the nesting count is zero, and will also just return.

Thus, everything works in that case.

In contrast, if rcu_nmi_enter() was invoked from the idle loop, it would see that RCU is not paying attention to this CPU and that the NMI nesting depth (which rcu_nmi_enter() increments) used to be zero. It would then atomically increment rtdp->dynticks, forcing RCU to start paying attention to this CPU. The matching rcu_nmi_exit() will see that the nesting count was non-zero, but became zero when decremented. This will cause rcu_nmi_exit() to atomically increment rtdp->dynticks, which will tell RCU to stop paying attention to this CPU.

Thanx, Paul

------------------------------------------------------------------------

Quoted 7 lines

rcu: Make rcu_nmi_enter() handle nesting

Andy Lutomirski is introducing ISTs into x86, which from RCU's viewpoint are NMIs. Because ISTs and NMIs can nest, rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting. As luck would have it, rcu_nmi_exit() handles nesting but rcu_nmi_enter() does not. This patch therefore makes rcu_nmi_enter() handle nesting.

Thanks. Should I add this to v5 of my series?

--Andy

Quoted 9 lines

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

kernel/rcu/tree.c +0 -0

index 8749f43f3f05..875421aff6e3 100644

kernel/rcu/tree.c void rcu_nmi_enter(void) · line 777

Quoted 9 lines

kernel/rcu/tree.c +2 -1

@@ -770,7 +770,8 @@ void rcu_nmi_enter(void)
        if (rdtp->dynticks_nmi_nesting == 0 &&
            (atomic_read(&rdtp->dynticks) & 0x1))
                return;
-       rdtp->dynticks_nmi_nesting++;
+       if (rdtp->dynticks_nmi_nesting++ != 0)
+               return; /* Nested NMI/IST/whatever. */
        smp_mb__before_atomic();  /* Force delay from prior write. */
        atomic_inc(&rdtp->dynticks);
        /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */

--
Andy Lutomirski
AMA Capital Management, LLC

Quoted 69 lines · Paul E. McKenney

Quoted 68 lines · Andy Lutomirski

Quoted 56 lines · Paul E. McKenney

Quoted 36 lines · Andy Lutomirski

Quoted 34 lines · Paul E. McKenney

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

Quoted 6 lines

Quoted 4 lines

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

I guess so, as long as there are really no page faults in the idle loop.

As far as I know, there are not. If there are, someone needs to let me know! ;-)

Quoted 2 lines

There are, however, machine checks in the idle loop, and maybe kprobes (haven't checked), so I think this patch might fix real bugs.

If you can get ISTs from the idle loop, then the patch is needed.

Quoted 4 lines

Quoted 3 lines

Just out of curiosity... Can an NMI occur in IST context? If it can, I need to make rcu_nmi_enter() and rcu_nmi_exit() deal properly with nested calls.

Quoted 6 lines

Quoted 2 lines

Yes, and vice versa. That code looked like it handled nesting correctly, but I wasn't entirely sure.

It currently does not, please see below patch. Are you able to test nesting? It would be really cool if you could do so -- I have no way to test this patch.

I can try. It's sort of easy -- I'll put an int3 into do_nmi and add a fixup to avoid crashing.

Quoted 5 lines

Quoted 2 lines

What should I look for? Should I try to force full nohz on and assert something? I don't really know how to make full nohz work.

You should look for the WARN_ON_ONCE() calls in rcu_nmi_enter() and rcu_nmi_exit() to fire.

No warning with or without your patch, maybe because all of those returns skip the labels.

Also, an NMI can happen *during* rcu_nmi_enter or rcu_nmi_exit. Is that okay? Should those dynticks_nmi_nesting++ things be local_inc and local_dec_and_test?

That dynticks_nmi_nesting thing seems scary to me. Shouldn't the code unconditionally increment dynticks_nmi_nesting in rcu_nmi_enter and unconditionally decrement it in rcu_nmi_exit?

--Andy

Quoted 67 lines

Thanx, Paul

Quoted 63 lines

Quoted 26 lines

Quoted 3 lines

Also, just to make sure: are we okay if rcu_nmi_enter() is called before exception_enter if context tracking is on and we came directly from userspace?

If I understand correctly, this will result in context tracking invoking rcu_user_enter(), which will result in the rcu_dynticks counter having an odd value. In that case, rcu_nmi_enter() will notice that RCU is already paying attention to this CPU via its check of atomic_read(&rdtp->dynticks) & 0x1), and will thus just return. The matching rcu_nmi_exit() will notice that the nesting count is zero, and will also just return.

Thus, everything works in that case.

In contrast, if rcu_nmi_enter() was invoked from the idle loop, it would see that RCU is not paying attention to this CPU and that the NMI nesting depth (which rcu_nmi_enter() increments) used to be zero. It would then atomically increment rtdp->dynticks, forcing RCU to start paying attention to this CPU. The matching rcu_nmi_exit() will see that the nesting count was non-zero, but became zero when decremented. This will cause rcu_nmi_exit() to atomically increment rtdp->dynticks, which will tell RCU to stop paying attention to this CPU.

Thanx, Paul

------------------------------------------------------------------------

Quoted 7 lines

rcu: Make rcu_nmi_enter() handle nesting

Andy Lutomirski is introducing ISTs into x86, which from RCU's viewpoint are NMIs. Because ISTs and NMIs can nest, rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting. As luck would have it, rcu_nmi_exit() handles nesting but rcu_nmi_enter() does not. This patch therefore makes rcu_nmi_enter() handle nesting.

Thanks. Should I add this to v5 of my series?

--Andy

Quoted 9 lines

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

kernel/rcu/tree.c +0 -0

index 8749f43f3f05..875421aff6e3 100644

kernel/rcu/tree.c void rcu_nmi_enter(void) · line 777

Quoted 9 lines

kernel/rcu/tree.c +2 -1

@@ -770,7 +770,8 @@ void rcu_nmi_enter(void)
        if (rdtp->dynticks_nmi_nesting == 0 &&
            (atomic_read(&rdtp->dynticks) & 0x1))
                return;
-       rdtp->dynticks_nmi_nesting++;
+       if (rdtp->dynticks_nmi_nesting++ != 0)
+               return; /* Nested NMI/IST/whatever. */
        smp_mb__before_atomic();  /* Force delay from prior write. */
        atomic_inc(&rdtp->dynticks);
        /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */

--
Andy Lutomirski
AMA Capital Management, LLC

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 71 lines · Andy Lutomirski

Quoted 69 lines · Paul E. McKenney

Quoted 68 lines · Andy Lutomirski

Quoted 56 lines · Paul E. McKenney

Quoted 36 lines · Andy Lutomirski

Quoted 34 lines · Paul E. McKenney

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

Quoted 6 lines

Quoted 4 lines

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

I guess so, as long as there are really no page faults in the idle loop.

As far as I know, there are not. If there are, someone needs to let me know! ;-)

Quoted 2 lines

There are, however, machine checks in the idle loop, and maybe kprobes (haven't checked), so I think this patch might fix real bugs.

If you can get ISTs from the idle loop, then the patch is needed.

Quoted 4 lines

Quoted 3 lines

Just out of curiosity... Can an NMI occur in IST context? If it can, I need to make rcu_nmi_enter() and rcu_nmi_exit() deal properly with nested calls.

Quoted 6 lines

Quoted 2 lines

Yes, and vice versa. That code looked like it handled nesting correctly, but I wasn't entirely sure.

It currently does not, please see below patch. Are you able to test nesting? It would be really cool if you could do so -- I have no way to test this patch.

I can try. It's sort of easy -- I'll put an int3 into do_nmi and add a fixup to avoid crashing.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

What should I look for? Should I try to force full nohz on and assert something? I don't really know how to make full nohz work.

You should look for the WARN_ON_ONCE() calls in rcu_nmi_enter() and rcu_nmi_exit() to fire.

No warning with or without your patch, maybe because all of those returns skip the labels.

I will be guardedly optimistic and take this as a good sign. ;-)

Quoted 3 lines

Also, an NMI can happen *during* rcu_nmi_enter or rcu_nmi_exit. Is that okay? Should those dynticks_nmi_nesting++ things be local_inc and local_dec_and_test?

Yep, it is OK during rcu_nmi_enter() or rcu_nmi_exit(). The nested NMI will put the dynticks_nmi_nesting counter back where it was, so no chance of confusion.

Quoted 3 lines

That dynticks_nmi_nesting thing seems scary to me. Shouldn't the code unconditionally increment dynticks_nmi_nesting in rcu_nmi_enter and unconditionally decrement it in rcu_nmi_exit?

You might be able to get that to work, but the reason it is not done that way is because we might get an NMI while not in dyntick-idle state. In that case, it would be very bad to atomically increment rcu_dynticks, because that would tell RCU to ignore the CPU while it was in the NMI handler, which is the opposite of what we want.

But what did you have in mind?

Thanx, Paul

Quoted 76 lines

--Andy

Quoted 67 lines

Thanx, Paul

Quoted 63 lines

Quoted 26 lines

Quoted 3 lines

Also, just to make sure: are we okay if rcu_nmi_enter() is called before exception_enter if context tracking is on and we came directly from userspace?

If I understand correctly, this will result in context tracking invoking rcu_user_enter(), which will result in the rcu_dynticks counter having an odd value. In that case, rcu_nmi_enter() will notice that RCU is already paying attention to this CPU via its check of atomic_read(&rdtp->dynticks) & 0x1), and will thus just return. The matching rcu_nmi_exit() will notice that the nesting count is zero, and will also just return.

Thus, everything works in that case.

In contrast, if rcu_nmi_enter() was invoked from the idle loop, it would see that RCU is not paying attention to this CPU and that the NMI nesting depth (which rcu_nmi_enter() increments) used to be zero. It would then atomically increment rtdp->dynticks, forcing RCU to start paying attention to this CPU. The matching rcu_nmi_exit() will see that the nesting count was non-zero, but became zero when decremented. This will cause rcu_nmi_exit() to atomically increment rtdp->dynticks, which will tell RCU to stop paying attention to this CPU.

Thanx, Paul

------------------------------------------------------------------------

Quoted 7 lines

rcu: Make rcu_nmi_enter() handle nesting

Andy Lutomirski is introducing ISTs into x86, which from RCU's viewpoint are NMIs. Because ISTs and NMIs can nest, rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting. As luck would have it, rcu_nmi_exit() handles nesting but rcu_nmi_enter() does not. This patch therefore makes rcu_nmi_enter() handle nesting.

Thanks. Should I add this to v5 of my series?

--Andy

Quoted 9 lines

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

kernel/rcu/tree.c +0 -0

index 8749f43f3f05..875421aff6e3 100644

kernel/rcu/tree.c void rcu_nmi_enter(void) · line 777

Quoted 9 lines

kernel/rcu/tree.c +2 -1

@@ -770,7 +770,8 @@ void rcu_nmi_enter(void)
        if (rdtp->dynticks_nmi_nesting == 0 &&
            (atomic_read(&rdtp->dynticks) & 0x1))
                return;
-       rdtp->dynticks_nmi_nesting++;
+       if (rdtp->dynticks_nmi_nesting++ != 0)
+               return; /* Nested NMI/IST/whatever. */
        smp_mb__before_atomic();  /* Force delay from prior write. */
        atomic_inc(&rdtp->dynticks);
        /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */

--
Andy Lutomirski
AMA Capital Management, LLC

--
Andy Lutomirski
AMA Capital Management, LLC

Quoted 87 lines · Paul E. McKenney

Quoted 71 lines · Andy Lutomirski

Quoted 69 lines · Paul E. McKenney

Quoted 68 lines · Andy Lutomirski

Quoted 56 lines · Paul E. McKenney

Quoted 36 lines · Andy Lutomirski

Quoted 34 lines · Paul E. McKenney

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

Quoted 6 lines

Quoted 4 lines

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

I guess so, as long as there are really no page faults in the idle loop.

As far as I know, there are not. If there are, someone needs to let me know! ;-)

Quoted 2 lines

There are, however, machine checks in the idle loop, and maybe kprobes (haven't checked), so I think this patch might fix real bugs.

If you can get ISTs from the idle loop, then the patch is needed.

Quoted 4 lines

Quoted 3 lines

Just out of curiosity... Can an NMI occur in IST context? If it can, I need to make rcu_nmi_enter() and rcu_nmi_exit() deal properly with nested calls.

Quoted 6 lines

Quoted 2 lines

Yes, and vice versa. That code looked like it handled nesting correctly, but I wasn't entirely sure.

It currently does not, please see below patch. Are you able to test nesting? It would be really cool if you could do so -- I have no way to test this patch.

I can try. It's sort of easy -- I'll put an int3 into do_nmi and add a fixup to avoid crashing.

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

What should I look for? Should I try to force full nohz on and assert something? I don't really know how to make full nohz work.

You should look for the WARN_ON_ONCE() calls in rcu_nmi_enter() and rcu_nmi_exit() to fire.

No warning with or without your patch, maybe because all of those returns skip the labels.

I will be guardedly optimistic and take this as a good sign. ;-)

Quoted 3 lines

Also, an NMI can happen *during* rcu_nmi_enter or rcu_nmi_exit. Is that okay? Should those dynticks_nmi_nesting++ things be local_inc and local_dec_and_test?

Quoted 3 lines

Yep, it is OK during rcu_nmi_enter() or rcu_nmi_exit(). The nested NMI will put the dynticks_nmi_nesting counter back where it was, so no chance of confusion.

That sounds like it's making a scary assumption about the code generated by the ++ operator.

Quoted 11 lines

Quoted 3 lines

That dynticks_nmi_nesting thing seems scary to me. Shouldn't the code unconditionally increment dynticks_nmi_nesting in rcu_nmi_enter and unconditionally decrement it in rcu_nmi_exit?

You might be able to get that to work, but the reason it is not done that way is because we might get an NMI while not in dyntick-idle state. In that case, it would be very bad to atomically increment rcu_dynticks, because that would tell RCU to ignore the CPU while it was in the NMI handler, which is the opposite of what we want.

But what did you have in mind?

If you're willing to have rcu_nmi_enter return a value and pass it back to rcu_nmi_exit, then it could be like:

int rcu_nmi_enter()
{

  if (rdtp->dynticks & 1) {
    return 0;
  } else {
    rdtp->dynticks++;
    return 1;

}
}

but wrapped in a cmpxchg loop to make it atomic, and

void rcu_nmi_exit(int state)
{
if (state)
rdtp->dynticks++;
}

Otherwise, I think that there may need to be enough state somewhere so that the outermost nested rcu_nmi_enter knows whether to increment dynticks. For example, dynticks_nmi_nesting could store the nesting count * 2 - (1 if the outermost nested user needs to increment dynticks). Something like:

void rcu_nmi_enter(void)
{
/* Be very careful -- this function may be called reentrently on the
same CPU. */
atomically: increment dynticks if it's even.

/* If an rcu_nmi_enter/rcu_nmi_exit pair happens here, then it will not change
* the state. */

  local_inc(&dynticks_nmi_nesting, (we incremented dynticks ? 1 : 2));

WARN_ON(we incremented dynticks and dynticks_nmi_nesting was nonzero);
}

void rcu_nmi_exit(void)
{
WARN_ON(!(dynticks & 1));
locally atomically: dynticks_nmi_nesting -= 2, unless
dynticks_nmi_nesting == 1, in which case set it to zero

if (dynticks_nmi_nesting was 1)

    atomic_inc(&dynticks);
}

The invariant here is that, for a single unnested enter/exit, if dynticks_nmi_nesting != 0, then dynticks is odd. As a result, an rcu_nmi_enter/rcu_nmi_exit pair at any time when dynticks_nmi_nesting != 0 *or* dynticks is odd will have no net effect, so the invariant, in fact, holds for all invocations, nested or otherwise.

At least one of those conditions is true at all times during the execution of outermost pair, starting with the first atomic operation and ending with the final atomic_inc. So they nest properly no matter what else happens (unless, of course, someone else pokes dynticks in the middle).

Thoughts?

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Andy Lutomirski

On Fri, Nov 21, 2014 at 3:38 PM, Paul E. McKenney
<[email protected]> wrote:

Returning state sounds like a bad idea, if we can reasonably avoid it.

And I think I finally see what you are pointing out about my code: If another NMI comes in between the time I increment ->dynticks_nmi_nesting and the time I atomically increment ->dynticks, the nested NMI handler will incorrectly believe that RCU is already paying attention to this CPU. Which would indeed not be at all good, so good catch!!!

Quoted 36 lines

Otherwise, I think that there may need to be enough state somewhere so that the outermost nested rcu_nmi_enter knows whether to increment dynticks. For example, dynticks_nmi_nesting could store the nesting count * 2 - (1 if the outermost nested user needs to increment dynticks). Something like:

void rcu_nmi_enter(void)
{
/* Be very careful -- this function may be called reentrently on the
same CPU. */
atomically: increment dynticks if it's even.

/* If an rcu_nmi_enter/rcu_nmi_exit pair happens here, then it will not change
* the state. */

  local_inc(&dynticks_nmi_nesting, (we incremented dynticks ? 1 : 2));

WARN_ON(we incremented dynticks and dynticks_nmi_nesting was nonzero);
}

void rcu_nmi_exit(void)
{
WARN_ON(!(dynticks & 1));
locally atomically: dynticks_nmi_nesting -= 2, unless
dynticks_nmi_nesting == 1, in which case set it to zero

if (dynticks_nmi_nesting was 1)

    atomic_inc(&dynticks);
}

The invariant here is that, for a single unnested enter/exit, if dynticks_nmi_nesting != 0, then dynticks is odd. As a result, an rcu_nmi_enter/rcu_nmi_exit pair at any time when dynticks_nmi_nesting != 0 *or* dynticks is odd will have no net effect, so the invariant, in fact, holds for all invocations, nested or otherwise.

Quoted 7 lines

At least one of those conditions is true at all times during the execution of outermost pair, starting with the first atomic operation and ending with the final atomic_inc. So they nest properly no matter what else happens (unless, of course, someone else pokes dynticks in the middle).

Thoughts?

Let's see... The evenness of ->dynticks should be preserved by nested NMI handlers, so the check and increment need not be atomic. We don't have any way (other than atomic operations) to do local atomic modifications on all architectures, because we cannot mask NMIs. (Yes, it can work on x86, but this is common code that needs to work everywhere.) On the other hand, presumably NMIs are rare, so atomic modification of the NMI nesting counter should be OK, at least if it proves absolutely necessary. And I am thinking that a mechanical proof will be needed here. :-/

But first, let me try generating the code and informally evaluating it:

         1   struct rcu_dynticks {
         2     long long dynticks_nesting;
         3     int dynticks_nmi_nesting;
         4     atomic_t dynticks;
         5   };
         6
         7   void rcu_nmi_enter(void)
         8   {
         9     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);

        10     int incby = 2;
        11
        12     if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
        13       smp_mb__before_atomic();
        14       atomic_inc(&rdtp->dynticks);
        15       smp_mb__after_atomic();
        16       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        17       incby = 1;

        18     }
        19     rdtp->dynticks_nmi_nesting += incby;

        20     barrier();
        21   }
        22
        23   void rcu_nmi_exit(void)
        24   {
        25     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
        26
        27     WARN_ON_ONCE(!rdtp->dynticks_nmi_nesting);
        28     WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        29     if (rdtp->dynticks_nmi_nesting != 1) {

        30       rdtp->dynticks_nmi_nesting -= 2;
        31       return;
        32     }
        33     rdtp->dynticks_nmi_nesting = 0;
        34     smp_mb__before_atomic();

        35     atomic_inc(&rdtp->dynticks);
        36     smp_mb__after_atomic();
        37     WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        38   }

Line 9 picks up a pointer to this CPU's rcu_dynticks structure and line 10 assumes that we don't need to increment ->dynticks.

Line 12 checks to see if ->dynticks is even. Note that this check is
stable: If there are nested NMIs, they will increment ->dynticks twice or not at all, and either way preserves the evenness (to be proven, of course, but that is the plan). If ->dynticks is even, lines 13-15 atomically increment it, line 16 complains if still even, and line 17 says we will increment ->dynticks_nmi_nesting by only 1.

Either way, line 19 increments ->dynticks_nmi_nesting as needed and line 20 keeps the compiler from getting too cute.

For rcu_nmi_exit(), line 25 again picks up this CPUs rcu_dynticks structure. Lines 27 and 28 complain bitterly if invariants are violated. If line 29 finds that the value of ->dynticks_nmi_nesting is not 1, then line 30 subtracts 2 from ->dynticks_nmi_nesting and line 31 returns.

Otherwise, line 33 sets ->dynticks_nmi_nesting to zero, lines 34-36 atomically increment ->dynticks with full ordering, and line 37 complains bitterly if ->dynticks is not even.

So, if an NMI occurs before rcu_nmi_enter's atomic increment, then the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will think that they are not nested, which is the correct thing for them to think in that case. They will increment ->dynticks twice and restore ->dynticks_nmi_nesting to zero (adding and then subtracting 1). If the NMI happens after the atomic increment, then the nested rcu_nmi_enter() and rcu_nmi_exit() will leave ->dynticks alone, and will restore ->dynticks_nmi_nesting to zero (adding and subtracting two again). If the NMI happens after the increment of ->dynticks_nmi_nesting, the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will again restore ->dynticks_nmi_nesting, but this time to one (again adding and subtracting two).

In rcu_nmi_exit(), ->dynticks_nmi_nesting of zero had better not happen, one means we need to atomically increment ->dynticks, and other values mean that we are partially or fully nested. Reasoning proceeds as for rcu_nmi_enter(), but in the opposite direction.

Whew! That might even work.

But how about taking a different approach. Assuming that there can never be more than (say) 14 nesting NMI-like things, use the lower four bits of ->dynticks to represent the NMI nesting and the upper 28 bits as the counter. This of course requires modifying lots of places in RCU that check the counter, but it is probably time to abstract the check anyway.

This would allow my earlier attempted logic to work and (maybe) simplify the reasoning a bit (and yes, the "magic" constants need macros):

        void rcu_nmi_enter(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x01;

                WARN_ON_ONCE(nexting == 0xf);
                if (nesting == 0) {
                        if (atomic_read(&rdtp->dynticks) & 0x10)
                                return;
                        incby = 0x11;
                }
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        }

        void rcu_nmi_exit(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x0f;

                if (nesting == 0)
                        return;
                if (nesting > 1)
                        incby = -1;
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        }

Over to you! ;-)

Thanx, Paul

Quoted 8 lines · Paul E. McKenney

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Andy Lutomirski

On Fri, Nov 21, 2014 at 3:38 PM, Paul E. McKenney
<[email protected]> wrote:

Returning state sounds like a bad idea, if we can reasonably avoid it.

I agree, except that we already do it for exception_enter(), etc. But yes, changing fewer things is nice.

Quoted 71 lines

And I think I finally see what you are pointing out about my code: If another NMI comes in between the time I increment ->dynticks_nmi_nesting and the time I atomically increment ->dynticks, the nested NMI handler will incorrectly believe that RCU is already paying attention to this CPU. Which would indeed not be at all good, so good catch!!!

Quoted 36 lines

Otherwise, I think that there may need to be enough state somewhere so that the outermost nested rcu_nmi_enter knows whether to increment dynticks. For example, dynticks_nmi_nesting could store the nesting count * 2 - (1 if the outermost nested user needs to increment dynticks). Something like:

void rcu_nmi_enter(void)
{
/* Be very careful -- this function may be called reentrently on the
same CPU. */
atomically: increment dynticks if it's even.

/* If an rcu_nmi_enter/rcu_nmi_exit pair happens here, then it will not change
* the state. */

  local_inc(&dynticks_nmi_nesting, (we incremented dynticks ? 1 : 2));

WARN_ON(we incremented dynticks and dynticks_nmi_nesting was nonzero);
}

void rcu_nmi_exit(void)
{
WARN_ON(!(dynticks & 1));
locally atomically: dynticks_nmi_nesting -= 2, unless
dynticks_nmi_nesting == 1, in which case set it to zero

if (dynticks_nmi_nesting was 1)

    atomic_inc(&dynticks);
}

The invariant here is that, for a single unnested enter/exit, if dynticks_nmi_nesting != 0, then dynticks is odd. As a result, an rcu_nmi_enter/rcu_nmi_exit pair at any time when dynticks_nmi_nesting != 0 *or* dynticks is odd will have no net effect, so the invariant, in fact, holds for all invocations, nested or otherwise.

Quoted 7 lines

At least one of those conditions is true at all times during the execution of outermost pair, starting with the first atomic operation and ending with the final atomic_inc. So they nest properly no matter what else happens (unless, of course, someone else pokes dynticks in the middle).

Thoughts?

Let's see... The evenness of ->dynticks should be preserved by nested NMI handlers, so the check and increment need not be atomic. We don't have any way (other than atomic operations) to do local atomic modifications on all architectures, because we cannot mask NMIs. (Yes, it can work on x86, but this is common code that needs to work everywhere.) On the other hand, presumably NMIs are rare, so atomic modification of the NMI nesting counter should be OK, at least if it proves absolutely necessary. And I am thinking that a mechanical proof will be needed here. :-/

But first, let me try generating the code and informally evaluating it:

         1   struct rcu_dynticks {
         2     long long dynticks_nesting;
         3     int dynticks_nmi_nesting;
         4     atomic_t dynticks;
         5   };
         6
         7   void rcu_nmi_enter(void)
         8   {
         9     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);

Quoted 8 lines

        10     int incby = 2;
        11
        12     if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
        13       smp_mb__before_atomic();
        14       atomic_inc(&rdtp->dynticks);
        15       smp_mb__after_atomic();
        16       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        17       incby = 1;

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 1) here, perhaps?

Quoted 2 lines

        18     }
        19     rdtp->dynticks_nmi_nesting += incby;

Oh, I see why you don't need local_add -- it's because an nmi in the middle of this increment won't have any effect on the interrupted code, so even a software RMW will be okay.

Quoted 10 lines

        20     barrier();
        21   }
        22
        23   void rcu_nmi_exit(void)
        24   {
        25     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
        26
        27     WARN_ON_ONCE(!rdtp->dynticks_nmi_nesting);
        28     WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        29     if (rdtp->dynticks_nmi_nesting != 1) {

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 2), perhaps?

Quoted 5 lines

        30       rdtp->dynticks_nmi_nesting -= 2;
        31       return;
        32     }
        33     rdtp->dynticks_nmi_nesting = 0;
        34     smp_mb__before_atomic();

This implies barrier(), right?

Quoted 39 lines

        35     atomic_inc(&rdtp->dynticks);
        36     smp_mb__after_atomic();
        37     WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        38   }

Line 9 picks up a pointer to this CPU's rcu_dynticks structure and line 10 assumes that we don't need to increment ->dynticks.

Line 12 checks to see if ->dynticks is even. Note that this check is
stable: If there are nested NMIs, they will increment ->dynticks twice or not at all, and either way preserves the evenness (to be proven, of course, but that is the plan). If ->dynticks is even, lines 13-15 atomically increment it, line 16 complains if still even, and line 17 says we will increment ->dynticks_nmi_nesting by only 1.

Either way, line 19 increments ->dynticks_nmi_nesting as needed and line 20 keeps the compiler from getting too cute.

For rcu_nmi_exit(), line 25 again picks up this CPUs rcu_dynticks structure. Lines 27 and 28 complain bitterly if invariants are violated. If line 29 finds that the value of ->dynticks_nmi_nesting is not 1, then line 30 subtracts 2 from ->dynticks_nmi_nesting and line 31 returns.

Otherwise, line 33 sets ->dynticks_nmi_nesting to zero, lines 34-36 atomically increment ->dynticks with full ordering, and line 37 complains bitterly if ->dynticks is not even.

So, if an NMI occurs before rcu_nmi_enter's atomic increment, then the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will think that they are not nested, which is the correct thing for them to think in that case. They will increment ->dynticks twice and restore ->dynticks_nmi_nesting to zero (adding and then subtracting 1). If the NMI happens after the atomic increment, then the nested rcu_nmi_enter() and rcu_nmi_exit() will leave ->dynticks alone, and will restore ->dynticks_nmi_nesting to zero (adding and subtracting two again). If the NMI happens after the increment of ->dynticks_nmi_nesting, the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will again restore ->dynticks_nmi_nesting, but this time to one (again adding and subtracting two).

Quoted 6 lines

In rcu_nmi_exit(), ->dynticks_nmi_nesting of zero had better not happen, one means we need to atomically increment ->dynticks, and other values mean that we are partially or fully nested. Reasoning proceeds as for rcu_nmi_enter(), but in the opposite direction.

Whew! That might even work.

I think I like this, with the warnings above.

Quoted 44 lines

But how about taking a different approach. Assuming that there can never be more than (say) 14 nesting NMI-like things, use the lower four bits of ->dynticks to represent the NMI nesting and the upper 28 bits as the counter. This of course requires modifying lots of places in RCU that check the counter, but it is probably time to abstract the check anyway.

This would allow my earlier attempted logic to work and (maybe) simplify the reasoning a bit (and yes, the "magic" constants need macros):

        void rcu_nmi_enter(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x01;

                WARN_ON_ONCE(nexting == 0xf);
                if (nesting == 0) {
                        if (atomic_read(&rdtp->dynticks) & 0x10)
                                return;
                        incby = 0x11;
                }
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        }

        void rcu_nmi_exit(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x0f;

                if (nesting == 0)
                        return;
                if (nesting > 1)
                        incby = -1;
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        }

Quoted 1 line

Over to you! ;-)

This latter one is all you :)

--Andy

Quoted 3 lines

Thanx, Paul

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 94 lines · Andy Lutomirski

Quoted 8 lines · Paul E. McKenney

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Andy Lutomirski

On Fri, Nov 21, 2014 at 3:38 PM, Paul E. McKenney
<[email protected]> wrote:

Returning state sounds like a bad idea, if we can reasonably avoid it.

I agree, except that we already do it for exception_enter(), etc. But yes, changing fewer things is nice.

Quoted 71 lines

And I think I finally see what you are pointing out about my code: If another NMI comes in between the time I increment ->dynticks_nmi_nesting and the time I atomically increment ->dynticks, the nested NMI handler will incorrectly believe that RCU is already paying attention to this CPU. Which would indeed not be at all good, so good catch!!!

Quoted 36 lines

Otherwise, I think that there may need to be enough state somewhere so that the outermost nested rcu_nmi_enter knows whether to increment dynticks. For example, dynticks_nmi_nesting could store the nesting count * 2 - (1 if the outermost nested user needs to increment dynticks). Something like:

void rcu_nmi_enter(void)
{
/* Be very careful -- this function may be called reentrently on the
same CPU. */
atomically: increment dynticks if it's even.

/* If an rcu_nmi_enter/rcu_nmi_exit pair happens here, then it will not change
* the state. */

  local_inc(&dynticks_nmi_nesting, (we incremented dynticks ? 1 : 2));

WARN_ON(we incremented dynticks and dynticks_nmi_nesting was nonzero);
}

void rcu_nmi_exit(void)
{
WARN_ON(!(dynticks & 1));
locally atomically: dynticks_nmi_nesting -= 2, unless
dynticks_nmi_nesting == 1, in which case set it to zero

if (dynticks_nmi_nesting was 1)

    atomic_inc(&dynticks);
}

The invariant here is that, for a single unnested enter/exit, if dynticks_nmi_nesting != 0, then dynticks is odd. As a result, an rcu_nmi_enter/rcu_nmi_exit pair at any time when dynticks_nmi_nesting != 0 *or* dynticks is odd will have no net effect, so the invariant, in fact, holds for all invocations, nested or otherwise.

Quoted 7 lines

At least one of those conditions is true at all times during the execution of outermost pair, starting with the first atomic operation and ending with the final atomic_inc. So they nest properly no matter what else happens (unless, of course, someone else pokes dynticks in the middle).

Thoughts?

Let's see... The evenness of ->dynticks should be preserved by nested NMI handlers, so the check and increment need not be atomic. We don't have any way (other than atomic operations) to do local atomic modifications on all architectures, because we cannot mask NMIs. (Yes, it can work on x86, but this is common code that needs to work everywhere.) On the other hand, presumably NMIs are rare, so atomic modification of the NMI nesting counter should be OK, at least if it proves absolutely necessary. And I am thinking that a mechanical proof will be needed here. :-/

But first, let me try generating the code and informally evaluating it:

         1   struct rcu_dynticks {
         2     long long dynticks_nesting;
         3     int dynticks_nmi_nesting;
         4     atomic_t dynticks;
         5   };
         6
         7   void rcu_nmi_enter(void)
         8   {
         9     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);

Quoted 8 lines

        10     int incby = 2;
        11
        12     if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
        13       smp_mb__before_atomic();
        14       atomic_inc(&rdtp->dynticks);
        15       smp_mb__after_atomic();
        16       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        17       incby = 1;

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 1) here, perhaps?

That would make sense.

Quoted 6 lines

Quoted 2 lines

        18     }
        19     rdtp->dynticks_nmi_nesting += incby;

Oh, I see why you don't need local_add -- it's because an nmi in the middle of this increment won't have any effect on the interrupted code, so even a software RMW will be okay.

Yep! ;-)

Quoted 11 lines

Quoted 10 lines

        20     barrier();
        21   }
        22
        23   void rcu_nmi_exit(void)
        24   {
        25     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
        26
        27     WARN_ON_ONCE(!rdtp->dynticks_nmi_nesting);
        28     WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        29     if (rdtp->dynticks_nmi_nesting != 1) {

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 2), perhaps?

This is already implied by the WARN_ON_ONCE() on line 27 and the check on line 29.

Quoted 8 lines

Quoted 6 lines

        30       rdtp->dynticks_nmi_nesting -= 2;
        31       return;
        32     }
        33     rdtp->dynticks_nmi_nesting = 0;
        34     smp_mb__before_atomic();

This implies barrier(), right?

Yep!

Quoted 44 lines

Quoted 44 lines

        35     atomic_inc(&rdtp->dynticks);
        36     smp_mb__after_atomic();
        37     WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        38   }

Line 9 picks up a pointer to this CPU's rcu_dynticks structure and line 10 assumes that we don't need to increment ->dynticks.

Line 12 checks to see if ->dynticks is even. Note that this check is
stable: If there are nested NMIs, they will increment ->dynticks twice or not at all, and either way preserves the evenness (to be proven, of course, but that is the plan). If ->dynticks is even, lines 13-15 atomically increment it, line 16 complains if still even, and line 17 says we will increment ->dynticks_nmi_nesting by only 1.

Either way, line 19 increments ->dynticks_nmi_nesting as needed and line 20 keeps the compiler from getting too cute.

For rcu_nmi_exit(), line 25 again picks up this CPUs rcu_dynticks structure. Lines 27 and 28 complain bitterly if invariants are violated. If line 29 finds that the value of ->dynticks_nmi_nesting is not 1, then line 30 subtracts 2 from ->dynticks_nmi_nesting and line 31 returns.

Otherwise, line 33 sets ->dynticks_nmi_nesting to zero, lines 34-36 atomically increment ->dynticks with full ordering, and line 37 complains bitterly if ->dynticks is not even.

So, if an NMI occurs before rcu_nmi_enter's atomic increment, then the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will think that they are not nested, which is the correct thing for them to think in that case. They will increment ->dynticks twice and restore ->dynticks_nmi_nesting to zero (adding and then subtracting 1). If the NMI happens after the atomic increment, then the nested rcu_nmi_enter() and rcu_nmi_exit() will leave ->dynticks alone, and will restore ->dynticks_nmi_nesting to zero (adding and subtracting two again). If the NMI happens after the increment of ->dynticks_nmi_nesting, the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will again restore ->dynticks_nmi_nesting, but this time to one (again adding and subtracting two).

In rcu_nmi_exit(), ->dynticks_nmi_nesting of zero had better not happen, one means we need to atomically increment ->dynticks, and other values mean that we are partially or fully nested. Reasoning proceeds as for rcu_nmi_enter(), but in the opposite direction.

Quoted 3 lines

Quoted 1 line

Whew! That might even work.

I think I like this, with the warnings above.

OK with dropping the one that I called out as redundant?

Quoted 45 lines

Quoted 45 lines

But how about taking a different approach. Assuming that there can never be more than (say) 14 nesting NMI-like things, use the lower four bits of ->dynticks to represent the NMI nesting and the upper 28 bits as the counter. This of course requires modifying lots of places in RCU that check the counter, but it is probably time to abstract the check anyway.

This would allow my earlier attempted logic to work and (maybe) simplify the reasoning a bit (and yes, the "magic" constants need macros):

        void rcu_nmi_enter(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x01;

                WARN_ON_ONCE(nexting == 0xf);
                if (nesting == 0) {
                        if (atomic_read(&rdtp->dynticks) & 0x10)
                                return;
                        incby = 0x11;
                }
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        }

        void rcu_nmi_exit(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x0f;

                if (nesting == 0)
                        return;
                if (nesting > 1)
                        incby = -1;
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        }

Quoted 3 lines

Quoted 1 line

Over to you! ;-)

This latter one is all you :)

Well, let's see how I feel about it after trying a Promela model of the first code sequence. ;-)

Quoted 119 lines · Paul E. McKenney

Quoted 94 lines · Andy Lutomirski

Quoted 8 lines · Paul E. McKenney

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Andy Lutomirski

On Fri, Nov 21, 2014 at 3:38 PM, Paul E. McKenney
<[email protected]> wrote:

Returning state sounds like a bad idea, if we can reasonably avoid it.

I agree, except that we already do it for exception_enter(), etc. But yes, changing fewer things is nice.

Quoted 71 lines

And I think I finally see what you are pointing out about my code: If another NMI comes in between the time I increment ->dynticks_nmi_nesting and the time I atomically increment ->dynticks, the nested NMI handler will incorrectly believe that RCU is already paying attention to this CPU. Which would indeed not be at all good, so good catch!!!

Quoted 36 lines

Otherwise, I think that there may need to be enough state somewhere so that the outermost nested rcu_nmi_enter knows whether to increment dynticks. For example, dynticks_nmi_nesting could store the nesting count * 2 - (1 if the outermost nested user needs to increment dynticks). Something like:

void rcu_nmi_enter(void)
{
/* Be very careful -- this function may be called reentrently on the
same CPU. */
atomically: increment dynticks if it's even.

/* If an rcu_nmi_enter/rcu_nmi_exit pair happens here, then it will not change
* the state. */

  local_inc(&dynticks_nmi_nesting, (we incremented dynticks ? 1 : 2));

WARN_ON(we incremented dynticks and dynticks_nmi_nesting was nonzero);
}

void rcu_nmi_exit(void)
{
WARN_ON(!(dynticks & 1));
locally atomically: dynticks_nmi_nesting -= 2, unless
dynticks_nmi_nesting == 1, in which case set it to zero

if (dynticks_nmi_nesting was 1)

    atomic_inc(&dynticks);
}

The invariant here is that, for a single unnested enter/exit, if dynticks_nmi_nesting != 0, then dynticks is odd. As a result, an rcu_nmi_enter/rcu_nmi_exit pair at any time when dynticks_nmi_nesting != 0 *or* dynticks is odd will have no net effect, so the invariant, in fact, holds for all invocations, nested or otherwise.

Quoted 7 lines

At least one of those conditions is true at all times during the execution of outermost pair, starting with the first atomic operation and ending with the final atomic_inc. So they nest properly no matter what else happens (unless, of course, someone else pokes dynticks in the middle).

Thoughts?

Let's see... The evenness of ->dynticks should be preserved by nested NMI handlers, so the check and increment need not be atomic. We don't have any way (other than atomic operations) to do local atomic modifications on all architectures, because we cannot mask NMIs. (Yes, it can work on x86, but this is common code that needs to work everywhere.) On the other hand, presumably NMIs are rare, so atomic modification of the NMI nesting counter should be OK, at least if it proves absolutely necessary. And I am thinking that a mechanical proof will be needed here. :-/

But first, let me try generating the code and informally evaluating it:

         1   struct rcu_dynticks {
         2     long long dynticks_nesting;
         3     int dynticks_nmi_nesting;
         4     atomic_t dynticks;
         5   };
         6
         7   void rcu_nmi_enter(void)
         8   {
         9     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);

Quoted 8 lines

        10     int incby = 2;
        11
        12     if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
        13       smp_mb__before_atomic();
        14       atomic_inc(&rdtp->dynticks);
        15       smp_mb__after_atomic();
        16       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        17       incby = 1;

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 1) here, perhaps?

That would make sense.

Quoted 6 lines

Quoted 2 lines

        18     }
        19     rdtp->dynticks_nmi_nesting += incby;

Oh, I see why you don't need local_add -- it's because an nmi in the middle of this increment won't have any effect on the interrupted code, so even a software RMW will be okay.

Yep! ;-)

Quoted 11 lines

Quoted 10 lines

        20     barrier();
        21   }
        22
        23   void rcu_nmi_exit(void)
        24   {
        25     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
        26
        27     WARN_ON_ONCE(!rdtp->dynticks_nmi_nesting);
        28     WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        29     if (rdtp->dynticks_nmi_nesting != 1) {

Quoted 4 lines

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 2), perhaps?

This is already implied by the WARN_ON_ONCE() on line 27 and the check on line 29.

I was worried about negative numbers. Maybe change line 27 to WARN_ON_ONCE(rdtp->dynticks_nmi_nesting <= 0), then? (Or is it unsigned? If so, let's make to signed to catch this type of error.)

Quoted 54 lines

Quoted 7 lines

Quoted 5 lines

        30       rdtp->dynticks_nmi_nesting -= 2;
        31       return;
        32     }
        33     rdtp->dynticks_nmi_nesting = 0;
        34     smp_mb__before_atomic();

This implies barrier(), right?

Yep!

Quoted 44 lines

Quoted 44 lines

        35     atomic_inc(&rdtp->dynticks);
        36     smp_mb__after_atomic();
        37     WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        38   }

Line 9 picks up a pointer to this CPU's rcu_dynticks structure and line 10 assumes that we don't need to increment ->dynticks.

Line 12 checks to see if ->dynticks is even. Note that this check is
stable: If there are nested NMIs, they will increment ->dynticks twice or not at all, and either way preserves the evenness (to be proven, of course, but that is the plan). If ->dynticks is even, lines 13-15 atomically increment it, line 16 complains if still even, and line 17 says we will increment ->dynticks_nmi_nesting by only 1.

Either way, line 19 increments ->dynticks_nmi_nesting as needed and line 20 keeps the compiler from getting too cute.

For rcu_nmi_exit(), line 25 again picks up this CPUs rcu_dynticks structure. Lines 27 and 28 complain bitterly if invariants are violated. If line 29 finds that the value of ->dynticks_nmi_nesting is not 1, then line 30 subtracts 2 from ->dynticks_nmi_nesting and line 31 returns.

Otherwise, line 33 sets ->dynticks_nmi_nesting to zero, lines 34-36 atomically increment ->dynticks with full ordering, and line 37 complains bitterly if ->dynticks is not even.

So, if an NMI occurs before rcu_nmi_enter's atomic increment, then the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will think that they are not nested, which is the correct thing for them to think in that case. They will increment ->dynticks twice and restore ->dynticks_nmi_nesting to zero (adding and then subtracting 1). If the NMI happens after the atomic increment, then the nested rcu_nmi_enter() and rcu_nmi_exit() will leave ->dynticks alone, and will restore ->dynticks_nmi_nesting to zero (adding and subtracting two again). If the NMI happens after the increment of ->dynticks_nmi_nesting, the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will again restore ->dynticks_nmi_nesting, but this time to one (again adding and subtracting two).

In rcu_nmi_exit(), ->dynticks_nmi_nesting of zero had better not happen, one means we need to atomically increment ->dynticks, and other values mean that we are partially or fully nested. Reasoning proceeds as for rcu_nmi_enter(), but in the opposite direction.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Whew! That might even work.

I think I like this, with the warnings above.

OK with dropping the one that I called out as redundant?

Sure, but see about.

Quoted 49 lines

Quoted 48 lines

Quoted 45 lines

But how about taking a different approach. Assuming that there can never be more than (say) 14 nesting NMI-like things, use the lower four bits of ->dynticks to represent the NMI nesting and the upper 28 bits as the counter. This of course requires modifying lots of places in RCU that check the counter, but it is probably time to abstract the check anyway.

This would allow my earlier attempted logic to work and (maybe) simplify the reasoning a bit (and yes, the "magic" constants need macros):

        void rcu_nmi_enter(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x01;

                WARN_ON_ONCE(nexting == 0xf);
                if (nesting == 0) {
                        if (atomic_read(&rdtp->dynticks) & 0x10)
                                return;
                        incby = 0x11;
                }
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        }

        void rcu_nmi_exit(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x0f;

                if (nesting == 0)
                        return;
                if (nesting > 1)
                        incby = -1;
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        }

Quoted 1 line

Over to you! ;-)

This latter one is all you :)

Quoted 2 lines

Well, let's see how I feel about it after trying a Promela model of the first code sequence. ;-)

:)

Does Promela understand the differences between this type of reentrancy and real threading?

Quoted 121 lines · Andy Lutomirski

Quoted 119 lines · Paul E. McKenney

Quoted 94 lines · Andy Lutomirski

Quoted 8 lines · Paul E. McKenney

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Andy Lutomirski

On Fri, Nov 21, 2014 at 3:38 PM, Paul E. McKenney
<[email protected]> wrote:

Returning state sounds like a bad idea, if we can reasonably avoid it.

I agree, except that we already do it for exception_enter(), etc. But yes, changing fewer things is nice.

Quoted 71 lines

And I think I finally see what you are pointing out about my code: If another NMI comes in between the time I increment ->dynticks_nmi_nesting and the time I atomically increment ->dynticks, the nested NMI handler will incorrectly believe that RCU is already paying attention to this CPU. Which would indeed not be at all good, so good catch!!!

Quoted 36 lines

Otherwise, I think that there may need to be enough state somewhere so that the outermost nested rcu_nmi_enter knows whether to increment dynticks. For example, dynticks_nmi_nesting could store the nesting count * 2 - (1 if the outermost nested user needs to increment dynticks). Something like:

void rcu_nmi_enter(void)
{
/* Be very careful -- this function may be called reentrently on the
same CPU. */
atomically: increment dynticks if it's even.

/* If an rcu_nmi_enter/rcu_nmi_exit pair happens here, then it will not change
* the state. */

  local_inc(&dynticks_nmi_nesting, (we incremented dynticks ? 1 : 2));

WARN_ON(we incremented dynticks and dynticks_nmi_nesting was nonzero);
}

void rcu_nmi_exit(void)
{
WARN_ON(!(dynticks & 1));
locally atomically: dynticks_nmi_nesting -= 2, unless
dynticks_nmi_nesting == 1, in which case set it to zero

if (dynticks_nmi_nesting was 1)

    atomic_inc(&dynticks);
}

The invariant here is that, for a single unnested enter/exit, if dynticks_nmi_nesting != 0, then dynticks is odd. As a result, an rcu_nmi_enter/rcu_nmi_exit pair at any time when dynticks_nmi_nesting != 0 *or* dynticks is odd will have no net effect, so the invariant, in fact, holds for all invocations, nested or otherwise.

Quoted 7 lines

At least one of those conditions is true at all times during the execution of outermost pair, starting with the first atomic operation and ending with the final atomic_inc. So they nest properly no matter what else happens (unless, of course, someone else pokes dynticks in the middle).

Thoughts?

Let's see... The evenness of ->dynticks should be preserved by nested NMI handlers, so the check and increment need not be atomic. We don't have any way (other than atomic operations) to do local atomic modifications on all architectures, because we cannot mask NMIs. (Yes, it can work on x86, but this is common code that needs to work everywhere.) On the other hand, presumably NMIs are rare, so atomic modification of the NMI nesting counter should be OK, at least if it proves absolutely necessary. And I am thinking that a mechanical proof will be needed here. :-/

But first, let me try generating the code and informally evaluating it:

         1   struct rcu_dynticks {
         2     long long dynticks_nesting;
         3     int dynticks_nmi_nesting;
         4     atomic_t dynticks;
         5   };
         6
         7   void rcu_nmi_enter(void)
         8   {
         9     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);

Quoted 8 lines

        10     int incby = 2;
        11
        12     if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
        13       smp_mb__before_atomic();
        14       atomic_inc(&rdtp->dynticks);
        15       smp_mb__after_atomic();
        16       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        17       incby = 1;

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 1) here, perhaps?

That would make sense.

Quoted 6 lines

Quoted 2 lines

        18     }
        19     rdtp->dynticks_nmi_nesting += incby;

Oh, I see why you don't need local_add -- it's because an nmi in the middle of this increment won't have any effect on the interrupted code, so even a software RMW will be okay.

Yep! ;-)

Quoted 11 lines

Quoted 10 lines

        20     barrier();
        21   }
        22
        23   void rcu_nmi_exit(void)
        24   {
        25     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
        26
        27     WARN_ON_ONCE(!rdtp->dynticks_nmi_nesting);
        28     WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        29     if (rdtp->dynticks_nmi_nesting != 1) {

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 2), perhaps?

This is already implied by the WARN_ON_ONCE() on line 27 and the check on line 29.

I was worried about negative numbers. Maybe change line 27 to WARN_ON_ONCE(rdtp->dynticks_nmi_nesting <= 0), then? (Or is it unsigned? If so, let's make to signed to catch this type of error.)

Good point, they are signed, so your WARN_ON_ONCE() would work.

Quoted 116 lines

Quoted 54 lines

Quoted 7 lines

Quoted 5 lines

        30       rdtp->dynticks_nmi_nesting -= 2;
        31       return;
        32     }
        33     rdtp->dynticks_nmi_nesting = 0;
        34     smp_mb__before_atomic();

This implies barrier(), right?

Yep!

Quoted 44 lines

Quoted 44 lines

        35     atomic_inc(&rdtp->dynticks);
        36     smp_mb__after_atomic();
        37     WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        38   }

Line 9 picks up a pointer to this CPU's rcu_dynticks structure and line 10 assumes that we don't need to increment ->dynticks.

Line 12 checks to see if ->dynticks is even. Note that this check is
stable: If there are nested NMIs, they will increment ->dynticks twice or not at all, and either way preserves the evenness (to be proven, of course, but that is the plan). If ->dynticks is even, lines 13-15 atomically increment it, line 16 complains if still even, and line 17 says we will increment ->dynticks_nmi_nesting by only 1.

Either way, line 19 increments ->dynticks_nmi_nesting as needed and line 20 keeps the compiler from getting too cute.

For rcu_nmi_exit(), line 25 again picks up this CPUs rcu_dynticks structure. Lines 27 and 28 complain bitterly if invariants are violated. If line 29 finds that the value of ->dynticks_nmi_nesting is not 1, then line 30 subtracts 2 from ->dynticks_nmi_nesting and line 31 returns.

Otherwise, line 33 sets ->dynticks_nmi_nesting to zero, lines 34-36 atomically increment ->dynticks with full ordering, and line 37 complains bitterly if ->dynticks is not even.

So, if an NMI occurs before rcu_nmi_enter's atomic increment, then the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will think that they are not nested, which is the correct thing for them to think in that case. They will increment ->dynticks twice and restore ->dynticks_nmi_nesting to zero (adding and then subtracting 1). If the NMI happens after the atomic increment, then the nested rcu_nmi_enter() and rcu_nmi_exit() will leave ->dynticks alone, and will restore ->dynticks_nmi_nesting to zero (adding and subtracting two again). If the NMI happens after the increment of ->dynticks_nmi_nesting, the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will again restore ->dynticks_nmi_nesting, but this time to one (again adding and subtracting two).

In rcu_nmi_exit(), ->dynticks_nmi_nesting of zero had better not happen, one means we need to atomically increment ->dynticks, and other values mean that we are partially or fully nested. Reasoning proceeds as for rcu_nmi_enter(), but in the opposite direction.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Whew! That might even work.

I think I like this, with the warnings above.

OK with dropping the one that I called out as redundant?

Sure, but see about.

Quoted 45 lines

Quoted 45 lines

Quoted 45 lines

But how about taking a different approach. Assuming that there can never be more than (say) 14 nesting NMI-like things, use the lower four bits of ->dynticks to represent the NMI nesting and the upper 28 bits as the counter. This of course requires modifying lots of places in RCU that check the counter, but it is probably time to abstract the check anyway.

This would allow my earlier attempted logic to work and (maybe) simplify the reasoning a bit (and yes, the "magic" constants need macros):

        void rcu_nmi_enter(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x01;

                WARN_ON_ONCE(nexting == 0xf);
                if (nesting == 0) {
                        if (atomic_read(&rdtp->dynticks) & 0x10)
                                return;
                        incby = 0x11;
                }
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        }

        void rcu_nmi_exit(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x0f;

                if (nesting == 0)
                        return;
                if (nesting > 1)
                        incby = -1;
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        }

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Over to you! ;-)

This latter one is all you :)

Well, let's see how I feel about it after trying a Promela model of the first code sequence. ;-)

:)

Quoted 2 lines

Does Promela understand the differences between this type of reentrancy and real threading?

Not as far as I know. But it can be tricked into making this distinction. One thread just has the Promela code as is, and the other thread has the same Promela code entirely contained in an atomic block. This means that the entire second thread must executed at one point in the first thread, just like an NMI would.

Quoted 252 lines · Paul E. McKenney

Quoted 121 lines · Andy Lutomirski

Quoted 119 lines · Paul E. McKenney

Quoted 94 lines · Andy Lutomirski

Quoted 8 lines · Paul E. McKenney

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Andy Lutomirski

On Fri, Nov 21, 2014 at 3:38 PM, Paul E. McKenney
<[email protected]> wrote:

Returning state sounds like a bad idea, if we can reasonably avoid it.

I agree, except that we already do it for exception_enter(), etc. But yes, changing fewer things is nice.

Quoted 71 lines

And I think I finally see what you are pointing out about my code: If another NMI comes in between the time I increment ->dynticks_nmi_nesting and the time I atomically increment ->dynticks, the nested NMI handler will incorrectly believe that RCU is already paying attention to this CPU. Which would indeed not be at all good, so good catch!!!

Quoted 36 lines

Otherwise, I think that there may need to be enough state somewhere so that the outermost nested rcu_nmi_enter knows whether to increment dynticks. For example, dynticks_nmi_nesting could store the nesting count * 2 - (1 if the outermost nested user needs to increment dynticks). Something like:

void rcu_nmi_enter(void)
{
/* Be very careful -- this function may be called reentrently on the
same CPU. */
atomically: increment dynticks if it's even.

/* If an rcu_nmi_enter/rcu_nmi_exit pair happens here, then it will not change
* the state. */

  local_inc(&dynticks_nmi_nesting, (we incremented dynticks ? 1 : 2));

WARN_ON(we incremented dynticks and dynticks_nmi_nesting was nonzero);
}

void rcu_nmi_exit(void)
{
WARN_ON(!(dynticks & 1));
locally atomically: dynticks_nmi_nesting -= 2, unless
dynticks_nmi_nesting == 1, in which case set it to zero

if (dynticks_nmi_nesting was 1)

    atomic_inc(&dynticks);
}

The invariant here is that, for a single unnested enter/exit, if dynticks_nmi_nesting != 0, then dynticks is odd. As a result, an rcu_nmi_enter/rcu_nmi_exit pair at any time when dynticks_nmi_nesting != 0 *or* dynticks is odd will have no net effect, so the invariant, in fact, holds for all invocations, nested or otherwise.

Quoted 7 lines

At least one of those conditions is true at all times during the execution of outermost pair, starting with the first atomic operation and ending with the final atomic_inc. So they nest properly no matter what else happens (unless, of course, someone else pokes dynticks in the middle).

Thoughts?

Let's see... The evenness of ->dynticks should be preserved by nested NMI handlers, so the check and increment need not be atomic. We don't have any way (other than atomic operations) to do local atomic modifications on all architectures, because we cannot mask NMIs. (Yes, it can work on x86, but this is common code that needs to work everywhere.) On the other hand, presumably NMIs are rare, so atomic modification of the NMI nesting counter should be OK, at least if it proves absolutely necessary. And I am thinking that a mechanical proof will be needed here. :-/

But first, let me try generating the code and informally evaluating it:

         1   struct rcu_dynticks {
         2     long long dynticks_nesting;
         3     int dynticks_nmi_nesting;
         4     atomic_t dynticks;
         5   };
         6
         7   void rcu_nmi_enter(void)
         8   {
         9     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);

Quoted 8 lines

        10     int incby = 2;
        11
        12     if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
        13       smp_mb__before_atomic();
        14       atomic_inc(&rdtp->dynticks);
        15       smp_mb__after_atomic();
        16       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        17       incby = 1;

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 1) here, perhaps?

That would make sense.

Quoted 6 lines

Quoted 2 lines

        18     }
        19     rdtp->dynticks_nmi_nesting += incby;

Oh, I see why you don't need local_add -- it's because an nmi in the middle of this increment won't have any effect on the interrupted code, so even a software RMW will be okay.

Yep! ;-)

Quoted 11 lines

Quoted 10 lines

        20     barrier();
        21   }
        22
        23   void rcu_nmi_exit(void)
        24   {
        25     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
        26
        27     WARN_ON_ONCE(!rdtp->dynticks_nmi_nesting);
        28     WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        29     if (rdtp->dynticks_nmi_nesting != 1) {

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 2), perhaps?

This is already implied by the WARN_ON_ONCE() on line 27 and the check on line 29.

I was worried about negative numbers. Maybe change line 27 to WARN_ON_ONCE(rdtp->dynticks_nmi_nesting <= 0), then? (Or is it unsigned? If so, let's make to signed to catch this type of error.)

Good point, they are signed, so your WARN_ON_ONCE() would work.

Quoted 118 lines

Quoted 54 lines

Quoted 7 lines

Quoted 5 lines

        30       rdtp->dynticks_nmi_nesting -= 2;
        31       return;
        32     }
        33     rdtp->dynticks_nmi_nesting = 0;
        34     smp_mb__before_atomic();

This implies barrier(), right?

Yep!

Quoted 44 lines

Quoted 44 lines

        35     atomic_inc(&rdtp->dynticks);
        36     smp_mb__after_atomic();
        37     WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        38   }

Line 9 picks up a pointer to this CPU's rcu_dynticks structure and line 10 assumes that we don't need to increment ->dynticks.

Line 12 checks to see if ->dynticks is even. Note that this check is
stable: If there are nested NMIs, they will increment ->dynticks twice or not at all, and either way preserves the evenness (to be proven, of course, but that is the plan). If ->dynticks is even, lines 13-15 atomically increment it, line 16 complains if still even, and line 17 says we will increment ->dynticks_nmi_nesting by only 1.

Either way, line 19 increments ->dynticks_nmi_nesting as needed and line 20 keeps the compiler from getting too cute.

For rcu_nmi_exit(), line 25 again picks up this CPUs rcu_dynticks structure. Lines 27 and 28 complain bitterly if invariants are violated. If line 29 finds that the value of ->dynticks_nmi_nesting is not 1, then line 30 subtracts 2 from ->dynticks_nmi_nesting and line 31 returns.

Otherwise, line 33 sets ->dynticks_nmi_nesting to zero, lines 34-36 atomically increment ->dynticks with full ordering, and line 37 complains bitterly if ->dynticks is not even.

So, if an NMI occurs before rcu_nmi_enter's atomic increment, then the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will think that they are not nested, which is the correct thing for them to think in that case. They will increment ->dynticks twice and restore ->dynticks_nmi_nesting to zero (adding and then subtracting 1). If the NMI happens after the atomic increment, then the nested rcu_nmi_enter() and rcu_nmi_exit() will leave ->dynticks alone, and will restore ->dynticks_nmi_nesting to zero (adding and subtracting two again). If the NMI happens after the increment of ->dynticks_nmi_nesting, the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will again restore ->dynticks_nmi_nesting, but this time to one (again adding and subtracting two).

In rcu_nmi_exit(), ->dynticks_nmi_nesting of zero had better not happen, one means we need to atomically increment ->dynticks, and other values mean that we are partially or fully nested. Reasoning proceeds as for rcu_nmi_enter(), but in the opposite direction.

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Whew! That might even work.

I think I like this, with the warnings above.

OK with dropping the one that I called out as redundant?

Sure, but see about.

Quoted 45 lines

Quoted 45 lines

Quoted 45 lines

But how about taking a different approach. Assuming that there can never be more than (say) 14 nesting NMI-like things, use the lower four bits of ->dynticks to represent the NMI nesting and the upper 28 bits as the counter. This of course requires modifying lots of places in RCU that check the counter, but it is probably time to abstract the check anyway.

This would allow my earlier attempted logic to work and (maybe) simplify the reasoning a bit (and yes, the "magic" constants need macros):

        void rcu_nmi_enter(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x01;

                WARN_ON_ONCE(nexting == 0xf);
                if (nesting == 0) {
                        if (atomic_read(&rdtp->dynticks) & 0x10)
                                return;
                        incby = 0x11;
                }
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        }

        void rcu_nmi_exit(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x0f;

                if (nesting == 0)
                        return;
                if (nesting > 1)
                        incby = -1;
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        }

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Over to you! ;-)

This latter one is all you :)

Well, let's see how I feel about it after trying a Promela model of the first code sequence. ;-)

:)

Does Promela understand the differences between this type of reentrancy and real threading?

Quoted 5 lines

Not as far as I know. But it can be tricked into making this distinction. One thread just has the Promela code as is, and the other thread has the same Promela code entirely contained in an atomic block. This means that the entire second thread must executed at one point in the first thread, just like an NMI would.

And the following Promela model claims that your approach works. Should I trust it? ;-)

Thanx, Paul

------------------------------------------------------------------------

/*
* Promela model for Andy Lutomirski's suggested change to rcu_nmi_enter()
* that allows nesting.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, you can access it online at
* http://www.gnu.org/licenses/gpl-2.0.html.
*
* Copyright IBM Corporation, 2014
*
* Author: Paul E. McKenney <[email protected]>
*/

byte dynticks_nesting;
byte dynticks_nmi_nesting;
byte dynticks;
byte busy;

/*
* Promela verision of rcu_nmi_enter().
*/
inline rcu_nmi_enter()

{
        assert(dynticks_nmi_nesting >= 0);
        if
        :: (dynticks & 1) == 0 ->
                atomic {
                        dynticks = dynticks + 1;
                }
                assert((dynticks & 1) == 1);
                dynticks_nmi_nesting = dynticks_nmi_nesting + 1;
                assert(dynticks_nmi_nesting >= 1);
        :: else ->
                dynticks_nmi_nesting = dynticks_nmi_nesting + 2;
        fi;
}

/*
* Promela verision of rcu_nmi_exit().
*/
inline rcu_nmi_exit()

{
        assert(dynticks_nmi_nesting > 0);
        assert((dynticks & 1) != 0);
        if
        :: dynticks_nmi_nesting != 1 ->
                dynticks_nmi_nesting = dynticks_nmi_nesting - 2;
        :: else ->
                dynticks_nmi_nesting = 0;
                atomic {
                        dynticks = dynticks + 1;
                }
                assert((dynticks & 1) == 0);
        fi;
}

/*
* Base-level NMI runs non-atomically. Crudely emulates process-level
* dynticks-idle entry/exit.
*/
proctype base_NMI()

{
        do
        ::      if
                :: 1 -> atomic {
                                dynticks = dynticks + 1;
                        }
                        busy = 0;
                :: 1 -> skip;
                fi;
                rcu_nmi_enter();
                assert((dynticks & 1) == 1);
                rcu_nmi_exit();
                if
                :: busy -> skip;
                :: !busy ->
                        atomic {
                                dynticks = dynticks + 1;
                        }
                        busy = 1;
                fi;
        od;
}

/*
* Nested NMI runs atomically to emulate interrupting base_level(). */
proctype nested_NMI()

{
        do
        ::      atomic {
                        rcu_nmi_enter();
                        assert((dynticks & 1) == 1);
                        rcu_nmi_exit();
                }
        od;
}

init {
        dynticks_nesting = 0;
        dynticks_nmi_nesting = 0;
        dynticks = 0;
        busy = 0;
        run base_NMI();
        run nested_NMI();
}

Quoted 1 line · Paul E. McKenney

On Mon, Nov 24, 2014 at 01:35:01PM -0800, Paul E. McKenney wrote:

Quoted 3 lines

And the following Promela model claims that your approach works. Should I trust it? ;-)

I think so.

Want to write a patch? If so, whose tree should it go in? I can add it to my IST series, but that seems a bit odd.

--Andy

Quoted 121 lines

Thanx, Paul

------------------------------------------------------------------------

/*
* Promela model for Andy Lutomirski's suggested change to rcu_nmi_enter()
* that allows nesting.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, you can access it online at
* http://www.gnu.org/licenses/gpl-2.0.html.
*
* Copyright IBM Corporation, 2014
*
* Author: Paul E. McKenney <[email protected]>
*/

byte dynticks_nesting;
byte dynticks_nmi_nesting;
byte dynticks;
byte busy;

/*
* Promela verision of rcu_nmi_enter().
*/
inline rcu_nmi_enter()

{
        assert(dynticks_nmi_nesting >= 0);
        if
        :: (dynticks & 1) == 0 ->
                atomic {
                        dynticks = dynticks + 1;
                }
                assert((dynticks & 1) == 1);
                dynticks_nmi_nesting = dynticks_nmi_nesting + 1;
                assert(dynticks_nmi_nesting >= 1);
        :: else ->
                dynticks_nmi_nesting = dynticks_nmi_nesting + 2;
        fi;
}

/*
* Promela verision of rcu_nmi_exit().
*/
inline rcu_nmi_exit()

{
        assert(dynticks_nmi_nesting > 0);
        assert((dynticks & 1) != 0);
        if
        :: dynticks_nmi_nesting != 1 ->
                dynticks_nmi_nesting = dynticks_nmi_nesting - 2;
        :: else ->
                dynticks_nmi_nesting = 0;
                atomic {
                        dynticks = dynticks + 1;
                }
                assert((dynticks & 1) == 0);
        fi;
}

/*
* Base-level NMI runs non-atomically. Crudely emulates process-level
* dynticks-idle entry/exit.
*/
proctype base_NMI()

{
        do
        ::      if
                :: 1 -> atomic {
                                dynticks = dynticks + 1;
                        }
                        busy = 0;
                :: 1 -> skip;
                fi;
                rcu_nmi_enter();
                assert((dynticks & 1) == 1);
                rcu_nmi_exit();
                if
                :: busy -> skip;
                :: !busy ->
                        atomic {
                                dynticks = dynticks + 1;
                        }
                        busy = 1;
                fi;
        od;
}

/*
* Nested NMI runs atomically to emulate interrupting base_level(). */
proctype nested_NMI()

{
        do
        ::      atomic {
                        rcu_nmi_enter();
                        assert((dynticks & 1) == 1);
                        rcu_nmi_exit();
                }
        od;
}

init {
        dynticks_nesting = 0;
        dynticks_nmi_nesting = 0;
        dynticks = 0;
        busy = 0;
        run base_NMI();
        run nested_NMI();
}

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Paul E. McKenney

On Mon, Nov 24, 2014 at 01:35:01PM -0800, Paul E. McKenney wrote:

[ . . . ]

Quoted 4 lines

Quoted 3 lines

And the following Promela model claims that your approach works. Should I trust it? ;-)

Quoted 4 lines

I think so.

Want to write a patch? If so, whose tree should it go in? I can add it to my IST series, but that seems a bit odd.

Working on it. ;-)

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Paul E. McKenney

On Mon, Nov 24, 2014 at 01:35:01PM -0800, Paul E. McKenney wrote:

[ . . . ]

Quoted 4 lines

Quoted 3 lines

And the following Promela model claims that your approach works. Should I trust it? ;-)

Quoted 6 lines

Quoted 4 lines

I think so.

Want to write a patch? If so, whose tree should it go in? I can add it to my IST series, but that seems a bit odd.

Working on it. ;-)

And here is a sneak preview of the patch. Thoughts?

Thanx, Paul

------------------------------------------------------------------------

rcu: Make rcu_nmi_enter() handle nesting

Andy Lutomirski is introducing ISTs into x86, which from RCU's viewpoint are NMIs. Because ISTs and NMIs can nest, rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting.

This commit therefore
introduces nesting, using a clever NMI-coordination algorithm suggested by Andy. The trick is to atomically increment ->dynticks (if needed) before manipulating ->dynticks_nmi_nesting on entry (and, accordingly, after on exit). In addition, ->dynticks_nmi_nesting is incremented by one if ->dynticks was incremented and by two otherwise. This means that when rcu_nmi_exit() sees ->dynticks_nmi_nesting equal to one, it knows that ->dynticks must be atomically incremented.

This NMI-coordination algorithms has been validated by the following Promela model, for whatever that might be worth:

/*
* Promela model for Andy Lutomirski's suggested change to rcu_nmi_enter()
* that allows nesting.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, you can access it online at
* http://www.gnu.org/licenses/gpl-2.0.html.
*
* Copyright IBM Corporation, 2014
*
* Author: Paul E. McKenney <[email protected]>
*/

byte dynticks_nesting = 0;
byte dynticks_nmi_nesting = 0;
byte dynticks = 0;

/*
* Promela verision of rcu_nmi_enter().
*/
inline rcu_nmi_enter()

{
        byte incby;

        incby = 2;
        assert(dynticks_nmi_nesting >= 0);
        if
        :: (dynticks & 1) == 0 ->
                atomic {
                        dynticks = dynticks + 1;
                }
                assert((dynticks & 1) == 1);
                incby = 1;
        :: else ->
                skip;
        fi;
        dynticks_nmi_nesting = dynticks_nmi_nesting + incby;
        assert(dynticks_nmi_nesting >= 1);
}

/*
* Promela verision of rcu_nmi_exit().
*/
inline rcu_nmi_exit()

{
        assert(dynticks_nmi_nesting > 0);
        assert((dynticks & 1) != 0);
        if
        :: dynticks_nmi_nesting != 1 ->
                dynticks_nmi_nesting = dynticks_nmi_nesting - 2;
        :: else ->
                dynticks_nmi_nesting = 0;
                atomic {
                        dynticks = dynticks + 1;
                }
                assert((dynticks & 1) == 0);
        fi;
}

/*
* Base-level NMI runs non-atomically. Crudely emulates process-level
* dynticks-idle entry/exit.
*/
proctype base_NMI()

{
        byte busy;

        busy = 0;
        do
        ::      if
                :: 1 -> atomic {
                                dynticks = dynticks + 1;
                        }
                        busy = 0;
                :: 1 -> skip;
                fi;
                rcu_nmi_enter();
                assert((dynticks & 1) == 1);
                rcu_nmi_exit();
                if
                :: busy -> skip;
                :: !busy ->
                        atomic {
                                dynticks = dynticks + 1;
                        }
                        busy = 1;
                fi;
        od;
}

/*
* Nested NMI runs atomically to emulate interrupting base_level(). */
proctype nested_NMI()

{
        do
        ::      atomic {
                        rcu_nmi_enter();
                        assert((dynticks & 1) == 1);
                        rcu_nmi_exit();
                }
        od;
}

init {
        run base_NMI();
        run nested_NMI();
}

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

kernel/rcu/tree.c +49 -17

index 8749f43f3f05..fc0236992655 100644
@@ -759,39 +759,71 @@ void rcu_irq_enter(void)
 /**
  * rcu_nmi_enter - inform RCU of entry to NMI context
  *
- * If the CPU was idle with dynamic ticks active, and there is no
- * irq handler running, this updates rdtp->dynticks_nmi to let the
- * RCU grace-period handling know that the CPU is active.
+ * If the CPU was idle from RCU's viewpoint, update rdtp->dynticks and
+ * rdtp->dynticks_nmi_nesting to let the RCU grace-period handling know
+ * that the CPU is active.  This implementation permits nested NMIs, as
+ * long as the nesting level does not overflow an int.  (You will probably
+ * run out of stack space first.)
  */
 void rcu_nmi_enter(void)
 {
        struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
+       int incby = 2;
 
-       if (rdtp->dynticks_nmi_nesting == 0 &&
-           (atomic_read(&rdtp->dynticks) & 0x1))
-               return;
-       rdtp->dynticks_nmi_nesting++;
-       smp_mb__before_atomic();  /* Force delay from prior write. */
-       atomic_inc(&rdtp->dynticks);
-       /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */
-       smp_mb__after_atomic();  /* See above. */
-       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+       /* Complain about underflow. */
+       WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 0);
+
+       /*
+        * If idle from RCU viewpoint, atomically increment ->dynticks
+        * to mark non-idle and increment ->dynticks_nmi_nesting by one.
+        * Otherwise, increment ->dynticks_nmi_nesting by two.  This means
+        * if ->dynticks_nmi_nesting is equal to one, we are guaranteed
+        * to be in the outermost NMI handler that interrupted an RCU-idle
+        * period (observation due to Andy Lutomirski).
+        */
+       if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
+               smp_mb__before_atomic();  /* Force delay from prior write. */
+               atomic_inc(&rdtp->dynticks);
+               /* atomic_inc() before later RCU read-side crit sects */
+               smp_mb__after_atomic();  /* See above. */
+               WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+               incby = 1;
+       }
+       rdtp->dynticks_nmi_nesting += incby;
+       barrier();
 }
 
 /**
  * rcu_nmi_exit - inform RCU of exit from NMI context
  *
- * If the CPU was idle with dynamic ticks active, and there is no
- * irq handler running, this updates rdtp->dynticks_nmi to let the
- * RCU grace-period handling know that the CPU is no longer active.
+ * If we are returning from the outermost NMI handler that interrupted an
+ * RCU-idle period, update rdtp->dynticks and rdtp->dynticks_nmi_nesting
+ * to let the RCU grace-period handling know that the CPU is back to
+ * being RCU-idle.
  */
 void rcu_nmi_exit(void)
 {
        struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
 
-       if (rdtp->dynticks_nmi_nesting == 0 ||
-           --rdtp->dynticks_nmi_nesting != 0)
+       /*
+        * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
+        * (We are exiting an NMI handler, so RCU better be paying attention
+        * to us!)
+        */
+       WARN_ON_ONCE(rdtp->dynticks_nmi_nesting <= 0);
+       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+
+       /*
+        * If the nesting level is not 1, the CPU wasn't RCU-idle, so
+        * leave it in non-RCU-idle state.
+        */
+       if (rdtp->dynticks_nmi_nesting != 1) {
+               rdtp->dynticks_nmi_nesting -= 2;
                return;
+       }
+
+       /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
+       rdtp->dynticks_nmi_nesting = 0;
        /* CPUs seeing atomic_inc() must see prior RCU read-side crit sects */
        smp_mb__before_atomic();  /* See above. */
        atomic_inc(&rdtp->dynticks);

Quoted 23 lines · Paul E. McKenney

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Paul E. McKenney

On Mon, Nov 24, 2014 at 01:35:01PM -0800, Paul E. McKenney wrote:

[ . . . ]

Quoted 4 lines

Quoted 3 lines

And the following Promela model claims that your approach works. Should I trust it? ;-)

Quoted 6 lines

Quoted 4 lines

I think so.

Want to write a patch? If so, whose tree should it go in? I can add it to my IST series, but that seems a bit odd.

Working on it. ;-)

And here is a sneak preview of the patch. Thoughts?

Thanx, Paul

Quoted 7 lines

------------------------------------------------------------------------

rcu: Make rcu_nmi_enter() handle nesting

Andy Lutomirski is introducing ISTs into x86, which from RCU's viewpoint are NMIs. Because ISTs and NMIs can nest, rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting.

You must not be a frequent reader of entry_64.S and the Intel SDM :) IOW, IST is just a stack switching mechanism, and these interrupts have been around forever -- they're just buggy right now.

How about:

x86 has multiple types of NMI-like interrupts: real NMIs, machine checks, and, for some values of NMI-like, debugging and breakpoint interrupts. These interrupts can nest inside each other. Andy Lutomirski is adding RCU support to these interrupts, so rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting.

Other than that, I like it.

Quoted 45 lines · Andy Lutomirski

Quoted 23 lines · Paul E. McKenney

Quoted 11 lines · Paul E. McKenney

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Paul E. McKenney

On Mon, Nov 24, 2014 at 01:35:01PM -0800, Paul E. McKenney wrote:

[ . . . ]

Quoted 4 lines

Quoted 3 lines

And the following Promela model claims that your approach works. Should I trust it? ;-)

Quoted 6 lines

Quoted 4 lines

I think so.

Want to write a patch? If so, whose tree should it go in? I can add it to my IST series, but that seems a bit odd.

Working on it. ;-)

And here is a sneak preview of the patch. Thoughts?

Thanx, Paul

Quoted 7 lines

------------------------------------------------------------------------

rcu: Make rcu_nmi_enter() handle nesting

Andy Lutomirski is introducing ISTs into x86, which from RCU's viewpoint are NMIs. Because ISTs and NMIs can nest, rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting.

You must not be a frequent reader of entry_64.S and the Intel SDM :) IOW, IST is just a stack switching mechanism, and these interrupts have been around forever -- they're just buggy right now.

How about:

x86 has multiple types of NMI-like interrupts: real NMIs, machine checks, and, for some values of NMI-like, debugging and breakpoint interrupts. These interrupts can nest inside each other. Andy Lutomirski is adding RCU support to these interrupts, so rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting.

Quoted 1 line

Other than that, I like it.

And here is the updated version. Left to my normal workflow, this would go into 3.20. Please let me know if you need it earlier, but in that case, I will need you to test it fairly soon. (I don't have any way to test nested NMI-like things.)

Thanx, Paul

------------------------------------------------------------------------

rcu: Make rcu_nmi_enter() handle nesting

The x86 architecture has multiple types of NMI-like interrupts: real NMIs, machine checks, and, for some values of NMI-like, debugging and breakpoint interrupts. These interrupts can nest inside each other. Andy Lutomirski is adding RCU support to these interrupts, so rcu_nmi_enter() and rcu_nmi_exit() must now correctly handle nesting.

This commit therefore introduces nesting, using a clever NMI-coordination algorithm suggested by Andy. The trick is to atomically increment ->dynticks (if needed) before manipulating ->dynticks_nmi_nesting on entry (and, accordingly, after on exit). In addition, ->dynticks_nmi_nesting is incremented by one if ->dynticks was incremented and by two otherwise. This means that when rcu_nmi_exit() sees ->dynticks_nmi_nesting equal to one, it knows that ->dynticks must be atomically incremented.

This NMI-coordination algorithms has been validated by the following Promela model, for whatever that might be worth:

/*
* Promela model for Andy Lutomirski's suggested change to rcu_nmi_enter()
* that allows nesting.
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program; if not, you can access it online at
* http://www.gnu.org/licenses/gpl-2.0.html.
*
* Copyright IBM Corporation, 2014
*
* Author: Paul E. McKenney <[email protected]>
*/

byte dynticks_nesting = 0;
byte dynticks_nmi_nesting = 0;
byte dynticks = 0;

/*
* Promela verision of rcu_nmi_enter().
*/
inline rcu_nmi_enter()

{
        byte incby;

        incby = 2;
        assert(dynticks_nmi_nesting >= 0);
        if
        :: (dynticks & 1) == 0 ->
                atomic {
                        dynticks = dynticks + 1;
                }
                assert((dynticks & 1) == 1);
                incby = 1;
        :: else ->
                skip;
        fi;
        dynticks_nmi_nesting = dynticks_nmi_nesting + incby;
        assert(dynticks_nmi_nesting >= 1);
}

/*
* Promela verision of rcu_nmi_exit().
*/
inline rcu_nmi_exit()

{
        assert(dynticks_nmi_nesting > 0);
        assert((dynticks & 1) != 0);
        if
        :: dynticks_nmi_nesting != 1 ->
                dynticks_nmi_nesting = dynticks_nmi_nesting - 2;
        :: else ->
                dynticks_nmi_nesting = 0;
                atomic {
                        dynticks = dynticks + 1;
                }
                assert((dynticks & 1) == 0);
        fi;
}

/*
* Base-level NMI runs non-atomically. Crudely emulates process-level
* dynticks-idle entry/exit.
*/
proctype base_NMI()

{
        byte busy;

        busy = 0;
        do
        ::      if
                :: 1 -> atomic {
                                dynticks = dynticks + 1;
                        }
                        busy = 0;
                :: 1 -> skip;
                fi;
                rcu_nmi_enter();
                assert((dynticks & 1) == 1);
                rcu_nmi_exit();
                if
                :: busy -> skip;
                :: !busy ->
                        atomic {
                                dynticks = dynticks + 1;
                        }
                        busy = 1;
                fi;
        od;
}

/*
* Nested NMI runs atomically to emulate interrupting base_level(). */
proctype nested_NMI()

{
        do
        ::      atomic {
                        rcu_nmi_enter();
                        assert((dynticks & 1) == 1);
                        rcu_nmi_exit();
                }
        od;
}

init {
        run base_NMI();
        run nested_NMI();
}

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

kernel/rcu/tree.c +49 -17

index 8749f43f3f05..fc0236992655 100644
@@ -759,39 +759,71 @@ void rcu_irq_enter(void)
 /**
  * rcu_nmi_enter - inform RCU of entry to NMI context
  *
- * If the CPU was idle with dynamic ticks active, and there is no
- * irq handler running, this updates rdtp->dynticks_nmi to let the
- * RCU grace-period handling know that the CPU is active.
+ * If the CPU was idle from RCU's viewpoint, update rdtp->dynticks and
+ * rdtp->dynticks_nmi_nesting to let the RCU grace-period handling know
+ * that the CPU is active.  This implementation permits nested NMIs, as
+ * long as the nesting level does not overflow an int.  (You will probably
+ * run out of stack space first.)
  */
 void rcu_nmi_enter(void)
 {
        struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
+       int incby = 2;
 
-       if (rdtp->dynticks_nmi_nesting == 0 &&
-           (atomic_read(&rdtp->dynticks) & 0x1))
-               return;
-       rdtp->dynticks_nmi_nesting++;
-       smp_mb__before_atomic();  /* Force delay from prior write. */
-       atomic_inc(&rdtp->dynticks);
-       /* CPUs seeing atomic_inc() must see later RCU read-side crit sects */
-       smp_mb__after_atomic();  /* See above. */
-       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+       /* Complain about underflow. */
+       WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 0);
+
+       /*
+        * If idle from RCU viewpoint, atomically increment ->dynticks
+        * to mark non-idle and increment ->dynticks_nmi_nesting by one.
+        * Otherwise, increment ->dynticks_nmi_nesting by two.  This means
+        * if ->dynticks_nmi_nesting is equal to one, we are guaranteed
+        * to be in the outermost NMI handler that interrupted an RCU-idle
+        * period (observation due to Andy Lutomirski).
+        */
+       if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
+               smp_mb__before_atomic();  /* Force delay from prior write. */
+               atomic_inc(&rdtp->dynticks);
+               /* atomic_inc() before later RCU read-side crit sects */
+               smp_mb__after_atomic();  /* See above. */
+               WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+               incby = 1;
+       }
+       rdtp->dynticks_nmi_nesting += incby;
+       barrier();
 }
 
 /**
  * rcu_nmi_exit - inform RCU of exit from NMI context
  *
- * If the CPU was idle with dynamic ticks active, and there is no
- * irq handler running, this updates rdtp->dynticks_nmi to let the
- * RCU grace-period handling know that the CPU is no longer active.
+ * If we are returning from the outermost NMI handler that interrupted an
+ * RCU-idle period, update rdtp->dynticks and rdtp->dynticks_nmi_nesting
+ * to let the RCU grace-period handling know that the CPU is back to
+ * being RCU-idle.
  */
 void rcu_nmi_exit(void)
 {
        struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
 
-       if (rdtp->dynticks_nmi_nesting == 0 ||
-           --rdtp->dynticks_nmi_nesting != 0)
+       /*
+        * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
+        * (We are exiting an NMI handler, so RCU better be paying attention
+        * to us!)
+        */
+       WARN_ON_ONCE(rdtp->dynticks_nmi_nesting <= 0);
+       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+
+       /*
+        * If the nesting level is not 1, the CPU wasn't RCU-idle, so
+        * leave it in non-RCU-idle state.
+        */
+       if (rdtp->dynticks_nmi_nesting != 1) {
+               rdtp->dynticks_nmi_nesting -= 2;
                return;
+       }
+
+       /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
+       rdtp->dynticks_nmi_nesting = 0;
        /* CPUs seeing atomic_inc() must see prior RCU read-side crit sects */
        smp_mb__before_atomic();  /* See above. */
        atomic_inc(&rdtp->dynticks);

Dunno. Tony and Borislav -- when do you want the IST stack switching stuff?

Quoted 2 lines · Andy Lutomirski

Dunno. Tony and Borislav -- when do you want the IST stack switching stuff?

I'd leave that up to Tony and his testbench. I mean, we can hammer on it as much as we can and it can pass all testing locally but the real fun starts once it hits upstream and for that it doesn't matter which release... IMHO.

Quoted 2 lines · Andy Lutomirski

Dunno. Tony and Borislav -- when do you want the IST stack switching stuff?

I'd leave that up to Tony and his testbench. I mean, we can hammer on it as much as we can and it can pass all testing locally but the real fun starts once it hits upstream and for that it doesn't matter which release... IMHO.

Quoted 8 lines · Borislav Petkov

Quoted 2 lines · Andy Lutomirski

Dunno. Tony and Borislav -- when do you want the IST stack switching stuff?

I'd leave that up to Tony and his testbench. I mean, we can hammer on it as much as we can and it can pass all testing locally but the real fun starts once it hits upstream and for that it doesn't matter which release... IMHO.

So what was the net result of all the mode/RCU discussions?

Do I need some extra magic incantations in the final version of do_machine_check() beyond what was in this patch:

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

to make everything happy?

-Tony

I think you need ist_begin_non_atomic() before local_irq_enable() and ist_end_non_atomic() after local_irq_disable(). Otherwise it should be good.

Quoted 3 lines · Andy Lutomirski

I think you need ist_begin_non_atomic() before local_irq_enable() and ist_end_non_atomic() after local_irq_disable(). Otherwise it should be good.

In your x86/paranoid branch you added:

     prev_state = ist_enter(regs);
          .... body of do_machine_check() here ...
     ist_exit(regs, prev_state);

Does that override the previous advice? Or do I still need something before I call local_irq_enable() and after local_irq_disable()?

-Tony

We now switch to the kernel stack when a machine check interrupts during user mode. This means that we can perform recovery actions in the tail of do_machine_check()

Signed-off-by Tony Luck <[email protected]>

---
On top of Andy's x86/paranoid branch
Andy: Should I really move that:
pr_err("Uncorrected hardware memory error ...
inside the ist_begin_non_atomic() section?

arch/x86/kernel/cpu/mcheck/mce.c +6 -0

+       /*
+        * We must call memory_failure() here even if the current process is
+        * doomed. We still need to mark the page as poisoned and alert any
+        * other users of the page.
+        */
+       ist_begin_non_atomic(regs);

For the context-related bits:

Reviewed-by Andy Lutomirski <[email protected]>

Should I stick this in my -next branch so it can stew?

That's fine with me. Boris ... any comments?

Quoted 1 line · Tony

That's fine with me. Boris ... any comments?

Same here:

Acked-by Borislav Petkov <[email protected]>

I like negative diffstats:

 include/asm/mce.h         |    1
 include/asm/thread_info.h |    4 -
 kernel/cpu/mcheck/mce.c   |  109 ++++++++++------------------------------------
 kernel/signal.c           |    6 --
 4 files changed, 26 insertions(+), 94 deletions(-)

Procedural question: so this is going in, in the next merge window, aimed at 3.20, right?

IMO that is a good plan as we have max time to deal with fallout, should there be any.

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine. --

When does Paul usually send the rcu pull request? I can send these as a pull request or an emailed series after that happens.

Quoted 15 lines · Borislav Petkov

Quoted 1 line · Tony

That's fine with me. Boris ... any comments?

Same here:

Acked-by Borislav Petkov <[email protected]>

I like negative diffstats:

 include/asm/mce.h         |    1
 include/asm/thread_info.h |    4 -
 kernel/cpu/mcheck/mce.c   |  109 ++++++++++------------------------------------
 kernel/signal.c           |    6 --
 4 files changed, 26 insertions(+), 94 deletions(-)

Quoted 2 lines

Procedural question: so this is going in, in the next merge window, aimed at 3.20, right?

Yes, assuming that Ingo and Thomas agree.

When does Paul usually send the rcu pull request? I can send these as a pull request or an emailed series after that happens.

--Andy

Quoted 12 lines

IMO that is a good plan as we have max time to deal with fallout, should there be any.

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine. --

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 8 lines · Andy Lutomirski

Quoted 6 lines · Paul E. McKenney

Quoted 5 lines · Paul E. McKenney

Quoted 3 lines · Andy Lutomirski

Quoted 1 line · Paul E. McKenney

On Mon, Nov 24, 2014 at 01:35:01PM -0800, Paul E. McKenney wrote:

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

[ . . . ]

Quoted 5 lines

Quoted 3 lines

And the following Promela model claims that your approach works. Should I trust it? ;-)

I think so.

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

kernel/rcu/tree.c +16 -12

index 8749f43f3f05..fc0236992655 100644
@@ -759,39 +759,71 @@ void rcu_irq_enter(void)
 /**
  * rcu_nmi_enter - inform RCU of entry to NMI context
  *
- * If the CPU was idle with dynamic ticks active, and there is no
- * irq handler running, this updates rdtp->dynticks_nmi to let the
- * RCU grace-period handling know that the CPU is active.
+ * If the CPU was idle from RCU's viewpoint, update rdtp->dynticks and
+ * rdtp->dynticks_nmi_nesting to let the RCU grace-period handling know
+ * that the CPU is active.  This implementation permits nested NMIs, as
+ * long as the nesting level does not overflow an int.  (You will probably
+ * run out of stack space first.)
  */
 void rcu_nmi_enter(void)
 {
 	struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
+	int incby = 2;
 
-	if (rdtp->dynticks_nmi_nesting == 0 &&
-	    (atomic_read(&rdtp->dynticks) & 0x1))
-		return;
-	rdtp->dynticks_nmi_nesting++;
-	smp_mb__before_atomic();  /* Force delay from prior write. */
-	atomic_inc(&rdtp->dynticks);
-	/* CPUs seeing atomic_inc() must see later RCU read-side crit sects */
-	smp_mb__after_atomic();  /* See above. */
-	WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+	/* Complain about underflow. */
+	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 0);
+
+	/*
+	 * If idle from RCU viewpoint, atomically increment ->dynticks
+	 * to mark non-idle and increment ->dynticks_nmi_nesting by one.
+	 * Otherwise, increment ->dynticks_nmi_nesting by two.  This means
+	 * if ->dynticks_nmi_nesting is equal to one, we are guaranteed
+	 * to be in the outermost NMI handler that interrupted an RCU-idle
+	 * period (observation due to Andy Lutomirski).

kernel/rcu/tree.c +23 -6

+	barrier();
 }
 
 /**
  * rcu_nmi_exit - inform RCU of exit from NMI context
  *
- * If the CPU was idle with dynamic ticks active, and there is no
- * irq handler running, this updates rdtp->dynticks_nmi to let the
- * RCU grace-period handling know that the CPU is no longer active.
+ * If we are returning from the outermost NMI handler that interrupted an
+ * RCU-idle period, update rdtp->dynticks and rdtp->dynticks_nmi_nesting
+ * to let the RCU grace-period handling know that the CPU is back to
+ * being RCU-idle.
  */
 void rcu_nmi_exit(void)
 {
 	struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
 
-	if (rdtp->dynticks_nmi_nesting == 0 ||
-	    --rdtp->dynticks_nmi_nesting != 0)
+	/*
+	 * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
+	 * (We are exiting an NMI handler, so RCU better be paying attention
+	 * to us!)
+	 */
+	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting <= 0);
+	WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+
+	/*
+	 * If the nesting level is not 1, the CPU wasn't RCU-idle, so
+	 * leave it in non-RCU-idle state.
+	 */
+	if (rdtp->dynticks_nmi_nesting != 1) {
+		rdtp->dynticks_nmi_nesting -= 2;
 		return;
+	}
+
+	/* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
+	rdtp->dynticks_nmi_nesting = 0;
 	/* CPUs seeing atomic_inc() must see prior RCU read-side crit sects */
 	smp_mb__before_atomic();  /* See above. */
 	atomic_inc(&rdtp->dynticks);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
.

Quoted 3 lines

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

Reviewed-by Lai Jiangshan <[email protected]>

Quoted 41 lines

kernel/rcu/tree.c +16 -12

index 8749f43f3f05..fc0236992655 100644
@@ -759,39 +759,71 @@ void rcu_irq_enter(void)
 /**
  * rcu_nmi_enter - inform RCU of entry to NMI context
  *
- * If the CPU was idle with dynamic ticks active, and there is no
- * irq handler running, this updates rdtp->dynticks_nmi to let the
- * RCU grace-period handling know that the CPU is active.
+ * If the CPU was idle from RCU's viewpoint, update rdtp->dynticks and
+ * rdtp->dynticks_nmi_nesting to let the RCU grace-period handling know
+ * that the CPU is active.  This implementation permits nested NMIs, as
+ * long as the nesting level does not overflow an int.  (You will probably
+ * run out of stack space first.)
  */
 void rcu_nmi_enter(void)
 {
 	struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
+	int incby = 2;
 
-	if (rdtp->dynticks_nmi_nesting == 0 &&
-	    (atomic_read(&rdtp->dynticks) & 0x1))
-		return;
-	rdtp->dynticks_nmi_nesting++;
-	smp_mb__before_atomic();  /* Force delay from prior write. */
-	atomic_inc(&rdtp->dynticks);
-	/* CPUs seeing atomic_inc() must see later RCU read-side crit sects */
-	smp_mb__after_atomic();  /* See above. */
-	WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+	/* Complain about underflow. */
+	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 0);
+
+	/*
+	 * If idle from RCU viewpoint, atomically increment ->dynticks
+	 * to mark non-idle and increment ->dynticks_nmi_nesting by one.
+	 * Otherwise, increment ->dynticks_nmi_nesting by two.  This means
+	 * if ->dynticks_nmi_nesting is equal to one, we are guaranteed
+	 * to be in the outermost NMI handler that interrupted an RCU-idle
+	 * period (observation due to Andy Lutomirski).

kernel/rcu/tree.c void rcu_irq_enter(void) · line 792

Quoted 10 lines

kernel/rcu/tree.c +10 -0

+	 */
+	if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
+		smp_mb__before_atomic();  /* Force delay from prior write. */
+		atomic_inc(&rdtp->dynticks);
+		/* atomic_inc() before later RCU read-side crit sects */
+		smp_mb__after_atomic();  /* See above. */
+		WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+		incby = 1;
+	}
+	rdtp->dynticks_nmi_nesting += incby;

I prefer a "else" branch here.

	if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
		...
		WARN_ON_ONCE(rdtp->dynticks_nmi_nesting); /* paired with "rdtp->dynticks_nmi_nesting = 0" */
		rdtp->dynticks_nmi_nesting = 1;  /* paired with "if (rdtp->dynticks_nmi_nesting != 1) {" */
	} else {
		rdtp->dynticks_nmi_nesting += 2;
	}

Quoted 50 lines

kernel/rcu/tree.c +23 -6

+	barrier();
 }
 
 /**
  * rcu_nmi_exit - inform RCU of exit from NMI context
  *
- * If the CPU was idle with dynamic ticks active, and there is no
- * irq handler running, this updates rdtp->dynticks_nmi to let the
- * RCU grace-period handling know that the CPU is no longer active.
+ * If we are returning from the outermost NMI handler that interrupted an
+ * RCU-idle period, update rdtp->dynticks and rdtp->dynticks_nmi_nesting
+ * to let the RCU grace-period handling know that the CPU is back to
+ * being RCU-idle.
  */
 void rcu_nmi_exit(void)
 {
 	struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
 
-	if (rdtp->dynticks_nmi_nesting == 0 ||
-	    --rdtp->dynticks_nmi_nesting != 0)
+	/*
+	 * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
+	 * (We are exiting an NMI handler, so RCU better be paying attention
+	 * to us!)
+	 */
+	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting <= 0);
+	WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
+
+	/*
+	 * If the nesting level is not 1, the CPU wasn't RCU-idle, so
+	 * leave it in non-RCU-idle state.
+	 */
+	if (rdtp->dynticks_nmi_nesting != 1) {
+		rdtp->dynticks_nmi_nesting -= 2;
 		return;
+	}
+
+	/* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
+	rdtp->dynticks_nmi_nesting = 0;
 	/* CPUs seeing atomic_inc() must see prior RCU read-side crit sects */
 	smp_mb__before_atomic();  /* See above. */
 	atomic_inc(&rdtp->dynticks);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
.

Quoted 94 lines · Andy Lutomirski

Quoted 8 lines · Paul E. McKenney

Quoted 5 lines · Andy Lutomirski

Quoted 3 lines · Paul E. McKenney

Quoted 2 lines · Andy Lutomirski

On Fri, Nov 21, 2014 at 3:38 PM, Paul E. McKenney
<[email protected]> wrote:

Returning state sounds like a bad idea, if we can reasonably avoid it.

I agree, except that we already do it for exception_enter(), etc. But yes, changing fewer things is nice.

Quoted 79 lines

And I think I finally see what you are pointing out about my code: If another NMI comes in between the time I increment ->dynticks_nmi_nesting and the time I atomically increment ->dynticks, the nested NMI handler will incorrectly believe that RCU is already paying attention to this CPU. Which would indeed not be at all good, so good catch!!!

Quoted 36 lines

Otherwise, I think that there may need to be enough state somewhere so that the outermost nested rcu_nmi_enter knows whether to increment dynticks. For example, dynticks_nmi_nesting could store the nesting count * 2 - (1 if the outermost nested user needs to increment dynticks). Something like:

void rcu_nmi_enter(void)
{
/* Be very careful -- this function may be called reentrently on the
same CPU. */
atomically: increment dynticks if it's even.

/* If an rcu_nmi_enter/rcu_nmi_exit pair happens here, then it will not change
* the state. */

  local_inc(&dynticks_nmi_nesting, (we incremented dynticks ? 1 : 2));

WARN_ON(we incremented dynticks and dynticks_nmi_nesting was nonzero);
}

void rcu_nmi_exit(void)
{
WARN_ON(!(dynticks & 1));
locally atomically: dynticks_nmi_nesting -= 2, unless
dynticks_nmi_nesting == 1, in which case set it to zero

if (dynticks_nmi_nesting was 1)

    atomic_inc(&dynticks);
}

The invariant here is that, for a single unnested enter/exit, if dynticks_nmi_nesting != 0, then dynticks is odd. As a result, an rcu_nmi_enter/rcu_nmi_exit pair at any time when dynticks_nmi_nesting != 0 *or* dynticks is odd will have no net effect, so the invariant, in fact, holds for all invocations, nested or otherwise.

Quoted 7 lines

At least one of those conditions is true at all times during the execution of outermost pair, starting with the first atomic operation and ending with the final atomic_inc. So they nest properly no matter what else happens (unless, of course, someone else pokes dynticks in the middle).

Thoughts?

Let's see... The evenness of ->dynticks should be preserved by nested NMI handlers, so the check and increment need not be atomic. We don't have any way (other than atomic operations) to do local atomic modifications on all architectures, because we cannot mask NMIs. (Yes, it can work on x86, but this is common code that needs to work everywhere.) On the other hand, presumably NMIs are rare, so atomic modification of the NMI nesting counter should be OK, at least if it proves absolutely necessary. And I am thinking that a mechanical proof will be needed here. :-/

But first, let me try generating the code and informally evaluating it:

         1   struct rcu_dynticks {
         2     long long dynticks_nesting;
         3     int dynticks_nmi_nesting;
         4     atomic_t dynticks;
         5   };
         6
         7   void rcu_nmi_enter(void)
         8   {
         9     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
        10     int incby = 2;
        11
        12     if (!(atomic_read(&rdtp->dynticks) & 0x1)) {
        13       smp_mb__before_atomic();
        14       atomic_inc(&rdtp->dynticks);
        15       smp_mb__after_atomic();
        16       WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        17       incby = 1;

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 1) here, perhaps?

That would make sense.

Quoted 6 lines

Quoted 2 lines

        18     }
        19     rdtp->dynticks_nmi_nesting += incby;

Oh, I see why you don't need local_add -- it's because an nmi in the middle of this increment won't have any effect on the interrupted code, so even a software RMW will be okay.

Yep! ;-)

Quoted 11 lines

Quoted 10 lines

        20     barrier();
        21   }
        22
        23   void rcu_nmi_exit(void)
        24   {
        25     struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
        26
        27     WARN_ON_ONCE(!rdtp->dynticks_nmi_nesting);
        28     WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        29     if (rdtp->dynticks_nmi_nesting != 1) {

Quoted 1 line

WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 2), perhaps?

This is already implied by the WARN_ON_ONCE() on line 27 and the check on line 29.

Quoted 7 lines

Quoted 5 lines

        30       rdtp->dynticks_nmi_nesting -= 2;
        31       return;
        32     }
        33     rdtp->dynticks_nmi_nesting = 0;
        34     smp_mb__before_atomic();

This implies barrier(), right?

Yep!

Quoted 39 lines

Quoted 39 lines

        35     atomic_inc(&rdtp->dynticks);
        36     smp_mb__after_atomic();
        37     WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        38   }

Line 9 picks up a pointer to this CPU's rcu_dynticks structure and line 10 assumes that we don't need to increment ->dynticks.

Line 12 checks to see if ->dynticks is even. Note that this check is
stable: If there are nested NMIs, they will increment ->dynticks twice or not at all, and either way preserves the evenness (to be proven, of course, but that is the plan). If ->dynticks is even, lines 13-15 atomically increment it, line 16 complains if still even, and line 17 says we will increment ->dynticks_nmi_nesting by only 1.

Either way, line 19 increments ->dynticks_nmi_nesting as needed and line 20 keeps the compiler from getting too cute.

For rcu_nmi_exit(), line 25 again picks up this CPUs rcu_dynticks structure. Lines 27 and 28 complain bitterly if invariants are violated. If line 29 finds that the value of ->dynticks_nmi_nesting is not 1, then line 30 subtracts 2 from ->dynticks_nmi_nesting and line 31 returns.

Otherwise, line 33 sets ->dynticks_nmi_nesting to zero, lines 34-36 atomically increment ->dynticks with full ordering, and line 37 complains bitterly if ->dynticks is not even.

So, if an NMI occurs before rcu_nmi_enter's atomic increment, then the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will think that they are not nested, which is the correct thing for them to think in that case. They will increment ->dynticks twice and restore ->dynticks_nmi_nesting to zero (adding and then subtracting 1). If the NMI happens after the atomic increment, then the nested rcu_nmi_enter() and rcu_nmi_exit() will leave ->dynticks alone, and will restore ->dynticks_nmi_nesting to zero (adding and subtracting two again). If the NMI happens after the increment of ->dynticks_nmi_nesting, the nested NMI's rcu_nmi_enter() and rcu_nmi_exit() will again restore ->dynticks_nmi_nesting, but this time to one (again adding and subtracting two).

Quoted 8 lines

Quoted 6 lines

In rcu_nmi_exit(), ->dynticks_nmi_nesting of zero had better not happen, one means we need to atomically increment ->dynticks, and other values mean that we are partially or fully nested. Reasoning proceeds as for rcu_nmi_enter(), but in the opposite direction.

Whew! That might even work.

I think I like this, with the warnings above.

OK with dropping the one that I called out as redundant?

Quoted 44 lines

Quoted 44 lines

But how about taking a different approach. Assuming that there can never be more than (say) 14 nesting NMI-like things, use the lower four bits of ->dynticks to represent the NMI nesting and the upper 28 bits as the counter. This of course requires modifying lots of places in RCU that check the counter, but it is probably time to abstract the check anyway.

This would allow my earlier attempted logic to work and (maybe) simplify the reasoning a bit (and yes, the "magic" constants need macros):

        void rcu_nmi_enter(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x01;

                WARN_ON_ONCE(nexting == 0xf);
                if (nesting == 0) {
                        if (atomic_read(&rdtp->dynticks) & 0x10)
                                return;
                        incby = 0x11;
                }
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(!(atomic_read(&rdtp->dynticks) & 0x1));
        }

        void rcu_nmi_exit(void)
        {
                struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
                int nesting = atomic_read(&rdtp->dynticks) & 0xf;
                int incby = 0x0f;

                if (nesting == 0)
                        return;
                if (nesting > 1)
                        incby = -1;
                smp_mb__before_atomic();
                atomic_add(&rdtp->dynticks, incby);
                smp_mb__after_atomic();
                WARN_ON_ONCE(atomic_read(&rdtp->dynticks) & 0x1);
        }

Quoted 3 lines

Quoted 1 line

Over to you! ;-)

This latter one is all you :)

Well, let's see how I feel about it after trying a Promela model of the first code sequence. ;-)

Thanx, Paul

Quoted 12 lines

--Andy

Quoted 3 lines

Thanx, Paul

Signature · 2 lines

Andy Lutomirski
AMA Capital Management, LLC

Quoted 12 lines · Andy Lutomirski

Quoted 10 lines · Andy Lutomirski

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

Quoted 6 lines

I should add:

I have no idea why RCU read-side critical sections are safe inside __do_page_fault today. It's guarded by exception_enter(), but that doesn't do anything if context tracking is off, and context tracking is usually off. What am I missing here?

Ah! There are three cases:

1. Context tracking is off on a non-idle CPU. In this case, RCU is
still paying attention to CPUs running in both userspace and in the kernel. So if a page fault happens, RCU will be set up to notice any RCU read-side critical sections.

2. Context tracking is on on a non-idle CPU. In this case, RCU
might well be ignoring userspace execution: NO_HZ_FULL and all that. However, as you pointed out, in this case the context-tracking code lets RCU know that we have entered the kernel, which means that RCU will again be paying attention to RCU read-side critical sections.

3. The CPU is idle. In this case, RCU is ignoring the CPU, so
if we take a page fault when context tracking is off, life will be hard. But the kernel is not supposed to take page faults in the idle loop, so this is not a problem.

Make sense?

We currently pretend that IST context is like standard exception context, but this is incorrect. IST entries from userspace are like standard exceptions except that they use per-cpu stacks, so they are atomic. IST entries from kernel space are like NMIs from RCU's perspective -- they are not quiescent states even if they interrupted the kernel during a quiescent state.

Add and use ist_enter and ist_exit to track IST context. Even though x86_32 has no IST stacks, we track these interrupts the same way.

This fixes two issues:

- Scheduling from an IST interrupt handler will now warn. It would previously appear to work as long as we got lucky and nothing overwrote the stack frame. (I don't know of any bugs in this that would trigger the warning, but it's good to be on the safe side.)

- RCU handling in IST context was dangerous. As far as I know, only machine checks were likely to trigger this, but it's good to be on the safe side.

Note that the machine check handlers appears to have been missing any context tracking at all before this patch.

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

Cc Josh Triplett <[email protected]>

Cc Frédéric Weisbecker <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/traps.h         |  4 +++
 arch/x86/kernel/cpu/mcheck/mce.c     |  5 ++++
 arch/x86/kernel/cpu/mcheck/p5.c      |  6 +++++
 arch/x86/kernel/cpu/mcheck/winchip.c |  5 ++++
 arch/x86/kernel/traps.c              | 49 ++++++++++++++++++++++++++++++------
 5 files changed, 61 insertions(+), 8 deletions(-)

arch/x86/include/asm/traps.h +4 -0

index bc8352e7010a..eb16a61bfd06 100644
@@ -1,6 +1,7 @@
 #ifndef _ASM_X86_TRAPS_H
 #define _ASM_X86_TRAPS_H
 
+#include <linux/context_tracking_state.h>
 #include <linux/kprobes.h>
 
 #include <asm/debugreg.h>

@@ -109,6 +110,9 @@ asmlinkage void smp_thermal_interrupt(void);
 asmlinkage void mce_threshold_interrupt(void);
 #endif
 
+extern enum ctx_state ist_enter(struct pt_regs *regs);
+extern void ist_exit(struct pt_regs *regs, enum ctx_state prev_state);
+
 /* Interrupts/Exceptions */
 enum {
 	X86_TRAP_DE = 0,	/*  0, Divide-by-zero */

arch/x86/kernel/cpu/mcheck/mce.c +5 -0

index 61a9668cebfd..b72509d77337 100644
@@ -43,6 +43,7 @@
 #include <linux/export.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 

@@ -1016,6 +1017,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 {
 	struct mca_config *cfg = &mca_cfg;
 	struct mce m, *final;
+	enum ctx_state prev_state;
 	int i;
 	int worst = 0;
 	int severity;

@@ -1038,6 +1040,8 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	DECLARE_BITMAP(valid_banks, MAX_NR_BANKS);
 	char *msg = "Unknown";
 
+	prev_state = ist_enter(regs);
+
 	this_cpu_inc(mce_exception_count);
 
 	if (!cfg->banks)

@@ -1168,6 +1172,7 @@ void do_machine_check(struct pt_regs *regs, long error_code)
 	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0);
 out:
 	sync_core();
+	ist_exit(regs, prev_state);
 }
 EXPORT_SYMBOL_GPL(do_machine_check);
 

arch/x86/kernel/cpu/mcheck/p5.c +6 -0

index a3042989398c..ec2663a708e4 100644
@@ -8,6 +8,7 @@
 #include <linux/smp.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 

@@ -17,8 +18,11 @@ int mce_p5_enabled __read_mostly;
 /* Machine check handler for Pentium class Intel CPUs: */
 static void pentium_machine_check(struct pt_regs *regs, long error_code)
 {
+	enum ctx_state prev_state;
 	u32 loaddr, hi, lotype;
 
+	prev_state = ist_enter(regs);
+
 	rdmsr(MSR_IA32_P5_MC_ADDR, loaddr, hi);
 	rdmsr(MSR_IA32_P5_MC_TYPE, lotype, hi);
 

@@ -33,6 +37,8 @@ static void pentium_machine_check(struct pt_regs *regs, long error_code)
 	}
 
 	add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
+
+	ist_exit(regs, prev_state);
 }
 
 /* Set up machine check reporting for processors with Intel style MCE: */

arch/x86/kernel/cpu/mcheck/winchip.c +5 -0

index 7dc5564d0cdf..bd5d46a32210 100644
@@ -7,14 +7,19 @@
 #include <linux/types.h>
 
 #include <asm/processor.h>
+#include <asm/traps.h>
 #include <asm/mce.h>
 #include <asm/msr.h>
 
 /* Machine check handler for WinChip C6: */
 static void winchip_machine_check(struct pt_regs *regs, long error_code)
 {
+	enum ctx_state prev_state = ist_enter(regs);
+
 	printk(KERN_EMERG "CPU0: Machine Check Exception.\n");
 	add_taint(TAINT_MACHINE_CHECK, LOCKDEP_NOW_UNRELIABLE);
+
+	ist_exit(regs, prev_state);
 }
 
 /* Set up machine check reporting on the Winchip C6 series */

arch/x86/kernel/traps.c +41 -8

index 0d0e922fafc1..f5c4b8813774 100644
@@ -107,6 +107,39 @@ static inline void preempt_conditional_cli(struct pt_regs *regs)
 	preempt_count_dec();
 }
 
+enum ctx_state ist_enter(struct pt_regs *regs)
+{
+	/*
+	 * We are atomic because we're on the IST stack (or we're on x86_32,
+	 * in which case we still shouldn't schedule.
+	 */
+	preempt_count_add(HARDIRQ_OFFSET);
+
+	if (user_mode_vm(regs)) {
+		/* Other than that, we're just an exception. */
+		return exception_enter();
+	} else {
+		/*
+		 * We might have interrupted pretty much anything.  In
+		 * fact, if we're a machine check, we can even interrupt
+		 * NMI processing.  We don't want in_nmi() to return true,
+		 * but we need to notify RCU.
+		 */
+		rcu_nmi_enter();
+		return IN_KERNEL;  /* the value is irrelevant. */
+	}
+}
+
+void ist_exit(struct pt_regs *regs, enum ctx_state prev_state)
+{
+	preempt_count_sub(HARDIRQ_OFFSET);
+
+	if (user_mode_vm(regs))
+		return exception_exit(prev_state);
+	else
+		rcu_nmi_exit();
+}
+
 static nokprobe_inline int
 do_trap_no_signal(struct task_struct *tsk, int trapnr, char *str,
 		  struct pt_regs *regs,	long error_code)

@@ -244,14 +277,14 @@ dotraplinkage void do_stack_segment(struct pt_regs *regs, long error_code)
 {
 	enum ctx_state prev_state;
 
-	prev_state = exception_enter();
+	prev_state = ist_enter(regs);
 	if (notify_die(DIE_TRAP, "stack segment", regs, error_code,
 		       X86_TRAP_SS, SIGBUS) != NOTIFY_STOP) {
 		preempt_conditional_sti(regs);
 		do_trap(X86_TRAP_SS, SIGBUS, "stack segment", regs, error_code, NULL);
 		preempt_conditional_cli(regs);
 	}
-	exception_exit(prev_state);
+	ist_exit(regs, prev_state);
 }
 
 dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)

@@ -259,8 +292,8 @@ dotraplinkage void do_double_fault(struct pt_regs *regs, long error_code)
 	static const char str[] = "double fault";
 	struct task_struct *tsk = current;
 
-	exception_enter();
-	/* Return not checked because double check cannot be ignored */
+	ist_enter(regs);
+	/* Return not checked because double fault cannot be ignored */
 	notify_die(DIE_TRAP, str, regs, error_code, X86_TRAP_DF, SIGSEGV);
 
 	tsk->thread.error_code = error_code;

@@ -343,7 +376,7 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
 	if (poke_int3_handler(regs))
 		return;
 
-	prev_state = exception_enter();
+	prev_state = ist_enter(regs);
 #ifdef CONFIG_KGDB_LOW_LEVEL_TRAP
 	if (kgdb_ll_trap(DIE_INT3, "int3", regs, error_code, X86_TRAP_BP,
 				SIGTRAP) == NOTIFY_STOP)

@@ -369,7 +402,7 @@ dotraplinkage void notrace do_int3(struct pt_regs *regs, long error_code)
 	preempt_conditional_cli(regs);
 	debug_stack_usage_dec();
 exit:
-	exception_exit(prev_state);
+	ist_exit(regs, prev_state);
 }
 NOKPROBE_SYMBOL(do_int3);
 

@@ -433,7 +466,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	unsigned long dr6;
 	int si_code;
 
-	prev_state = exception_enter();
+	prev_state = ist_enter(regs);
 
 	get_debugreg(dr6, 6);
 

@@ -508,7 +541,7 @@ dotraplinkage void do_debug(struct pt_regs *regs, long error_code)
 	debug_stack_usage_dec();
 
 exit:
-	exception_exit(prev_state);
+	ist_exit(regs, prev_state);
 }
 NOKPROBE_SYMBOL(do_debug);
 

Signature · 1 line

1.9.3