All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v12 0/3] printk: Make printk() completely async
@ 2016-04-22 13:52 Sergey Senozhatsky
  2016-04-22 13:52 ` [PATCH v12 1/3] " Sergey Senozhatsky
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-04-22 13:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jan Kara, Petr Mladek, Tejun Heo, Tetsuo Handa, linux-kernel,
	Byungchul Park, Sergey Senozhatsky, Sergey Senozhatsky

Hello,

This patch set makes printk() completely asynchronous: new messages
are getting upended to the kernel printk buffer, but instead of 'direct'
printing the actual print job is performed by a dedicated kthread.
This has the advantage that printing always happens from a schedulable
context and thus we don't lockup any particular CPU or even interrupts.

against next-20160422

v12:
-- rename printk_kthread_can_run bool flag
-- update printk_kthread_can_run comment (Petr)
-- drop mutex from printk_sync_set(), sysfs writes are synchronised (Petr)

v11:
-- switch default to sync printk
-- make `synchronous' param RW (Andrew, Jan)
-- set RT priority to printk kthread (Andrew)
-- correct comments (Andrew)

v10:
-- simplify printk_kthread_need_flush_console (Jan, Petr)

v9:
-- move need_flush_console assignment down in vprintk_emit (Jan)
-- simplify need_flush_console assignment rules (Petr)
-- clear need_flush_console in printing function (Petr)
-- rename need_flush_console (Petr)

v8:
-- rename kthread printing function (Petr)
-- clear need_flush_console in console_unlock() under logbuf (Petr)

v7:
-- do not set global printk_sync in panic in vrintk_emit() (Petr)
-- simplify vprintk_emit(). drop some of local variables (Petr)
-- move handling of LOGLEVEL_SCHED messages back to printk_deferred()
   so we wake_up_process()/console_trylock() in vprintk_emit() only
   for !in_sched messages

v6:
-- move wake_up_process out of logbuf lock (Jan, Byungchul)
-- do not disable async printk in recursion handling code.
-- rebase against next-20160321 (w/NMI patches)

v5:
-- make printk.synchronous RO (Petr)
-- make printing_func() correct and do not use wait_queue (Petr)
-- do not panic() when can't allocate printing thread (Petr)
-- do not wake_up_process() only in IRQ, prefer vprintk_emit() (Jan)
-- move wake_up_klogd_work_func() to a separate patch (Petr)
-- move wake_up_process() under logbuf lock so printk recursion logic can
   help us out
-- switch to sync_print mode if printk recursion occured
-- drop "printk: Skip messages on oops" patch

v4:
-- do not directly wake_up() the printing kthread from vprintk_emit(), need
   to go via IRQ->wake_up() to avoid sched deadlocks (Jan)

v3:
-- use a dedicated kthread for printing instead of using wq (Jan, Tetsuo, Tejun)

v2:
- use dedicated printk workqueue with WQ_MEM_RECLAIM bit
- fallback to system-wide workqueue only if allocation of printk_wq has
  failed
- do not use system_wq as a fallback wq. both console_lock() and onsole_unlock()
  can spend a significant amount of time; so we need to use system_long_wq.
- rework sync/!sync detection logic
  a) we can have deferred (in_sched) messages before we allocate printk_wq,
     so the only way to handle those messages is via IRQ context
  b) even in printk.synchronous mode, deferred messages must not be printed
     directly, and should go via IRQ context
  c) even if we allocated printk_wq and have !sync_printk mode, we must route
     deferred messages via IRQ context
