Re: [PATCH v5 0/2] printk: Console owner and waiter logic cleanup

Steven Rostedt
2018-01-17T18:42:07Z

On Wed, 17 Jan 2018 12:12:51 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> @@ -2393,15 +2451,20 @@ void console_unlock(void)
> * waiter waiting to take over.
> */
> console_lock_spinning_enable();
> + offload = recursion_check_start();
>
> stop_critical_timings(); /* don't trace print latency */
> call_console_drivers(ext_text, ext_len, text, len);
> start_critical_timings();
>
> + recursion_check_finish(offload);
> +
> if (console_lock_spinning_disable_and_check()) {
> printk_safe_exit_irqrestore(flags);
> return;
> }
> + if (offload)
> + kick_offload_thread();
>

Ah, major flaw in this code. The recursion check needs to be in
printk() itself around the trylock.

-- Steve

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9cb943c90d98..31df145cc4d7 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1826,6 +1826,63 @@ static size_t log_output(int facility, int level, enum log_flags lflags, const c
/* Store it in the record log */
return log_store(facility, level, lflags, 0, dict, dictlen, text, text_len);
}
+/*
+ * Used for which context the printk is in.
+ * NMI = 0
+ * IRQ = 1
+ * SOFTIRQ = 2
+ * NORMAL = 3
+ *
+ * Stack ordered, where the lower number can preempt
+ * the higher number: mask &= mask - 1, will only clear
+ * the lowerest set bit.
+ */
+enum {
+ CTX_NMI,
+ CTX_IRQ,
+ CTX_SOFTIRQ,
+ CTX_NORMAL,
+};
+
+static DEFINE_PER_CPU(int, recursion_bits);
+
+static bool recursion_check_start(void)
+{
+ unsigned long pc = preempt_count();
+ int val = this_cpu_read(recursion_bits);
+
+ if (!(pc & (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET)))
+ bit = CTX_NORMAL;
+ else
+ bit = pc & NMI_MASK ? CTX_NMI :
+ pc & HARDIRQ_MASK ? CTX_IRQ : CTX_SOFTIRQ;
+
+ if (unlikely(val & (1 << bit)))
+ return true;
+
+ val |= (1 << bit);
+ this_cpu_write(recursion_bits, val);
+ return false;
+}
+
+static void recursion_check_finish(bool offload)
+{
+ int val = this_cpu_read(recursion_bits);
+
+ if (offload)
+ return;
+
+ val &= val - 1;
+ this_cpu_write(recursion_bits, val);
+}
+
+static void kick_offload_thread(void)
+{
+ /*
+ * Consoles are triggering printks, offload the printks
+ * to another CPU to hopefully avoid a lockup.
+ */
+}

asmlinkage int vprintk_emit(int facility, int level,
const char *dict, size_t dictlen,
@@ -1895,12 +1952,14 @@ asmlinkage int vprintk_emit(int facility, int level,

/* If called from the scheduler, we can not call up(). */
if (!in_sched) {
+ bool offload;
/*
* Disable preemption to avoid being preempted while holding
* console_sem which would prevent anyone from printing to
* console
*/
preempt_disable();
+ offload = recursion_check_start();
/*
* Try to acquire and then immediately release the console
* semaphore. The release will print out buffers and wake up
@@ -1908,7 +1967,12 @@ asmlinkage int vprintk_emit(int facility, int level,
*/
if (console_trylock_spinning())
console_unlock();
+
+ recursion_check_finish(offload);
preempt_enable();
+
+ if (offload)
+ kick_offload_thread();
}

return printed_len;