Lock depth checking allows panicy prints
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 9 Jan 2013 01:00:01 +0000 (17:00 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 9 Jan 2013 01:19:38 +0000 (17:19 -0800)
We use locks when printing, so once one lock failed, it would
recursively fail if the condition remained true (such as having a bad
kernel stack depth).  This would happen for any kernel trap or trace
coretf, or any other place where I don't care about the deadlock but
want to get the printf.

Note that there are a couple locks involved in printing - two on the
printk path.  If we fail (e.g. for IRQSAVE reasons) on the SECOND lock
(after grabbing the first lock), we will still deadlock, since we
basically grabbed lock 1, failed when getting 2, then attempted to get 1
again for another printk.  In lieu of making this debug code more
robust, we just will leave those locks as irqsave for a long time.

kern/arch/i686/trap.c
kern/include/smp.h
kern/src/atomic.c
kern/src/init.c

index 8cb59d9..41ce704 100644 (file)
@@ -218,6 +218,11 @@ print_trapframe(trapframe_t *tf)
 {
        static spinlock_t ptf_lock = SPINLOCK_INITIALIZER_IRQSAVE;
 
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
+       /* This is only called in debug scenarios, and often when the kernel trapped
+        * and needs to tell us about it.  Disable the lock checker so it doesn't go
+        * nuts when we print/panic */
+       pcpui->__lock_depth_disabled++;
        spin_lock_irqsave(&ptf_lock);
        printk("TRAP frame at %p on core %d\n", tf, core_id());
        print_regs(&tf->tf_regs);
@@ -236,20 +241,27 @@ print_trapframe(trapframe_t *tf)
                printk("  ss   0x----%04x\n", tf->tf_ss);
        }
        spin_unlock_irqsave(&ptf_lock);
+       pcpui->__lock_depth_disabled--;
 }
 
 /* Certain traps want IRQs enabled, such as the syscall.  Others can't handle
  * it, like the page fault handler.  Turn them on on a case-by-case basis. */
 static void trap_dispatch(struct trapframe *tf)
 {
+       struct per_cpu_info *pcpui;
        // Handle processor exceptions.
        switch(tf->tf_trapno) {
                case T_NMI:
+                       /* Temporarily disable deadlock detection when we print.  We could
+                        * deadlock if we were printing when we NMIed. */
+                       pcpui = &per_cpu_info[core_id()];
+                       pcpui->__lock_depth_disabled++;
                        print_trapframe(tf);
                        char *fn_name = get_fn_name(tf->tf_eip);
                        printk("Core %d is at %08p (%s)\n", core_id(), tf->tf_eip, fn_name);
                        kfree(fn_name);
                        print_kmsgs(core_id());
+                       pcpui->__lock_depth_disabled--;
                        break;
                case T_BRKPT:
                        enable_irq();
@@ -485,8 +497,7 @@ void page_fault_handler(struct trapframe *tf)
                panic("Page Fault in the Kernel at 0x%08x!", fault_va);
                /* if we want to do something like kill a process or other code, be
                 * aware we are in a sort of irq-like context, meaning the main kernel
-                * code we 'interrupted' could be holding locks - even irqsave locks.
-                * Send yourself a kernel message to do this sort of work. */
+                * code we 'interrupted' could be holding locks - even irqsave locks. */
        }
        /* safe to reenable after rcr2 */
        enable_irq();
index 6a2b52c..5010b46 100644 (file)
@@ -32,6 +32,7 @@ struct per_cpu_info {
        struct trapframe *cur_tf;       /* user tf we came in on (can be 0) */
        struct trapframe actual_tf;     /* storage for cur_tf */
        uint32_t __ctx_depth;           /* don't access directly.  see trap.h. */
+       int __lock_depth_disabled;      /* disables spinlock depth checking */
        struct syscall *cur_sysc;       /* ptr is into cur_proc's address space */
        struct kthread *spare;          /* useful when restarting */
        struct timer_chain tchain;      /* for the per-core alarm */
index 9a9a042..6a30d8f 100644 (file)
@@ -28,19 +28,26 @@ void spin_lock(spinlock_t *lock)
 {
        uint32_t coreid = core_id();
        struct per_cpu_info *pcpui = &per_cpu_info[coreid];
-       /* TODO: don't print directly.  If we have a lock on the print path that
-        * fails, we'll recurse and/or deadlock */
+       /* Short circuit our lock checking, so we can print or do other things to
+        * announce the failure that require locks. */
+       if (pcpui->__lock_depth_disabled)
+               goto lock;
        if (lock->irq_okay) {
                if (!can_spinwait_irq(pcpui)) {
+                       pcpui->__lock_depth_disabled++;
                        print_kctx_depths("IRQOK");
                        panic("Lock %08p tried to spin when it shouldn't\n", lock);
+                       pcpui->__lock_depth_disabled--;
                }
        } else {
                if (!can_spinwait_noirq(pcpui)) {
+                       pcpui->__lock_depth_disabled++;
                        print_kctx_depths("NOIRQ");
                        panic("Lock %08p tried to spin when it shouldn't\n", lock);
+                       pcpui->__lock_depth_disabled--;
                }
        }
+lock:
        __spin_lock(lock);
        lock->call_site = get_caller_pc();
        lock->calling_core = coreid;
index d744b11..bd45cf1 100644 (file)
@@ -120,7 +120,10 @@ void kernel_init(multiboot_info_t *mboot_info)
 void _panic(const char *file, int line, const char *fmt,...)
 {
        va_list ap;
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id()];
 
+       /* We're panicing, possibly in a place that can't handle the lock checker */
+       pcpui->__lock_depth_disabled++;
        va_start(ap, fmt);
        cprintf("kernel panic at %s:%d, from core %d: ", file, line, core_id());
        vcprintf(fmt, ap);
@@ -129,6 +132,10 @@ void _panic(const char *file, int line, const char *fmt,...)
 
 dead:
        monitor(NULL);
+       /* We could consider turning the lock checker back on here, but things are
+        * probably a mess anyways, and with it on we would probably lock up right
+        * away when we idle. */
+       //pcpui->__lock_depth_disabled--;
        smp_idle();
 }