- so this adds additional bool flags to vprint_emit() and introduces a new
  pending bit to `printk_pending'
- fix build on !PRINTK configs


Jan Kara (2):
  printk: Make printk() completely async
  printk: Make wake_up_klogd_work_func() async

Sergey Senozhatsky (1):
  printk: make printk.synchronous param rw

 Documentation/kernel-parameters.txt |  12 +++
 kernel/printk/printk.c              | 141 +++++++++++++++++++++++++++++++++---
 2 files changed, 143 insertions(+), 10 deletions(-)

-- 
2.8.0

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v12 1/3] printk: Make printk() completely async
  2016-04-22 13:52 [PATCH v12 0/3] printk: Make printk() completely async Sergey Senozhatsky
@ 2016-04-22 13:52 ` Sergey Senozhatsky
  2016-04-26 14:13   ` Petr Mladek
  2016-04-22 13:52 ` [PATCH v12 2/3] printk: Make wake_up_klogd_work_func() async Sergey Senozhatsky
  2016-04-22 13:53 ` [PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky
  2 siblings, 1 reply; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-04-22 13:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jan Kara, Petr Mladek, Tejun Heo, Tetsuo Handa, linux-kernel,
	Byungchul Park, Sergey Senozhatsky, Sergey Senozhatsky, Jan Kara

From: Jan Kara <jack@suse.cz>

Currently, printk() sometimes waits for message to be printed to console
and sometimes it does not (when console_sem is held by some other
process). In case printk() grabs console_sem and starts printing to
console, it prints messages from kernel printk buffer until the buffer
is empty. When serial console is attached, printing is slow and thus
other CPUs in the system have plenty of time to append new messages to
the buffer while one CPU is printing. Thus the CPU can spend unbounded
amount of time doing printing in console_unlock().  This is especially
serious problem if the printk() calling console_unlock() was called with
interrupts disabled.

In practice users have observed a CPU can spend tens of seconds printing
in console_unlock() (usually during boot when hundreds of SCSI devices
are discovered) resulting in RCU stalls (CPU doing printing doesn't
reach quiescent state for a long time), softlockup reports (IPIs for the
printing CPU don't get served and thus other CPUs are spinning waiting
for the printing CPU to process IPIs), and eventually a machine death
(as messages from stalls and lockups append to printk buffer faster than
we are able to print). So these machines are unable to boot with serial
console attached. Another observed issue is that due to slow printk,
hardware discovery is slow and udev times out before kernel manages to
discover all the attached HW. Also during artificial stress testing SATA
disk disappears from the system because its interrupts aren't served for
too long.

This patch makes printk() completely asynchronous (similar to what
printk_deferred() did until now). It appends message to the kernel
printk buffer and wake_up()s a special dedicated kthread to do the
printing to console. This has the advantage that printing always happens
from a schedulable contex and thus we don't lockup any particular CPU or
even interrupts. Also it has the advantage that printk() is fast and
thus kernel booting is not slowed down by slow serial console.
Disadvantage of this method is that in case of crash there is higher
chance that important messages won't appear in console output (we may
need working scheduling to print message to console). We somewhat
mitigate this risk by switching printk to the original method of
immediate printing to console if oops is in progress.

Async printk, for the time being, is considered to be less reliable
than the synchronous one, so by default we keep printk operating in
synchronous mode. There is a printk.synchronous kernel parameter which
permits to select sync/async mode as a boot parameter or later on
from user space via sysfs knob.

printk() is expected to work under different conditions and in different
scenarios, including corner cases of OOM when all of the workers are busy
(e.g. allocating memory), thus printk() uses its own dedicated printing
kthread, rather than relying on workqueue (even with WQ_MEM_RECLAIM bit
set we potentially can receive delays in printing until workqueue
declares a ->mayday, as noted by Tetsuo Handa).

Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 Documentation/kernel-parameters.txt | 12 +++++
 kernel/printk/printk.c              | 99 ++++++++++++++++++++++++++++++++++---
 2 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 55990e4..0c3bee3 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3135,6 +3135,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 	printk.time=	Show timing data prefixed to each printk message line
 			Format: <bool>  (1/Y/y=enable, 0/N/n=disable)
 
+	printk.synchronous=
+			This parameter controls whether kernel messages must be
+			printed to console synchronously or asynchronously.
+			Asynchronous printing avoids kernel stalling behind
+			slow serial console and thus avoids softlockups,
+			interrupt timeouts, or userspace timing out during
+			heavy printing. printk switches back to synchronous mode
+			during early boot or when oops is happening. For the
+			time being, synchronous mode considered to be more
+			reliable, besides, for debugging, printing messages to
+			console immediately (synchronous mode) is desirable.
+
 	processor.max_cstate=	[HW,ACPI]
 			Limit processor to maximum C-state
 			max_cstate=9 overrides any DMI blacklist limit.
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index bfbf284..64a98ea 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -46,6 +46,8 @@
 #include <linux/utsname.h>
 #include <linux/ctype.h>
 #include <linux/uio.h>
+#include <linux/kthread.h>
+#include <linux/sched/rt.h>
 
 #include <asm/uaccess.h>
 #include <asm-generic/sections.h>
@@ -284,6 +286,16 @@ static char __log_buf[__LOG_BUF_LEN] __aligned(LOG_ALIGN);
 static char *log_buf = __log_buf;
 static u32 log_buf_len = __LOG_BUF_LEN;
 
+/* Control whether printing to console must be synchronous. */
+static bool __read_mostly printk_sync = true;
+module_param_named(synchronous, printk_sync, bool, S_IRUGO);
+MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
+
+/* Printing kthread for async printk */
+static struct task_struct *printk_kthread;
+/* When `true' printing thread has messages to print */
+static bool printk_kthread_need_flush_console;
+
 /* Return log buffer address */
 char *log_buf_addr_get(void)
 {
@@ -1608,6 +1620,8 @@ asmlinkage int vprintk_emit(int facility, int level,
 			    const char *dict, size_t dictlen,
 			    const char *fmt, va_list args)
 {
+	/* cpu currently holding logbuf_lock in this function */
+	static unsigned int logbuf_cpu = UINT_MAX;
 	static bool recursion_bug;
 	static char textbuf[LOG_LINE_MAX];
 	char *text = textbuf;
@@ -1617,8 +1631,6 @@ asmlinkage int vprintk_emit(int facility, int level,
 	int this_cpu;
 	int printed_len = 0;
 	bool in_sched = false;
-	/* cpu currently holding logbuf_lock in this function */
-	static unsigned int logbuf_cpu = UINT_MAX;
 
 	if (level == LOGLEVEL_SCHED) {
 		level = LOGLEVEL_DEFAULT;
@@ -1757,12 +1769,35 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (!in_sched) {
 		lockdep_off();
 		/*
-		 * Try to acquire and then immediately release the console
-		 * semaphore.  The release will print out buffers and wake up
-		 * /dev/kmsg and syslog() users.
+		 * Attempt to print the messages to console asynchronously so
+		 * that the kernel doesn't get stalled due to slow serial
+		 * console. That can lead to softlockups, lost interrupts, or
+		 * userspace timing out under heavy printing load.
+		 *
+		 * However we resort to synchronous printing of messages during
+		 * early boot, when synchronous printing was explicitly
+		 * requested by a kernel parameter, or when console_verbose()
+		 * was called to print everything during panic / oops.
+		 * Unlike bust_spinlocks() and oops_in_progress,
+		 * console_verbose() sets console_loglevel to MOTORMOUTH and
+		 * never clears it, while oops_in_progress can go back to 0,
+		 * switching printk back to async mode; we want printk to
+		 * operate in sync mode once panic() occurred.
 		 */
-		if (console_trylock())
-			console_unlock();
+		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
+				printk_kthread) {
+			/* Offload printing to a schedulable context. */
+			printk_kthread_need_flush_console = true;
+			wake_up_process(printk_kthread);
+		} else {
+			/*
+			 * Try to acquire and then immediately release the
+			 * console semaphore.  The release will print out
+			 * buffers and wake up /dev/kmsg and syslog() users.
+			 */
+			if (console_trylock())
+				console_unlock();
+		}
 		lockdep_on();
 	}
 
@@ -2722,6 +2757,56 @@ static int __init printk_late_init(void)
 late_initcall(printk_late_init);
 
 #if defined CONFIG_PRINTK
+static int printk_kthread_func(void *data)
+{
+	while (1) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (!printk_kthread_need_flush_console)
+			schedule();
+
+		__set_current_state(TASK_RUNNING);
+		/*
+		 * Avoid an infinite loop when console_unlock() cannot
+		 * access consoles, e.g. because console_suspended is
+		 * true. schedule(), someone else will print the messages
+		 * from resume_console().
+		 */
+		printk_kthread_need_flush_console = false;
+
+		console_lock();
+		console_unlock();
+	}
+
+	return 0;
+}
+
+/*
+ * Init async printk via late_initcall, after core/arch/device/etc.
+ * initialization.
+ */
+static int __init init_printk_kthread(void)
+{
+	struct task_struct *thread;
+	struct sched_param param = {
+		.sched_priority = MAX_RT_PRIO - 1,
+	};
+
+	if (printk_sync)
+		return 0;
+
+	thread = kthread_run(printk_kthread_func, NULL, "printk");
+	if (IS_ERR(thread)) {
+		pr_err("printk: unable to create printing thread\n");
+		printk_sync = true;
+		return PTR_ERR(thread);
+	}
+
+	sched_setscheduler(thread, SCHED_FIFO, &param);
+	printk_kthread = thread;
+	return 0;
+}
+late_initcall(init_printk_kthread);
+
 /*
  * Delayed printk version, for scheduler-internal messages:
  */
-- 
2.8.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v12 2/3] printk: Make wake_up_klogd_work_func() async
  2016-04-22 13:52 [PATCH v12 0/3] printk: Make printk() completely async Sergey Senozhatsky
  2016-04-22 13:52 ` [PATCH v12 1/3] " Sergey Senozhatsky
@ 2016-04-22 13:52 ` Sergey Senozhatsky
  2016-04-22 13:53 ` [PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky
  2 siblings, 0 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-04-22 13:52 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jan Kara, Petr Mladek, Tejun Heo, Tetsuo Handa, linux-kernel,
	Byungchul Park, Sergey Senozhatsky, Sergey Senozhatsky, Jan Kara

From: Jan Kara <jack@suse.cz>

Offload printing of scheduler deferred messages from IRQ context
to a schedulable printing kthread, when possible (the same way we
do it in vprintk_emit()). Otherwise, the CPU can spend unbounded
amount of time doing printing in console_unlock() from IRQ.

Signed-off-by: Jan Kara <jack@suse.cz>
Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 64a98ea..89f5441 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2820,9 +2820,16 @@ static void wake_up_klogd_work_func(struct irq_work *irq_work)
 	int pending = __this_cpu_xchg(printk_pending, 0);
 
 	if (pending & PRINTK_PENDING_OUTPUT) {
-		/* If trylock fails, someone else is doing the printing */
-		if (console_trylock())
-			console_unlock();
+		if (printk_kthread) {
+			wake_up_process(printk_kthread);
+		} else {
+			/*
+			 * If trylock fails, someone else is doing
+			 * the printing
+			 */
+			if (console_trylock())
+				console_unlock();
+		}
 	}
 
 	if (pending & PRINTK_PENDING_WAKEUP)
-- 
2.8.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH v12 3/3] printk: make printk.synchronous param rw
  2016-04-22 13:52 [PATCH v12 0/3] printk: Make printk() completely async Sergey Senozhatsky
  2016-04-22 13:52 ` [PATCH v12 1/3] " Sergey Senozhatsky
  2016-04-22 13:52 ` [PATCH v12 2/3] printk: Make wake_up_klogd_work_func() async Sergey Senozhatsky
@ 2016-04-22 13:53 ` Sergey Senozhatsky
  2016-04-23  6:56   ` Jan Kara
  2016-04-23 15:01   ` [PATCH] " Sergey Senozhatsky
  2 siblings, 2 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-04-22 13:53 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jan Kara, Petr Mladek, Tejun Heo, Tetsuo Handa, linux-kernel,
	Byungchul Park, Sergey Senozhatsky, Sergey Senozhatsky

Change `synchronous' printk param to be RW, so user space
can change printk mode back and forth to/from sync mode
(which is considered to be more reliable).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
---
 kernel/printk/printk.c | 51 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 40 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 89f5441..61e639a 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -288,9 +288,6 @@ static u32 log_buf_len = __LOG_BUF_LEN;
 
 /* Control whether printing to console must be synchronous. */
 static bool __read_mostly printk_sync = true;
-module_param_named(synchronous, printk_sync, bool, S_IRUGO);
-MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
-
 /* Printing kthread for async printk */
 static struct task_struct *printk_kthread;
 /* When `true' printing thread has messages to print */
@@ -1785,7 +1782,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 		 * operate in sync mode once panic() occurred.
 		 */
 		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
-				printk_kthread) {
+				!printk_sync && printk_kthread) {
 			/* Offload printing to a schedulable context. */
 			printk_kthread_need_flush_console = true;
 			wake_up_process(printk_kthread);
@@ -2757,6 +2754,13 @@ static int __init printk_late_init(void)
 late_initcall(printk_late_init);
 
 #if defined CONFIG_PRINTK
+/*
+ * Prevent starting printk_kthread from start_kernel()->parse_args().
+ * It's not possible at this stage. Instead, do it via the inticall
+ * or a sysfs knob.
+ */
+static bool printk_kthread_can_run;
+
 static int printk_kthread_func(void *data)
 {
 	while (1) {
@@ -2780,18 +2784,14 @@ static int printk_kthread_func(void *data)
 	return 0;
 }
 
-/*
- * Init async printk via late_initcall, after core/arch/device/etc.
- * initialization.
- */
-static int __init init_printk_kthread(void)
+static int __init_printk_kthread(void)
 {
 	struct task_struct *thread;
 	struct sched_param param = {
 		.sched_priority = MAX_RT_PRIO - 1,
 	};
 
-	if (printk_sync)
+	if (!printk_kthread_can_run || printk_sync || printk_kthread)
 		return 0;
 
 	thread = kthread_run(printk_kthread_func, NULL, "printk");
@@ -2805,6 +2805,35 @@ static int __init init_printk_kthread(void)
 	printk_kthread = thread;
 	return 0;
 }
+
+static int printk_sync_set(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+
+	ret = param_set_bool(val, kp);
+	if (ret)
+		return ret;
+	return __init_printk_kthread();
+}
+
+static const struct kernel_param_ops param_ops_printk_sync = {
+	.set = printk_sync_set,
+	.get = param_get_bool,
+};
+
+module_param_cb(synchronous, &param_ops_printk_sync, &printk_sync,
+		S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
+
+/*
+ * Init async printk via late_initcall, after core/arch/etc.
+ * initialization.
+ */
+static __init int init_printk_kthread(void)
+{
+	printk_kthread_can_run = true;
+	return __init_printk_kthread();
+}
 late_initcall(init_printk_kthread);
 
 /*
@@ -2820,7 +2849,7 @@ static void wake_up_klogd_work_func(struct irq_work *irq_work)
 	int pending = __this_cpu_xchg(printk_pending, 0);
 
 	if (pending & PRINTK_PENDING_OUTPUT) {
-		if (printk_kthread) {
+		if (!printk_sync && printk_kthread) {
 			wake_up_process(printk_kthread);
 		} else {
 			/*
-- 
2.8.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 3/3] printk: make printk.synchronous param rw
  2016-04-22 13:53 ` [PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky
@ 2016-04-23  6:56   ` Jan Kara
  2016-04-23  8:26     ` Sergey Senozhatsky
  2016-04-23 15:01   ` [PATCH] " Sergey Senozhatsky
  1 sibling, 1 reply; 11+ messages in thread
From: Jan Kara @ 2016-04-23  6:56 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Andrew Morton, Jan Kara, Petr Mladek, Tejun Heo, Tetsuo Handa,
	linux-kernel, Byungchul Park, Sergey Senozhatsky

On Fri 22-04-16 22:53:00, Sergey Senozhatsky wrote:
> Change `synchronous' printk param to be RW, so user space
> can change printk mode back and forth to/from sync mode
> (which is considered to be more reliable).
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

The patch looks good to me. One suggestion below:

> @@ -1785,7 +1782,7 @@ asmlinkage int vprintk_emit(int facility, int level,
>  		 * operate in sync mode once panic() occurred.
>  		 */
>  		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
> -				printk_kthread) {
> +				!printk_sync && printk_kthread) {
>  			/* Offload printing to a schedulable context. */
>  			printk_kthread_need_flush_console = true;
>  			wake_up_process(printk_kthread);

It would seem more future-proof to hide '!printk_sync && printk_kthread'
into a wrapper function as it is somewhat subtle detail that printk_kthread
needn't exist while !printk_sync and I can imagine someone forgetting to
check that in the future. Something like 'can_print_async()'? But I don't
feel too strongly about that so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

regardless whether you change this or not.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 3/3] printk: make printk.synchronous param rw
  2016-04-23  6:56   ` Jan Kara
@ 2016-04-23  8:26     ` Sergey Senozhatsky
  0 siblings, 0 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-04-23  8:26 UTC (permalink / raw)
  To: Jan Kara
  Cc: Sergey Senozhatsky, Andrew Morton, Jan Kara, Petr Mladek,
	Tejun Heo, Tetsuo Handa, linux-kernel, Byungchul Park,
	Sergey Senozhatsky

Hello,

On (04/23/16 08:56), Jan Kara wrote:
> > 
> > Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> 
> The patch looks good to me. One suggestion below:
> 
> > @@ -1785,7 +1782,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> >  		 * operate in sync mode once panic() occurred.
> >  		 */
> >  		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
> > -				printk_kthread) {
> > +				!printk_sync && printk_kthread) {
> >  			/* Offload printing to a schedulable context. */
> >  			printk_kthread_need_flush_console = true;
> >  			wake_up_process(printk_kthread);
> 
> It would seem more future-proof to hide '!printk_sync && printk_kthread'
> into a wrapper function as it is somewhat subtle detail that printk_kthread
> needn't exist while !printk_sync and I can imagine someone forgetting to
> check that in the future. Something like 'can_print_async()'? But I don't
> feel too strongly about that so feel free to add:

hm, yes. this is what I eventually do in "yet to be posted"
make-console_unlock()-async patch. I move printing kthread
wakeup-s and those async printing checks out of vprintk_emit()
and wake_up_klogd_work_func() to a special function:

static bool console_unlock_async_flush(void)
{
...
       if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
                       !printk_sync && printk_kthread) {
               /* Offload printing to a schedulable context. */
               printk_kthread_need_flush_console = true;
               console_locked = 0;
               up_console_sem();
               wake_up_process(printk_kthread);
               return true;
       }
       return false;
}


so async_printk flags live in one place (which makes it easier
to maintain) and vprintk_emit()/wake_up_klogd_work_func() simply
do:

	if (console_trylock())
		console_unlock();


console_unlock() is the one who decides if it can do async
printk or a 'direct printing' via console_flush_and_unlock().

void console_unlock(void)
{
       if (console_unlock_async_flush())
               return;
       console_flush_and_unlock();
}


console_flush_and_unlock() is what was previously known
as console_unlock() - emit the messages and call_console_drivers().


I guess I can send out an updated version of 0003 as a reply
to the initial patch and hide '!printk_sync && printk_kthread'.


> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> regardless whether you change this or not.

thanks.

	-ss

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] printk: make printk.synchronous param rw
  2016-04-22 13:53 ` [PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky
  2016-04-23  6:56   ` Jan Kara
