Support atomic printks
authorBarret Rhoden <brho@cs.berkeley.edu>
Thu, 19 Jul 2018 18:54:09 +0000 (14:54 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 19 Jul 2018 18:54:09 +0000 (14:54 -0400)
printk() was atomic at the granularity of a line - most of the time.  It
wasn't for kernel faults, which leads to inscrutable bug reports.  At the
time, I avoided the lock to avoid deadlocking.  Instead, we can use a
recursive lock, which I'm not normally a fan of.

Additionally, other callers can use print_lock() to atomically print
multiple lines.  The classic example is panic() and warn().  panic() has a
few issues of its own, so we have print_unlock_force().

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/include/stdio.h
kern/src/init.c
kern/src/printf.c

index ce0f048..fd9cbaf 100644 (file)
@@ -35,6 +35,9 @@ void  printfmt(void (*putch)(int, void**), void **putdat, const char *fmt, ...);
 void   vprintfmt(void (*putch)(int, void**), void **putdat, const char *fmt, va_list);
 
 // lib/printf.c
+void print_lock(void);
+void print_unlock(void);
+void print_unlock_force(void);
 int    ( cprintf)(const char *fmt, ...);
 int    vcprintf(const char *fmt, va_list);
 
index 28b8e06..60c64a7 100644 (file)
@@ -229,11 +229,12 @@ static int run_init_script(void)
 #endif
 
 /* Multiple cores can panic concurrently.  We could also panic recursively,
- * which would deadlock.  We also only want to automatically backtrace the first
+ * which could deadlock.  We also only want to automatically backtrace the first
  * time through, since BTs are often the source of panics.  Finally, we want to
  * know when the other panicking cores are done (or likely to be done) before
- * entering the monitor. */
-static spinlock_t panic_lock = SPINLOCK_INITIALIZER_IRQSAVE;
+ * entering the monitor.
+ *
+ * We'll use the print_lock(), which is recursive, to protect panic_printing. */
 static bool panic_printing;
 static DEFINE_PERCPU(int, panic_depth);
 
@@ -244,17 +245,11 @@ static DEFINE_PERCPU(int, panic_depth);
 void _panic(struct hw_trapframe *hw_tf, const char *file, int line,
             const char *fmt, ...)
 {
+       struct per_cpu_info *pcpui = &per_cpu_info[core_id_early()];
        va_list ap;
-       struct per_cpu_info *pcpui;
 
-       /* We're panicing, possibly in a place that can't handle the lock checker */
-       pcpui = &per_cpu_info[core_id_early()];
-       pcpui->__lock_checking_enabled--;
-
-       if (!PERCPU_VAR(panic_depth)) {
-               spin_lock_irqsave(&panic_lock);
-               panic_printing = true;
-       }
+       print_lock();
+       panic_printing = true;
        PERCPU_VAR(panic_depth)++;
 
        va_start(ap, fmt);
@@ -278,11 +273,11 @@ void _panic(struct hw_trapframe *hw_tf, const char *file, int line,
        }
        printk("\n");
 
-       /* If we're here, we panicked and currently hold the lock.  We might have
-        * panicked recursively.  We must unlock unconditionally, since the initial
-        * panic (which grabbed the lock) will never run again. */
+       /* If we're here, we panicked and currently hold the print_lock.  We might
+        * have panicked recursively.  We must unlock unconditionally, since the
+        * initial panic (which grabbed the lock) will never run again. */
        panic_printing = false;
-       spin_unlock_irqsave(&panic_lock);
+       print_unlock_force();
        /* And we have to clear the depth, so that we lock again next time in.
         * Otherwise, we'd be unlocking without locking (which is another panic). */
        PERCPU_VAR(panic_depth) = 0;
@@ -293,34 +288,39 @@ void _panic(struct hw_trapframe *hw_tf, const char *file, int line,
                udelay(500000);
                cmb();
        } while (panic_printing);
-       monitor(NULL);
 
-       if (pcpui->cur_proc) {
-               printk("panic killing proc %d\n", pcpui->cur_proc->pid);
-               proc_destroy(pcpui->cur_proc);
-       }
        /* Yikes!  We're claiming to be not in IRQ/trap ctx and not holding any
         * locks.  Obviously we could be wrong, and could easily deadlock.  We could
         * be in an IRQ handler, an unhandled kernel fault, or just a 'normal' panic
         * in a syscall - any of which can involve unrestore invariants. */
        pcpui->__ctx_depth = 0;
        pcpui->lock_depth = 0;
+       /* And keep this off, for good measure. */
+       pcpui->__lock_checking_enabled--;
+
+       monitor(NULL);
+
+       if (pcpui->cur_proc) {
+               printk("panic killing proc %d\n", pcpui->cur_proc->pid);
+               proc_destroy(pcpui->cur_proc);
+       }
        if (pcpui->cur_kthread)
                kth_panic_sysc(pcpui->cur_kthread);
        smp_idle();
 }
 
-/* like panic, but don't */
 void _warn(const char *file, int line, const char *fmt,...)
 {
        va_list ap;
 
+       print_lock();
        va_start(ap, fmt);
        printk("kernel warning at %s:%d, from core %d: ", file, line,
               core_id_early());
        vcprintf(fmt, ap);
        cprintf("\n");
        va_end(ap);
+       print_unlock();
 }
 
 static void run_links(linker_func_t *linkstart, linker_func_t *linkend)
index 6e54a04..7b415d9 100644 (file)
 #include <kprof.h>
 #include <init.h>
 
-spinlock_t output_lock = SPINLOCK_INITIALIZER_IRQSAVE;
+/* Recursive lock.  Would like to avoid spreading these in the kernel. */
+static spinlock_t output_lock = SPINLOCK_INITIALIZER_IRQSAVE;
+static int output_lock_holder = -1;    /* core_id. */
+static int output_lock_count;
+
+void print_lock(void)
+{
+       if (output_lock_holder == core_id_early()) {
+               output_lock_count++;
+               return;
+       }
+       pcpui_var(core_id_early(), __lock_checking_enabled)--;
+       spin_lock_irqsave(&output_lock);
+       output_lock_holder = core_id_early();
+       output_lock_count = 1;
+}
+
+void print_unlock(void)
+{
+       output_lock_count--;
+       if (output_lock_count)
+               return;
+       output_lock_holder = -1;
+       spin_unlock_irqsave(&output_lock);
+       pcpui_var(core_id_early(), __lock_checking_enabled)++;
+}
+
+/* Regardless of where we are, unlock.  This is dangerous, and only used when
+ * you know you will never unwind your stack, such as for a panic. */
+void print_unlock_force(void)
+{
+       output_lock_holder = -1;
+       output_lock_count = 0;
+       spin_unlock_irqsave(&output_lock);
+       pcpui_var(core_id_early(), __lock_checking_enabled)++;
+}
 
 void putch(int ch, int **cnt)
 {
@@ -43,50 +78,24 @@ void buffered_putch(int ch, int **cnt)
 
 int vcprintf(const char *fmt, va_list ap)
 {
-       struct per_cpu_info *pcpui;
        int cnt = 0;
        int *cntp = &cnt;
        volatile int i;
-       int8_t irq_state = 0;
        va_list args;
 
+       print_lock();
+
        va_copy(args, ap);
        trace_vprintk(fmt, args);
        va_end(args);
 
-       /* this ktrap depth stuff is in case the kernel faults in a printfmt call.
-        * we disable the locking if we're in a fault handler so that we don't
-        * deadlock. */
-       if (booting)
-               pcpui = &per_cpu_info[0];
-       else
-               pcpui = &per_cpu_info[core_id()];
-       /* lock all output.  this will catch any printfs at line granularity.  when
-        * tracing, we short-circuit the main lock call, so as not to clobber the
-        * results as we print. */
-       if (!ktrap_depth(pcpui)) {
-               #ifdef CONFIG_TRACE_LOCKS
-               disable_irqsave(&irq_state);
-               __spin_lock(&output_lock);
-               #else
-               spin_lock_irqsave(&output_lock);
-               #endif
-       }
-
        // do the buffered printf
        vprintfmt((void*)buffered_putch, (void*)&cntp, fmt, ap);
 
        // write out remaining chars in the buffer
        buffered_putch(-1,&cntp);
 
-       if (!ktrap_depth(pcpui)) {
-               #ifdef CONFIG_TRACE_LOCKS
-               __spin_unlock(&output_lock);
-               enable_irqsave(&irq_state);
-               #else
-               spin_unlock_irqsave(&output_lock);
-               #endif
-       }
+       print_unlock();
 
        return cnt;
 }