Clean up smp_idle's stack jumping
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 22 Jul 2016 18:41:55 +0000 (14:41 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 27 Jul 2016 16:52:43 +0000 (12:52 -0400)
First off, RESET_STACKS is no longer an option.  There are weird cases
where you want to backtrace beyond smp_idle.  You can comment that out
manually if you want, but there's no need to make it a regular thing.
If you don't RESET_STACKS, you'll potentially run off the stack.

Other things:
- that disable_irq in smp_idle() wasn't really protecting anything.  No
  sense in doing it there.  We might need it to protect parts of the
KMSG subsystem, so I moved it to __smp_idle() for now.
- that cmb() did nothing.  The compiler won't reorder those asm
  operations.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
Kconfig
config-default
kern/src/smp.c

diff --git a/Kconfig b/Kconfig
index 0813fed..1414f23 100644 (file)
--- a/Kconfig
+++ b/Kconfig
@@ -192,14 +192,6 @@ config SEM_TRACE_BLOCKERS
                races).  To access the trace data, you'll need to use the opcontrols
                and process the data with op2.
 
-config RESET_STACKS
-       bool "Reset Stacks"
-       default y
-       help
-               When idling, the kernel will reset the stack of its kernel thread to
-               the top of the stack, abandoning its context.  This is useful so we do
-               not run off the end of our stacks, but makes backtracing difficult.
-
 config LARGE_KSTACKS
        bool "Large (two-page) Kernel Stacks"
        default y
index f7b61c5..4aba9d8 100644 (file)
@@ -46,7 +46,6 @@ CONFIG_KFS_CPIO_BIN=""
 #
 # CONFIG_SPINLOCK_DEBUG is not set
 # CONFIG_SEQLOCK_DEBUG is not set
-CONFIG_RESET_STACKS=y
 # CONFIG_KTHREAD_POISON is not set
 # CONFIG_DISABLE_SMT is not set
 # CONFIG_PRINTK_NO_BACKSPACE is not set
index a920c41..08edcc7 100644 (file)
@@ -62,6 +62,7 @@ static void __attribute__((noinline, noreturn)) __smp_idle(void)
 {
        struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
 
+       disable_irq();  /* might not be needed - need to look at KMSGs closely */
        clear_rkmsg(pcpui);
        pcpui->cur_kthread->flags = KTH_DEFAULT_FLAGS;
        enable_irq();   /* one-shot change to get any IRQs before we halt later */
@@ -84,21 +85,13 @@ static void __attribute__((noinline, noreturn)) __smp_idle(void)
 void smp_idle(void)
 {
        /* FP must be zeroed before SP.  Ideally, we'd do both atomically.  If we
-        * take an IRQ in between and set SP first, then a backtrace would be
+        * take an IRQ/NMI in between and set SP first, then a backtrace would be
         * confused since FP points *below* the SP that the *IRQ handler* is now
-        * using.  Disabling IRQs gets us most of the way, but we could have an NMI
-        * that does a BT (e.g. for debugging).  By zeroing FP first, at least we
-        * won't BT at all (though FP is still out of sync with SP).
-        *
-        * Disabling IRQs here also will help with general sanity. */
-       disable_irq();
-       #ifdef CONFIG_RESET_STACKS
+        * using.  By zeroing FP first, at least we won't BT at all (though FP is
+        * still out of sync with SP). */
        set_frame_pointer(0);
-       cmb();
        set_stack_pointer(get_stack_top());
-       #endif /* CONFIG_RESET_STACKS */
        __smp_idle();
-       assert(0);
 }
 
 /* Arch-independent per-cpu initialization.  This will call the arch dependent