@ 2016-04-23 15:01   ` Sergey Senozhatsky
  2016-04-26 14:15     ` Petr Mladek
  1 sibling, 1 reply; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-04-23 15:01 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jan Kara, Petr Mladek, Tejun Heo, Tetsuo Handa, linux-kernel,
	Byungchul Park, Sergey Senozhatsky, Sergey Senozhatsky

Change `synchronous' printk param to be RW, so user space
can change printk mode back and forth to/from sync mode
(which is considered to be more reliable).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Reviewed-by: Jan Kara <jack@suse.cz>
---

-- added Jan's Reviewed-by
-- factored out async printk checks to can_printk_async()

 kernel/printk/printk.c | 56 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 45 insertions(+), 11 deletions(-)

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 89f5441..9345a29 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -288,14 +288,16 @@ static u32 log_buf_len = __LOG_BUF_LEN;
 
 /* Control whether printing to console must be synchronous. */
 static bool __read_mostly printk_sync = true;
-module_param_named(synchronous, printk_sync, bool, S_IRUGO);
-MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
-
 /* Printing kthread for async printk */
 static struct task_struct *printk_kthread;
 /* When `true' printing thread has messages to print */
 static bool printk_kthread_need_flush_console;
 
+static inline bool can_printk_async(void)
+{
+	return !printk_sync && printk_kthread;
+}
+
 /* Return log buffer address */
 char *log_buf_addr_get(void)
 {
@@ -1785,7 +1787,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 		 * operate in sync mode once panic() occurred.
 		 */
 		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
-				printk_kthread) {
+				can_printk_async()) {
 			/* Offload printing to a schedulable context. */
 			printk_kthread_need_flush_console = true;
 			wake_up_process(printk_kthread);
@@ -2757,6 +2759,13 @@ static int __init printk_late_init(void)
 late_initcall(printk_late_init);
 
 #if defined CONFIG_PRINTK
+/*
+ * Prevent starting printk_kthread from start_kernel()->parse_args().
+ * It's not possible at this stage. Instead, do it via the inticall
+ * or a sysfs knob.
+ */
+static bool printk_kthread_can_run;
+
 static int printk_kthread_func(void *data)
 {
 	while (1) {
@@ -2780,18 +2789,14 @@ static int printk_kthread_func(void *data)
 	return 0;
 }
 
-/*
- * Init async printk via late_initcall, after core/arch/device/etc.
- * initialization.
- */
-static int __init init_printk_kthread(void)
+static int __init_printk_kthread(void)
 {
 	struct task_struct *thread;
 	struct sched_param param = {
 		.sched_priority = MAX_RT_PRIO - 1,
 	};
 
-	if (printk_sync)
+	if (!printk_kthread_can_run || printk_sync || printk_kthread)
 		return 0;
 
 	thread = kthread_run(printk_kthread_func, NULL, "printk");
@@ -2805,6 +2810,35 @@ static int __init init_printk_kthread(void)
 	printk_kthread = thread;
 	return 0;
 }
+
+static int printk_sync_set(const char *val, const struct kernel_param *kp)
+{
+	int ret;
+
+	ret = param_set_bool(val, kp);
+	if (ret)
+		return ret;
+	return __init_printk_kthread();
+}
+
+static const struct kernel_param_ops param_ops_printk_sync = {
+	.set = printk_sync_set,
+	.get = param_get_bool,
+};
+
+module_param_cb(synchronous, &param_ops_printk_sync, &printk_sync,
+		S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
+
+/*
+ * Init async printk via late_initcall, after core/arch/etc.
+ * initialization.
+ */
+static __init int init_printk_kthread(void)
+{
+	printk_kthread_can_run = true;
+	return __init_printk_kthread();
+}
 late_initcall(init_printk_kthread);
 
 /*
@@ -2820,7 +2854,7 @@ static void wake_up_klogd_work_func(struct irq_work *irq_work)
 	int pending = __this_cpu_xchg(printk_pending, 0);
 
 	if (pending & PRINTK_PENDING_OUTPUT) {
-		if (printk_kthread) {
+		if (can_printk_async()) {
 			wake_up_process(printk_kthread);
 		} else {
 			/*
-- 
2.8.0

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH v12 1/3] printk: Make printk() completely async
  2016-04-22 13:52 ` [PATCH v12 1/3] " Sergey Senozhatsky
@ 2016-04-26 14:13   ` Petr Mladek
  0 siblings, 0 replies; 11+ messages in thread
From: Petr Mladek @ 2016-04-26 14:13 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Andrew Morton, Jan Kara, Tejun Heo, Tetsuo Handa, linux-kernel,
	Byungchul Park, Sergey Senozhatsky, Jan Kara

On Fri 2016-04-22 22:52:58, Sergey Senozhatsky wrote:
> This patch makes printk() completely asynchronous (similar to what
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] printk: make printk.synchronous param rw
  2016-04-23 15:01   ` [PATCH] " Sergey Senozhatsky
@ 2016-04-26 14:15     ` Petr Mladek
  2016-04-27  1:09       ` Sergey Senozhatsky
  0 siblings, 1 reply; 11+ messages in thread
From: Petr Mladek @ 2016-04-26 14:15 UTC (permalink / raw)
  To: Sergey Senozhatsky
  Cc: Andrew Morton, Jan Kara, Tejun Heo, Tetsuo Handa, linux-kernel,
	Byungchul Park, Sergey Senozhatsky

On Sun 2016-04-24 00:01:05, Sergey Senozhatsky wrote:
> Change `synchronous' printk param to be RW, so user space
> can change printk mode back and forth to/from sync mode
> (which is considered to be more reliable).
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> ---
> 
> -- added Jan's Reviewed-by
> -- factored out async printk checks to can_printk_async()
> 
>  kernel/printk/printk.c | 56 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 89f5441..9345a29 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2757,6 +2759,13 @@ static int __init printk_late_init(void)
>  late_initcall(printk_late_init);
>  
>  #if defined CONFIG_PRINTK
> +/*
> + * Prevent starting printk_kthread from start_kernel()->parse_args().
> + * It's not possible at this stage. Instead, do it via the inticall
                                                              ^^^^^^^^

s/inticall/initcall/

Otherwise, it looks fine. I like the series and keep my fingers
crossed.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] printk: make printk.synchronous param rw
  2016-04-26 14:15     ` Petr Mladek
@ 2016-04-27  1:09       ` Sergey Senozhatsky
  0 siblings, 0 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-04-27  1:09 UTC (permalink / raw)
  To: Petr Mladek
  Cc: Sergey Senozhatsky, Andrew Morton, Jan Kara, Tejun Heo,
	Tetsuo Handa, linux-kernel, Byungchul Park, Sergey Senozhatsky

Hello,

On (04/26/16 16:15), Petr Mladek wrote:
[..]
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 89f5441..9345a29 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2757,6 +2759,13 @@ static int __init printk_late_init(void)
> >  late_initcall(printk_late_init);
> >  
> >  #if defined CONFIG_PRINTK
> > +/*
> > + * Prevent starting printk_kthread from start_kernel()->parse_args().
> > + * It's not possible at this stage. Instead, do it via the inticall
>                                                               ^^^^^^^^
> 
> s/inticall/initcall/

geez, english is hard   :(


> Otherwise, it looks fine. I like the series and keep my fingers
> crossed.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

thanks!

my system was massively Oopsing (due to kcompactd direct freepage
allocation, I suspect), OOM panicking, etc. during zsmalloc/zram
stress tests yesterday. and async printk worked fine for me. just
for note.

	-ss

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v12 1/3] printk: Make printk() completely async
  2016-05-13 13:18 [PATCH v12 0/3] printk: Make printk() completely async Sergey Senozhatsky
@ 2016-05-13 13:18 ` Sergey Senozhatsky
  0 siblings, 0 replies; 11+ messages in thread
From: Sergey Senozhatsky @ 2016-05-13 13:18 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Jan Kara, Petr Mladek, Tejun Heo, Tetsuo Handa, linux-kernel,
	Byungchul Park, Sergey Senozhatsky, Sergey Senozhatsky, Jan Kara

From: Jan Kara <jack@suse.cz>

Currently, printk() sometimes waits for message to be printed to console
and sometimes it does not (when console_sem is held by some other
process). In case printk() grabs console_sem and starts printing to
console, it prints messages from kernel printk buffer until the buffer
is empty. When serial console is attached, printing is slow and thus
other CPUs in the system have plenty of time to append new messages to
the buffer while one CPU is printing. Thus the CPU can spend unbounded
amount of time doing printing in console_unlock().  This is especially
serious problem if the printk() calling console_unlock() was called with
interrupts disabled.

In practice users have observed a CPU can spend tens of seconds printing
in console_unlock() (usually during boot when hundreds of SCSI devices
are discovered) resulting in RCU stalls (CPU doing printing doesn't
reach quiescent state for a long time), softlockup reports (IPIs for the
printing CPU don't get served and thus other CPUs are spinning waiting
for the printing CPU to process IPIs), and eventually a machine death
(as messages from stalls and lockups append to printk buffer faster than
we are able to print). So these machines are unable to boot with serial
console attached. Another observed issue is that due to slow printk,
hardware discovery is slow and udev times out before kernel manages to
discover all the attached HW. Also during artificial stress testing SATA
disk disappears from the system because its interrupts aren't served for
too long.

This patch makes printk() completely asynchronous (similar to what
printk_deferred() did until now). It appends message to the kernel
printk buffer and wake_up()s a special dedicated kthread to do the
printing to console. This has the advantage that printing always happens
from a schedulable contex and thus we don't lockup any particular CPU or
even interrupts. Also it has the advantage that printk() is fast and
thus kernel booting is not slowed down by slow serial console.
Disadvantage of this method is that in case of crash there is higher
chance that important messages won't appear in console output (we may
need working scheduling to print message to console). We somewhat
mitigate this risk by switching printk to the original method of
immediate printing to console if oops is in progress.

Async printk, for the time being, is considered to be less reliable
than the synchronous one, so by default we keep printk operating in
synchronous mode. There is a printk.synchronous kernel parameter which
permits to select sync/async mode as a boot parameter or later on
from user space via sysfs knob.

printk() is expected to work under different conditions and in different
scenarios, including corner cases of OOM when all of the workers are busy
(e.g. allocating memory), thus printk() uses its own dedicated printing
kthread, rather than relying on workqueue (even with WQ_MEM_RECLAIM bit
set we potentially can receive delays in printing until workqueue
declares a ->mayday, as noted by Tetsuo Handa).

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: Jan Kara <jack@suse.cz>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
 Documentation/kernel-parameters.txt | 12 +++++
 kernel/printk/printk.c              | 99 ++++++++++++++++++++++++++++++++++---
 2 files changed, 104 insertions(+), 7 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 5349363..19afb61 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3155,6 +3155,18 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				(0/N/n = disable, 1/Y/y = local clock,
 				2 = monotonic clock, 3 = real clock)
 
+	printk.synchronous=
+			This parameter controls whether kernel messages must be
+			printed to console synchronously or asynchronously.
+			Asynchronous printing avoids kernel stalling behind
+			slow serial console and thus avoids softlockups,
+			interrupt timeouts, or userspace timing out during
+			heavy printing. printk switches back to synchronous mode
+			during early boot or when oops is happening. For the
+			time being, synchronous mode considered to be more
+			reliable, besides, for debugging, printing messages to
+			console immediately (synchronous mode) is desirable.
+
 	processor.max_cstate=	[HW,ACPI]
 			Limit processor to maximum C-state
 			max_cstate=9 overrides any DMI blacklist limit.
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 0a0e789..c0e49ff 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -46,6 +46,8 @@
 #include <linux/utsname.h>
 #include <linux/ctype.h>
 #include <linux/uio.h>
+#include <linux/kthread.h>
+#include <linux/sched/rt.h>
 
 #include <asm/uaccess.h>
 #include <asm-generic/sections.h>
@@ -285,6 +287,16 @@ static char __log_buf[__LOG_BUF_LEN] __aligned(LOG_ALIGN);
 static char *log_buf = __log_buf;
 static u32 log_buf_len = __LOG_BUF_LEN;
 
+/* Control whether printing to console must be synchronous. */
+static bool __read_mostly printk_sync = true;
+module_param_named(synchronous, printk_sync, bool, S_IRUGO);
+MODULE_PARM_DESC(synchronous, "make printing to console synchronous");
+
+/* Printing kthread for async printk */
+static struct task_struct *printk_kthread;
+/* When `true' printing thread has messages to print */
+static bool printk_kthread_need_flush_console;
+
 /* Return log buffer address */
 char *log_buf_addr_get(void)
 {
@@ -1677,6 +1689,8 @@ asmlinkage int vprintk_emit(int facility, int level,
 			    const char *dict, size_t dictlen,
 			    const char *fmt, va_list args)
 {
+	/* cpu currently holding logbuf_lock in this function */
+	static unsigned int logbuf_cpu = UINT_MAX;
 	static bool recursion_bug;
 	static char textbuf[LOG_LINE_MAX];
 	char *text = textbuf;
@@ -1687,8 +1701,6 @@ asmlinkage int vprintk_emit(int facility, int level,
 	int printed_len = 0;
 	int nmi_message_lost;
 	bool in_sched = false;
-	/* cpu currently holding logbuf_lock in this function */
-	static unsigned int logbuf_cpu = UINT_MAX;
 
 	if (level == LOGLEVEL_SCHED) {
 		level = LOGLEVEL_DEFAULT;
@@ -1836,12 +1848,35 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (!in_sched) {
 		lockdep_off();
 		/*
-		 * Try to acquire and then immediately release the console
-		 * semaphore.  The release will print out buffers and wake up
-		 * /dev/kmsg and syslog() users.
+		 * Attempt to print the messages to console asynchronously so
+		 * that the kernel doesn't get stalled due to slow serial
+		 * console. That can lead to softlockups, lost interrupts, or
+		 * userspace timing out under heavy printing load.
+		 *
+		 * However we resort to synchronous printing of messages during
+		 * early boot, when synchronous printing was explicitly
+		 * requested by a kernel parameter, or when console_verbose()
+		 * was called to print everything during panic / oops.
+		 * Unlike bust_spinlocks() and oops_in_progress,
+		 * console_verbose() sets console_loglevel to MOTORMOUTH and
+		 * never clears it, while oops_in_progress can go back to 0,
+		 * switching printk back to async mode; we want printk to
+		 * operate in sync mode once panic() occurred.
 		 */
-		if (console_trylock())
-			console_unlock();
+		if (console_loglevel != CONSOLE_LOGLEVEL_MOTORMOUTH &&
+				printk_kthread) {
+			/* Offload printing to a schedulable context. */
+			printk_kthread_need_flush_console = true;
+			wake_up_process(printk_kthread);
+		} else {
+			/*
+			 * Try to acquire and then immediately release the
+			 * console semaphore.  The release will print out
+			 * buffers and wake up /dev/kmsg and syslog() users.
+			 */
+			if (console_trylock())
+				console_unlock();
+		}
 		lockdep_on();
 	}
 
@@ -2783,6 +2818,56 @@ static int __init printk_late_init(void)
 late_initcall(printk_late_init);
 
 #if defined CONFIG_PRINTK
+static int printk_kthread_func(void *data)
+{
+	while (1) {
+		set_current_state(TASK_INTERRUPTIBLE);
+		if (!printk_kthread_need_flush_console)
+			schedule();
+
+		__set_current_state(TASK_RUNNING);
+		/*
+		 * Avoid an infinite loop when console_unlock() cannot
+		 * access consoles, e.g. because console_suspended is
+		 * true. schedule(), someone else will print the messages
+		 * from resume_console().
+		 */
+		printk_kthread_need_flush_console = false;
+
+		console_lock();
+		console_unlock();
+	}
+
+	return 0;
+}
+
+/*
+ * Init async printk via late_initcall, after core/arch/device/etc.
+ * initialization.
+ */
+static int __init init_printk_kthread(void)
+{
+	struct task_struct *thread;
+	struct sched_param param = {
+		.sched_priority = MAX_RT_PRIO - 1,
+	};
+
+	if (printk_sync)
+		return 0;
+
+	thread = kthread_run(printk_kthread_func, NULL, "printk");
+	if (IS_ERR(thread)) {
+		pr_err("printk: unable to create printing thread\n");
+		printk_sync = true;
+		return PTR_ERR(thread);
+	}
+
+	sched_setscheduler(thread, SCHED_FIFO, &param);
+	printk_kthread = thread;
+	return 0;
+}
+late_initcall(init_printk_kthread);
+
 /*
  * Delayed printk version, for scheduler-internal messages:
  */
-- 
2.8.2.372.g63a3502

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2016-05-13 12:21 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-22 13:52 [PATCH v12 0/3] printk: Make printk() completely async Sergey Senozhatsky
2016-04-22 13:52 ` [PATCH v12 1/3] " Sergey Senozhatsky
2016-04-26 14:13   ` Petr Mladek
2016-04-22 13:52 ` [PATCH v12 2/3] printk: Make wake_up_klogd_work_func() async Sergey Senozhatsky
2016-04-22 13:53 ` [PATCH v12 3/3] printk: make printk.synchronous param rw Sergey Senozhatsky
2016-04-23  6:56   ` Jan Kara
2016-04-23  8:26     ` Sergey Senozhatsky
2016-04-23 15:01   ` [PATCH] " Sergey Senozhatsky
2016-04-26 14:15     ` Petr Mladek
2016-04-27  1:09       ` Sergey Senozhatsky
2016-05-13 13:18 [PATCH v12 0/3] printk: Make printk() completely async Sergey Senozhatsky
2016-05-13 13:18 ` [PATCH v12 1/3] " Sergey Senozhatsky

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.