* [PATCH printk v2 01/18] printk: Add function to replay kernel log on consoles
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-03 23:24 ` [PATCH printk v2 02/18] tty/sysrq: Replay kernel log messages on consoles via sysrq John Ogness
` (17 subsequent siblings)
18 siblings, 0 replies; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Sreenath Vijayan, Shimoyashiki Taichi, Greg Kroah-Hartman
From: Sreenath Vijayan <sreenath.vijayan@sony.com>
Add a generic function console_replay_all() for replaying
the kernel log on consoles, in any context. It would allow
viewing the logs on an unresponsive terminal via sysrq.
Reuse the existing code from console_flush_on_panic() for
resetting the sequence numbers, by introducing a new helper
function __console_rewind_all(). It is safe to be called
under console_lock().
Try to acquire lock on the console subsystem without waiting.
If successful, reset the sequence number to oldest available
record on all consoles and call console_unlock() which will
automatically flush the messages to the consoles.
Suggested-by: John Ogness <john.ogness@linutronix.de>
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Shimoyashiki Taichi <taichi.shimoyashiki@sony.com>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Sreenath Vijayan <sreenath.vijayan@sony.com>
Link: https://lore.kernel.org/r/90ee131c643a5033d117b556c0792de65129d4c3.1710220326.git.sreenath.vijayan@sony.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
include/linux/printk.h | 6 +++-
kernel/printk/printk.c | 77 +++++++++++++++++++++++++++++-------------
2 files changed, 58 insertions(+), 25 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 69f40a71c438..784d2298f4cf 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -195,6 +195,7 @@ void show_regs_print_info(const char *log_lvl);
extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
extern asmlinkage void dump_stack(void) __cold;
void printk_trigger_flush(void);
+void console_replay_all(void);
void printk_legacy_allow_panic_sync(void);
extern bool nbcon_device_try_acquire(struct console *con);
extern void nbcon_device_release(struct console *con);
@@ -279,6 +280,10 @@ static inline void printk_trigger_flush(void)
{
}
+static inline void console_replay_all(void)
+{
+}
+
static inline void printk_legacy_allow_panic_sync(void)
{
}
@@ -295,7 +300,6 @@ static inline void nbcon_device_release(struct console *con)
static inline void nbcon_atomic_flush_unsafe(void)
{
}
-
#endif
bool this_cpu_in_panic(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b4a202591e06..b3fe1b6d7dbd 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3201,6 +3201,40 @@ void console_unblank(void)
pr_flush(1000, true);
}
+/*
+ * Rewind all consoles to the oldest available record.
+ *
+ * IMPORTANT: The function is safe only when called under
+ * console_lock(). It is not enforced because
+ * it is used as a best effort in panic().
+ */
+static void __console_rewind_all(void)
+{
+ struct console *c;
+ short flags;
+ int cookie;
+ u64 seq;
+
+ seq = prb_first_valid_seq(prb);
+
+ cookie = console_srcu_read_lock();
+ for_each_console_srcu(c) {
+ flags = console_srcu_read_flags(c);
+
+ if (flags & CON_NBCON) {
+ nbcon_seq_force(c, seq);
+ } else {
+ /*
+ * This assignment is safe only when called under
+ * console_lock(). On panic, legacy consoles are
+ * only best effort.
+ */
+ c->seq = seq;
+ }
+ }
+ console_srcu_read_unlock(cookie);
+}
+
/**
* console_flush_on_panic - flush console content on panic
* @mode: flush all messages in buffer or just the pending ones
@@ -3229,30 +3263,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
*/
console_may_schedule = 0;
- if (mode == CONSOLE_REPLAY_ALL) {
- struct console *c;
- short flags;
- int cookie;
- u64 seq;
-
- seq = prb_first_valid_seq(prb);
-
- cookie = console_srcu_read_lock();
- for_each_console_srcu(c) {
- flags = console_srcu_read_flags(c);
-
- if (flags & CON_NBCON) {
- nbcon_seq_force(c, seq);
- } else {
- /*
- * This is an unsynchronized assignment. On
- * panic legacy consoles are only best effort.
- */
- c->seq = seq;
- }
- }
- console_srcu_read_unlock(cookie);
- }
+ if (mode == CONSOLE_REPLAY_ALL)
+ __console_rewind_all();
nbcon_atomic_flush_pending();
@@ -4447,6 +4459,23 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
}
EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
+/**
+ * console_replay_all - replay kernel log on consoles
+ *
+ * Try to obtain lock on console subsystem and replay all
+ * available records in printk buffer on the consoles.
+ * Does nothing if lock is not obtained.
+ *
+ * Context: Any context.
+ */
+void console_replay_all(void)
+{
+ if (console_trylock()) {
+ __console_rewind_all();
+ /* Consoles are flushed as part of console_unlock(). */
+ console_unlock();
+ }
+}
#endif
#ifdef CONFIG_SMP
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH printk v2 02/18] tty/sysrq: Replay kernel log messages on consoles via sysrq
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
2024-06-03 23:24 ` [PATCH printk v2 01/18] printk: Add function to replay kernel log on consoles John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-03 23:24 ` [PATCH printk v2 03/18] printk: Rename console_replay_all() and update context John Ogness
` (16 subsequent siblings)
18 siblings, 0 replies; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Sreenath Vijayan, Shimoyashiki Taichi, Greg Kroah-Hartman
From: Sreenath Vijayan <sreenath.vijayan@sony.com>
When terminal is unresponsive, one cannot use dmesg to view
the printk ring buffer messages. Also, syslog services may be
disabled, especially on embedded systems, to check the messages
after a reboot. In this scenario, replay the messages in printk
ring buffer on consoles via sysrq by pressing sysrq+R.
The console loglevel will determine which all kernel log messages
are displayed. The messages will be displayed only when
console_trylock() succeeds. Users could repeat the sysrq key when
it fails. If the owner of console subsystem lock is stuck,
repeating the key won't work.
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Shimoyashiki Taichi <taichi.shimoyashiki@sony.com>
Reviewed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Sreenath Vijayan <sreenath.vijayan@sony.com>
Link: https://lore.kernel.org/r/cc3b9b1aae60a236c6aed1dc7b0ffa2c7cd1f183.1710220326.git.sreenath.vijayan@sony.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Documentation/admin-guide/sysrq.rst | 9 +++++++++
drivers/tty/sysrq.c | 13 ++++++++++++-
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/sysrq.rst b/Documentation/admin-guide/sysrq.rst
index 2f2e5bd440f9..a85b3384d1e7 100644
--- a/Documentation/admin-guide/sysrq.rst
+++ b/Documentation/admin-guide/sysrq.rst
@@ -161,6 +161,8 @@ Command Function
will be printed to your console. (``0``, for example would make
it so that only emergency messages like PANICs or OOPSes would
make it to your console.)
+
+``R`` Replay the kernel log messages on consoles.
=========== ===================================================================
Okay, so what can I use them for?
@@ -211,6 +213,13 @@ processes.
"just thaw ``it(j)``" is useful if your system becomes unresponsive due to a
frozen (probably root) filesystem via the FIFREEZE ioctl.
+``Replay logs(R)`` is useful to view the kernel log messages when system is hung
+or you are not able to use dmesg command to view the messages in printk buffer.
+User may have to press the key combination multiple times if console system is
+busy. If it is completely locked up, then messages won't be printed. Output
+messages depend on current console loglevel, which can be modified using
+sysrq[0-9] (see above).
+
Sometimes SysRq seems to get 'stuck' after using it, what can I do?
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 02217e3c916b..e5974b8239c9 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -450,6 +450,17 @@ static const struct sysrq_key_op sysrq_unrt_op = {
.enable_mask = SYSRQ_ENABLE_RTNICE,
};
+static void sysrq_handle_replay_logs(u8 key)
+{
+ console_replay_all();
+}
+static struct sysrq_key_op sysrq_replay_logs_op = {
+ .handler = sysrq_handle_replay_logs,
+ .help_msg = "replay-kernel-logs(R)",
+ .action_msg = "Replay kernel logs on consoles",
+ .enable_mask = SYSRQ_ENABLE_DUMP,
+};
+
/* Key Operations table and lock */
static DEFINE_SPINLOCK(sysrq_key_table_lock);
@@ -519,7 +530,7 @@ static const struct sysrq_key_op *sysrq_key_table[62] = {
NULL, /* O */
NULL, /* P */
NULL, /* Q */
- NULL, /* R */
+ &sysrq_replay_logs_op, /* R */
NULL, /* S */
NULL, /* T */
NULL, /* U */
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH printk v2 03/18] printk: Rename console_replay_all() and update context
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
2024-06-03 23:24 ` [PATCH printk v2 01/18] printk: Add function to replay kernel log on consoles John Ogness
2024-06-03 23:24 ` [PATCH printk v2 02/18] tty/sysrq: Replay kernel log messages on consoles via sysrq John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-03 23:24 ` [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads John Ogness
` (15 subsequent siblings)
18 siblings, 0 replies; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Sreenath Vijayan, Shimoyashiki Taichi
From: Sreenath Vijayan <sreenath.vijayan@sony.com>
Rename console_replay_all() to console_try_replay_all() to make
clear that the implementation is best effort. Also, the function
should not be called in NMI context as it takes locks, so update
the comment in code.
Fixes: 693f75b91a91 ("printk: Add function to replay kernel log on consoles")
Fixes: 1b743485e27f ("tty/sysrq: Replay kernel log messages on consoles via sysrq")
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Shimoyashiki Taichi <taichi.shimoyashiki@sony.com>
Signed-off-by: Sreenath Vijayan <sreenath.vijayan@sony.com>
Link: https://lore.kernel.org/r/Zlguq/wU21Z8MqI4@sreenath.vijayan@sony.com
Reviewed-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
drivers/tty/sysrq.c | 2 +-
include/linux/printk.h | 4 ++--
kernel/printk/printk.c | 6 +++---
3 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index e5974b8239c9..53f8c2329c30 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -452,7 +452,7 @@ static const struct sysrq_key_op sysrq_unrt_op = {
static void sysrq_handle_replay_logs(u8 key)
{
- console_replay_all();
+ console_try_replay_all();
}
static struct sysrq_key_op sysrq_replay_logs_op = {
.handler = sysrq_handle_replay_logs,
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 784d2298f4cf..20e1942f63ae 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -195,7 +195,7 @@ void show_regs_print_info(const char *log_lvl);
extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
extern asmlinkage void dump_stack(void) __cold;
void printk_trigger_flush(void);
-void console_replay_all(void);
+void console_try_replay_all(void);
void printk_legacy_allow_panic_sync(void);
extern bool nbcon_device_try_acquire(struct console *con);
extern void nbcon_device_release(struct console *con);
@@ -280,7 +280,7 @@ static inline void printk_trigger_flush(void)
{
}
-static inline void console_replay_all(void)
+static inline void console_try_replay_all(void)
{
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b3fe1b6d7dbd..5dcc05e1aa56 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -4460,15 +4460,15 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
/**
- * console_replay_all - replay kernel log on consoles
+ * console_try_replay_all - try to replay kernel log on consoles
*
* Try to obtain lock on console subsystem and replay all
* available records in printk buffer on the consoles.
* Does nothing if lock is not obtained.
*
- * Context: Any context.
+ * Context: Any, except for NMI.
*/
-void console_replay_all(void)
+void console_try_replay_all(void)
{
if (console_trylock()) {
__console_rewind_all();
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (2 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 03/18] printk: Rename console_replay_all() and update context John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-07 13:17 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 05/18] printk: Atomic print in printk context on shutdown John Ogness
` (14 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
From: Thomas Gleixner <tglx@linutronix.de>
Provide the main implementation for running a printer kthread
per nbcon console that is takeover/handover aware.
Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
include/linux/console.h | 26 ++++++
kernel/printk/internal.h | 26 ++++++
kernel/printk/nbcon.c | 196 +++++++++++++++++++++++++++++++++++++--
kernel/printk/printk.c | 34 +++++++
4 files changed, 275 insertions(+), 7 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 4aaf053840ee..4de42ec1527c 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -17,6 +17,7 @@
#include <linux/atomic.h>
#include <linux/bits.h>
#include <linux/rculist.h>
+#include <linux/rcuwait.h>
#include <linux/types.h>
#include <linux/vesa.h>
@@ -324,6 +325,8 @@ struct nbcon_write_context {
* @nbcon_seq: Sequence number of the next record for nbcon to print
* @nbcon_device_ctxt: Context available for non-printing operations
* @pbufs: Pointer to nbcon private buffer
+ * @kthread: Printer kthread for this console
+ * @rcuwait: RCU-safe wait object for @kthread waking
*/
struct console {
char name[16];
@@ -373,6 +376,27 @@ struct console {
*/
void (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
+ /**
+ * @write_thread:
+ *
+ * NBCON callback to write out text in task context.
+ *
+ * This callback is called after device_lock() and with the nbcon
+ * console acquired. Any necessary driver synchronization should have
+ * been performed by the device_lock() callback.
+ *
+ * This callback is always called from task context but with migration
+ * disabled.
+ *
+ * The same criteria for console ownership verification and unsafe
+ * sections applies as with write_atomic(). The difference between
+ * this callback and write_atomic() is that this callback is used
+ * during normal operation and is always called from task context.
+ * This allows drivers to operate in their own locking context for
+ * synchronizing output to the hardware.
+ */
+ void (*write_thread)(struct console *con, struct nbcon_write_context *wctxt);
+
/**
* @device_lock:
*
@@ -420,6 +444,8 @@ struct console {
atomic_long_t __private nbcon_seq;
struct nbcon_context __private nbcon_device_ctxt;
struct printk_buffers *pbufs;
+ struct task_struct *kthread;
+ struct rcuwait rcuwait;
};
#ifdef CONFIG_LOCKDEP
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 0439cf2fdc22..38680c6b2b39 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -92,6 +92,7 @@ enum nbcon_prio nbcon_get_default_prio(void);
void nbcon_atomic_flush_pending(void);
bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
int cookie);
+void nbcon_kthread_create(struct console *con);
/*
* Check if the given console is currently capable and allowed to print
@@ -110,6 +111,8 @@ static inline bool console_is_usable(struct console *con, short flags)
if (flags & CON_NBCON) {
if (!con->write_atomic)
return false;
+ if (!con->write_thread)
+ return false;
} else {
if (!con->write)
return false;
@@ -126,12 +129,35 @@ static inline bool console_is_usable(struct console *con, short flags)
return true;
}
+/**
+ * nbcon_kthread_wake - Wake up a printk thread
+ * @con: Console to operate on
+ */
+static inline void nbcon_kthread_wake(struct console *con)
+{
+ /*
+ * Guarantee any new records can be seen by tasks preparing to wait
+ * before this context checks if the rcuwait is empty.
+ *
+ * The full memory barrier in rcuwait_wake_up() pairs with the full
+ * memory barrier within set_current_state() of
+ * ___rcuwait_wait_event(), which is called after prepare_to_rcuwait()
+ * adds the waiter but before it has checked the wait condition.
+ *
+ * This pairs with nbcon_kthread_func:A.
+ */
+ rcuwait_wake_up(&con->rcuwait); /* LMM(nbcon_kthread_wake:A) */
+}
+
#else
#define PRINTK_PREFIX_MAX 0
#define PRINTK_MESSAGE_MAX 0
#define PRINTKRB_RECORD_MAX 0
+static inline void nbcon_kthread_wake(struct console *con) { }
+static inline void nbcon_kthread_create(struct console *con) { }
+
/*
* In !PRINTK builds we still export console_sem
* semaphore and some of console functions (console_unlock()/etc.), so
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 6e9e24aa0a7f..89b340ca303c 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -10,6 +10,7 @@
#include <linux/export.h>
#include <linux/init.h>
#include <linux/irqflags.h>
+#include <linux/kthread.h>
#include <linux/minmax.h>
#include <linux/percpu.h>
#include <linux/preempt.h>
@@ -837,6 +838,7 @@ EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
/**
* nbcon_emit_next_record - Emit a record in the acquired context
* @wctxt: The write context that will be handed to the write function
+ * @use_atomic: True if the write_atomic() callback is to be used
*
* Return: True if this context still owns the console. False if
* ownership was handed over or taken.
@@ -850,7 +852,7 @@ EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
* When true is returned, @wctxt->ctxt.backlog indicates whether there are
* still records pending in the ringbuffer,
*/
-static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
+static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_atomic)
{
struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
struct console *con = ctxt->console;
@@ -899,8 +901,14 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
nbcon_state_read(con, &cur);
wctxt->unsafe_takeover = cur.unsafe_takeover;
- if (con->write_atomic) {
+ if (use_atomic &&
+ con->write_atomic) {
con->write_atomic(con, wctxt);
+
+ } else if (!use_atomic &&
+ con->write_thread) {
+ con->write_thread(con, wctxt);
+
} else {
/*
* This function should never be called for legacy consoles.
@@ -936,6 +944,120 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
return nbcon_context_exit_unsafe(ctxt);
}
+/**
+ * nbcon_kthread_should_wakeup - Check whether a printer thread should wakeup
+ * @con: Console to operate on
+ * @ctxt: The nbcon context from nbcon_context_try_acquire()
+ *
+ * Return: True if the thread should shutdown or if the console is
+ * allowed to print and a record is available. False otherwise.
+ *
+ * After the thread wakes up, it must first check if it should shutdown before
+ * attempting any printing.
+ */
+static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_context *ctxt)
+{
+ bool ret = false;
+ short flags;
+ int cookie;
+
+ if (kthread_should_stop())
+ return true;
+
+ cookie = console_srcu_read_lock();
+
+ flags = console_srcu_read_flags(con);
+ if (console_is_usable(con, flags)) {
+ /* Bring the sequence in @ctxt up to date */
+ ctxt->seq = nbcon_seq_read(con);
+
+ ret = prb_read_valid(prb, ctxt->seq, NULL);
+ }
+
+ console_srcu_read_unlock(cookie);
+ return ret;
+}
+
+/**
+ * nbcon_kthread_func - The printer thread function
+ * @__console: Console to operate on
+ *
+ * Return: 0
+ */
+static int nbcon_kthread_func(void *__console)
+{
+ struct console *con = __console;
+ struct nbcon_write_context wctxt = {
+ .ctxt.console = con,
+ .ctxt.prio = NBCON_PRIO_NORMAL,
+ };
+ struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
+ short con_flags;
+ bool backlog;
+ int cookie;
+ int ret;
+
+wait_for_event:
+ /*
+ * Guarantee this task is visible on the rcuwait before
+ * checking the wake condition.
+ *
+ * The full memory barrier within set_current_state() of
+ * ___rcuwait_wait_event() pairs with the full memory
+ * barrier within rcuwait_has_sleeper().
+ *
+ * This pairs with rcuwait_has_sleeper:A and nbcon_kthread_wake:A.
+ */
+ ret = rcuwait_wait_event(&con->rcuwait,
+ nbcon_kthread_should_wakeup(con, ctxt),
+ TASK_INTERRUPTIBLE); /* LMM(nbcon_kthread_func:A) */
+
+ if (kthread_should_stop())
+ return 0;
+
+ /* Wait was interrupted by a spurious signal, go back to sleep. */
+ if (ret)
+ goto wait_for_event;
+
+ do {
+ backlog = false;
+
+ cookie = console_srcu_read_lock();
+
+ con_flags = console_srcu_read_flags(con);
+
+ if (console_is_usable(con, con_flags)) {
+ unsigned long lock_flags;
+
+ con->device_lock(con, &lock_flags);
+
+ /*
+ * Ensure this stays on the CPU to make handover and
+ * takeover possible.
+ */
+ cant_migrate();
+
+ if (nbcon_context_try_acquire(ctxt)) {
+ /*
+ * If the emit fails, this context is no
+ * longer the owner.
+ */
+ if (nbcon_emit_next_record(&wctxt, false)) {
+ nbcon_context_release(ctxt);
+ backlog = ctxt->backlog;
+ }
+ }
+
+ con->device_unlock(con, lock_flags);
+ }
+
+ console_srcu_read_unlock(cookie);
+
+ } while (backlog);
+
+ goto wait_for_event;
+}
+
/* Track the nbcon emergency nesting per CPU. */
static DEFINE_PER_CPU(unsigned int, nbcon_pcpu_emergency_nesting);
static unsigned int early_nbcon_pcpu_emergency_nesting __initdata;
@@ -1012,7 +1134,7 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
* The higher priority printing context takes over responsibility
* to print the pending records.
*/
- if (!nbcon_emit_next_record(wctxt))
+ if (!nbcon_emit_next_record(wctxt, true))
return false;
nbcon_context_release(ctxt);
@@ -1113,7 +1235,7 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq,
* handed over or taken over. In both cases the context is no
* longer valid.
*/
- if (!nbcon_emit_next_record(&wctxt))
+ if (!nbcon_emit_next_record(&wctxt, true))
return -EAGAIN;
if (!ctxt->backlog) {
@@ -1172,10 +1294,10 @@ static void nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq,
/*
* If flushing was successful but more records are available, this
- * context must flush those remaining records because there is no
- * other context that will do it.
+ * context must flush those remaining records if the printer thread
+ * is not available do it.
*/
- if (prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
+ if (!con->kthread && prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
stop_seq = prb_next_reserve_seq(prb);
goto again;
}
@@ -1332,6 +1454,63 @@ void nbcon_cpu_emergency_flush(void)
}
}
+/*
+ * nbcon_kthread_stop - Stop a printer thread
+ * @con: Console to operate on
+ */
+static void nbcon_kthread_stop(struct console *con)
+{
+ lockdep_assert_console_list_lock_held();
+
+ if (!con->kthread)
+ return;
+
+ kthread_stop(con->kthread);
+ con->kthread = NULL;
+}
+
+/**
+ * nbcon_kthread_create - Create a printer thread
+ * @con: Console to operate on
+ *
+ * If it fails, let the console proceed. The atomic part might
+ * be usable and useful.
+ */
+void nbcon_kthread_create(struct console *con)
+{
+ struct task_struct *kt;
+
+ lockdep_assert_console_list_lock_held();
+
+ if (!(con->flags & CON_NBCON) || !con->write_thread)
+ return;
+
+ if (con->kthread)
+ return;
+
+ /*
+ * Printer threads cannot be started as long as any boot console is
+ * registered because there is no way to synchronize the hardware
+ * registers between boot console code and regular console code.
+ */
+ if (have_boot_console)
+ return;
+
+ kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name, con->index);
+ if (IS_ERR(kt)) {
+ con_printk(KERN_ERR, con, "failed to start printing thread\n");
+ return;
+ }
+
+ con->kthread = kt;
+
+ /*
+ * It is important that console printing threads are scheduled
+ * shortly after a printk call and with generous runtime budgets.
+ */
+ sched_set_normal(con->kthread, -20);
+}
+
/**
* nbcon_alloc - Allocate buffers needed by the nbcon console
* @con: Console to allocate buffers for
@@ -1377,6 +1556,7 @@ void nbcon_init(struct console *con, u64 init_seq)
/* nbcon_alloc() must have been called and successful! */
BUG_ON(!con->pbufs);
+ rcuwait_init(&con->rcuwait);
nbcon_seq_force(con, init_seq);
nbcon_state_set(con, &state);
}
@@ -1389,6 +1569,7 @@ void nbcon_free(struct console *con)
{
struct nbcon_state state = { };
+ nbcon_kthread_stop(con);
nbcon_state_set(con, &state);
/* Boot consoles share global printk buffers. */
@@ -1458,6 +1639,7 @@ void nbcon_device_release(struct console *con)
*/
cookie = console_srcu_read_lock();
if (console_is_usable(con, console_srcu_read_flags(con)) &&
+ !con->kthread &&
prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
__nbcon_atomic_flush_pending_con(con, prb_next_reserve_seq(prb), false);
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 5dcc05e1aa56..137bd9a721c4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2685,6 +2685,8 @@ void suspend_console(void)
void resume_console(void)
{
struct console *con;
+ short flags;
+ int cookie;
if (!console_suspend_enabled)
return;
@@ -2701,6 +2703,14 @@ void resume_console(void)
*/
synchronize_srcu(&console_srcu);
+ cookie = console_srcu_read_lock();
+ for_each_console_srcu(con) {
+ flags = console_srcu_read_flags(con);
+ if (flags & CON_NBCON)
+ nbcon_kthread_wake(con);
+ }
+ console_srcu_read_unlock(cookie);
+
pr_flush(1000, true);
}
@@ -3021,6 +3031,13 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
u64 printk_seq;
bool progress;
+ /*
+ * console_flush_all() is only for legacy consoles,
+ * unless the nbcon console has no kthread printer.
+ */
+ if ((flags & CON_NBCON) && con->kthread)
+ continue;
+
if (!console_is_usable(con, flags))
continue;
any_usable = true;
@@ -3326,9 +3343,26 @@ EXPORT_SYMBOL(console_stop);
void console_start(struct console *console)
{
+ short flags;
+ int cookie;
+
console_list_lock();
console_srcu_write_flags(console, console->flags | CON_ENABLED);
console_list_unlock();
+
+ /*
+ * Ensure that all SRCU list walks have completed. The related
+ * printing context must be able to see it is enabled so that
+ * it is guaranteed to wake up and resume printing.
+ */
+ synchronize_srcu(&console_srcu);
+
+ cookie = console_srcu_read_lock();
+ flags = console_srcu_read_flags(console);
+ if (flags & CON_NBCON)
+ nbcon_kthread_wake(console);
+ console_srcu_read_unlock(cookie);
+
__pr_flush(console, 1000, true);
}
EXPORT_SYMBOL(console_start);
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-03 23:24 ` [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads John Ogness
@ 2024-06-07 13:17 ` Petr Mladek
2024-06-10 12:09 ` John Ogness
0 siblings, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-06-07 13:17 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Tue 2024-06-04 01:30:39, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Provide the main implementation for running a printer kthread
> per nbcon console that is takeover/handover aware.
The changes done by this patch deserve a more detailed commit message ;-)
I updated this commit message in parallel with commenting the related
code changes. My later comment might explain the motivation for
writing the commit message this way.
<proposal>
Provide the main implementation for running a printer kthread per
nbcon console that is takeover/handover aware.
Once the kthread is running and available, it gets responsible for
flushing any pending messages which were added in NBCON_PRIO_NORMAL
context. Namely the legacy console_flush_all() and device_release()
do not longer flush the console. And nbcon_atomic_flush_pending()
used by nbcon_cpu_emergency_exit() does not longer flush messages
added after the emergency ones.
The kthread uses a new write_thread() callback with both device_lock()
and acquired the console context.
The console ownership handling is necessary for synchronization against
write_atomic() which is synchronized only via the console context
ownership.
The device_lock() serializes acquiring the console context with
NBCON_PRIO_NORMAL. It is needed when the device_lock() does not
disable preemption. It prevents the following race:
CPU0 CPU1
[ task A ]
nbcon_context_try_acquire()
# success with NORMAL prio
# .unsafe == false; // safe for takeover
[ schedule: task A -> B ]
WARN_ON()
nbcon_atomic_flush_pending()
nbcon_context_try_acquire()
# success with EMERGENCY prio
# flushing
nbcon_context_release()
# HERE: con->nbcon_state is free
# to take by anyone !!!
nbcon_context_try_acquire()
# success with NORMAL prio [ task B ]
[ schedule: task B -> A ]
nbcon_enter_unsafe()
nbcon_context_can_proceed()
BUG: nbcon_context_can_proceed() returns "true" because
the console is owned by a context on CPU0 with
NBCON_PRIO_NORMAL.
But it should return "false". The console is owned
by a context from task B and we do the check
in a context from task A.
The write context in the kthread will stay safe only when either of
the following conditions are true:
1. The kthread is the only context which acquires the console
with NBCON_PRIO_NORMAL.
2. Other caller acquires the console context with NBCON_PRIO_NORMAL
under the device_lock.
3. Other caller acquires the console context with NBCON_PRIO_NORMAL
with disabled preemption. It will release the context before
rescheduling.
It is even double guaranteed. First, __nbcon_atomic_flush_pending_con()
is called:
+ with disabled interrupts from nbcon_atomic_flush_pending_con()
+ under device_lock() from nbcon_device_release()
Second, they are not longer called with NBCON_PRIO_NORMAL when
con->kthread exists.
Also note that the kthreads are running with nice -20 so that they are
scheduled shortly after a printk call and with generous runtime
budgets.
This patch provides just the basic functionality. The followup changes
will add the necessary integration, for example, creating the kthreads
at the right time or waking them when a new message appear.
</proposal>
BTW: It really looks like the safety is double guaranteed. Maybe
the con->device_lock() is not needed in nbcon_kthread_func()
after all. Well, I would keep it to be on the safe side.
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -373,6 +376,27 @@ struct console {
> */
> void (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
>
> + /**
> + * @write_thread:
> + *
> + * NBCON callback to write out text in task context.
> + *
> + * This callback is called after device_lock() and with the nbcon
> + * console acquired. Any necessary driver synchronization should have
> + * been performed by the device_lock() callback.
> + *
> + * This callback is always called from task context but with migration
> + * disabled.
> + *
> + * The same criteria for console ownership verification and unsafe
> + * sections applies as with write_atomic(). The difference between
> + * this callback and write_atomic() is that this callback is used
> + * during normal operation and is always called from task context.
> + * This allows drivers to operate in their own locking context for
> + * synchronizing output to the hardware.
> + */
The description is not bad. It seems to include all the important
information. Except than I still needed to scratch my head around it
to understand the real purpose and rules.
Namely, the following sentences are kind of vague and bring questions
which I had in the past before I understood all the relations:
1. "Any necessary driver synchronization should have been performed
by the device_lock() callback."
Q: Why do we need to take both device_lock() and the context then?
Q: Why the acquired context is not enough?
2. "This allows drivers to operate in their own locking context for
synchronizing output to the hardware."
Q: What exactly does this sentence mean?
Q: What is the driver?
Q: What are the output callbacks?
Q: How exactly is this related to write_atomic() and write_thread()?
Q: Is the context more strict or relaxed, in which way?
Also I still keep in my mind that nbcon_context_try_acquire() is not
safe with NBCON_NORMAL_PRIO with enabled preemption. There is
the race discussed in the previous patchset, see
https://lore.kernel.org/r/ZiD3FNBZh_iMOVWY@pathway.suse.cz
I wonder if we could be more strigthforward.
<my-take>
/**
* @write_thread:
*
* NBCON callback to write out text in task context.
*
* This callback must be called only in task context with both
* device_lock() and the nbcon console acquired.
*
* The same rules for console ownership verification and unsafe
* sections handling applies as with write_atomic().
*
* The console ownership handling is necessary for synchronization
* against write_atomic() which is synchronized only via the context.
*
* The device_lock() serializes acquiring the console context
* with NBCON_PRIO_NORMAL. It is necessary when the device_lock()
* does not disable preemption. The console context locking is
* not able to detect a race in the following scenario:
*
* 1. [Task A] owns a context with NBCON_PRIO_NORMAL on [CPU X]
* and is scheduled.
*
* 2. Another context takes over the lock with NBCON_PRIO_EMERGENCY
* and releases it.
*
* 3. [Task B] acquires a context with NBCON_PRIO_NORMAL on [CPU X]
* and is scheduled.
*
* 4. [Task A] gets running on [CPU X] and see that the console is
* is still owned by a task on [CPU X] with NBON_PRIO_NORMAL.
* It can't detect that it is actually owned by another task.
*/
</my-take>
My variant describes the purpose of device_lock() quite different way.
But I think that this is the real purpose and we must be clear about
it.
Sigh, I was not able to describe the race reasonably a shorter
way. Maybe, we should move this detailed explanation above,
nbcon_context_try_acquire() and use just a reference here.
> + void (*write_thread)(struct console *con, struct nbcon_write_context *wctxt);
> +
> /**
> * @device_lock:
> *
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -936,6 +944,120 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
> return nbcon_context_exit_unsafe(ctxt);
> }
>
> +/**
> + * nbcon_kthread_should_wakeup - Check whether a printer thread should wakeup
> + * @con: Console to operate on
> + * @ctxt: The nbcon context from nbcon_context_try_acquire()
> + *
> + * Return: True if the thread should shutdown or if the console is
> + * allowed to print and a record is available. False otherwise.
> + *
> + * After the thread wakes up, it must first check if it should shutdown before
> + * attempting any printing.
> + */
> +static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_context *ctxt)
> +{
> + bool ret = false;
> + short flags;
> + int cookie;
> +
> + if (kthread_should_stop())
> + return true;
> +
> + cookie = console_srcu_read_lock();
> +
> + flags = console_srcu_read_flags(con);
> + if (console_is_usable(con, flags)) {
> + /* Bring the sequence in @ctxt up to date */
> + ctxt->seq = nbcon_seq_read(con);
> +
> + ret = prb_read_valid(prb, ctxt->seq, NULL);
> + }
> +
> + console_srcu_read_unlock(cookie);
> + return ret;
> +}
> +
> +/**
> + * nbcon_kthread_func - The printer thread function
> + * @__console: Console to operate on
> + *
> + * Return: 0
> + */
> +static int nbcon_kthread_func(void *__console)
> +{
> + struct console *con = __console;
> + struct nbcon_write_context wctxt = {
> + .ctxt.console = con,
> + .ctxt.prio = NBCON_PRIO_NORMAL,
> + };
> + struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
> + short con_flags;
> + bool backlog;
> + int cookie;
> + int ret;
> +
> +wait_for_event:
> + /*
> + * Guarantee this task is visible on the rcuwait before
> + * checking the wake condition.
> + *
> + * The full memory barrier within set_current_state() of
> + * ___rcuwait_wait_event() pairs with the full memory
> + * barrier within rcuwait_has_sleeper().
> + *
> + * This pairs with rcuwait_has_sleeper:A and nbcon_kthread_wake:A.
> + */
> + ret = rcuwait_wait_event(&con->rcuwait,
> + nbcon_kthread_should_wakeup(con, ctxt),
> + TASK_INTERRUPTIBLE); /* LMM(nbcon_kthread_func:A) */
> +
> + if (kthread_should_stop())
> + return 0;
> +
> + /* Wait was interrupted by a spurious signal, go back to sleep. */
> + if (ret)
> + goto wait_for_event;
> +
> + do {
> + backlog = false;
> +
I am not sure how obvious is that we take the lock around the entire
operation. In principle, the console could not disappear. So it might
look like it is not really necessary. But it actually plays important
role when stopping/suspending the console. I would add a comment:
/*
* Keep the read lock around the entire operation so that
* synchronize_srcu() could prove that the kthread stopped
* or suspended printing.
*/
> + cookie = console_srcu_read_lock();
> +
> + con_flags = console_srcu_read_flags(con);
> +
> + if (console_is_usable(con, con_flags)) {
> + unsigned long lock_flags;
> +
> + con->device_lock(con, &lock_flags);
> +
> + /*
> + * Ensure this stays on the CPU to make handover and
> + * takeover possible.
> + */
> + cant_migrate();
> +
> + if (nbcon_context_try_acquire(ctxt)) {
> + /*
> + * If the emit fails, this context is no
> + * longer the owner.
> + */
> + if (nbcon_emit_next_record(&wctxt, false)) {
> + nbcon_context_release(ctxt);
> + backlog = ctxt->backlog;
> + }
> + }
> +
> + con->device_unlock(con, lock_flags);
> + }
> +
> + console_srcu_read_unlock(cookie);
> +
> + } while (backlog);
Thinking loudly:
We do not check kthread_should_stop() in the cycle. It means that it
would flush all messages before stopping the kthread. But only
when it is already in the cycle. It would not flush the messages
in the following scenario:
CPU0 CPU1
printk("Unregistering console\n");
nbcon_wake_threads();
irq_work_queue(&con->irq_work);
kthread_stop(con->thread);
nbcon_kthread_func()
rcuwait_wait_event()
nbcon_kthread_should_wakeup()
if (kthread_should_stop())
// true
return 0
Result: The kthread did not flush the pending messages in this case.
I do not have strong opinion. Well, it would make sense to flush
all messages before stopping the kthread,
Maybe, we should move kthread_should_stop() check here.
Maybe, we should move the flushing cycle into a separate function
and the kthread might look like:
do {
rcuwait_wait_event(&con->rcuwait,
nbcon_kthread_should_wakeup(con, ctxt),
TASK_INTERRUPTIBLE); /* LMM(nbcon_kthread_func:A) */
nbcon_kthread_flush();
while (!kthread_should_stop);
Note that I did not add the check for the spurious signal. It looks
just like an optimization. IMHO, it is not worth the code churn.
Also the spurious signal does not mean that there are no
pending messages. And if we want to flush everything before exiting...
Anyway, we probably should document the (desired) behavior in the function
description.
> +
> + goto wait_for_event;
> +}
> +
> /* Track the nbcon emergency nesting per CPU. */
> static DEFINE_PER_CPU(unsigned int, nbcon_pcpu_emergency_nesting);
> static unsigned int early_nbcon_pcpu_emergency_nesting __initdata;
> @@ -1332,6 +1454,63 @@ void nbcon_cpu_emergency_flush(void)
> }
> }
>
> +/*
> + * nbcon_kthread_stop - Stop a printer thread
> + * @con: Console to operate on
> + */
> +static void nbcon_kthread_stop(struct console *con)
> +{
> + lockdep_assert_console_list_lock_held();
> +
> + if (!con->kthread)
> + return;
> +
> + kthread_stop(con->kthread);
> + con->kthread = NULL;
> +}
> +
> +/**
> + * nbcon_kthread_create - Create a printer thread
> + * @con: Console to operate on
> + *
> + * If it fails, let the console proceed. The atomic part might
> + * be usable and useful.
> + */
> +void nbcon_kthread_create(struct console *con)
> +{
> + struct task_struct *kt;
> +
> + lockdep_assert_console_list_lock_held();
> +
> + if (!(con->flags & CON_NBCON) || !con->write_thread)
> + return;
> +
> + if (con->kthread)
> + return;
> +
> + /*
> + * Printer threads cannot be started as long as any boot console is
> + * registered because there is no way to synchronize the hardware
> + * registers between boot console code and regular console code.
> + */
> + if (have_boot_console)
> + return;
> +
> + kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name, con->index);
> + if (IS_ERR(kt)) {
> + con_printk(KERN_ERR, con, "failed to start printing thread\n");
> + return;
I am a bit surprised that we ignore the error here.
Hmm, it likely works in this patch because the legacy code would still
flush the console when con->thread is not assigned.
But it would stop working later when the legacy loop is disabled
by the global @printk_threads_enabled variable.
> + }
> +
Thinking loudly:
The kthread is running and started processing messages at this moment.
But con->kthraed is not set yet.
All this is done under @console_list_lock. It only guarantees that
the struct console would not disappear while some code access it under
console_srcu_read_lock().
The value is used in console_flush_all() and in nbcon_device_release()
to decide whether to flush the console directly or not.
I first though that console_flush_all() and nbcon_device_release()
could flush the messages using write_atomic() in parallel with
the kthread and it might create the race with NBCON_PRIO_NORMAL
context ownership.
But both calls will ignore this console until it is added into
@console_list which is done later under con->device_lock().
Not to say that:
+ console_flush_all() acquires the nbcon context with interrupts
disabled => race prevented
+ nbcon_device_release() acquires the nbcon context under device
lock => serialized with the kthread
So we are on the safe side (double guaranteed). Well, it took me quite
some time to realize this. It might be worth a comment.
<proposal>
/*
* Some users check con->kthread to decide whether to flush
* the messages directly using con->write_atomic(). Note that
* possible races with the kthread are double prevented.
*
* First, the users ignore this console until it is added into
* @console_list which is done under con->device_lock().
* Second, the calls would be additionaly serialized by acquiring
* the console context.
*/
</proposal>
> + con->kthread = kt;
> +
> + /*
> + * It is important that console printing threads are scheduled
> + * shortly after a printk call and with generous runtime budgets.
> + */
> + sched_set_normal(con->kthread, -20);
I would prefer to move this into a separate patch. I feel that it
might be kind of controversial and subject to a change in the future.
We should not hide this "tiny" detail in this mega patch ;-)
That said, the low "nice" value makes perfect sense to me.
And I makes me feel comfortable.
> +}
> +
> /**
> * nbcon_alloc - Allocate buffers needed by the nbcon console
> * @con: Console to allocate buffers for
> @@ -1458,6 +1639,7 @@ void nbcon_device_release(struct console *con)
> */
> cookie = console_srcu_read_lock();
> if (console_is_usable(con, console_srcu_read_flags(con)) &&
> + !con->kthread &&
This would deserve updating the comment like in nbcon_atomic_flush_pending_con().
> prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
> __nbcon_atomic_flush_pending_con(con, prb_next_reserve_seq(prb), false);
> }
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-07 13:17 ` Petr Mladek
@ 2024-06-10 12:09 ` John Ogness
2024-06-11 14:51 ` Petr Mladek
0 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-10 12:09 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2024-06-07, Petr Mladek <pmladek@suse.com> wrote:
> I updated this commit message in parallel with commenting the related
> code changes. My later comment might explain the motivation for
> writing the commit message this way.
I am OK with your proposed commit message. Thank you for taking the time
to fill in all the details.
> <proposal>
[...]
> The write context in the kthread will stay safe only when either of
> the following conditions are true:
>
> 1. The kthread is the only context which acquires the console
> with NBCON_PRIO_NORMAL.
>
> 2. Other caller acquires the console context with NBCON_PRIO_NORMAL
> under the device_lock.
>
> 3. Other caller acquires the console context with NBCON_PRIO_NORMAL
> with disabled preemption. It will release the context before
> rescheduling.
>
> It is even double guaranteed. First, __nbcon_atomic_flush_pending_con()
> is called:
>
> + with disabled interrupts from nbcon_atomic_flush_pending_con()
> + under device_lock() from nbcon_device_release()
[...]
> </proposal>
>
> BTW: It really looks like the safety is double guaranteed. Maybe
> the con->device_lock() is not needed in nbcon_kthread_func()
> after all. Well, I would keep it to be on the safe side.
For the threaded case it is technically correct to say the safety is
double guaranteed. But the outer safety (device_lock()) can provide a
preemptible lock, which is the key for the non-interference property of
the threaded printers.
For example, for an nbcon framebuffer console, device_lock() most likely
will be a mutex.
During normal operation, the inner safety (console context) will never
be contended. That really only exists to synchronize the atomic case.
>> --- a/include/linux/console.h
>> +++ b/include/linux/console.h
>> @@ -373,6 +376,27 @@ struct console {
>> */
>> void (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
>>
>> + /**
>> + * @write_thread:
>> + *
>> + * NBCON callback to write out text in task context.
>> + *
>> + * This callback is called after device_lock() and with the nbcon
>> + * console acquired. Any necessary driver synchronization should have
>> + * been performed by the device_lock() callback.
>> + *
>> + * This callback is always called from task context but with migration
>> + * disabled.
>> + *
>> + * The same criteria for console ownership verification and unsafe
>> + * sections applies as with write_atomic(). The difference between
>> + * this callback and write_atomic() is that this callback is used
>> + * during normal operation and is always called from task context.
>> + * This allows drivers to operate in their own locking context for
>> + * synchronizing output to the hardware.
>> + */
>
> The description is not bad. It seems to include all the important
> information. Except than I still needed to scratch my head around it
> to understand the real purpose and rules.
>
> Namely, the following sentences are kind of vague and bring questions
> which I had in the past before I understood all the relations:
>
> 1. "Any necessary driver synchronization should have been performed
> by the device_lock() callback."
>
> Q: Why do we need to take both device_lock() and the context then?
The device_lock() enables the non-interference property during normal
operation. OTOH the context is taken to synchronize against non-normal
(emergency/panic) operation.
> Q: Why the acquired context is not enough?
Because the context _requires_ disabling preemption during all driver
activity, which is a major source of interference to the system.
If you only care about UART drivers and !PREEMPT_RT, then there really
is not much difference between spinning on a spinlock vs. spinning on a
context. But for non-UART drivers or PREEMPT_RT, there is a huge
difference between blocking on device_lock() with preemption enabled vs.
spinning on a context.
I think it really helps to see the importance if you assume
device_lock() is a mutex. (All the printk code must also make this
assumption.)
> 2. "This allows drivers to operate in their own locking context for
> synchronizing output to the hardware."
>
> Q: What exactly does this sentence mean?
For example, if a console driver uses a mutex to synchronize hardware
access, it can implement this callback knowing it is under that
mutex. This also applies for any locking mechanism the driver might use.
> Q: What is the driver?
The code that implements _all_ access to the hardware for _all_
purposes.
> Q: What are the output callbacks?
I am not referring to callbacks. I am referring to any code in the
driver that is accessing the parts of the hardware that are related to
outputting kernel messages.
> Q: How exactly is this related to write_atomic() and write_thread()?
Both nbcon console write callbacks must synchronize against other
activities of the driver. For write_thread(), it is primarily via
device_lock().
> Q: Is the context more strict or relaxed, in which way?
If device_lock() is using a locking mechanism that does not disable
preemption, write_thread() is quite a bit more relaxed than the disabled
preemption context of write_atomic().
> Also I still keep in my mind that nbcon_context_try_acquire() is not
> safe with NBCON_NORMAL_PRIO with enabled preemption. There is
> the race discussed in the previous patchset, see
> https://lore.kernel.org/r/ZiD3FNBZh_iMOVWY@pathway.suse.cz
>
> I wonder if we could be more strigthforward.
>
> <my-take>
> /**
> * @write_thread:
> *
> * NBCON callback to write out text in task context.
> *
> * This callback must be called only in task context with both
> * device_lock() and the nbcon console acquired.
> *
> * The same rules for console ownership verification and unsafe
> * sections handling applies as with write_atomic().
> *
> * The console ownership handling is necessary for synchronization
> * against write_atomic() which is synchronized only via the context.
> *
> * The device_lock() serializes acquiring the console context
> * with NBCON_PRIO_NORMAL. It is necessary when the device_lock()
> * does not disable preemption. The console context locking is
> * not able to detect a race in the following scenario:
> *
> * 1. [Task A] owns a context with NBCON_PRIO_NORMAL on [CPU X]
> * and is scheduled.
> *
> * 2. Another context takes over the lock with NBCON_PRIO_EMERGENCY
> * and releases it.
> *
> * 3. [Task B] acquires a context with NBCON_PRIO_NORMAL on [CPU X]
> * and is scheduled.
> *
> * 4. [Task A] gets running on [CPU X] and see that the console is
> * is still owned by a task on [CPU X] with NBON_PRIO_NORMAL.
> * It can't detect that it is actually owned by another task.
> */
> </my-take>
>
> My variant describes the purpose of device_lock() quite different way.
> But I think that this is the real purpose and we must be clear about
> it.
I would move that last part (starting with "The device_lock()
serializes...") into the kerneldoc for device_lock(). It really has
nothing to do with the write_thread() callback.
> Sigh, I was not able to describe the race reasonably a shorter
> way. Maybe, we should move this detailed explanation above,
> nbcon_context_try_acquire() and use just a reference here.
How about putting it as kerneldoc for nbcon_owner_matches() and refer to
it there from the device_lock() kerneldoc (or vice versa)?
For the write_thread() kerneldoc we could refer to the device_lock()
kerneldoc.
>> +/**
>> + * nbcon_kthread_func - The printer thread function
>> + * @__console: Console to operate on
>> + *
>> + * Return: 0
>> + */
>> +static int nbcon_kthread_func(void *__console)
>> +{
>> + struct console *con = __console;
>> + struct nbcon_write_context wctxt = {
>> + .ctxt.console = con,
>> + .ctxt.prio = NBCON_PRIO_NORMAL,
>> + };
>> + struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
>> + short con_flags;
>> + bool backlog;
>> + int cookie;
>> + int ret;
>> +
>> +wait_for_event:
>> + /*
>> + * Guarantee this task is visible on the rcuwait before
>> + * checking the wake condition.
>> + *
>> + * The full memory barrier within set_current_state() of
>> + * ___rcuwait_wait_event() pairs with the full memory
>> + * barrier within rcuwait_has_sleeper().
>> + *
>> + * This pairs with rcuwait_has_sleeper:A and nbcon_kthread_wake:A.
>> + */
>> + ret = rcuwait_wait_event(&con->rcuwait,
>> + nbcon_kthread_should_wakeup(con, ctxt),
>> + TASK_INTERRUPTIBLE); /* LMM(nbcon_kthread_func:A) */
>> +
>> + if (kthread_should_stop())
>> + return 0;
>> +
>> + /* Wait was interrupted by a spurious signal, go back to sleep. */
>> + if (ret)
>> + goto wait_for_event;
>> +
>> + do {
>> + backlog = false;
>> +
>
> I am not sure how obvious is that we take the lock around the entire
> operation. In principle, the console could not disappear. So it might
> look like it is not really necessary. But it actually plays important
> role when stopping/suspending the console. I would add a comment:
>
> /*
> * Keep the read lock around the entire operation so that
> * synchronize_srcu() could prove that the kthread stopped
> * or suspended printing.
> */
Agreed.
>> + cookie = console_srcu_read_lock();
>> +
>> + con_flags = console_srcu_read_flags(con);
>> +
>> + if (console_is_usable(con, con_flags)) {
>> + unsigned long lock_flags;
>> +
>> + con->device_lock(con, &lock_flags);
>> +
>> + /*
>> + * Ensure this stays on the CPU to make handover and
>> + * takeover possible.
>> + */
>> + cant_migrate();
>> +
>> + if (nbcon_context_try_acquire(ctxt)) {
>> + /*
>> + * If the emit fails, this context is no
>> + * longer the owner.
>> + */
>> + if (nbcon_emit_next_record(&wctxt, false)) {
>> + nbcon_context_release(ctxt);
>> + backlog = ctxt->backlog;
>> + }
>> + }
>> +
>> + con->device_unlock(con, lock_flags);
>> + }
>> +
>> + console_srcu_read_unlock(cookie);
>> +
>> + } while (backlog);
>
> Thinking loudly:
>
> We do not check kthread_should_stop() in the cycle. It means that it
> would flush all messages before stopping the kthread. But only
> when it is already in the cycle. It would not flush the messages
> in the following scenario:
>
> CPU0 CPU1
>
> printk("Unregistering console\n");
> nbcon_wake_threads();
> irq_work_queue(&con->irq_work);
>
> kthread_stop(con->thread);
>
> nbcon_kthread_func()
> rcuwait_wait_event()
> nbcon_kthread_should_wakeup()
>
> if (kthread_should_stop())
> // true
> return 0
>
> Result: The kthread did not flush the pending messages in this case.
Nice catch. How about if we add the following to
unregister_console_locked():
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f2ac7aaab234..fab69ca7f938 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3995,6 +3995,8 @@ static int unregister_console_locked(struct console *console)
if (res > 0)
return 0;
+ __pr_flush(console, 1000, true);
+
/* Disable it unconditionally */
console_srcu_write_flags(console, console->flags & ~CON_ENABLED);
>> + kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name, con->index);
>> + if (IS_ERR(kt)) {
>> + con_printk(KERN_ERR, con, "failed to start printing thread\n");
>> + return;
>
> I am a bit surprised that we ignore the error here.
>
>
> Hmm, it likely works in this patch because the legacy code would still
> flush the console when con->thread is not assigned.
>
> But it would stop working later when the legacy loop is disabled
> by the global @printk_threads_enabled variable.
The thread failing to start is a serious issue. Particularly for
PREEMPT_RT. Probably it should be something like:
if (WARN_ON(IS_ERR(kt))) {
In fact, I nbcon_atomic_flush_pending_con() and nbcon_device_release()
need to check @printk_threads_enabled _instead_ of con->kthread. Once
"threading mode" has been activated, there should be _no_ atomic
printing during normal operation.
> The kthread is running and started processing messages at this moment.
> But con->kthread is not set yet.
[...]
> It might be worth a comment.
>
> <proposal>
> /*
> * Some users check con->kthread to decide whether to flush
> * the messages directly using con->write_atomic(). Note that
> * possible races with the kthread are double prevented.
> *
> * First, the users ignore this console until it is added into
> * @console_list which is done under con->device_lock().
> * Second, the calls would be additionaly serialized by acquiring
> * the console context.
> */
> </proposal>
It is enough to mention only the calling context. How about:
/*
* Some users check con->kthread to decide whether to flush the
* messages directly using con->write_atomic(). A possible race
* with the kthread is prevented because all printing is
* serialized by acquiring the console context.
*/
>> + con->kthread = kt;
>> + /*
>> + * It is important that console printing threads are scheduled
>> + * shortly after a printk call and with generous runtime budgets.
>> + */
>> + sched_set_normal(con->kthread, -20);
>
> I would prefer to move this into a separate patch. I feel that it
> might be kind of controversial and subject to a change in the future.
> We should not hide this "tiny" detail in this mega patch ;-)
OK.
>> @@ -1458,6 +1639,7 @@ void nbcon_device_release(struct console *con)
>> */
>> cookie = console_srcu_read_lock();
>> if (console_is_usable(con, console_srcu_read_flags(con)) &&
>> + !con->kthread &&
>
> This would deserve updating the comment like in
> nbcon_atomic_flush_pending_con().
OK.
John
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-10 12:09 ` John Ogness
@ 2024-06-11 14:51 ` Petr Mladek
2024-06-12 8:51 ` John Ogness
0 siblings, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-06-11 14:51 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Mon 2024-06-10 14:15:10, John Ogness wrote:
> On 2024-06-07, Petr Mladek <pmladek@suse.com> wrote:
> > I updated this commit message in parallel with commenting the related
> > code changes. My later comment might explain the motivation for
> > writing the commit message this way.
>
> I am OK with your proposed commit message. Thank you for taking the time
> to fill in all the details.
We should probably update it a bit so that it describes the role
of the device_lock() more precisely. See the discussion below.
> > <proposal>
>
> [...]
>
> > The write context in the kthread will stay safe only when either of
> > the following conditions are true:
> >
> > 1. The kthread is the only context which acquires the console
> > with NBCON_PRIO_NORMAL.
> >
> > 2. Other caller acquires the console context with NBCON_PRIO_NORMAL
> > under the device_lock.
> >
> > 3. Other caller acquires the console context with NBCON_PRIO_NORMAL
> > with disabled preemption. It will release the context before
> > rescheduling.
> >
> > It is even double guaranteed. First, __nbcon_atomic_flush_pending_con()
> > is called:
> >
> > + with disabled interrupts from nbcon_atomic_flush_pending_con()
> > + under device_lock() from nbcon_device_release()
>
> [...]
>
> > </proposal>
> >
> > BTW: It really looks like the safety is double guaranteed. Maybe
> > the con->device_lock() is not needed in nbcon_kthread_func()
> > after all. Well, I would keep it to be on the safe side.
>
> For the threaded case it is technically correct to say the safety is
> double guaranteed. But the outer safety (device_lock()) can provide a
> preemptible lock, which is the key for the non-interference property of
> the threaded printers.
>
> For example, for an nbcon framebuffer console, device_lock() most likely
> will be a mutex.
>
> During normal operation, the inner safety (console context) will never
> be contended. That really only exists to synchronize the atomic case.
I see. For example, the spin_lock_irqsave() in __uart_port_lock_irqsave()
will be sleeping lock in RT => preserving RT guarantees. And it will
make sure that the inner nbcon_context_try_acquire() would never
have to spin => never break the RT guarantees.
It makes perfect sense. Do I get it correctly?
> >> --- a/include/linux/console.h
> >> +++ b/include/linux/console.h
> >> @@ -373,6 +376,27 @@ struct console {
> >> */
> >> void (*write_atomic)(struct console *con, struct nbcon_write_context *wctxt);
> >>
> >> + /**
> >> + * @write_thread:
> >> + *
> >> + * NBCON callback to write out text in task context.
> >> + *
> >> + * This callback is called after device_lock() and with the nbcon
> >> + * console acquired. Any necessary driver synchronization should have
> >> + * been performed by the device_lock() callback.
> >> + *
> >> + * This callback is always called from task context but with migration
> >> + * disabled.
> >> + *
> >> + * The same criteria for console ownership verification and unsafe
> >> + * sections applies as with write_atomic(). The difference between
> >> + * this callback and write_atomic() is that this callback is used
> >> + * during normal operation and is always called from task context.
> >> + * This allows drivers to operate in their own locking context for
> >> + * synchronizing output to the hardware.
> >> + */
> >
> > The description is not bad. It seems to include all the important
> > information. Except than I still needed to scratch my head around it
> > to understand the real purpose and rules.
> >
> > Namely, the following sentences are kind of vague and bring questions
> > which I had in the past before I understood all the relations:
> >
> > 1. "Any necessary driver synchronization should have been performed
> > by the device_lock() callback."
> >
> > Q: Why do we need to take both device_lock() and the context then?
>
> The device_lock() enables the non-interference property during normal
> operation. OTOH the context is taken to synchronize against non-normal
> (emergency/panic) operation.
>
> > Q: Why the acquired context is not enough?
>
> Because the context _requires_ disabling preemption during all driver
> activity, which is a major source of interference to the system.
>
> If you only care about UART drivers and !PREEMPT_RT, then there really
> is not much difference between spinning on a spinlock vs. spinning on a
> context. But for non-UART drivers or PREEMPT_RT, there is a huge
> difference between blocking on device_lock() with preemption enabled vs.
> spinning on a context.
This helped a lot. My brain is not trained to see these RT-specific
effects.
> I think it really helps to see the importance if you assume
> device_lock() is a mutex. (All the printk code must also make this
> assumption.)
From my POV, the game changer is the RT aspect. I knew that
for graphics console drivers the mutex was a nice-to-have thing.
I did not realize that for RT it was a must-to-have thing.
I mean, I did not realize that nbcon_console_try_acquire() was
not RT friendly. We could also say that nbcon_console_try_acquire()
is not acceptable for RT as the primary synchronization primitive.
> > I wonder if we could be more strigthforward.
> >
> > <my-take>
> > /**
> > * @write_thread:
> > *
> > * NBCON callback to write out text in task context.
> > *
> > * This callback must be called only in task context with both
> > * device_lock() and the nbcon console acquired.
> > *
> > * The same rules for console ownership verification and unsafe
> > * sections handling applies as with write_atomic().
> > *
> > * The console ownership handling is necessary for synchronization
> > * against write_atomic() which is synchronized only via the context.
> > *
> > * The device_lock() serializes acquiring the console context
> > * with NBCON_PRIO_NORMAL. It is necessary when the device_lock()
> > * does not disable preemption. The console context locking is
> > * not able to detect a race in the following scenario:
> > *
> > * 1. [Task A] owns a context with NBCON_PRIO_NORMAL on [CPU X]
> > * and is scheduled.
> > *
> > * 2. Another context takes over the lock with NBCON_PRIO_EMERGENCY
> > * and releases it.
> > *
> > * 3. [Task B] acquires a context with NBCON_PRIO_NORMAL on [CPU X]
> > * and is scheduled.
> > *
> > * 4. [Task A] gets running on [CPU X] and see that the console is
> > * is still owned by a task on [CPU X] with NBON_PRIO_NORMAL.
> > * It can't detect that it is actually owned by another task.
> > */
> > </my-take>
> >
> > My variant describes the purpose of device_lock() quite different way.
> > But I think that this is the real purpose and we must be clear about
> > it.
>
> I would move that last part (starting with "The device_lock()
> serializes...") into the kerneldoc for device_lock(). It really has
> nothing to do with the write_thread() callback.
I agree that describing the details of the race is not important here.
Well, I would still like to describe the role of device_lock()
for write_kthread(). It would help even me to create a better
mental model ;-)
What about the following?
<proposal-2>
/**
* @write_thread:
*
* NBCON callback to write out text in task context.
*
* This callback must be called only in task context with both
* device_lock() and the nbcon console acquired with
* NBCON_PRIO_NORMAL.
*
* The same rules for console ownership verification and unsafe
* sections handling applies as with write_atomic().
*
* The console ownership handling is necessary for synchronization
* against write_atomic() which is synchronized only via the context.
*
* The device_lock() provides the primary serialization for operations
* on the device. It might be as relaxed (mutex)[*] or as tight
* (disabled preemption and interrupts) as needed. It allows
* the kthread to operate in the least restrictive mode[**].
*
* [*] Standalone nbcon_context_try_acquire() is not safe with
* the preemption enabled, see nbcon_owner_matches(). But it
* can be safe when always called in the preemptive context
* under the device_lock().
*
* [**] The device_lock() makes sure that nbcon_context_try_acquire()
* would never need to spin which is important especially with
* PREEMPT_RT.
*/
</proposal-2>
> > Sigh, I was not able to describe the race reasonably a shorter
> > way. Maybe, we should move this detailed explanation above,
> > nbcon_context_try_acquire() and use just a reference here.
>
> How about putting it as kerneldoc for nbcon_owner_matches() and refer to
> it there from the device_lock() kerneldoc (or vice versa)?
Yup. nbcon_owner_matches() looks like the right place for details
about the possible race.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3995,6 +3995,8 @@ static int unregister_console_locked(struct console *console)
> if (res > 0)
> return 0;
>
> + __pr_flush(console, 1000, true);
> +
> /* Disable it unconditionally */
> console_srcu_write_flags(console, console->flags & ~CON_ENABLED);
Makes sense.
And it actually means that it is too late to flush messages
when kthread_should_stop() returns true. So, it does not matter if
we check it in the while(backlog) loop or not. Well, it might be
worth a comment in the code.
> >> + kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name, con->index);
> >> + if (IS_ERR(kt)) {
> >> + con_printk(KERN_ERR, con, "failed to start printing thread\n");
> >> + return;
> >
> > I am a bit surprised that we ignore the error here.
> >
> >
> > Hmm, it likely works in this patch because the legacy code would still
> > flush the console when con->thread is not assigned.
> >
> > But it would stop working later when the legacy loop is disabled
> > by the global @printk_threads_enabled variable.
>
> The thread failing to start is a serious issue. Particularly for
> PREEMPT_RT.
I agree.
> Probably it should be something like:
>
> if (WARN_ON(IS_ERR(kt))) {
Might make sense.
> In fact, I nbcon_atomic_flush_pending_con() and nbcon_device_release()
> need to check @printk_threads_enabled _instead_ of con->kthread. Once
> "threading mode" has been activated, there should be _no_ atomic
> printing during normal operation.
OK, we have two possibilities when a kthread could not get started:
1. printk() could fallback to flushing messages via write_atomic()
in the legacy console_unlock() loop.
It might break RT guarantees but people would see the messages.
2. The affected console would flush the messages only in emergency
or panic mode.
This would preserve RT guarantees. And printk() would still be
able to show emergency and panic messages, including the WARN_ON()
about that the printk kthread did not start.
It seems that you prefer the 2nd variant. But you are RT-biased ;-)
I would personally prefer the 1st variant. But I am printk-biased ;-)
Honestly, if the system is not able to start the kthread then
it is probably useless anyway. I would prefer if printk keeps working
so that people know what is going on ;-)
> > The kthread is running and started processing messages at this moment.
> > But con->kthread is not set yet.
>
> [...]
>
> > It might be worth a comment.
> >
> > <proposal>
> > /*
> > * Some users check con->kthread to decide whether to flush
> > * the messages directly using con->write_atomic(). Note that
> > * possible races with the kthread are double prevented.
> > *
> > * First, the users ignore this console until it is added into
> > * @console_list which is done under con->device_lock().
> > * Second, the calls would be additionaly serialized by acquiring
> > * the console context.
> > */
> > </proposal>
>
> It is enough to mention only the calling context. How about:
>
> /*
> * Some users check con->kthread to decide whether to flush the
> * messages directly using con->write_atomic(). A possible race
> * with the kthread is prevented because all printing is
> * serialized by acquiring the console context.
> */
I am not sure what is more important. In fact, all these users
check con->kthread only when the console is on @console_list.
So, maybe the note about #console_list is more useful.
BTW: uart_port_lock() allows access to the device and it acquires
the context only when the console appears on @console_list.
Well, the access is serialized by the device_lock.
> >> + con->kthread = kt;
After all, I would add two comments, like these:
<proposal-2>
/*
* Any access to the console device is serialized either by
* device_lock() or console context or both.
*/
kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name, con->index);
[...]
/*
* Some users check con->kthread to decide whether to flush
* the messages directly using con->write_atomic(). But they
* do so only when the console is already in @console_list.
*/
con->kthread = kt;
</proposal-2>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-11 14:51 ` Petr Mladek
@ 2024-06-12 8:51 ` John Ogness
2024-06-12 9:24 ` Petr Mladek
2024-06-13 15:21 ` John Ogness
0 siblings, 2 replies; 63+ messages in thread
From: John Ogness @ 2024-06-12 8:51 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2024-06-11, Petr Mladek <pmladek@suse.com> wrote:
>> During normal operation, the inner safety (console context) will never
>> be contended. That really only exists to synchronize the atomic case.
>
> I see. For example, the spin_lock_irqsave() in __uart_port_lock_irqsave()
> will be sleeping lock in RT => preserving RT guarantees. And it will
> make sure that the inner nbcon_context_try_acquire() would never
> have to spin => never break the RT guarantees.
>
> It makes perfect sense. Do I get it correctly?
Yes.
> Well, I would still like to describe the role of device_lock()
> for write_kthread(). It would help even me to create a better
> mental model ;-)
>
> What about the following?
>
> <proposal-2>
> /**
> * @write_thread:
> *
> * NBCON callback to write out text in task context.
> *
> * This callback must be called only in task context with both
> * device_lock() and the nbcon console acquired with
> * NBCON_PRIO_NORMAL.
> *
> * The same rules for console ownership verification and unsafe
> * sections handling applies as with write_atomic().
> *
> * The console ownership handling is necessary for synchronization
> * against write_atomic() which is synchronized only via the context.
> *
> * The device_lock() provides the primary serialization for operations
> * on the device. It might be as relaxed (mutex)[*] or as tight
> * (disabled preemption and interrupts) as needed. It allows
> * the kthread to operate in the least restrictive mode[**].
> *
> * [*] Standalone nbcon_context_try_acquire() is not safe with
> * the preemption enabled, see nbcon_owner_matches(). But it
> * can be safe when always called in the preemptive context
> * under the device_lock().
> *
> * [**] The device_lock() makes sure that nbcon_context_try_acquire()
> * would never need to spin which is important especially with
> * PREEMPT_RT.
> */
> </proposal-2>
OK. I will use this.
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -3995,6 +3995,8 @@ static int unregister_console_locked(struct console *console)
>> if (res > 0)
>> return 0;
>>
>> + __pr_flush(console, 1000, true);
>> +
>> /* Disable it unconditionally */
>> console_srcu_write_flags(console, console->flags & ~CON_ENABLED);
>
> Makes sense.
>
> And it actually means that it is too late to flush messages
> when kthread_should_stop() returns true. So, it does not matter if
> we check it in the while(backlog) loop or not. Well, it might be
> worth a comment in the code.
I will add a comment for this.
>> The thread failing to start is a serious issue. Particularly for
>> PREEMPT_RT.
>
> I agree.
>
>> Probably it should be something like:
>>
>> if (WARN_ON(IS_ERR(kt))) {
>
> Might make sense.
I will add this for v2.
> Honestly, if the system is not able to start the kthread then
> it is probably useless anyway. I would prefer if printk keeps working
> so that people know what is going on ;-)
OK. For v2 I will change it to fallback to the legacy printing for those
consoles that do not have a kthread.
> After all, I would add two comments, like these:
>
> <proposal-2>
> /*
> * Any access to the console device is serialized either by
> * device_lock() or console context or both.
> */
> kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name,
> con->index);
> [...]
>
> /*
> * Some users check con->kthread to decide whether to flush
> * the messages directly using con->write_atomic(). But they
> * do so only when the console is already in @console_list.
> */
I do not understand how @console_list is related to racing between
non-thread and thread. kthreads are not only created during
registration. For example, they can be created much later when the last
boot console unregisters.
I am OK with the first comment of this proposal. I do not understand the
second comment.
> con->kthread = kt;
> </proposal-2>
John
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-12 8:51 ` John Ogness
@ 2024-06-12 9:24 ` Petr Mladek
2024-06-12 11:18 ` John Ogness
2024-06-13 15:21 ` John Ogness
1 sibling, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-06-12 9:24 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Wed 2024-06-12 10:57:11, John Ogness wrote:
> On 2024-06-11, Petr Mladek <pmladek@suse.com> wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> The thread failing to start is a serious issue. Particularly for
> >> PREEMPT_RT.
> >
> > I agree.
> >
> >> Probably it should be something like:
> >>
> >> if (WARN_ON(IS_ERR(kt))) {
> >
> > Might make sense.
>
> I will add this for v2.
>
> > Honestly, if the system is not able to start the kthread then
> > it is probably useless anyway. I would prefer if printk keeps working
> > so that people know what is going on ;-)
>
> OK. For v2 I will change it to fallback to the legacy printing for those
> consoles that do not have a kthread.
>
> > After all, I would add two comments, like these:
> >
> > <proposal-2>
> > /*
> > * Any access to the console device is serialized either by
> > * device_lock() or console context or both.
> > */
> > kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name,
> > con->index);
> > [...]
> >
> > /*
> > * Some users check con->kthread to decide whether to flush
> > * the messages directly using con->write_atomic(). But they
> > * do so only when the console is already in @console_list.
> > */
>
> I do not understand how @console_list is related to racing between
> non-thread and thread. kthreads are not only created during
> registration. For example, they can be created much later when the last
> boot console unregisters.
I had in mind two particular code paths:
1. The check of con->kthread in nbcon_device_release() before
calling __nbcon_atomic_flush_pending_con().
But it is called only when __uart_port_using_nbcon() returns true.
And it would fail when nbcon_kthread_create() is called because
checks hlist_unhashed_lockless(&up->cons->node)
would fail. Which checks of the console is in @console_list
2. The following check in console_flush_all()
if ((flags & CON_NBCON) && con->kthread)
continue;
The result affects whether the legacy flush would call
nbcon_legacy_emit_next_record().
But this is called only for_each_console_srcu(con)
=> it could not race with nbcon_kthread_create()
because this console is not in @console_list at this moment.
By other words, I was curious whether some other code paths might
call con->write_atomic() while the kthread is already running.
It is not that important because it would be safe anyway.
I was checking this before I realized that it would be safe.
Anyway, the information about that the console is not in @console_list
when we set con->kthread still looks useful. At minimum,
the check would be racy if the console was on the list.
Does it make any sense now?
> I am OK with the first comment of this proposal. I do not understand the
> second comment.
Feel free to propose another comment. Or you could ignore the proposal
if you think that it does more harm than good.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-12 9:24 ` Petr Mladek
@ 2024-06-12 11:18 ` John Ogness
2024-06-12 11:33 ` Petr Mladek
0 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-12 11:18 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2024-06-12, Petr Mladek <pmladek@suse.com> wrote:
>> > After all, I would add two comments, like these:
>> >
>> > <proposal-2>
>> > /*
>> > * Any access to the console device is serialized either by
>> > * device_lock() or console context or both.
>> > */
>> > kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name,
>> > con->index);
>> > [...]
>> >
>> > /*
>> > * Some users check con->kthread to decide whether to flush
>> > * the messages directly using con->write_atomic(). But they
>> > * do so only when the console is already in @console_list.
>> > */
>>
>> I do not understand how @console_list is related to racing between
>> non-thread and thread. kthreads are not only created during
>> registration. For example, they can be created much later when the last
>> boot console unregisters.
>
> I had in mind two particular code paths:
>
> 1. The check of con->kthread in nbcon_device_release() before
> calling __nbcon_atomic_flush_pending_con().
>
> But it is called only when __uart_port_using_nbcon() returns true.
> And it would fail when nbcon_kthread_create() is called because
>
> checks hlist_unhashed_lockless(&up->cons->node)
>
> would fail. Which checks of the console is in @console_list
>
>
> 2. The following check in console_flush_all()
>
> if ((flags & CON_NBCON) && con->kthread)
> continue;
>
> The result affects whether the legacy flush would call
> nbcon_legacy_emit_next_record().
>
> But this is called only for_each_console_srcu(con)
> => it could not race with nbcon_kthread_create()
> because this console is not in @console_list at this moment.
>
> By other words, I was curious whether some other code paths might
> call con->write_atomic() while the kthread is already running.
>
> It is not that important because it would be safe anyway.
> I was checking this before I realized that it would be safe.
Yes, it must be safe because it can happen at any time. For example,
when flushing after an emergency section.
> Anyway, the information about that the console is not in @console_list
> when we set con->kthread still looks useful.
Except that it is not always true. If boot consoles are registered, the
kthread is created later, after the console _is_ in
@console_list. Setting con->kthread really has nothing to do with
@console_list.
> At minimum, the check would be racy if the console was on the list.
The con->kthread check _is_ racey, but it doesn't matter. Perhaps you
just want to make it clear that it is racey but it does not matter.
How about:
/*
* Some users check con->kthread to decide whether to flush
* the messages directly using con->write_atomic(). Although
* racey, such a check for that purpose is safe because both
* threaded and atomic printing are serialized by the
* console context.
*/
con->kthread = kt;
John
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-12 11:18 ` John Ogness
@ 2024-06-12 11:33 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-12 11:33 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Wed 2024-06-12 13:24:24, John Ogness wrote:
> On 2024-06-12, Petr Mladek <pmladek@suse.com> wrote:
> >> > After all, I would add two comments, like these:
> >> >
> >> > <proposal-2>
> >> > /*
> >> > * Any access to the console device is serialized either by
> >> > * device_lock() or console context or both.
> >> > */
> >> > kt = kthread_run(nbcon_kthread_func, con, "pr/%s%d", con->name,
> >> > con->index);
> >> > [...]
> >> >
> >> > /*
> >> > * Some users check con->kthread to decide whether to flush
> >> > * the messages directly using con->write_atomic(). But they
> >> > * do so only when the console is already in @console_list.
> >> > */
> >>
> >> I do not understand how @console_list is related to racing between
> >> non-thread and thread. kthreads are not only created during
> >> registration. For example, they can be created much later when the last
> >> boot console unregisters.
> >
> > I had in mind two particular code paths:
> >
> > 1. The check of con->kthread in nbcon_device_release() before
> > calling __nbcon_atomic_flush_pending_con().
> >
> > But it is called only when __uart_port_using_nbcon() returns true.
> > And it would fail when nbcon_kthread_create() is called because
> >
> > checks hlist_unhashed_lockless(&up->cons->node)
> >
> > would fail. Which checks of the console is in @console_list
> >
> >
> > 2. The following check in console_flush_all()
> >
> > if ((flags & CON_NBCON) && con->kthread)
> > continue;
> >
> > The result affects whether the legacy flush would call
> > nbcon_legacy_emit_next_record().
> >
> > But this is called only for_each_console_srcu(con)
> > => it could not race with nbcon_kthread_create()
> > because this console is not in @console_list at this moment.
> >
> > By other words, I was curious whether some other code paths might
> > call con->write_atomic() while the kthread is already running.
> >
> > It is not that important because it would be safe anyway.
> > I was checking this before I realized that it would be safe.
>
> Yes, it must be safe because it can happen at any time. For example,
> when flushing after an emergency section.
>
> > Anyway, the information about that the console is not in @console_list
> > when we set con->kthread still looks useful.
>
> Except that it is not always true. If boot consoles are registered, the
> kthread is created later, after the console _is_ in
> @console_list. Setting con->kthread really has nothing to do with
> @console_list.
Right. I have missed this.
> > At minimum, the check would be racy if the console was on the list.
>
> The con->kthread check _is_ racey, but it doesn't matter. Perhaps you
> just want to make it clear that it is racey but it does not matter.
>
> How about:
>
> /*
> * Some users check con->kthread to decide whether to flush
> * the messages directly using con->write_atomic(). Although
> * racey, such a check for that purpose is safe because both
> * threaded and atomic printing are serialized by the
> * console context.
> */
> con->kthread = kt;
Yes, it sounds good.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-12 8:51 ` John Ogness
2024-06-12 9:24 ` Petr Mladek
@ 2024-06-13 15:21 ` John Ogness
2024-06-14 7:40 ` Petr Mladek
1 sibling, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-13 15:21 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2024-06-11, Petr Mladek <pmladek@suse.com> wrote:
> Honestly, if the system is not able to start the kthread then
> it is probably useless anyway. I would prefer if printk keeps working
> so that people know what is going on ;-)
I have been looking into and thinking about this some more. I do not
like the idea of the system doing some sort of fallback if some of the
kthreads fail to start. Not only does it complicate the code, but it
greatly increases the variants of how the system during runtime.
I (strongly) suggest the following:
- The kthread is created in nbcon_alloc(). If the kthread fails, then
nbcon_alloc() fails and the console will not register.
- nbcon_kthread_should_wakeup() will return false if the console is not
registered or if @have_boot_console=1.
Then there would be no need to ever check con->kthread. Instead we can
rely on the global state of the system transitioning to relying on
threading.
I think it is totally appropriate that register_console() fails if the
kthread cannot be created, just as it already fails if the kmalloc() for
the pbufs fails.
Any objections?
John
P.S. There are other minor details involved, such as calling
kthread_stop() before removing a console from the list. But I have gone
through all these and it appears to generally simplify things a lot.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads
2024-06-13 15:21 ` John Ogness
@ 2024-06-14 7:40 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-14 7:40 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Thu 2024-06-13 17:27:33, John Ogness wrote:
> On 2024-06-11, Petr Mladek <pmladek@suse.com> wrote:
> > Honestly, if the system is not able to start the kthread then
> > it is probably useless anyway. I would prefer if printk keeps working
> > so that people know what is going on ;-)
>
> I have been looking into and thinking about this some more. I do not
> like the idea of the system doing some sort of fallback if some of the
> kthreads fail to start. Not only does it complicate the code, but it
> greatly increases the variants of how the system during runtime.
Fair enough.
> I (strongly) suggest the following:
>
> - The kthread is created in nbcon_alloc(). If the kthread fails, then
> nbcon_alloc() fails and the console will not register.
>
> - nbcon_kthread_should_wakeup() will return false if the console is not
> registered or if @have_boot_console=1.
I like this.
I guess that this would require moving some pieces from nbcon_init
to nbcon_alloc(). It might make sense to move there everything except
for setting the initial seq number.
> Then there would be no need to ever check con->kthread. Instead we can
> rely on the global state of the system transitioning to relying on
> threading.
>
> I think it is totally appropriate that register_console() fails if the
> kthread cannot be created, just as it already fails if the kmalloc() for
> the pbufs fails.
>
> Any objections?
Not from my side ;-)
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 05/18] printk: Atomic print in printk context on shutdown
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (3 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 04/18] printk: nbcon: Introduce printing kthreads John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-13 12:32 ` Petr Mladek
2024-06-13 12:44 ` atomic_flush vs boot consoles - was: " Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 06/18] printk: nbcon: Add context to console_is_usable() John Ogness
` (13 subsequent siblings)
18 siblings, 2 replies; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
For nbcon consoles, normally the printing is handled by the
dedicated console printing threads. However, on shutdown the
printing threads may not get a chance to print the final
messages.
When shutting down or rebooting (system_state > SYSTEM_RUNNING),
perform atomic printing from the printk() caller context.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/nbcon.c | 5 +++--
kernel/printk/printk.c | 7 ++++++-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 89b340ca303c..19f0db6945e4 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1297,7 +1297,8 @@ static void nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq,
* context must flush those remaining records if the printer thread
* is not available do it.
*/
- if (!con->kthread && prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
+ if ((!con->kthread || (system_state > SYSTEM_RUNNING)) &&
+ prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
stop_seq = prb_next_reserve_seq(prb);
goto again;
}
@@ -1639,7 +1640,7 @@ void nbcon_device_release(struct console *con)
*/
cookie = console_srcu_read_lock();
if (console_is_usable(con, console_srcu_read_flags(con)) &&
- !con->kthread &&
+ (!con->kthread || (system_state > SYSTEM_RUNNING)) &&
prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
__nbcon_atomic_flush_pending_con(con, prb_next_reserve_seq(prb), false);
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 137bd9a721c4..b58db15d2033 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2389,12 +2389,17 @@ asmlinkage int vprintk_emit(int facility, int level,
*
* - When this CPU is in panic.
*
+ * - During shutdown, since the printing threads may not get
+ * a chance to print the final messages.
+ *
* Note that if boot consoles are registered, the console
* lock/unlock dance must be relied upon instead because nbcon
* consoles cannot print simultaneously with boot consoles.
*/
- if (is_panic_context)
+ if (is_panic_context ||
+ (system_state > SYSTEM_RUNNING)) {
nbcon_atomic_flush_pending();
+ }
}
if (do_trylock_unlock) {
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 05/18] printk: Atomic print in printk context on shutdown
2024-06-03 23:24 ` [PATCH printk v2 05/18] printk: Atomic print in printk context on shutdown John Ogness
@ 2024-06-13 12:32 ` Petr Mladek
2024-06-13 12:44 ` atomic_flush vs boot consoles - was: " Petr Mladek
1 sibling, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-13 12:32 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-04 01:30:40, John Ogness wrote:
> For nbcon consoles, normally the printing is handled by the
> dedicated console printing threads. However, on shutdown the
> printing threads may not get a chance to print the final
> messages.
>
> When shutting down or rebooting (system_state > SYSTEM_RUNNING),
> perform atomic printing from the printk() caller context.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
Sigh, the checks/logic gets really complicated. But I can't think
of anything better at the moment. And it seems that the check for
the system state is added at the right locations.
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* atomic_flush vs boot consoles - was: Re: [PATCH printk v2 05/18] printk: Atomic print in printk context on shutdown
2024-06-03 23:24 ` [PATCH printk v2 05/18] printk: Atomic print in printk context on shutdown John Ogness
2024-06-13 12:32 ` Petr Mladek
@ 2024-06-13 12:44 ` Petr Mladek
2024-06-13 12:52 ` [PATCH] printk: nbcon_atomic_flush_pending() is safe only when there is no boot console Petr Mladek
1 sibling, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-06-13 12:44 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-04 01:30:40, John Ogness wrote:
> For nbcon consoles, normally the printing is handled by the
> dedicated console printing threads. However, on shutdown the
> printing threads may not get a chance to print the final
> messages.
>
> When shutting down or rebooting (system_state > SYSTEM_RUNNING),
> perform atomic printing from the printk() caller context.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> kernel/printk/nbcon.c | 5 +++--
> kernel/printk/printk.c | 7 ++++++-
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2389,12 +2389,17 @@ asmlinkage int vprintk_emit(int facility, int level,
> *
> * - When this CPU is in panic.
> *
> + * - During shutdown, since the printing threads may not get
> + * a chance to print the final messages.
> + *
> * Note that if boot consoles are registered, the console
> * lock/unlock dance must be relied upon instead because nbcon
> * consoles cannot print simultaneously with boot consoles.
> */
> - if (is_panic_context)
> + if (is_panic_context ||
> + (system_state > SYSTEM_RUNNING)) {
> nbcon_atomic_flush_pending();
> + }
> }
I was curious why we do not need to add this check also to
console_flush_all() which checks con->kthread as well.
I realized that we call it only when
#define printing_via_unlock (have_legacy_console || have_boot_console)
=> it handles nbcon consoles only when there is still a boot console.
It actually have to handle them in this case because the boot consoles
can be serialized only via console_lock().
In fact, nbcon_atomic_flush_pending() is not safe when there are boot
consoles. This is handled in vprintk_emit(). But it is not enforced
on several other locations.
I played with a possible solutions. The conditions around the console
calls are getting more and more complicated. I am going to send a
solution which looks acceptable to me.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* [PATCH] printk: nbcon_atomic_flush_pending() is safe only when there is no boot console
2024-06-13 12:44 ` atomic_flush vs boot consoles - was: " Petr Mladek
@ 2024-06-13 12:52 ` Petr Mladek
2024-06-13 15:10 ` Petr Mladek
2024-06-25 20:53 ` John Ogness
0 siblings, 2 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-13 12:52 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
Boot consoles are not serialized with the nbcon consoles via the nbcon
console context or con->device_lock(). The serialization is possible only
via the legacy console_lock().
The decision whether nbcon_atomic_flush_pending() should and can be
called safely is similar and closely related to the decision
whether the legacy loop should be used.
Define printing_via_context_safe symmetrically with printing_via_unlock.
Allow to call nbcon_atomic_flush_pending() only when it is needed and safe.
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/printk/internal.h | 10 ++++++++++
kernel/printk/nbcon.c | 9 +++++++--
kernel/printk/printk.c | 7 ++++---
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 38680c6b2b39..bafec0a27da3 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -182,6 +182,7 @@ static inline bool console_is_usable(struct console *con, short flags) { return
#endif /* CONFIG_PRINTK */
extern bool have_boot_console;
+extern bool have_nbcon_console;
extern bool have_legacy_console;
/*
@@ -192,6 +193,15 @@ extern bool have_legacy_console;
*/
#define printing_via_unlock (have_legacy_console || have_boot_console)
+/*
+ * Specifies if printing on nbcon consoles is needed and also safe
+ * when serialized only by the nbcon context. If @have_boot_console
+ * is true, the nbcon consoles must be serialized with the boot
+ * consoles using the legacy console_lock().
+ */
+#define printing_via_context_safe (have_nbcon_console || !have_boot_console)
+
+
extern struct printk_buffers printk_shared_pbufs;
/**
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 89b340ca303c..9b2df848718c 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1404,7 +1404,8 @@ void nbcon_cpu_emergency_exit(void)
* for the emergency messages is NBCON_PRIO_EMERGENCY.
*/
if (*cpu_emergency_nesting == 1) {
- nbcon_atomic_flush_pending();
+ if (printing_via_context_safe)
+ nbcon_atomic_flush_pending();
/*
* Safely attempt to flush the legacy consoles in this
@@ -1446,7 +1447,8 @@ void nbcon_cpu_emergency_flush(void)
if (*(nbcon_get_cpu_emergency_nesting()) == 0)
return;
- nbcon_atomic_flush_pending();
+ if (printing_via_context_safe)
+ nbcon_atomic_flush_pending();
if (printing_via_unlock && !is_printk_deferred()) {
if (console_trylock())
@@ -1637,6 +1639,9 @@ void nbcon_device_release(struct console *con)
* was locked. The console_srcu_read_lock must be taken to ensure
* the console is usable throughout flushing.
*/
+ if (!printing_via_context_safe)
+ return;
+
cookie = console_srcu_read_lock();
if (console_is_usable(con, console_srcu_read_flags(con)) &&
!con->kthread &&
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 137bd9a721c4..3183db5b4180 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -475,7 +475,7 @@ bool have_legacy_console;
* synchronous printing of legacy consoles will not occur during panic until
* the backtrace has been stored to the ringbuffer.
*/
-static bool have_nbcon_console;
+bool have_nbcon_console;
/*
* Specifies if a boot console is registered. If boot consoles are present,
@@ -2370,7 +2370,7 @@ asmlinkage int vprintk_emit(int facility, int level,
printed_len = vprintk_store(facility, level, dev_info, fmt, args);
- if (have_nbcon_console && !have_boot_console) {
+ if (printing_via_context_safe) {
bool is_panic_context = this_cpu_in_panic();
/*
@@ -3283,7 +3283,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
if (mode == CONSOLE_REPLAY_ALL)
__console_rewind_all();
- nbcon_atomic_flush_pending();
+ if (printing_via_context_safe)
+ nbcon_atomic_flush_pending();
if (printing_via_unlock)
console_flush_all(false, &next_seq, &handover);
--
2.45.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH] printk: nbcon_atomic_flush_pending() is safe only when there is no boot console
2024-06-13 12:52 ` [PATCH] printk: nbcon_atomic_flush_pending() is safe only when there is no boot console Petr Mladek
@ 2024-06-13 15:10 ` Petr Mladek
2024-06-25 20:53 ` John Ogness
1 sibling, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-13 15:10 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Thu 2024-06-13 14:52:24, Petr Mladek wrote:
> Boot consoles are not serialized with the nbcon consoles via the nbcon
> console context or con->device_lock(). The serialization is possible only
> via the legacy console_lock().
>
> The decision whether nbcon_atomic_flush_pending() should and can be
> called safely is similar and closely related to the decision
> whether the legacy loop should be used.
>
> Define printing_via_context_safe symmetrically with printing_via_unlock.
> Allow to call nbcon_atomic_flush_pending() only when it is needed and safe.
>
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -192,6 +193,15 @@ extern bool have_legacy_console;
> */
> #define printing_via_unlock (have_legacy_console || have_boot_console)
>
> +/*
> + * Specifies if printing on nbcon consoles is needed and also safe
> + * when serialized only by the nbcon context. If @have_boot_console
> + * is true, the nbcon consoles must be serialized with the boot
> + * consoles using the legacy console_lock().
> + */
> +#define printing_via_context_safe (have_nbcon_console || !have_boot_console)
Oops, this should be:
#define printing_via_context_safe (have_nbcon_console && !have_boot_console)
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] printk: nbcon_atomic_flush_pending() is safe only when there is no boot console
2024-06-13 12:52 ` [PATCH] printk: nbcon_atomic_flush_pending() is safe only when there is no boot console Petr Mladek
2024-06-13 15:10 ` Petr Mladek
@ 2024-06-25 20:53 ` John Ogness
2024-06-28 15:51 ` how to flush consoles: was: " Petr Mladek
1 sibling, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-25 20:53 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On 2024-06-13, Petr Mladek <pmladek@suse.com> wrote:
> Boot consoles are not serialized with the nbcon consoles via the nbcon
> console context or con->device_lock(). The serialization is possible only
> via the legacy console_lock().
>
> The decision whether nbcon_atomic_flush_pending() should and can be
> called safely is similar and closely related to the decision
> whether the legacy loop should be used.
>
> Define printing_via_context_safe symmetrically with printing_via_unlock.
> Allow to call nbcon_atomic_flush_pending() only when it is needed and safe.
This patch, along with other comments you made about the many different
checks to see when it is allowed to do what, forced me to take a step
back and look at the big picture. Indeed, by the end of this series we
have many boolean variables that influence decisions about how to handle
threads and how to print.
I am thinking it makes more sense to incorporate these booleans into a
single variable (printk_state?). The variable would only be changed
under the console_list_lock, which could allow lockless users to
leverage the console_srcu_read_lock for consistency.
The different orthogonal bits of the variable would be:
* have_boot_console - true if a boot console is registered
* have_legacy_console - true if a non-nbcon console is registered
* have_nbcon_console - true if an nbcon console is registered
* printk_threads_allowed - true when it is OK to have threads
(set/cleared by notifiers, and more or less represents
"system_state == SYSTEM_SCHEDULING || system_state == SYSTEM_RUNNING")
* printk_threads_available - true while all printing threads are running
(and implies at least 1 thread is running)
We could provide macros to interpret this multi-flag value for various
scenarios:
#define nbcon_may_create_threads()
(printk_threads_allowed)
#define nbcon_may_rely_on_threads()
(have_nbcon_console && !have_boot_console && printk_threads_available)
#define nbcon_may_flush_atomic()
(have_nbcon_console && !have_boot_console)
#define nbcon_must_flush_atomic()
(have_nbcon_console && !have_boot_console && !printk_threads_available)
#define nbcon_must_flush_via_unlock()
(have_nbcon_console && have_boot_console)
#define printing_via_unlock()
(have_legacy_console || have_boot_console)
Of course, the exact naming of these macros gives us a big shed to
paint. But I think this could help to simplify/unify the checks for the
various scenarios.
John Ogness
^ permalink raw reply [flat|nested] 63+ messages in thread
* how to flush consoles: was: Re: [PATCH] printk: nbcon_atomic_flush_pending() is safe only when there is no boot console
2024-06-25 20:53 ` John Ogness
@ 2024-06-28 15:51 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-28 15:51 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-25 22:59:34, John Ogness wrote:
> On 2024-06-13, Petr Mladek <pmladek@suse.com> wrote:
> > Boot consoles are not serialized with the nbcon consoles via the nbcon
> > console context or con->device_lock(). The serialization is possible only
> > via the legacy console_lock().
> >
> > The decision whether nbcon_atomic_flush_pending() should and can be
> > called safely is similar and closely related to the decision
> > whether the legacy loop should be used.
> >
> > Define printing_via_context_safe symmetrically with printing_via_unlock.
> > Allow to call nbcon_atomic_flush_pending() only when it is needed and safe.
>
> This patch, along with other comments you made about the many different
> checks to see when it is allowed to do what, forced me to take a step
> back and look at the big picture. Indeed, by the end of this series we
> have many boolean variables that influence decisions about how to handle
> threads and how to print.
>
> I am thinking it makes more sense to incorporate these booleans into a
> single variable (printk_state?). The variable would only be changed
> under the console_list_lock, which could allow lockless users to
> leverage the console_srcu_read_lock for consistency.
>
> The different orthogonal bits of the variable would be:
>
>
> * have_boot_console - true if a boot console is registered
>
> * have_legacy_console - true if a non-nbcon console is registered
>
> * have_nbcon_console - true if an nbcon console is registered
>
> * printk_threads_allowed - true when it is OK to have threads
> (set/cleared by notifiers, and more or less represents
> "system_state == SYSTEM_SCHEDULING || system_state == SYSTEM_RUNNING")
>
> * printk_threads_available - true while all printing threads are running
> (and implies at least 1 thread is running)
I would call the variable "printk_threads_running". It seems to have
more clear meaning.
12th patch adds:
* force_printkthread - which triggers offloading even the legacy
into a thread. Or does it force using kthreads even in
emergency?
> We could provide macros to interpret this multi-flag value for various
> scenarios:
>
> #define nbcon_may_create_threads()
> (printk_threads_allowed)
The force_printkthread involves also legacy consoles. As a result,
the "nbcon_" prefix does not look important.
> #define nbcon_may_rely_on_threads()
> (have_nbcon_console && !have_boot_console && printk_threads_available)
>
> #define nbcon_may_flush_atomic()
> (have_nbcon_console && !have_boot_console)
>
> #define nbcon_must_flush_atomic()
> (have_nbcon_console && !have_boot_console && !printk_threads_available)
>
> #define nbcon_must_flush_via_unlock()
> (have_nbcon_console && have_boot_console)
>
> #define printing_via_unlock()
> (have_legacy_console || have_boot_console)
I like this "may" vs "must" variants.
Good names and good macros might help, definitely. But I think that
there are more problems. There are also several variants of functions
flushing the consoles. Are they are orthogonal as well:
One aspect are the console callbacks:
+ write() # legacy consoles
+ write_atomic() # nbcon consoles in atomic or unknown context
+ write_thread # nbcon consoles in task context
Another aspect is the serialization:
+ console_lock() # legacy loop
+ con->driver_lock() + nbcon_context # nbcon consoles in normal context
+ nbcon_context # nbcon consoles in emergency
# or panic context
Caller:
+ printk()
+ explicit flush()
Context priority:
+ normal
+ emergency
+ panic
I still have to think if we could somehow improve the situation,
for example, by using some systematic names.
Well, the conditions are more or less straightforward in most
situations with three exceptions:
+ vprintk_emit()
+ nbcon_cpu_emergency_exit()
+ nbcon_cpu_emergency_flush()
, where:
+ The rules for the direct flush are the same in both
nbcon_cpu_emergency_exit() and nbcon_cpu_emergency_flush().
But only nbcon_cpu_emergency_exit() calls the trigger-offload
part.
+ The rules in vprintk_emit() are much more complicated
by the special handling in:
+ NBCON_PRIO_PANIC and legacy_allow_panic_sync
+ NBCON_PRIO_EMERGENCY
IMHO, it would help to:
+ avoid code duplication in nbcon_cpu_emergency_exit()/flush()
+ make the code in vprintk_emit() as compact as possible,
especially avoid updating "do_trylock_unlock" all over
+ make the code in vprintk_emit as similar to
nbcon_cpu_emergency_exit()/flush() as possible.
I have tried various variants and it always became complicated.
So I decided to make the logic as compact as possible in the
following two functions:
/**
* struct console_flush_type - Define how to flush the consoles.
* @nbcon_atomic: Flush directly using nbcon_atomic() callback
* @nbcon_thread: Offload the flush to the kthread
* @legacy_direct: Call the legacy loop in this context
* @legacy_offload: Offload the legacy loop into IRQ or kthread
*/
struct console_flush_type {
bool nbcon_atomic;
bool nbcon_thread;
bool legacy_direct;
bool legacy_offload;
};
/*
* Decide how the messages should get flushed from printk().
* It is supposed to be called in vprintk_emit()
*
* Variant per console type
*/
void printk_set_console_flush_type(struct console_flush *flush)
{
enum nbcon_prio nbcon_prio;
memset(flush, 0, sizeof(*flush));
/*
* nbcon_get_default_prio() can be read safely even in premptible
* context. NBCON_PRIO_PANIC is used only on panic-CPU.
* NBCON_PRIO_EMERGENCY is set only in context with CPU migragtion
* disabled.
*/
nbcon_prio = nbcon_get_default_prio();
/*
* Skip it in EMERGENCY priority. The console will be
* explicitly flushed when exiting the emergency section.
*/
if (nbcon_prio == NBCON_PRIO_EMERGENCY)
return;
/* How to flush nbcon consoles without legacy loop. */
if (have_nbcon_console && !have_boot_console) {
if (nbcon_prio == NBCON_PRIO_PANIC) {
flush->nbcon_atomic = true;
/* In panic, the legacy consoles are not allowed
* to print from the printk calling context unless
* explicitly allowed. This gives the safe nbcon
* consoles a chance to print out all the panic
* messages first. This restriction only applies
* if there are nbcon consoles registered.
*/
if (!legacy_allow_panic_sync)
return;
/* Only NBCON_PRIO_NORMAL left. */
} else if (nbcon_kthreads_running) {
flush->nbcon_thread = true;
} else {
flush->nbcon_atomic = true;
}
}
/* How to do the legacy loop. */
if (have_legacy_console || have_boot_console) {
if (nbcon_prio == NBCON_PRIO_PANIC &&
legacy_allow_panic_sync) {
flush->nbcon_direct = true;
/* Only NBCON_PRIO_NORMAL left. */
} if (is_printk_deferred() || console_legacy_thread)) {
flush->legacy_thread = true;
} else {
flush->legacy_direct = true;
}
}
}
/*
* Decide how the messages should get flushed from printk().
* It is supposed to be called in vprintk_emit()
*
* Variant per nbcon prio
*/
void printk_set_console_flush_type(struct console_flush *flush)
{
enum nbcon_prio nbcon_prio;
memset(flush, 0, sizeof(*flush));
/*
* nbcon_get_default_prio() can be read safely even in premptible
* context. NBCON_PRIO_PANIC is used only on panic-CPU.
* NBCON_PRIO_EMERGENCY is set only in context with CPU migragtion
* disabled.
*/
nbcon_prio = nbcon_get_default_prio();
switch(nbcon_prio):
NBCON_PRIO_NORMAL:
if (have_nbcon_console && !have_boot_console) {
if (nbcon_kthreads_running)
flush->nbcon_thread = true;
else
flush->nbcon_atomic = true;
}
if (have_legacy_console || have_boot_console ) {
if (is_printk_deferred() || console_legacy_thread))
flush->legacy_thread = true;
else
flush->legacy_direct = true;
}
break;
NBCON_PRIO_EMERGENCY:
/*
* Skip it in EMERGENCY priority. The console will be
* explicitly flushed when exiting the emergency section.
*/
break;
NBCON_PRIO_PANIC:
if (have_nbcon_console && !have_boot_console) {
flush->nbcon_atomic = true;
/* In panic, the legacy consoles are not allowed
* to print from the printk calling context unless
* explicitly allowed. This gives the safe nbcon
* consoles a chance to print out all the panic
* messages first. This restriction only applies
* if there are nbcon consoles registered.
*/
if (!legacy_allow_panic_sync)
break;
}
if ((have_legacy_console || have_boot_console) &&
flush->nbcon_direct = true;
break;
default:
WARN_ON_ONCE(1, "This should never happen\n");
}
}
/*
* Decide how the messages should get flushed from emergency context.
* This is called when we really want to flush the emergency messages.
*
* FIXME: Emergency messages should always get flushed directly, except
* when it is not safe.
*/
void get_console_emergency_flush_type(struct console_flush_type *flush)
{
enum nbcon_prio nbcon_prio = nbcon_get_default_prio();
memset(flush, 0, sizeof(*flush));
WARN_ON_ONCE(nbcon_prio != NBCON_PRIO_EMERGENCY);
if (have_nbcon_console && !have_boot_console)
flush->nbcon_atomic = true;
if (have_legacy_console || have_boot_console) {
/* FIXME: add force_legacy_kthread? */
if (is_printk_deferred())
flush->legacy_thread = true;
} else {
flush->legacy_direct = true;
}
}
}
The first function might be used in vprintk_emit as:
asmlinkage int vprintk_emit(int facility, int level,
const struct dev_printk_info *dev_info,
const char *fmt, va_list args)
{
struct console_flush_type flush;
bool do_trylock_unlock = !force_printkthreads() &&
printing_via_unlock;
int printed_len;
/* Suppress unimportant messages after panic happens */
if (unlikely(suppress_printk))
return 0;
/*
* The messages on the panic CPU are the most important. If
* non-panic CPUs are generating any messages, they will be
* silently dropped.
*/
if (other_cpu_in_panic())
return 0;
if (level == LOGLEVEL_SCHED) {
level = LOGLEVEL_DEFAULT;
/* If called from the scheduler, we can not call up(). */
do_trylock_unlock = false;
}
printk_delay(level);
printed_len = vprintk_store(facility, level, dev_info, fmt, args);
printk_set_console_flush_type(&flush);
if (flush.nbcon_direct)
nbcon_atomic_flush_pending();
if (flush.nbcon_thread)
nbcon_wake_threads();
if (flush.legacy_direct) {
/*
* The caller may be holding system-critical or
* timing-sensitive locks. Disable preemption during
* printing of all remaining records to all consoles so that
* this context can return as soon as possible. Hopefully
* another printk() caller will take over the printing.
*
* Also, nbcon_get_default_prio() requires migration disabled.
*/
preempt_disable();
/*
* Try to acquire and then immediately release the console
* semaphore. The release will print out buffers. With the
* spinning variant, this context tries to take over the
* printing from another printing context.
*/
if (console_trylock_spinning())
console_unlock();
preempt_enable();
}
if (flush.legacy_thread)
defer_console_output();
else
wake_up_klogd();
return printed_len;
}
EXPORT_SYMBOL(vprintk_emit);
I do not say that this is the best solution.
My view:
1. I like the 2nd variant of printk_set_console_flush_type() where the
code is primary split by nbcon_prio.
And I like that the rather complicated rules are as compact as
possible and quite clear for each scenario.
2. The situation is much easier in get_console_emergency_flush_type().
Note that it never sets "nbcon_thread". Maybe, we could flush the
consoles in this function. And just return whether it is needed
to offload the legacy loop.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 06/18] printk: nbcon: Add context to console_is_usable()
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (4 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 05/18] printk: Atomic print in printk context on shutdown John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-13 13:22 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 07/18] printk: nbcon: Add printer thread wakeups John Ogness
` (12 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
The nbcon consoles have two callbacks to be used for different
contexts. In order to determine if an nbcon console is usable,
console_is_usable() needs to know if it is a context that will
use the write_atomic() callback or the write_thread() callback.
Add an extra parameter @use_atomic to specify this.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/internal.h | 16 ++++++++++------
kernel/printk/nbcon.c | 8 ++++----
kernel/printk/printk.c | 6 ++++--
3 files changed, 18 insertions(+), 12 deletions(-)
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 38680c6b2b39..243d3d3bc889 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -100,7 +100,7 @@ void nbcon_kthread_create(struct console *con);
* which can also play a role in deciding if @con can be used to print
* records.
*/
-static inline bool console_is_usable(struct console *con, short flags)
+static inline bool console_is_usable(struct console *con, short flags, bool use_atomic)
{
if (!(flags & CON_ENABLED))
return false;
@@ -109,10 +109,13 @@ static inline bool console_is_usable(struct console *con, short flags)
return false;
if (flags & CON_NBCON) {
- if (!con->write_atomic)
- return false;
- if (!con->write_thread)
- return false;
+ if (use_atomic) {
+ if (!con->write_atomic)
+ return false;
+ } else {
+ if (!con->write_thread)
+ return false;
+ }
} else {
if (!con->write)
return false;
@@ -177,7 +180,8 @@ static inline void nbcon_atomic_flush_pending(void) { }
static inline bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
int cookie) { return false; }
-static inline bool console_is_usable(struct console *con, short flags) { return false; }
+static inline bool console_is_usable(struct console *con, short flags,
+ bool use_atomic) { return false; }
#endif /* CONFIG_PRINTK */
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 19f0db6945e4..4200953d368e 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -967,7 +967,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex
cookie = console_srcu_read_lock();
flags = console_srcu_read_flags(con);
- if (console_is_usable(con, flags)) {
+ if (console_is_usable(con, flags, false)) {
/* Bring the sequence in @ctxt up to date */
ctxt->seq = nbcon_seq_read(con);
@@ -1026,7 +1026,7 @@ static int nbcon_kthread_func(void *__console)
con_flags = console_srcu_read_flags(con);
- if (console_is_usable(con, con_flags)) {
+ if (console_is_usable(con, con_flags, false)) {
unsigned long lock_flags;
con->device_lock(con, &lock_flags);
@@ -1322,7 +1322,7 @@ static void __nbcon_atomic_flush_pending(u64 stop_seq, bool allow_unsafe_takeove
if (!(flags & CON_NBCON))
continue;
- if (!console_is_usable(con, flags))
+ if (!console_is_usable(con, flags, true))
continue;
if (nbcon_seq_read(con) >= stop_seq)
@@ -1639,7 +1639,7 @@ void nbcon_device_release(struct console *con)
* the console is usable throughout flushing.
*/
cookie = console_srcu_read_lock();
- if (console_is_usable(con, console_srcu_read_flags(con)) &&
+ if (console_is_usable(con, console_srcu_read_flags(con), true) &&
(!con->kthread || (system_state > SYSTEM_RUNNING)) &&
prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
__nbcon_atomic_flush_pending_con(con, prb_next_reserve_seq(prb), false);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index b58db15d2033..a2c4fe3d21cc 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3043,7 +3043,7 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
if ((flags & CON_NBCON) && con->kthread)
continue;
- if (!console_is_usable(con, flags))
+ if (!console_is_usable(con, flags, true))
continue;
any_usable = true;
@@ -4018,8 +4018,10 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
* that they make forward progress, so only increment
* @diff for usable consoles.
*/
- if (!console_is_usable(c, flags))
+ if (!console_is_usable(c, flags, true) &&
+ !console_is_usable(c, flags, false)) {
continue;
+ }
if (flags & CON_NBCON) {
printk_seq = nbcon_seq_read(c);
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 06/18] printk: nbcon: Add context to console_is_usable()
2024-06-03 23:24 ` [PATCH printk v2 06/18] printk: nbcon: Add context to console_is_usable() John Ogness
@ 2024-06-13 13:22 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-13 13:22 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-04 01:30:41, John Ogness wrote:
> The nbcon consoles have two callbacks to be used for different
> contexts. In order to determine if an nbcon console is usable,
> console_is_usable() needs to know if it is a context that will
> use the write_atomic() callback or the write_thread() callback.
>
> Add an extra parameter @use_atomic to specify this.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> kernel/printk/internal.h | 16 ++++++++++------
> kernel/printk/nbcon.c | 8 ++++----
> kernel/printk/printk.c | 6 ++++--
> 3 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 38680c6b2b39..243d3d3bc889 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -100,7 +100,7 @@ void nbcon_kthread_create(struct console *con);
> * which can also play a role in deciding if @con can be used to print
> * records.
> */
> -static inline bool console_is_usable(struct console *con, short flags)
> +static inline bool console_is_usable(struct console *con, short flags, bool use_atomic)
> {
> if (!(flags & CON_ENABLED))
> return false;
> @@ -109,10 +109,13 @@ static inline bool console_is_usable(struct console *con, short flags)
> return false;
>
> if (flags & CON_NBCON) {
> - if (!con->write_atomic)
> - return false;
> - if (!con->write_thread)
> - return false;
> + if (use_atomic) {
> + if (!con->write_atomic)
> + return false;
> + } else {
> + if (!con->write_thread)
> + return false;
> + }
> } else {
> if (!con->write)
> return false;
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -4018,8 +4018,10 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> * that they make forward progress, so only increment
> * @diff for usable consoles.
> */
> - if (!console_is_usable(c, flags))
> + if (!console_is_usable(c, flags, true) &&
> + !console_is_usable(c, flags, false)) {
> continue;
> + }
>
> if (flags & CON_NBCON) {
> printk_seq = nbcon_seq_read(c);
I wonder if it is worth it. Do we seriously want to support nbcon
without con->write_kthread() or con->write_atomic() callbacks?
For example, I see in kernel/printk/printk.c:
void register_console(struct console *newcon)
{
bool use_device_lock = (newcon->flags & CON_NBCON) && newcon->write_atomic;
We would need to extend this check if we wanted to allow nbcon
consoles without con->nbcon_atomic()...
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 07/18] printk: nbcon: Add printer thread wakeups
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (5 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 06/18] printk: nbcon: Add context to console_is_usable() John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-13 15:08 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 08/18] printk: nbcon: Stop threads on shutdown/reboot John Ogness
` (11 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman, Paul E. McKenney
From: Thomas Gleixner <tglx@linutronix.de>
Add a function to wakeup the printer threads. The printer threads
are woken when:
- a record is added to the printk ringbuffer
- consoles are resumed
- triggered via printk_trigger_flush()
- consoles should be replayed via sysrq
The actual waking is performed via irq_work so that the function
can be called from any context.
Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
include/linux/console.h | 3 +++
kernel/printk/internal.h | 1 +
kernel/printk/nbcon.c | 56 ++++++++++++++++++++++++++++++++++++++++
kernel/printk/printk.c | 8 ++++++
4 files changed, 68 insertions(+)
diff --git a/include/linux/console.h b/include/linux/console.h
index 4de42ec1527c..adcc1a74aeee 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -16,6 +16,7 @@
#include <linux/atomic.h>
#include <linux/bits.h>
+#include <linux/irq_work.h>
#include <linux/rculist.h>
#include <linux/rcuwait.h>
#include <linux/types.h>
@@ -327,6 +328,7 @@ struct nbcon_write_context {
* @pbufs: Pointer to nbcon private buffer
* @kthread: Printer kthread for this console
* @rcuwait: RCU-safe wait object for @kthread waking
+ * @irq_work: Defer @kthread waking to IRQ work context
*/
struct console {
char name[16];
@@ -446,6 +448,7 @@ struct console {
struct printk_buffers *pbufs;
struct task_struct *kthread;
struct rcuwait rcuwait;
+ struct irq_work irq_work;
};
#ifdef CONFIG_LOCKDEP
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 243d3d3bc889..b2b5c10a4321 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -93,6 +93,7 @@ void nbcon_atomic_flush_pending(void);
bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
int cookie);
void nbcon_kthread_create(struct console *con);
+void nbcon_wake_threads(void);
/*
* Check if the given console is currently capable and allowed to print
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 4200953d368e..4ab47cc014a3 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1058,6 +1058,61 @@ static int nbcon_kthread_func(void *__console)
goto wait_for_event;
}
+/**
+ * nbcon_irq_work - irq work to wake printk thread
+ * @irq_work: The irq work to operate on
+ */
+static void nbcon_irq_work(struct irq_work *irq_work)
+{
+ struct console *con = container_of(irq_work, struct console, irq_work);
+
+ nbcon_kthread_wake(con);
+}
+
+static inline bool rcuwait_has_sleeper(struct rcuwait *w)
+{
+ bool has_sleeper;
+
+ rcu_read_lock();
+ /*
+ * Guarantee any new records can be seen by tasks preparing to wait
+ * before this context checks if the rcuwait is empty.
+ *
+ * This full memory barrier pairs with the full memory barrier within
+ * set_current_state() of ___rcuwait_wait_event(), which is called
+ * after prepare_to_rcuwait() adds the waiter but before it has
+ * checked the wait condition.
+ *
+ * This pairs with nbcon_kthread_func:A.
+ */
+ smp_mb(); /* LMM(rcuwait_has_sleeper:A) */
+ has_sleeper = !!rcu_dereference(w->task);
+ rcu_read_unlock();
+
+ return has_sleeper;
+}
+
+/**
+ * nbcon_wake_threads - Wake up printing threads using irq_work
+ */
+void nbcon_wake_threads(void)
+{
+ struct console *con;
+ int cookie;
+
+ cookie = console_srcu_read_lock();
+ for_each_console_srcu(con) {
+ /*
+ * Only schedule irq_work if the printing thread is
+ * actively waiting. If not waiting, the thread will
+ * notice by itself that it has work to do.
+ */
+ if (con->kthread && rcuwait_has_sleeper(&con->rcuwait))
+ irq_work_queue(&con->irq_work);
+ }
+ console_srcu_read_unlock(cookie);
+}
+
/* Track the nbcon emergency nesting per CPU. */
static DEFINE_PER_CPU(unsigned int, nbcon_pcpu_emergency_nesting);
static unsigned int early_nbcon_pcpu_emergency_nesting __initdata;
@@ -1558,6 +1613,7 @@ void nbcon_init(struct console *con, u64 init_seq)
BUG_ON(!con->pbufs);
rcuwait_init(&con->rcuwait);
+ init_irq_work(&con->irq_work, nbcon_irq_work);
nbcon_seq_force(con, init_seq);
nbcon_state_set(con, &state);
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a2c4fe3d21cc..457c01311a95 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2402,6 +2402,8 @@ asmlinkage int vprintk_emit(int facility, int level,
}
}
+ nbcon_wake_threads();
+
if (do_trylock_unlock) {
/*
* The caller may be holding system-critical or
@@ -2708,6 +2710,10 @@ void resume_console(void)
*/
synchronize_srcu(&console_srcu);
+ /*
+ * Since this runs in task context, wake the threaded printers
+ * directly rather than scheduling irq_work to do it.
+ */
cookie = console_srcu_read_lock();
for_each_console_srcu(con) {
flags = console_srcu_read_flags(con);
@@ -4178,6 +4184,7 @@ void defer_console_output(void)
void printk_trigger_flush(void)
{
+ nbcon_wake_threads();
defer_console_output();
}
@@ -4513,6 +4520,7 @@ void console_try_replay_all(void)
{
if (console_trylock()) {
__console_rewind_all();
+ nbcon_wake_threads();
/* Consoles are flushed as part of console_unlock(). */
console_unlock();
}
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 07/18] printk: nbcon: Add printer thread wakeups
2024-06-03 23:24 ` [PATCH printk v2 07/18] printk: nbcon: Add printer thread wakeups John Ogness
@ 2024-06-13 15:08 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-13 15:08 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman, Paul E. McKenney
On Tue 2024-06-04 01:30:42, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Add a function to wakeup the printer threads. The printer threads
> are woken when:
>
> - a record is added to the printk ringbuffer
> - consoles are resumed
> - triggered via printk_trigger_flush()
> - consoles should be replayed via sysrq
>
> The actual waking is performed via irq_work so that the function
> can be called from any context.
>
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1058,6 +1058,61 @@ static int nbcon_kthread_func(void *__console)
> goto wait_for_event;
> }
>
> +/**
> + * nbcon_irq_work - irq work to wake printk thread
> + * @irq_work: The irq work to operate on
> + */
> +static void nbcon_irq_work(struct irq_work *irq_work)
> +{
> + struct console *con = container_of(irq_work, struct console, irq_work);
> +
> + nbcon_kthread_wake(con);
> +}
> +
> +static inline bool rcuwait_has_sleeper(struct rcuwait *w)
> +{
> + bool has_sleeper;
> +
> + rcu_read_lock();
> + /*
> + * Guarantee any new records can be seen by tasks preparing to wait
> + * before this context checks if the rcuwait is empty.
> + *
> + * This full memory barrier pairs with the full memory barrier within
> + * set_current_state() of ___rcuwait_wait_event(), which is called
> + * after prepare_to_rcuwait() adds the waiter but before it has
> + * checked the wait condition.
> + *
> + * This pairs with nbcon_kthread_func:A.
> + */
> + smp_mb(); /* LMM(rcuwait_has_sleeper:A) */
> + has_sleeper = !!rcu_dereference(w->task);
We should use the existing API rcuwait_active().
> + rcu_read_unlock();
> +
> + return has_sleeper;
> +}
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2402,6 +2402,8 @@ asmlinkage int vprintk_emit(int facility, int level,
> }
> }
>
> + nbcon_wake_threads();
This need to be called only when there is a nbcon console
and no boot console and it has not been flushed explicitly.
I think that we could move this slightly above:
if (have_nbcon_console && !have_boot_console) {
[...]
if (is_panic_context ||
!printk_threads_enabled ||
(system_state > SYSTEM_RUNNING)) {
nbcon_atomic_flush_pending();
} else {
nbcon_wake_threads();
}
}
> +
> if (do_trylock_unlock) {
> /*
> * The caller may be holding system-critical or
> @@ -2708,6 +2710,10 @@ void resume_console(void)
> */
> synchronize_srcu(&console_srcu);
>
> + /*
> + * Since this runs in task context, wake the threaded printers
> + * directly rather than scheduling irq_work to do it.
> + */
> cookie = console_srcu_read_lock();
> for_each_console_srcu(con) {
> flags = console_srcu_read_flags(con);
The wake up call has already been added in 4th patch of this patchset.
I would slightly prefer to move it from the 4th patch to this one.
Same with start_console(). But it is not super important.
> @@ -4178,6 +4184,7 @@ void defer_console_output(void)
>
> void printk_trigger_flush(void)
> {
> + nbcon_wake_threads();
IMHO, this is not needed. vprintk_emit() always either flushes nbcon
consoles directly or wakes them.
In each case, it is not needed when printk_trigger_flush() is called
from nbcon_cpu_emergency_exit().
Hmm, I am not sure about the situation in nmi_trigger_cpumask_backtrace().
printk_trigger_flush() is called there to queue the IRQ work yet
another CPU to be on the safe side. But the irq_work used by
nbcon is per-console (not per-CPU). I guess that an attempt to
queue it on 2nd CPU would be a NOP.
> defer_console_output();
> }
>
> @@ -4513,6 +4520,7 @@ void console_try_replay_all(void)
> {
> if (console_trylock()) {
> __console_rewind_all();
> + nbcon_wake_threads();
> /* Consoles are flushed as part of console_unlock(). */
> console_unlock();
> }
Just an idea. We probably could do better for nbcon consoles. Like try
to flush them directly with emergency prio. But it can be done
in a separate patch later.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 08/18] printk: nbcon: Stop threads on shutdown/reboot
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (6 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 07/18] printk: nbcon: Add printer thread wakeups John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-17 15:21 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 09/18] printk: nbcon: Start printing threads John Ogness
` (10 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
Register a syscore_ops shutdown function to stop all threaded
printers on shutdown/reboot. This allows printk to cleanly
transition back to atomic printing in order to provide a robust
mechanism for outputting the final messages.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/nbcon.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 4ab47cc014a3..f60d47b5db8a 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -18,6 +18,7 @@
#include <linux/smp.h>
#include <linux/stddef.h>
#include <linux/string.h>
+#include <linux/syscore_ops.h>
#include <linux/types.h>
#include "internal.h"
#include "printk_ringbuffer.h"
@@ -1703,3 +1704,33 @@ void nbcon_device_release(struct console *con)
console_srcu_read_unlock(cookie);
}
EXPORT_SYMBOL_GPL(nbcon_device_release);
+
+/**
+ * printk_kthread_shutdown - shutdown all threaded printers
+ *
+ * On system shutdown all threaded printers are stopped. This allows printk
+ * to transition back to atomic printing, thus providing a robust mechanism
+ * for the final shutdown/reboot messages to be output.
+ */
+static void printk_kthread_shutdown(void)
+{
+ struct console *con;
+
+ console_list_lock();
+ for_each_console(con) {
+ if (con->flags & CON_NBCON)
+ nbcon_kthread_stop(con);
+ }
+ console_list_unlock();
+}
+
+static struct syscore_ops printk_syscore_ops = {
+ .shutdown = printk_kthread_shutdown,
+};
+
+static int __init printk_init_ops(void)
+{
+ register_syscore_ops(&printk_syscore_ops);
+ return 0;
+}
+device_initcall(printk_init_ops);
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 08/18] printk: nbcon: Stop threads on shutdown/reboot
2024-06-03 23:24 ` [PATCH printk v2 08/18] printk: nbcon: Stop threads on shutdown/reboot John Ogness
@ 2024-06-17 15:21 ` Petr Mladek
2024-06-25 19:56 ` John Ogness
0 siblings, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-06-17 15:21 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-04 01:30:43, John Ogness wrote:
> Register a syscore_ops shutdown function to stop all threaded
> printers on shutdown/reboot. This allows printk to cleanly
> transition back to atomic printing in order to provide a robust
> mechanism for outputting the final messages.
It looks like a simple straightforward patch. But there are some
hidden questions.
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1703,3 +1704,33 @@ void nbcon_device_release(struct console *con)
> console_srcu_read_unlock(cookie);
> }
> EXPORT_SYMBOL_GPL(nbcon_device_release);
> +
> +/**
> + * printk_kthread_shutdown - shutdown all threaded printers
> + *
> + * On system shutdown all threaded printers are stopped. This allows printk
> + * to transition back to atomic printing, thus providing a robust mechanism
> + * for the final shutdown/reboot messages to be output.
> + */
> +static void printk_kthread_shutdown(void)
I would rename this to "printk_shutdown_threads() because it
is symmetric to printk_setup_threads().
> +{
> + struct console *con;
> +
> + console_list_lock();
First ideas (how the waterfall of questions started):
+ I though that we should do here:
printk_threads_enabled = false;
It would tell vprintk_emit() to call nbcon_atomic_flush_pending().
And it would be symetric with printk_setup_threads().
+ Also I though whether to call it before or after stopping the kthreads.
If we set the variable before stopping kthreads then the kthreads
might try to flush the messages in parallel with the flush from
vprintk_emit(). It should be safe because it would be serialized
via the nbcon console context. But it would be the first scenario
where this happen => not much tested.
Well, we should start the direct flush before stopping kthreads.
So that we could see the messages when the stopping fails.
But then I realized:
Ha, vprintk_emit() would call nbcon_atomic_flush_pending() anyway
because these notifiers are called with:
system_state > SYSTEM_RUNNING
This makes me wonder whether we need to stop the kthreads at all.
How exactly does this make printk more robust during shutdown?
Well, there is one differece. The kthreads could not interferre with
the direct flush when they are stopped.
But we have the same problem also with suspend/resume. And we do not
stop the kthreads in this case.
Maybe, we should just add a check of
system_state > SYSTEM_RUNNING
also into the nbcon_kthread_func(). Maybe, the kthread should not try
to flush the messages when they are flushed directly by
vprintk_emit().
But then there is the problem with "the current owner is responsible
for flushing everything". vprintk_emit() could not flush the messages
when nbcon_atomic_flush_pending() races with the kthread. And if
the kthread stops flushing then nobody would flush the rest
of the pending messages.
Maybe, the notifier could just call pr_flush() like in suspend_console().
Maybe it is not important to stop the kthreads.
But maybe, the messages get flushed before the kthread is stopped.
Also the kthread could not interfere with direct flush once stopped.
Mabye, this is the idea behind "more robust during suspend".
But why is suspend solved another way then?
Is the suspend less important than shutdown?
My opinion:
I am not against stopping the kthreads. But the mechanism of switching
between kthreads and direct flush should be the same for both suspend
and shutdown. And I am not sure if we need to stop the kthreads then.
> + for_each_console(con) {
> + if (con->flags & CON_NBCON)
> + nbcon_kthread_stop(con);
> + }
> + console_list_unlock();
> +}
> +
> +static struct syscore_ops printk_syscore_ops = {
> + .shutdown = printk_kthread_shutdown,
> +};
> +
> +static int __init printk_init_ops(void)
> +{
> + register_syscore_ops(&printk_syscore_ops);
> + return 0;
> +}
> +device_initcall(printk_init_ops);
I guess that "device_initcall" is cut&pasted ;-) IMHO, it does not
fit much. That said, I do not have strong opinion where
it belongs.
IMHO, the best solution would be to register the notifier in
printk_setup_threads() before we actually allow creating them.
(Later update: if we really need to stop them).
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 08/18] printk: nbcon: Stop threads on shutdown/reboot
2024-06-17 15:21 ` Petr Mladek
@ 2024-06-25 19:56 ` John Ogness
2024-06-26 12:14 ` Petr Mladek
0 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-25 19:56 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On 2024-06-17, Petr Mladek <pmladek@suse.com> wrote:
> First ideas (how the waterfall of questions started):
>
> + I though that we should do here:
>
> printk_threads_enabled = false;
>
> It would tell vprintk_emit() to call
> nbcon_atomic_flush_pending(). And it would be symetric with
> printk_setup_threads().
This makes sense. I created @printk_threads_enabled to have a clean
startup of the threads. I did not consider recycling it for shutdown
purposes.
> + Also I though whether to call it before or after stopping the
> kthreads.
>
> If we set the variable before stopping kthreads then the
> kthreads might try to flush the messages in parallel with the
> flush from vprintk_emit(). It should be safe because it would be
> serialized via the nbcon console context. But it would be the
> first scenario where this happen => not much tested.
It is safe. The scenario is not that unusual. Non-printing activities
can contend with thread via the port_lock wrapper quite often.
> Well, we should start the direct flush before stopping kthreads.
> So that we could see the messages when the stopping fails.
>
>
> But then I realized:
>
> Ha, vprintk_emit() would call nbcon_atomic_flush_pending() anyway
> because these notifiers are called with:
>
> system_state > SYSTEM_RUNNING
>
> This makes me wonder whether we need to stop the kthreads at all.
Well, that overlap was not intentional. There probably should be a flag
to signify the transition rather than just looking at @system_state.
> How exactly does this make printk more robust during shutdown?
Because by stopping them, the printing threads are guaranteed to go
silent before they suddenly freeze. Without the clean shutdown, I could
create shutdown/reboot scenarios where the printing thread would stop
mid-line. Then the atomic printing would never be able to finish because
it is a non-panic situation and the thread was frozen while holding the
context. The result: the user never sees the final printk messages.
> Well, there is one differece. The kthreads could not interferre with
> the direct flush when they are stopped.
>
> But we have the same problem also with suspend/resume. And we do not
> stop the kthreads in this case.
Suspend/resume is something different. Suspend needs to stop _all_
printing. I do not think it makes sense to shutdown threads for
this. The consoles becoming temporarily !usable() is enough.
For shutdown, the consoles are usable() the entire time. I just want the
threads to get out of the way before they freeze so that the user can
see all the messages on shutdown/reboot.
> Maybe, we should just add a check of
>
> system_state > SYSTEM_RUNNING
>
> also into the nbcon_kthread_func(). Maybe, the kthread should not try
> to flush the messages when they are flushed directly by
> vprintk_emit().
It gets racy when we start relying on the contexts noticing some
variables. By racy, I mean there are scenarios where there are unprinted
records and no context is printing. I think it is easiest when the
following steps occur within a notifier:
1. notifier sets a flag to allow atomic printing from vprintk_emit()
2. notifier stops all threads (with the kthread_should_stop() moved
inside the printing loop of nbcon_kthread_func())
3. notifier performs nbcon_atomic_flush_pending()
This guarantees that no messages are lost and that all get flushed.
>> +static int __init printk_init_ops(void)
>> +{
>> + register_syscore_ops(&printk_syscore_ops);
>> + return 0;
>> +}
>> +device_initcall(printk_init_ops);
>
> I guess that "device_initcall" is cut&pasted ;-) IMHO, it does not
> fit much. That said, I do not have strong opinion where
> it belongs.
>
> IMHO, the best solution would be to register the notifier in
> printk_setup_threads() before we actually allow creating them.
OK.
John Ogness
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 08/18] printk: nbcon: Stop threads on shutdown/reboot
2024-06-25 19:56 ` John Ogness
@ 2024-06-26 12:14 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-26 12:14 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-25 22:02:30, John Ogness wrote:
> On 2024-06-17, Petr Mladek <pmladek@suse.com> wrote:
> > This makes me wonder whether we need to stop the kthreads at all.
>
> > How exactly does this make printk more robust during shutdown?
>
> Because by stopping them, the printing threads are guaranteed to go
> silent before they suddenly freeze. Without the clean shutdown, I could
> create shutdown/reboot scenarios where the printing thread would stop
> mid-line. Then the atomic printing would never be able to finish because
> it is a non-panic situation and the thread was frozen while holding the
> context. The result: the user never sees the final printk messages.
I see.
> > Well, there is one differece. The kthreads could not interferre with
> > the direct flush when they are stopped.
> >
> > But we have the same problem also with suspend/resume. And we do not
> > stop the kthreads in this case.
>
> Suspend/resume is something different. Suspend needs to stop _all_
> printing. I do not think it makes sense to shutdown threads for
> this. The consoles becoming temporarily !usable() is enough.
Just to be sure that we are on the same page. I made a closer
look at the suspend code. I see 4 important milestones:
1. suspend_console();
The consoles are intentionally stopped after this point.
It allows to suspend the related hardware, for example,
the serial port.
2. pm_sleep_disable_secondary_cpus();
Other CPUs are stopped at this point. The kthread might still
be theoretically scheduled.
3. local_irq_disable();
IRQs get disabled. Neither the hardware nor the kthread could
work after this point.
4. system_state = SYSTEM_SUSPEND;
The global value is set. vprintk_emit() starts calling
nbcon_atomic_flush_pending().
From some reasons, I thought that ordering was different and
the "system_state" was set much earlier. This is why I thought
that it would make sense to "stop" the kthreads.
But it seems that, in reality, the nbcon_atomic_flush_pending() in
vprintk_emit() would never do anything during suspend. The consoles
are already suspended (unusable) when the system_state gets set
to SYSTEM_SUSPEND.
It means that we do not need to synchronize the printk kthreads with
vprintk_emit() for suspend. The only important thing is
the pr_flush() in suspend_console().
> For shutdown, the consoles are usable() the entire time. I just want the
> threads to get out of the way before they freeze so that the user can
> see all the messages on shutdown/reboot.
I see what you mean now. The consoles are not suspended => they are
usable all the time.
> > Maybe, we should just add a check of
> >
> > system_state > SYSTEM_RUNNING
> >
> > also into the nbcon_kthread_func(). Maybe, the kthread should not try
> > to flush the messages when they are flushed directly by
> > vprintk_emit().
>
> It gets racy when we start relying on the contexts noticing some
> variables. By racy, I mean there are scenarios where there are unprinted
> records and no context is printing. I think it is easiest when the
> following steps occur within a notifier:
>
> 1. notifier sets a flag to allow atomic printing from vprintk_emit()
>
> 2. notifier stops all threads (with the kthread_should_stop() moved
> inside the printing loop of nbcon_kthread_func())
>
> 3. notifier performs nbcon_atomic_flush_pending()
>
> This guarantees that no messages are lost and that all get flushed.
Makes sense.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 09/18] printk: nbcon: Start printing threads
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (7 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 08/18] printk: nbcon: Stop threads on shutdown/reboot John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-18 15:34 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 10/18] printk: Provide helper for message prepending John Ogness
` (9 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
If there are no boot consoles, the printing threads are started
in early_initcall.
If there are boot consoles, the printing threads are started
after the last boot console has unregistered. The printing
threads do not need to be concerned about boot consoles because
boot consoles cannot register once a non-boot console has
registered.
Until a printing thread of a console has started, that console
will print using atomic_write() in the printk() caller context.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/internal.h | 2 ++
kernel/printk/nbcon.c | 18 +++++++++++++++++-
kernel/printk/printk.c | 14 ++++++++++++++
3 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index b2b5c10a4321..5d4722310850 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -48,6 +48,7 @@ struct printk_ringbuffer;
struct dev_printk_info;
extern struct printk_ringbuffer *prb;
+extern bool printk_threads_enabled;
__printf(4, 0)
int vprintk_store(int facility, int level,
@@ -161,6 +162,7 @@ static inline void nbcon_kthread_wake(struct console *con)
static inline void nbcon_kthread_wake(struct console *con) { }
static inline void nbcon_kthread_create(struct console *con) { }
+#define printk_threads_enabled (false)
/*
* In !PRINTK builds we still export console_sem
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index f60d47b5db8a..480a0ced2708 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -213,6 +213,8 @@ static void nbcon_seq_try_update(struct nbcon_context *ctxt, u64 new_seq)
}
}
+bool printk_threads_enabled __ro_after_init;
+
/**
* nbcon_context_try_acquire_direct - Try to acquire directly
* @ctxt: The context of the caller
@@ -1542,7 +1544,7 @@ void nbcon_kthread_create(struct console *con)
if (!(con->flags & CON_NBCON) || !con->write_thread)
return;
- if (con->kthread)
+ if (!printk_threads_enabled || con->kthread)
return;
/*
@@ -1568,6 +1570,19 @@ void nbcon_kthread_create(struct console *con)
sched_set_normal(con->kthread, -20);
}
+static int __init printk_setup_threads(void)
+{
+ struct console *con;
+
+ console_list_lock();
+ printk_threads_enabled = true;
+ for_each_console(con)
+ nbcon_kthread_create(con);
+ console_list_unlock();
+ return 0;
+}
+early_initcall(printk_setup_threads);
+
/**
* nbcon_alloc - Allocate buffers needed by the nbcon console
* @con: Console to allocate buffers for
@@ -1617,6 +1632,7 @@ void nbcon_init(struct console *con, u64 init_seq)
init_irq_work(&con->irq_work, nbcon_irq_work);
nbcon_seq_force(con, init_seq);
nbcon_state_set(con, &state);
+ nbcon_kthread_create(con);
}
/**
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 457c01311a95..4613f9145a8e 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2389,6 +2389,9 @@ asmlinkage int vprintk_emit(int facility, int level,
*
* - When this CPU is in panic.
*
+ * - When booting, before the printing threads have been
+ * started.
+ *
* - During shutdown, since the printing threads may not get
* a chance to print the final messages.
*
@@ -2397,6 +2400,7 @@ asmlinkage int vprintk_emit(int facility, int level,
* consoles cannot print simultaneously with boot consoles.
*/
if (is_panic_context ||
+ !printk_threads_enabled ||
(system_state > SYSTEM_RUNNING)) {
nbcon_atomic_flush_pending();
}
@@ -3742,6 +3746,7 @@ EXPORT_SYMBOL(register_console);
static int unregister_console_locked(struct console *console)
{
bool use_device_lock = (console->flags & CON_NBCON) && console->write_atomic;
+ bool is_boot_con = (console->flags & CON_BOOT);
bool found_legacy_con = false;
bool found_nbcon_con = false;
bool found_boot_con = false;
@@ -3824,6 +3829,15 @@ static int unregister_console_locked(struct console *console)
if (!found_nbcon_con)
have_nbcon_console = found_nbcon_con;
+ /*
+ * When the last boot console unregisters, start up the
+ * printing threads.
+ */
+ if (is_boot_con && !have_boot_console) {
+ for_each_console(c)
+ nbcon_kthread_create(c);
+ }
+
return res;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 09/18] printk: nbcon: Start printing threads
2024-06-03 23:24 ` [PATCH printk v2 09/18] printk: nbcon: Start printing threads John Ogness
@ 2024-06-18 15:34 ` Petr Mladek
2024-06-19 15:13 ` Petr Mladek
0 siblings, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-06-18 15:34 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-04 01:30:44, John Ogness wrote:
> If there are no boot consoles, the printing threads are started
> in early_initcall.
>
> If there are boot consoles, the printing threads are started
> after the last boot console has unregistered. The printing
> threads do not need to be concerned about boot consoles because
> boot consoles cannot register once a non-boot console has
> registered.
>
> Until a printing thread of a console has started, that console
> will print using atomic_write() in the printk() caller context.
>
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1568,6 +1570,19 @@ void nbcon_kthread_create(struct console *con)
> sched_set_normal(con->kthread, -20);
> }
>
> +static int __init printk_setup_threads(void)
> +{
> + struct console *con;
> +
> + console_list_lock();
> + printk_threads_enabled = true;
What is the actual meaning of the variable?
Does it mean that kthreads can be created? (can be forked?)
It does not guarantee that the kthreads will be running.
They might still get blocked by a boot console.
> + for_each_console(con)
> + nbcon_kthread_create(con);
> + console_list_unlock();
> + return 0;
> +}
> +early_initcall(printk_setup_threads);
> +
> /**
> * nbcon_alloc - Allocate buffers needed by the nbcon console
> * @con: Console to allocate buffers for
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2397,6 +2400,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> * consoles cannot print simultaneously with boot consoles.
> */
> if (is_panic_context ||
> + !printk_threads_enabled ||
> (system_state > SYSTEM_RUNNING)) {
> nbcon_atomic_flush_pending();
IMHO, this is not safe. nbcon_atomic_flush_pending()
is synchronized only via the nbcon console context.
It means that there might be races with boot consoles.
Another problem is the meaning of @printk_threads_enabled variable.
It does not guarantee that the kthreads are running. They might
still be blocked by boot consoles.
BTW: The same problem might have the system_state > SYSTEM_RUNNING.
The boot consoles are removed only when the preferred console
is registered and "keep_bootcon" is not set.
Note that printk_late_init() unregisters the boot consoles
only when they are using a code in the init sections.
> }
My view:
We need to enable creating the kthreads when:
1. the "kthreadd_task" is running. It is a kthread responsible for
forking new kthreads. And it is started after the init task.
2. There is no boot console.
Plus we could use con->write_atomic() only under console_lock()
when there is still a boot console.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 09/18] printk: nbcon: Start printing threads
2024-06-18 15:34 ` Petr Mladek
@ 2024-06-19 15:13 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-19 15:13 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-18 17:34:22, Petr Mladek wrote:
> On Tue 2024-06-04 01:30:44, John Ogness wrote:
> > If there are no boot consoles, the printing threads are started
> > in early_initcall.
> >
> > If there are boot consoles, the printing threads are started
> > after the last boot console has unregistered. The printing
> > threads do not need to be concerned about boot consoles because
> > boot consoles cannot register once a non-boot console has
> > registered.
> >
> > Until a printing thread of a console has started, that console
> > will print using atomic_write() in the printk() caller context.
>
> > --- a/kernel/printk/nbcon.c
> > +++ b/kernel/printk/nbcon.c
> > @@ -1568,6 +1570,19 @@ void nbcon_kthread_create(struct console *con)
> > sched_set_normal(con->kthread, -20);
> > }
> >
> > +static int __init printk_setup_threads(void)
> > +{
> > + struct console *con;
> > +
> > + console_list_lock();
> > + printk_threads_enabled = true;
>
> What is the actual meaning of the variable?
>
> Does it mean that kthreads can be created? (can be forked?)
>
> It does not guarantee that the kthreads will be running.
> They might still get blocked by a boot console.
I have investigated it more. And it looks like the variable
means that the system is initialized enough to support
the kthreads.
It is similar to the variable __printk_percpu_data_ready.
I am not sure if it would have helped to prevent the confusion
but I would suggest to rename it and add a comment:
/*
* The graphics console drivers are going to use workqueues.
* This variable is set to true when they are safe to use.
*/
static bool __printk_threads_ready __ro_after_init;
>
> > + for_each_console(con)
> > + nbcon_kthread_create(con);
Also we should make it more clear when this function is supposed
to succeed. I mean:
if (have_nbcon_console && !have_boot_console)
for_each_console(con)
nbcon_kthread_create(con);
And similar in unregister_console()
/*
* When the last boot console unregisters, start up the
* printing threads.
*/
if (is_boot_con && have_nbcon_console && !have_boot_console) {
for_each_console(c)
nbcon_kthread_create(c);
}
The same condition (have_nbcon_console && !have_boot_console)
is used on many other locations.
In another mail, I suggested to define a macro for this condition:
#define printing_via_context_safe (have_nbcon_console && !have_boot_console)
It might be even better because it would connect the related
code via cscope.
Hmm, the "_safe" suffix might be confusing in this context.
A better name might be "printing_via_nbcon_context" so that
we have:
/* Printing serialized by console_lock dance is needed. */
#define printing_via_unlock (have_legacy_console || have_boot_console)
/* Printing serialized by nbcon console context is needed and safe. */
#define printing_via_nbcon_context (have_nbcon_console && !have_boot_console)
> > + console_list_unlock();
> > + return 0;
> > +}
> > +early_initcall(printk_setup_threads);
> > +
> > /**
> > * nbcon_alloc - Allocate buffers needed by the nbcon console
> > * @con: Console to allocate buffers for
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2397,6 +2400,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> > * consoles cannot print simultaneously with boot consoles.
> > */
> > if (is_panic_context ||
> > + !printk_threads_enabled ||
> > (system_state > SYSTEM_RUNNING)) {
> > nbcon_atomic_flush_pending();
>
> IMHO, this is not safe. nbcon_atomic_flush_pending()
> is synchronized only via the nbcon console context.
> It means that there might be races with boot consoles.
>
> Another problem is the meaning of @printk_threads_enabled variable.
> It does not guarantee that the kthreads are running. They might
> still be blocked by boot consoles.
>
>
> BTW: The same problem might have the system_state > SYSTEM_RUNNING.
> The boot consoles are removed only when the preferred console
> is registered and "keep_bootcon" is not set.
>
> Note that printk_late_init() unregisters the boot consoles
> only when they are using a code in the init sections.
>
> > }
I take it back. We are on the safe side because the above code is
called only when
if (have_nbcon_console && !have_boot_console)
This is why I suggest to use the same condition around the code
starting the kthreads. It might help to create the mental
model.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 10/18] printk: Provide helper for message prepending
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (8 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 09/18] printk: nbcon: Start printing threads John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-20 10:28 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 11/18] printk: nbcon: Show replay message on takeover John Ogness
` (8 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
In order to support prepending different texts to printk
messages, split out the prepending code into a helper
function.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/printk.c | 38 +++++++++++++++++++++++++++-----------
1 file changed, 27 insertions(+), 11 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 4613f9145a8e..fcbf794a0aaf 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2806,30 +2806,25 @@ static void __console_unlock(void)
#ifdef CONFIG_PRINTK
/*
- * Prepend the message in @pmsg->pbufs->outbuf with a "dropped message". This
- * is achieved by shifting the existing message over and inserting the dropped
- * message.
+ * Prepend the message in @pmsg->pbufs->outbuf with the message in
+ * @pmsg->pbufs->scratchbuf. This is achieved by shifting the existing message
+ * over and inserting the scratchbuf message.
*
* @pmsg is the printk message to prepend.
*
- * @dropped is the dropped count to report in the dropped message.
+ * @len is the length of the message in @pmsg->pbufs->scratchbuf.
*
* If the message text in @pmsg->pbufs->outbuf does not have enough space for
- * the dropped message, the message text will be sufficiently truncated.
+ * the scratchbuf message, the message text will be sufficiently truncated.
*
* If @pmsg->pbufs->outbuf is modified, @pmsg->outbuf_len is updated.
*/
-void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
+static void __console_prepend_scratch(struct printk_message *pmsg, size_t len)
{
struct printk_buffers *pbufs = pmsg->pbufs;
- const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
const size_t outbuf_sz = sizeof(pbufs->outbuf);
char *scratchbuf = &pbufs->scratchbuf[0];
char *outbuf = &pbufs->outbuf[0];
- size_t len;
-
- len = scnprintf(scratchbuf, scratchbuf_sz,
- "** %lu printk messages dropped **\n", dropped);
/*
* Make sure outbuf is sufficiently large before prepending.
@@ -2851,6 +2846,27 @@ void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
pmsg->outbuf_len += len;
}
+/*
+ * Prepend the message in @pmsg->pbufs->outbuf with a "dropped message".
+ * @pmsg->outbuf_len is updated appropriately.
+ *
+ * @pmsg is the printk message to prepend.
+ *
+ * @dropped is the dropped count to report in the dropped message.
+ */
+void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
+{
+ struct printk_buffers *pbufs = pmsg->pbufs;
+ const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
+ char *scratchbuf = &pbufs->scratchbuf[0];
+ size_t len;
+
+ len = scnprintf(scratchbuf, scratchbuf_sz,
+ "** %lu printk messages dropped **\n", dropped);
+
+ __console_prepend_scratch(pmsg, len);
+}
+
/*
* Read and format the specified record (or a later record if the specified
* record is not available).
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 10/18] printk: Provide helper for message prepending
2024-06-03 23:24 ` [PATCH printk v2 10/18] printk: Provide helper for message prepending John Ogness
@ 2024-06-20 10:28 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-20 10:28 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-04 01:30:45, John Ogness wrote:
> In order to support prepending different texts to printk
> messages, split out the prepending code into a helper
> function.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> kernel/printk/printk.c | 38 +++++++++++++++++++++++++++-----------
> 1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 4613f9145a8e..fcbf794a0aaf 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2806,30 +2806,25 @@ static void __console_unlock(void)
> #ifdef CONFIG_PRINTK
>
> /*
> - * Prepend the message in @pmsg->pbufs->outbuf with a "dropped message". This
> - * is achieved by shifting the existing message over and inserting the dropped
> - * message.
> + * Prepend the message in @pmsg->pbufs->outbuf with the message in
> + * @pmsg->pbufs->scratchbuf. This is achieved by shifting the existing message
> + * over and inserting the scratchbuf message.
> *
> * @pmsg is the printk message to prepend.
> *
> - * @dropped is the dropped count to report in the dropped message.
> + * @len is the length of the message in @pmsg->pbufs->scratchbuf.
> *
> * If the message text in @pmsg->pbufs->outbuf does not have enough space for
> - * the dropped message, the message text will be sufficiently truncated.
> + * the scratchbuf message, the message text will be sufficiently truncated.
> *
> * If @pmsg->pbufs->outbuf is modified, @pmsg->outbuf_len is updated.
> */
> -void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
> +static void __console_prepend_scratch(struct printk_message *pmsg, size_t len)
> {
> struct printk_buffers *pbufs = pmsg->pbufs;
> - const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
> const size_t outbuf_sz = sizeof(pbufs->outbuf);
> char *scratchbuf = &pbufs->scratchbuf[0];
> char *outbuf = &pbufs->outbuf[0];
> - size_t len;
> -
> - len = scnprintf(scratchbuf, scratchbuf_sz,
> - "** %lu printk messages dropped **\n", dropped);
>
> /*
> * Make sure outbuf is sufficiently large before prepending.
> @@ -2851,6 +2846,27 @@ void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
> pmsg->outbuf_len += len;
> }
>
> +/*
> + * Prepend the message in @pmsg->pbufs->outbuf with a "dropped message".
> + * @pmsg->outbuf_len is updated appropriately.
> + *
> + * @pmsg is the printk message to prepend.
> + *
> + * @dropped is the dropped count to report in the dropped message.
> + */
> +void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
> +{
> + struct printk_buffers *pbufs = pmsg->pbufs;
> + const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
> + char *scratchbuf = &pbufs->scratchbuf[0];
> + size_t len;
> +
> + len = scnprintf(scratchbuf, scratchbuf_sz,
> + "** %lu printk messages dropped **\n", dropped);
> +
> + __console_prepend_scratch(pmsg, len);
> +}
There is a lot of boilerplate code around the custom part. A better
solution might be to allow passing a printf format to the function
prepending the existing message.
I have tried how it would look like. Here is the patch. Note that it
is only compile-only tested:
From 853a3761f4e0f13970ffc71d9463f79eaa06fca1 Mon Sep 17 00:00:00 2001
From: John Ogness <john.ogness@linutronix.de>
Date: Tue, 4 Jun 2024 01:30:45 +0206
Subject: [PATCH] printk: Provide helper for message prepending
In order to support prepending different texts to printk
messages, split out the prepending code into a helper
function.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
kernel/printk/printk.c | 36 +++++++++++++++++++++++++-----------
1 file changed, 25 insertions(+), 11 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 3e07e9884907..b0a6aab792fc 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2806,30 +2806,31 @@ static void __console_unlock(void)
#ifdef CONFIG_PRINTK
/*
- * Prepend the message in @pmsg->pbufs->outbuf with a "dropped message". This
- * is achieved by shifting the existing message over and inserting the dropped
- * message.
+ * Prepend the message in @pmsg->pbufs->outbuf. This is achieved by shifting
+ * the existing message over and inserting the scratchbuf message.
*
- * @pmsg is the printk message to prepend.
+ * @pmsg is the original printk message.
+ * @fmt is the printf format of the message which will prepend the existing one.
*
- * @dropped is the dropped count to report in the dropped message.
- *
- * If the message text in @pmsg->pbufs->outbuf does not have enough space for
- * the dropped message, the message text will be sufficiently truncated.
+ * If there is not enough space in @pmsg->pbufs->outbuf, the existing
+ * message text will be sufficiently truncated.
*
* If @pmsg->pbufs->outbuf is modified, @pmsg->outbuf_len is updated.
*/
-void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
+__printf(2, 3)
+static void console_prepend_message(struct printk_message *pmsg, const char *fmt, ...)
{
struct printk_buffers *pbufs = pmsg->pbufs;
const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
const size_t outbuf_sz = sizeof(pbufs->outbuf);
char *scratchbuf = &pbufs->scratchbuf[0];
char *outbuf = &pbufs->outbuf[0];
+ va_list args;
size_t len;
- len = scnprintf(scratchbuf, scratchbuf_sz,
- "** %lu printk messages dropped **\n", dropped);
+ va_start(args, fmt);
+ len = vscnprintf(scratchbuf, scratchbuf_sz, fmt, args);
+ va_end(args);
/*
* Make sure outbuf is sufficiently large before prepending.
@@ -2851,6 +2852,19 @@ void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
pmsg->outbuf_len += len;
}
+/*
+ * Prepend the message in @pmsg->pbufs->outbuf with a "dropped message".
+ * @pmsg->outbuf_len is updated appropriately.
+ *
+ * @pmsg is the printk message to prepend.
+ *
+ * @dropped is the dropped count to report in the dropped message.
+ */
+void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
+{
+ console_prepend_message(pmsg, "** %lu printk messages dropped **\n", dropped);
+}
+
/*
* Read and format the specified record (or a later record if the specified
* record is not available).
--
2.45.2
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH printk v2 11/18] printk: nbcon: Show replay message on takeover
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (9 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 10/18] printk: Provide helper for message prepending John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-20 13:02 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles John Ogness
` (7 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
An emergency or panic context can takeover console ownership
while the current owner was printing a printk message. The
atomic printer will re-print the message that the previous
owner was printing. However, this can look confusing to the
user and may even seem as though a message was lost.
[3430014.1
[3430014.181123] usb 1-2: Product: USB Audio
Add a new field @nbcon_prev_seq to struct console to track
the sequence number to print that was assigned to the previous
console owner. If this matches the sequence number to print
that the current owner is assigned, then a takeover must have
occurred. In this case, print an additional message to inform
the user that the previous message is being printed again.
[3430014.1
** replaying previous printk message **
[3430014.181123] usb 1-2: Product: USB Audio
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
include/linux/console.h | 3 +++
kernel/printk/internal.h | 1 +
kernel/printk/nbcon.c | 24 ++++++++++++++++++++++++
kernel/printk/printk.c | 19 +++++++++++++++++++
4 files changed, 47 insertions(+)
diff --git a/include/linux/console.h b/include/linux/console.h
index adcc1a74aeee..9927f08ac054 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -325,6 +325,7 @@ struct nbcon_write_context {
* @nbcon_state: State for nbcon consoles
* @nbcon_seq: Sequence number of the next record for nbcon to print
* @nbcon_device_ctxt: Context available for non-printing operations
+ * @nbcon_prev_seq: Seq num the previous nbcon owner was assigned to print
* @pbufs: Pointer to nbcon private buffer
* @kthread: Printer kthread for this console
* @rcuwait: RCU-safe wait object for @kthread waking
@@ -445,6 +446,8 @@ struct console {
atomic_t __private nbcon_state;
atomic_long_t __private nbcon_seq;
struct nbcon_context __private nbcon_device_ctxt;
+ atomic_long_t __private nbcon_prev_seq;
+
struct printk_buffers *pbufs;
struct task_struct *kthread;
struct rcuwait rcuwait;
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 5d4722310850..9e027e08918d 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -233,4 +233,5 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
#ifdef CONFIG_PRINTK
void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped);
+void console_prepend_replay(struct printk_message *pmsg);
#endif
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 480a0ced2708..8c92b076c745 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -866,6 +866,7 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
unsigned long con_dropped;
struct nbcon_state cur;
unsigned long dropped;
+ unsigned long ulseq;
/*
* The printk buffers are filled within an unsafe section. This
@@ -891,6 +892,28 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
if (dropped && !is_extended)
console_prepend_dropped(&pmsg, dropped);
+ /*
+ * If the previous owner was assigned the same record, this context
+ * has taken over ownership and is replaying the record. Prepend a
+ * message to let the user know the record is replayed.
+ */
+ ulseq = atomic_long_read(&ACCESS_PRIVATE(con, nbcon_prev_seq));
+ if (__ulseq_to_u64seq(prb, ulseq) == pmsg.seq) {
+ console_prepend_replay(&pmsg);
+ } else {
+ /*
+ * Ensure this context is still the owner before trying to
+ * update @nbcon_prev_seq. Otherwise the value in @ulseq may
+ * not be from the previous owner.
+ */
+ nbcon_state_read(con, &cur);
+ if (!nbcon_context_can_proceed(ctxt, &cur))
+ return false;
+
+ atomic_long_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_prev_seq), &ulseq,
+ __u64seq_to_ulseq(pmsg.seq));
+ }
+
if (!nbcon_context_exit_unsafe(ctxt))
return false;
@@ -1631,6 +1654,7 @@ void nbcon_init(struct console *con, u64 init_seq)
rcuwait_init(&con->rcuwait);
init_irq_work(&con->irq_work, nbcon_irq_work);
nbcon_seq_force(con, init_seq);
+ atomic_long_set(&ACCESS_PRIVATE(con, nbcon_prev_seq), -1UL);
nbcon_state_set(con, &state);
nbcon_kthread_create(con);
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index fcbf794a0aaf..cf0d612329bf 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2867,6 +2867,25 @@ void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
__console_prepend_scratch(pmsg, len);
}
+/*
+ * Prepend the message in @pmsg->pbufs->outbuf with a "replay message".
+ * @pmsg->outbuf_len is updated appropriately.
+ *
+ * @pmsg is the printk message to prepend.
+ */
+void console_prepend_replay(struct printk_message *pmsg)
+{
+ struct printk_buffers *pbufs = pmsg->pbufs;
+ const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
+ char *scratchbuf = &pbufs->scratchbuf[0];
+ size_t len;
+
+ len = scnprintf(scratchbuf, scratchbuf_sz,
+ "** replaying previous printk message **\n");
+
+ __console_prepend_scratch(pmsg, len);
+}
+
/*
* Read and format the specified record (or a later record if the specified
* record is not available).
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 11/18] printk: nbcon: Show replay message on takeover
2024-06-03 23:24 ` [PATCH printk v2 11/18] printk: nbcon: Show replay message on takeover John Ogness
@ 2024-06-20 13:02 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-06-20 13:02 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Tue 2024-06-04 01:30:46, John Ogness wrote:
> An emergency or panic context can takeover console ownership
> while the current owner was printing a printk message. The
> atomic printer will re-print the message that the previous
> owner was printing. However, this can look confusing to the
> user and may even seem as though a message was lost.
>
> [3430014.1
> [3430014.181123] usb 1-2: Product: USB Audio
>
> Add a new field @nbcon_prev_seq to struct console to track
> the sequence number to print that was assigned to the previous
> console owner. If this matches the sequence number to print
> that the current owner is assigned, then a takeover must have
> occurred. In this case, print an additional message to inform
> the user that the previous message is being printed again.
>
> [3430014.1
> ** replaying previous printk message **
> [3430014.181123] usb 1-2: Product: USB Audio
>
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -891,6 +892,28 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
> if (dropped && !is_extended)
> console_prepend_dropped(&pmsg, dropped);
>
> + /*
> + * If the previous owner was assigned the same record, this context
> + * has taken over ownership and is replaying the record. Prepend a
> + * message to let the user know the record is replayed.
> + */
> + ulseq = atomic_long_read(&ACCESS_PRIVATE(con, nbcon_prev_seq));
> + if (__ulseq_to_u64seq(prb, ulseq) == pmsg.seq) {
> + console_prepend_replay(&pmsg);
> + } else {
> + /*
> + * Ensure this context is still the owner before trying to
> + * update @nbcon_prev_seq. Otherwise the value in @ulseq may
> + * not be from the previous owner.
> + */
> + nbcon_state_read(con, &cur);
> + if (!nbcon_context_can_proceed(ctxt, &cur))
> + return false;
I would like to better understand the motivation here.
I guess that this pattern might be repeated in the future.
For example, it might bring the question why we do not do
the same in nbcon_seq_try_update().
OK, this is called in context where the takeover is unsafe.
It could race only with nbcon_atomic_flush_unsafe()
which is called at the very end of panic().
So we have:
+ The race is a corner case.
+ It could happen only once beause non-panic() CPUs should
not be able to acquire the console context any more.
+ The check reduces the race window but it does not prevent
the race.
Do we care about these races?
BTW: In this particular case, the race looks innocent. It might
happen only once. And the cmpxchg would fail in this case.
I would replace the check with a comment:
/*
* Only nbcon_atomic_flush_unsafe() could take over this
* context. It would be the only context which could move
* forward from now on. The try_cmpxchg() would fail in
* this case. It should succeed and be safe in all other
* scenarios.
*/
> +
> + atomic_long_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_prev_seq), &ulseq,
> + __u64seq_to_ulseq(pmsg.seq));
> + }
> +
> if (!nbcon_context_exit_unsafe(ctxt))
> return false;
>
Othrewise, it looks good.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (10 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 11/18] printk: nbcon: Show replay message on takeover John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-28 11:46 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 13/18] proc: consoles: Add notation to c_start/c_stop John Ogness
` (6 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
The write callback of legacy consoles makes use of spinlocks.
This is not permitted with PREEMPT_RT in atomic contexts.
For PREEMPT_RT, create a new kthread to handle printing of all
the legacy consoles (and nbcon consoles if boot consoles are
registered).
Since, if running from the kthread, the consoles are printing
in a task context, the legacy nbcon printing can use the
device_lock(), write_thread(), device_unlock() callbacks for
printing.
Introduce the macro force_printkthreads() to query if the
forced threading of legacy consoles is in effect.
These changes only affect CONFIG_PREEMPT_RT.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/internal.h | 11 +-
kernel/printk/nbcon.c | 54 ++++++---
kernel/printk/printk.c | 241 ++++++++++++++++++++++++++++++++-------
3 files changed, 246 insertions(+), 60 deletions(-)
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 9e027e08918d..b66dfa865591 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -21,6 +21,12 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
(con->flags & CON_BOOT) ? "boot" : "", \
con->name, con->index, ##__VA_ARGS__)
+#ifdef CONFIG_PREEMPT_RT
+# define force_printkthreads() (true)
+#else
+# define force_printkthreads() (false)
+#endif
+
#ifdef CONFIG_PRINTK
#ifdef CONFIG_PRINTK_CALLER
@@ -92,9 +98,10 @@ void nbcon_free(struct console *con);
enum nbcon_prio nbcon_get_default_prio(void);
void nbcon_atomic_flush_pending(void);
bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
- int cookie);
+ int cookie, bool use_atomic);
void nbcon_kthread_create(struct console *con);
void nbcon_wake_threads(void);
+void nbcon_legacy_kthread_create(void);
/*
* Check if the given console is currently capable and allowed to print
@@ -181,7 +188,7 @@ static inline void nbcon_free(struct console *con) { }
static inline enum nbcon_prio nbcon_get_default_prio(void) { return NBCON_PRIO_NONE; }
static inline void nbcon_atomic_flush_pending(void) { }
static inline bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
- int cookie) { return false; }
+ int cookie, bool use_atomic) { return false; }
static inline bool console_is_usable(struct console *con, short flags,
bool use_atomic) { return false; }
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 8c92b076c745..e8060b5abdf8 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1185,9 +1185,10 @@ enum nbcon_prio nbcon_get_default_prio(void)
}
/*
- * nbcon_atomic_emit_one - Print one record for an nbcon console using the
- * write_atomic() callback
+ * nbcon_emit_one - Print one record for an nbcon console using the
+ * specified callback
* @wctxt: An initialized write context struct to use for this context
+ * @use_atomic: True if the write_atomic() callback is to be used
*
* Return: True, when a record has been printed and there are still
* pending records. The caller might want to continue flushing.
@@ -1200,7 +1201,7 @@ enum nbcon_prio nbcon_get_default_prio(void)
* This is an internal helper to handle the locking of the console before
* calling nbcon_emit_next_record().
*/
-static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
+static bool nbcon_emit_one(struct nbcon_write_context *wctxt, bool use_atomic)
{
struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
@@ -1215,7 +1216,7 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
* The higher priority printing context takes over responsibility
* to print the pending records.
*/
- if (!nbcon_emit_next_record(wctxt, true))
+ if (!nbcon_emit_next_record(wctxt, use_atomic))
return false;
nbcon_context_release(ctxt);
@@ -1232,6 +1233,7 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
* both the console_lock and the SRCU read lock. Otherwise it
* is set to false.
* @cookie: The cookie from the SRCU read lock.
+ * @use_atomic: True if the write_atomic() callback is to be used
*
* Context: Any context except NMI.
* Return: True, when a record has been printed and there are still
@@ -1247,26 +1249,38 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
* Essentially it is the nbcon version of console_emit_next_record().
*/
bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
- int cookie)
+ int cookie, bool use_atomic)
{
struct nbcon_write_context wctxt = { };
struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
unsigned long flags;
bool progress;
- /* Use the same procedure as console_emit_next_record(). */
- printk_safe_enter_irqsave(flags);
- console_lock_spinning_enable();
- stop_critical_timings();
+ ctxt->console = con;
- ctxt->console = con;
- ctxt->prio = nbcon_get_default_prio();
+ if (use_atomic) {
+ /* Use the same procedure as console_emit_next_record(). */
+ printk_safe_enter_irqsave(flags);
+ console_lock_spinning_enable();
+ stop_critical_timings();
- progress = nbcon_atomic_emit_one(&wctxt);
+ ctxt->prio = nbcon_get_default_prio();
+ progress = nbcon_emit_one(&wctxt, use_atomic);
- start_critical_timings();
- *handover = console_lock_spinning_disable_and_check(cookie);
- printk_safe_exit_irqrestore(flags);
+ start_critical_timings();
+ *handover = console_lock_spinning_disable_and_check(cookie);
+ printk_safe_exit_irqrestore(flags);
+ } else {
+ *handover = false;
+
+ con->device_lock(con, &flags);
+ cant_migrate();
+
+ ctxt->prio = nbcon_get_default_prio();
+ progress = nbcon_emit_one(&wctxt, use_atomic);
+
+ con->device_unlock(con, flags);
+ }
return progress;
}
@@ -1494,7 +1508,9 @@ void nbcon_cpu_emergency_exit(void)
* to handle it.
*/
do_trigger_flush = true;
- if (printing_via_unlock && !is_printk_deferred()) {
+ if (!force_printkthreads() &&
+ printing_via_unlock &&
+ !is_printk_deferred()) {
if (console_trylock()) {
do_trigger_flush = false;
console_unlock();
@@ -1530,7 +1546,9 @@ void nbcon_cpu_emergency_flush(void)
nbcon_atomic_flush_pending();
- if (printing_via_unlock && !is_printk_deferred()) {
+ if (!force_printkthreads() &&
+ printing_via_unlock &&
+ !is_printk_deferred()) {
if (console_trylock())
console_unlock();
}
@@ -1601,6 +1619,8 @@ static int __init printk_setup_threads(void)
printk_threads_enabled = true;
for_each_console(con)
nbcon_kthread_create(con);
+ if (force_printkthreads() && printing_via_unlock)
+ nbcon_legacy_kthread_create();
console_list_unlock();
return 0;
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index cf0d612329bf..1c63fd0c1166 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -487,6 +487,9 @@ bool have_boot_console;
#ifdef CONFIG_PRINTK
DECLARE_WAIT_QUEUE_HEAD(log_wait);
+
+static DECLARE_WAIT_QUEUE_HEAD(legacy_wait);
+
/* All 3 protected by @syslog_lock. */
/* the next printk record to read by syslog(READ) or /proc/kmsg */
static u64 syslog_seq;
@@ -2345,7 +2348,8 @@ asmlinkage int vprintk_emit(int facility, int level,
const struct dev_printk_info *dev_info,
const char *fmt, va_list args)
{
- bool do_trylock_unlock = printing_via_unlock;
+ bool do_trylock_unlock = !force_printkthreads() &&
+ printing_via_unlock;
int printed_len;
/* Suppress unimportant messages after panic happens */
@@ -2468,6 +2472,14 @@ EXPORT_SYMBOL(_printk);
static bool pr_flush(int timeout_ms, bool reset_on_progress);
static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress);
+static struct task_struct *nbcon_legacy_kthread;
+
+static inline void wake_up_legacy_kthread(void)
+{
+ if (nbcon_legacy_kthread)
+ wake_up_interruptible(&legacy_wait);
+}
+
#else /* CONFIG_PRINTK */
#define printk_time false
@@ -2481,6 +2493,8 @@ static u64 syslog_seq;
static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
+static inline void nbcon_legacy_kthread_create(void) { }
+static inline void wake_up_legacy_kthread(void) { }
#endif /* CONFIG_PRINTK */
#ifdef CONFIG_EARLY_PRINTK
@@ -2726,6 +2740,8 @@ void resume_console(void)
}
console_srcu_read_unlock(cookie);
+ wake_up_legacy_kthread();
+
pr_flush(1000, true);
}
@@ -2740,7 +2756,9 @@ void resume_console(void)
*/
static int console_cpu_notify(unsigned int cpu)
{
- if (!cpuhp_tasks_frozen && printing_via_unlock) {
+ if (!force_printkthreads() &&
+ !cpuhp_tasks_frozen &&
+ printing_via_unlock) {
/* If trylock fails, someone else is doing the printing */
if (console_trylock())
console_unlock();
@@ -3000,31 +3018,43 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
con->dropped = 0;
}
- /*
- * While actively printing out messages, if another printk()
- * were to occur on another CPU, it may wait for this one to
- * finish. This task can not be preempted if there is a
- * waiter waiting to take over.
- *
- * Interrupts are disabled because the hand over to a waiter
- * must not be interrupted until the hand over is completed
- * (@console_waiter is cleared).
- */
- printk_safe_enter_irqsave(flags);
- console_lock_spinning_enable();
+ /* Write everything out to the hardware. */
- /* Do not trace print latency. */
- stop_critical_timings();
+ if (force_printkthreads()) {
+ /*
+ * With forced threading this function is either in a thread
+ * or panic context. So there is no need for concern about
+ * printk reentrance or handovers.
+ */
- /* Write everything out to the hardware. */
- con->write(con, outbuf, pmsg.outbuf_len);
+ con->write(con, outbuf, pmsg.outbuf_len);
+ con->seq = pmsg.seq + 1;
+ } else {
+ /*
+ * While actively printing out messages, if another printk()
+ * were to occur on another CPU, it may wait for this one to
+ * finish. This task can not be preempted if there is a
+ * waiter waiting to take over.
+ *
+ * Interrupts are disabled because the hand over to a waiter
+ * must not be interrupted until the hand over is completed
+ * (@console_waiter is cleared).
+ */
+ printk_safe_enter_irqsave(flags);
+ console_lock_spinning_enable();
- start_critical_timings();
+ /* Do not trace print latency. */
+ stop_critical_timings();
- con->seq = pmsg.seq + 1;
+ con->write(con, outbuf, pmsg.outbuf_len);
- *handover = console_lock_spinning_disable_and_check(cookie);
- printk_safe_exit_irqrestore(flags);
+ start_critical_timings();
+
+ con->seq = pmsg.seq + 1;
+
+ *handover = console_lock_spinning_disable_and_check(cookie);
+ printk_safe_exit_irqrestore(flags);
+ }
skip:
return true;
}
@@ -3088,12 +3118,13 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
if ((flags & CON_NBCON) && con->kthread)
continue;
- if (!console_is_usable(con, flags, true))
+ if (!console_is_usable(con, flags, !do_cond_resched))
continue;
any_usable = true;
if (flags & CON_NBCON) {
- progress = nbcon_legacy_emit_next_record(con, handover, cookie);
+ progress = nbcon_legacy_emit_next_record(con, handover, cookie,
+ !do_cond_resched);
printk_seq = nbcon_seq_read(con);
} else {
progress = console_emit_next_record(con, handover, cookie);
@@ -3132,19 +3163,7 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
return false;
}
-/**
- * console_unlock - unblock the console subsystem from printing
- *
- * Releases the console_lock which the caller holds to block printing of
- * the console subsystem.
- *
- * While the console_lock was held, console output may have been buffered
- * by printk(). If this is the case, console_unlock(); emits
- * the output prior to releasing the lock.
- *
- * console_unlock(); may be called from any context.
- */
-void console_unlock(void)
+static void console_flush_and_unlock(void)
{
bool do_cond_resched;
bool handover;
@@ -3188,6 +3207,32 @@ void console_unlock(void)
*/
} while (prb_read_valid(prb, next_seq, NULL) && console_trylock());
}
+
+/**
+ * console_unlock - unblock the console subsystem from printing
+ *
+ * Releases the console_lock which the caller holds to block printing of
+ * the console subsystem.
+ *
+ * While the console_lock was held, console output may have been buffered
+ * by printk(). If this is the case, console_unlock(); emits
+ * the output prior to releasing the lock.
+ *
+ * console_unlock(); may be called from any context.
+ */
+void console_unlock(void)
+{
+ /*
+ * Forced threading relies on kthread and atomic consoles for
+ * printing. It never attempts to print from console_unlock().
+ */
+ if (force_printkthreads()) {
+ __console_unlock();
+ return;
+ }
+
+ console_flush_and_unlock();
+}
EXPORT_SYMBOL(console_unlock);
/**
@@ -3411,12 +3456,107 @@ void console_start(struct console *console)
flags = console_srcu_read_flags(console);
if (flags & CON_NBCON)
nbcon_kthread_wake(console);
+ else
+ wake_up_legacy_kthread();
console_srcu_read_unlock(cookie);
__pr_flush(console, 1000, true);
}
EXPORT_SYMBOL(console_start);
+#ifdef CONFIG_PRINTK
+static bool printer_should_wake(void)
+{
+ bool available = false;
+ struct console *con;
+ int cookie;
+
+ if (kthread_should_stop())
+ return true;
+
+ cookie = console_srcu_read_lock();
+ for_each_console_srcu(con) {
+ short flags = console_srcu_read_flags(con);
+ u64 printk_seq;
+
+ /*
+ * The legacy printer thread is only for legacy consoles,
+ * unless the nbcon console has no kthread printer.
+ */
+ if ((flags & CON_NBCON) && con->kthread)
+ continue;
+
+ if (!console_is_usable(con, flags, false))
+ continue;
+
+ if (flags & CON_NBCON) {
+ printk_seq = nbcon_seq_read(con);
+ } else {
+ /*
+ * It is safe to read @seq because only this
+ * thread context updates @seq.
+ */
+ printk_seq = con->seq;
+ }
+
+ if (prb_read_valid(prb, printk_seq, NULL)) {
+ available = true;
+ break;
+ }
+ }
+ console_srcu_read_unlock(cookie);
+
+ return available;
+}
+
+static int nbcon_legacy_kthread_func(void *unused)
+{
+ int error;
+
+ for (;;) {
+ error = wait_event_interruptible(legacy_wait, printer_should_wake());
+
+ if (kthread_should_stop())
+ break;
+
+ if (error)
+ continue;
+
+ console_lock();
+ console_flush_and_unlock();
+ }
+
+ return 0;
+}
+
+void nbcon_legacy_kthread_create(void)
+{
+ struct task_struct *kt;
+
+ lockdep_assert_held(&console_mutex);
+
+ if (!force_printkthreads())
+ return;
+
+ if (!printk_threads_enabled || nbcon_legacy_kthread)
+ return;
+
+ kt = kthread_run(nbcon_legacy_kthread_func, NULL, "pr/legacy");
+ if (IS_ERR(kt)) {
+ pr_err("unable to start legacy printing thread\n");
+ return;
+ }
+
+ nbcon_legacy_kthread = kt;
+
+ /*
+ * It is important that console printing threads are scheduled
+ * shortly after a printk call and with generous runtime budgets.
+ */
+ sched_set_normal(nbcon_legacy_kthread, -20);
+}
+#endif /* CONFIG_PRINTK */
+
static int __read_mostly keep_bootcon;
static int __init keep_bootcon_setup(char *str)
@@ -3706,6 +3846,7 @@ void register_console(struct console *newcon)
} else {
have_legacy_console = true;
newcon->seq = init_seq;
+ nbcon_legacy_kthread_create();
}
if (newcon->flags & CON_BOOT)
@@ -3873,6 +4014,13 @@ static int unregister_console_locked(struct console *console)
nbcon_kthread_create(c);
}
+#ifdef CONFIG_PRINTK
+ if (!printing_via_unlock && nbcon_legacy_kthread) {
+ kthread_stop(nbcon_legacy_kthread);
+ nbcon_legacy_kthread = NULL;
+ }
+#endif
+
return res;
}
@@ -4031,7 +4179,11 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
seq = prb_next_reserve_seq(prb);
- /* Flush the consoles so that records up to @seq are printed. */
+ /*
+ * Flush the consoles so that records up to @seq are printed.
+ * Otherwise this function will just wait for the threaded printers
+ * to print up to @seq.
+ */
if (printing_via_unlock) {
console_lock();
console_unlock();
@@ -4146,9 +4298,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 (force_printkthreads()) {
+ wake_up_legacy_kthread();
+ } else {
+ /*
+ * If trylock fails, some other context
+ * will do the printing.
+ */
+ if (console_trylock())
+ console_unlock();
+ }
}
if (pending & PRINTK_PENDING_WAKEUP)
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-06-03 23:24 ` [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles John Ogness
@ 2024-06-28 11:46 ` Petr Mladek
2024-06-28 12:22 ` John Ogness
0 siblings, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-06-28 11:46 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-04 01:30:47, John Ogness wrote:
> The write callback of legacy consoles makes use of spinlocks.
> This is not permitted with PREEMPT_RT in atomic contexts.
>
> For PREEMPT_RT, create a new kthread to handle printing of all
> the legacy consoles (and nbcon consoles if boot consoles are
> registered).
>
> Since, if running from the kthread, the consoles are printing
> in a task context, the legacy nbcon printing can use the
> device_lock(), write_thread(), device_unlock() callbacks for
> printing.
>
> Introduce the macro force_printkthreads() to query if the
> forced threading of legacy consoles is in effect.
>
> These changes only affect CONFIG_PREEMPT_RT.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> kernel/printk/internal.h | 11 +-
> kernel/printk/nbcon.c | 54 ++++++---
> kernel/printk/printk.c | 241 ++++++++++++++++++++++++++++++++-------
> 3 files changed, 246 insertions(+), 60 deletions(-)
>
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 9e027e08918d..b66dfa865591 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -21,6 +21,12 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> (con->flags & CON_BOOT) ? "boot" : "", \
> con->name, con->index, ##__VA_ARGS__)
>
> +#ifdef CONFIG_PREEMPT_RT
> +# define force_printkthreads() (true)
> +#else
> +# define force_printkthreads() (false)
> +#endif
It seems that the only purpose of this variable is to decide
whether the nbcon_legacy_kthread will be used or not.
I know that force_printkthreads() has been choosen to correlate with
force_irqthreads(). But the situation is different here. In irq, there
is only one type of kthreads (ksoftirqd). But in printk(), we
have kthreads for nbcon consoles and a special kthread for legacy
consoles.
Also the variable is used together with:
+ printing_via_unlock: boolean variable which is true when
some console need to be flushed in the legacy loop
under console_lock().
+ nbcon_legacy_kthread: pointer to task_struct of the kthread
flushing the console in the legacy loop under console_lock().
+ wake_up_legacy_kthread(): function for waking up nbcon_legacy_kthread()
+ console_emit_next_record(): function emitting the message in
the legacy loop under console_lock()
It is a crazy mismatch of names ;-) The main used terms are: legacy, unlock,
nbcon, console. From my POV, the most meaningful are: legacy and unlock.
I would suggest to rename:
+ nbcon_legacy_kthread -> printk_legacy_kthread
+ force_printkthreads -> force_legacy_kthread
or another "more legacy" names.
> #ifdef CONFIG_PRINTK
>
> #ifdef CONFIG_PRINTK_CALLER
> @@ -92,9 +98,10 @@ void nbcon_free(struct console *con);
> enum nbcon_prio nbcon_get_default_prio(void);
> void nbcon_atomic_flush_pending(void);
> bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
> - int cookie);
> + int cookie, bool use_atomic);
> void nbcon_kthread_create(struct console *con);
> void nbcon_wake_threads(void);
> +void nbcon_legacy_kthread_create(void);
It is about legacy consoles. It is even implemented in printk.c.
I would call it:
+ legacy_kthread_create()
+ create_legacy_kthread()
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1232,6 +1233,7 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
> * both the console_lock and the SRCU read lock. Otherwise it
> * is set to false.
> * @cookie: The cookie from the SRCU read lock.
> + * @use_atomic: True if the write_atomic() callback is to be used
I would prefer to use a "hint style". Something like:
* @use_atomic: Set true when called in an atomic or unknown context.
* It affects which callback will be used: write_atomic()
* wrt. write_thread().
*
* When false, the write_thread() callback would be called
* in a preemtible context unless disabled by the device_lock().
* The handover won't be allowed in this mode.
> * Context: Any context except NMI.
> * Return: True, when a record has been printed and there are still
> @@ -1247,26 +1249,38 @@ static bool nbcon_atomic_emit_one(struct nbcon_write_context *wctxt)
> * Essentially it is the nbcon version of console_emit_next_record().
> */
> bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
> - int cookie)
> + int cookie, bool use_atomic)
> {
> struct nbcon_write_context wctxt = { };
> struct nbcon_context *ctxt = &ACCESS_PRIVATE(&wctxt, ctxt);
> unsigned long flags;
> bool progress;
>
> - /* Use the same procedure as console_emit_next_record(). */
> - printk_safe_enter_irqsave(flags);
> - console_lock_spinning_enable();
> - stop_critical_timings();
> + ctxt->console = con;
>
> - ctxt->console = con;
> - ctxt->prio = nbcon_get_default_prio();
> + if (use_atomic) {
> + /* Use the same procedure as console_emit_next_record(). */
I would extend it a bit:
/*
* In an atomic or unknown context, use the same procedure as in
* console_emit_next_record(). It allows to handower...
*/
> + printk_safe_enter_irqsave(flags);
> + console_lock_spinning_enable();
> + stop_critical_timings();
>
> - progress = nbcon_atomic_emit_one(&wctxt);
> + ctxt->prio = nbcon_get_default_prio();
> + progress = nbcon_emit_one(&wctxt, use_atomic);
>
> - start_critical_timings();
> - *handover = console_lock_spinning_disable_and_check(cookie);
> - printk_safe_exit_irqrestore(flags);
> + start_critical_timings();
> + *handover = console_lock_spinning_disable_and_check(cookie);
> + printk_safe_exit_irqrestore(flags);
> + } else {
I would add a comment:
/*
* In process context, use the same procedure as in
* nbcon_kthread_func(). It might allow scheduling
* depending on the devive_lock()...
*/
> + *handover = false;
> +
> + con->device_lock(con, &flags);
> + cant_migrate();
> +
> + ctxt->prio = nbcon_get_default_prio();
> + progress = nbcon_emit_one(&wctxt, use_atomic);
This is repeated in both branches.
> + con->device_unlock(con, flags);
> + }
I do not have strong opinion. But I slightly more like the variant
where we do not repeat the common functions. It is easier to see
the difference of the context in which nbcon_emit_one() is called.
if (use_atomic) {
/*
* In an atomic or unknown context, use the same procedure as in
* console_emit_next_record(). It allows to handower...
*/
printk_safe_enter_irqsave(flags);
console_lock_spinning_enable();
stop_critical_timings();
} else {
/*
* In process context, use the same procedure as in
* nbcon_kthread_func(). It might allow scheduling
* depending on the devive_lock()...
*/
con->device_lock(con, &flags);
cant_migrate();
}
ctxt->prio = nbcon_get_default_prio();
progress = nbcon_emit_one(&wctxt, use_atomic);
if (use_atomic) {
start_critical_timings();
*handover = console_lock_spinning_disable_and_check(cookie);
printk_safe_exit_irqrestore(flags);
} else {
con->device_unlock(con, flags);
*handover = false;
}
> return progress;
> }
> @@ -1494,7 +1508,9 @@ void nbcon_cpu_emergency_exit(void)
> * to handle it.
> */
> do_trigger_flush = true;
> - if (printing_via_unlock && !is_printk_deferred()) {
> + if (!force_printkthreads() &&
Is this correct? We still need to flush the messages directly
when the legacy kthread is not ready yet.
We should check !nbcon_legacy_kthread instead.
> + printing_via_unlock &&
> + !is_printk_deferred()) {
> if (console_trylock()) {
> do_trigger_flush = false;
> console_unlock();
> @@ -1530,7 +1546,9 @@ void nbcon_cpu_emergency_flush(void)
>
> nbcon_atomic_flush_pending();
>
> - if (printing_via_unlock && !is_printk_deferred()) {
> + if (!force_printkthreads() &&
Same here. It should rather be !nbcon_legacy_kthread.
> + printing_via_unlock &&
> + !is_printk_deferred()) {
> if (console_trylock())
> console_unlock();
The same tricky pattern is repeated in
+ vprintk_emit()
+ nbcon_cpu_emergency_exit()
+ nbcon_cpu_emergency_flush()
With each new condition, it is more and more complicated to see
if all the locations do the right thing.
It would be nice to somehow refactor this or create some
helper scripts. I would do it even at the cost that
some checks will be repeated.
It is already being discussed in the thread about 5th patch,
see https://lore.kernel.org/r/8734p0yca9.fsf@jogness.linutronix.de
Let's continue there.
> }
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index cf0d612329bf..1c63fd0c1166 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2345,7 +2348,8 @@ asmlinkage int vprintk_emit(int facility, int level,
> const struct dev_printk_info *dev_info,
> const char *fmt, va_list args)
> {
> - bool do_trylock_unlock = printing_via_unlock;
> + bool do_trylock_unlock = !force_printkthreads() &&
s/force_printkthreads/nbcon_legacy_kthread/ ???
> + printing_via_unlock;
> int printed_len;
>
> /* Suppress unimportant messages after panic happens */
> @@ -2740,7 +2756,9 @@ void resume_console(void)
> */
> static int console_cpu_notify(unsigned int cpu)
> {
> - if (!cpuhp_tasks_frozen && printing_via_unlock) {
> + if (!force_printkthreads() &&
s/force_printkthreads/nbcon_legacy_kthread/ ???
> + !cpuhp_tasks_frozen &&
> + printing_via_unlock) {
> /* If trylock fails, someone else is doing the printing */
> if (console_trylock())
> console_unlock();
> @@ -3000,31 +3018,43 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
> con->dropped = 0;
> }
>
> - /*
> - * While actively printing out messages, if another printk()
> - * were to occur on another CPU, it may wait for this one to
> - * finish. This task can not be preempted if there is a
> - * waiter waiting to take over.
> - *
> - * Interrupts are disabled because the hand over to a waiter
> - * must not be interrupted until the hand over is completed
> - * (@console_waiter is cleared).
> - */
> - printk_safe_enter_irqsave(flags);
> - console_lock_spinning_enable();
> + /* Write everything out to the hardware. */
>
> - /* Do not trace print latency. */
> - stop_critical_timings();
> + if (force_printkthreads()) {
s/force_printkthreads/nbcon_legacy_kthread/ ???
> + /*
> + * With forced threading this function is either in a thread
> + * or panic context. So there is no need for concern about
> + * printk reentrance or handovers.
> + */
The comment describes why it is "safe". But it does not describe the
motivation.
If I get it correctly, the motivation is to call con->write() with interrupts
and preemption enabled. We should document it so that people won't
break it in the future.
Another question is how it is guaranteed. OK, it is called only from
console_flush_all(). But it is called also from get_init_console_seq()
aka from register_console(). I guess that it is OK because it is
a well known and preemptible context. But it is not clear from
the comment.
> - /* Write everything out to the hardware. */
> - con->write(con, outbuf, pmsg.outbuf_len);
> + con->write(con, outbuf, pmsg.outbuf_len);
> + con->seq = pmsg.seq + 1;
> + } else {
> + /*
> + * While actively printing out messages, if another printk()
> + * were to occur on another CPU, it may wait for this one to
> + * finish. This task can not be preempted if there is a
> + * waiter waiting to take over.
> + *
> + * Interrupts are disabled because the hand over to a waiter
> + * must not be interrupted until the hand over is completed
> + * (@console_waiter is cleared).
> + */
> + printk_safe_enter_irqsave(flags);
> + console_lock_spinning_enable();
>
> - start_critical_timings();
> + /* Do not trace print latency. */
> + stop_critical_timings();
>
> - con->seq = pmsg.seq + 1;
> + con->write(con, outbuf, pmsg.outbuf_len);
>
> - *handover = console_lock_spinning_disable_and_check(cookie);
> - printk_safe_exit_irqrestore(flags);
> + start_critical_timings();
> +
> + con->seq = pmsg.seq + 1;
> +
> + *handover = console_lock_spinning_disable_and_check(cookie);
> + printk_safe_exit_irqrestore(flags);
> + }
> skip:
> return true;
> }
> @@ -3188,6 +3207,32 @@ void console_unlock(void)
> */
> } while (prb_read_valid(prb, next_seq, NULL) && console_trylock());
> }
> +
> +/**
> + * console_unlock - unblock the console subsystem from printing
> + *
> + * Releases the console_lock which the caller holds to block printing of
> + * the console subsystem.
> + *
> + * While the console_lock was held, console output may have been buffered
> + * by printk(). If this is the case, console_unlock(); emits
> + * the output prior to releasing the lock.
> + *
> + * console_unlock(); may be called from any context.
> + */
> +void console_unlock(void)
> +{
> + /*
> + * Forced threading relies on kthread and atomic consoles for
> + * printing. It never attempts to print from console_unlock().
> + */
> + if (force_printkthreads()) {
s/force_printkthreads/nbcon_legacy_kthread/ ???
> + __console_unlock();
> + return;
> + }
> +
> + console_flush_and_unlock();
> +}
> EXPORT_SYMBOL(console_unlock);
>
> /**
> @@ -3411,12 +3456,107 @@ void console_start(struct console *console)
> flags = console_srcu_read_flags(console);
> if (flags & CON_NBCON)
> nbcon_kthread_wake(console);
> + else
> + wake_up_legacy_kthread();
> console_srcu_read_unlock(cookie);
>
> __pr_flush(console, 1000, true);
> }
> EXPORT_SYMBOL(console_start);
>
> +#ifdef CONFIG_PRINTK
> +static bool printer_should_wake(void)
I would call it printk_legacy_kthread_should_wakeup()
to make it consistent with
+ printk_legacy_kthread # proposed above
+ nbcon_kthread_should_wakeup()
> +{
> + bool available = false;
Nit: I would use "bool ret = false".
The word "available" is not much helpful because it is not
immediately obvious what is available. It might be console,
kthread, or pending record. One has to read the code and
"available" is not cscope-friendly ;-)
> + struct console *con;
> + int cookie;
> +
> + if (kthread_should_stop())
> + return true;
> +
> + cookie = console_srcu_read_lock();
> + for_each_console_srcu(con) {
> + short flags = console_srcu_read_flags(con);
> + u64 printk_seq;
> +
> + /*
> + * The legacy printer thread is only for legacy consoles,
> + * unless the nbcon console has no kthread printer.
> + */
> + if ((flags & CON_NBCON) && con->kthread)
> + continue;
> +
> + if (!console_is_usable(con, flags, false))
> + continue;
> +
> + if (flags & CON_NBCON) {
> + printk_seq = nbcon_seq_read(con);
> + } else {
> + /*
> + * It is safe to read @seq because only this
> + * thread context updates @seq.
> + */
> + printk_seq = con->seq;
> + }
> +
> + if (prb_read_valid(prb, printk_seq, NULL)) {
> + available = true;
> + break;
> + }
> + }
> + console_srcu_read_unlock(cookie);
> +
> + return available;
> +}
> +
> +void nbcon_legacy_kthread_create(void)
> +{
> + struct task_struct *kt;
> +
> + lockdep_assert_held(&console_mutex);
> +
> + if (!force_printkthreads())
> + return;
> +
> + if (!printk_threads_enabled || nbcon_legacy_kthread)
> + return;
> +
> + kt = kthread_run(nbcon_legacy_kthread_func, NULL, "pr/legacy");
> + if (IS_ERR(kt)) {
> + pr_err("unable to start legacy printing thread\n");
> + return;
Is this acceptable for RT, please?
> + }
> +
> + nbcon_legacy_kthread = kt;
> +
> + /*
> + * It is important that console printing threads are scheduled
> + * shortly after a printk call and with generous runtime budgets.
> + */
> + sched_set_normal(nbcon_legacy_kthread, -20);
> +}
> +#endif /* CONFIG_PRINTK */
> +
> static int __read_mostly keep_bootcon;
>
> static int __init keep_bootcon_setup(char *str)
> @@ -3706,6 +3846,7 @@ void register_console(struct console *newcon)
> } else {
> have_legacy_console = true;
> newcon->seq = init_seq;
> + nbcon_legacy_kthread_create();
I would prefer to do:
if (force_printkthread)
nbcon_legacy_kthread_create();
to make it clear that we start it only when explicitly requested.
> }
>
> if (newcon->flags & CON_BOOT)
> @@ -4146,9 +4298,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 (force_printkthreads()) {
s/force_printkthreads/nbcon_legacy_kthread/ ???
> + wake_up_legacy_kthread();
> + } else {
> + /*
> + * If trylock fails, some other context
> + * will do the printing.
> + */
> + if (console_trylock())
> + console_unlock();
> + }
> }
>
> if (pending & PRINTK_PENDING_WAKEUP)
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-06-28 11:46 ` Petr Mladek
@ 2024-06-28 12:22 ` John Ogness
2024-06-28 13:32 ` Petr Mladek
0 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-28 12:22 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On 2024-06-28, Petr Mladek <pmladek@suse.com> wrote:
>> @@ -1494,7 +1508,9 @@ void nbcon_cpu_emergency_exit(void)
>> * to handle it.
>> */
>> do_trigger_flush = true;
>> - if (printing_via_unlock && !is_printk_deferred()) {
>> + if (!force_printkthreads() &&
>
> Is this correct? We still need to flush the messages directly
> when the legacy kthread is not ready yet.
No! If force_printkthreads() is set, messages will never flush directly
except for console_flush_on_panic().
I see that I implemented get_init_console_seq() to flush since it is
known to be in task state. But that is wrong. Also there it should not
flush directly. It is not about whether or not printing is safe. It is
about deferring to the thread 100% of the time except for panic.
>> +void nbcon_legacy_kthread_create(void)
>> +{
>> + struct task_struct *kt;
>> +
>> + lockdep_assert_held(&console_mutex);
>> +
>> + if (!force_printkthreads())
>> + return;
>> +
>> + if (!printk_threads_enabled || nbcon_legacy_kthread)
>> + return;
>> +
>> + kt = kthread_run(nbcon_legacy_kthread_func, NULL, "pr/legacy");
>> + if (IS_ERR(kt)) {
>> + pr_err("unable to start legacy printing thread\n");
>> + return;
>
> Is this acceptable for RT, please?
It is not acceptable for mainline. For the next version, any failed
thread creation leads to unregistering the console. In the case of the
legacy thread, it will mean unregistering all legacy consoles on
failure.
John
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-06-28 12:22 ` John Ogness
@ 2024-06-28 13:32 ` Petr Mladek
2024-06-28 14:11 ` John Ogness
0 siblings, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-06-28 13:32 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Fri 2024-06-28 14:28:47, John Ogness wrote:
> On 2024-06-28, Petr Mladek <pmladek@suse.com> wrote:
> >> @@ -1494,7 +1508,9 @@ void nbcon_cpu_emergency_exit(void)
> >> * to handle it.
> >> */
> >> do_trigger_flush = true;
> >> - if (printing_via_unlock && !is_printk_deferred()) {
> >> + if (!force_printkthreads() &&
> >
> > Is this correct? We still need to flush the messages directly
> > when the legacy kthread is not ready yet.
>
> No! If force_printkthreads() is set, messages will never flush directly
> except for console_flush_on_panic().
But this is an _emergency_ context! This would be inconsistent with
the nbcon consoles where the messages are flushed directly.
Is RT sheduling quaranteed when the system reached emergency state?
In fact, we probably should not check force_printkthreads() here at
all. It would be only for NBCON_PRIO_NORMAL context.
Or the option should force the kthreads even for nbcon consoles.
> I see that I implemented get_init_console_seq() to flush since it is
> known to be in task state. But that is wrong. Also there it should not
> flush directly. It is not about whether or not printing is safe. It is
> about deferring to the thread 100% of the time except for panic.
>
> >> +void nbcon_legacy_kthread_create(void)
> >> +{
> >> + struct task_struct *kt;
> >> +
> >> + lockdep_assert_held(&console_mutex);
> >> +
> >> + if (!force_printkthreads())
> >> + return;
> >> +
> >> + if (!printk_threads_enabled || nbcon_legacy_kthread)
> >> + return;
> >> +
> >> + kt = kthread_run(nbcon_legacy_kthread_func, NULL, "pr/legacy");
> >> + if (IS_ERR(kt)) {
> >> + pr_err("unable to start legacy printing thread\n");
> >> + return;
> >
> > Is this acceptable for RT, please?
>
> It is not acceptable for mainline. For the next version, any failed
> thread creation leads to unregistering the console. In the case of the
> legacy thread, it will mean unregistering all legacy consoles on
> failure.
It means that the system would become silent. Is this a good idea?
IMHO, we have a fundamental problem here.
Are RT guarantees more important than printk() or vice versa?
What is happening when the RT guarantees are violated?
For example, when the scheduler detects a problem?
Does it panic() or tries to continue?
From my POV, silent system is the worst solution. The user
might think: "no messages, no problem".
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-06-28 13:32 ` Petr Mladek
@ 2024-06-28 14:11 ` John Ogness
2024-06-28 15:56 ` John Ogness
2024-07-01 14:50 ` Petr Mladek
0 siblings, 2 replies; 63+ messages in thread
From: John Ogness @ 2024-06-28 14:11 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On 2024-06-28, Petr Mladek <pmladek@suse.com> wrote:
> On Fri 2024-06-28 14:28:47, John Ogness wrote:
>> On 2024-06-28, Petr Mladek <pmladek@suse.com> wrote:
>> >> @@ -1494,7 +1508,9 @@ void nbcon_cpu_emergency_exit(void)
>> >> * to handle it.
>> >> */
>> >> do_trigger_flush = true;
>> >> - if (printing_via_unlock && !is_printk_deferred()) {
>> >> + if (!force_printkthreads() &&
>> >
>> > Is this correct? We still need to flush the messages directly
>> > when the legacy kthread is not ready yet.
>>
>> No! If force_printkthreads() is set, messages will never flush directly
>> except for console_flush_on_panic().
>
> But this is an _emergency_ context! This would be inconsistent with
> the nbcon consoles where the messages are flushed directly.
>
> Is RT sheduling quaranteed when the system reached emergency state?
No.
> In fact, we probably should not check force_printkthreads() here at
> all. It would be only for NBCON_PRIO_NORMAL context.
On PREEMPT_RT, legacy consoles are not allowed to print from
non-preemptible contexts because they use spinlocks (rtmutexes).
This is a disadvantage for legacy consoles on PREEMPT_RT. If people want
a legacy console to gain the reliability attribute on PREEMPT_RT, then
that console _must_ be converted to nbcon.
force_printkthreads() is used to establish the same console printing
behavior as PREEMPT_RT. (A later patch makes this behavior available to
all preemption models so that users can enjoy the non-interference
attribute of the rework.)
>> For the next version, any failed
>> thread creation leads to unregistering the console. In the case of the
>> legacy thread, it will mean unregistering all legacy consoles on
>> failure.
>
> It means that the system would become silent. Is this a good idea?
In the threaded model, the thread is a critical component of the
console. If the thread cannot start, it is the same as if memory could
not be allocated. The registration must fail. And just as it is now, the
user will only see that it failed via dmesg (or some other console that
did not fail).
> IMHO, we have a fundamental problem here.
> Are RT guarantees more important than printk() or vice versa?
That is not the issue. The problem is that you want legacy consoles in
RT to print without the printing thread. That is not possible. Those
drivers are not RT compatible and will cause deadlocks. They _must_ be
modified i.e. converted to nbcon consoles.
We have worked really hard to allow legacy consoles to work "as is" for
!PREEMPT_RT. But when PREEMPT_RT is enabled legacy consoles must be
forced into the thread (except for panic, where we close our eyes and
hope for the best).
John
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-06-28 14:11 ` John Ogness
@ 2024-06-28 15:56 ` John Ogness
2024-07-01 15:33 ` Petr Mladek
2024-07-01 14:50 ` Petr Mladek
1 sibling, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-28 15:56 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
Hi Petr,
Your comments are sending me into deep thought about this
situation. Some more comments from me...
On 2024-06-28, John Ogness <john.ogness@linutronix.de> wrote:
> On PREEMPT_RT, legacy consoles are not allowed to print from
> non-preemptible contexts because they use spinlocks (rtmutexes).
The above statement is not true for legacy _boot_ consoles
(earlycon/earlyprintk). These are lockless and are intended to execute
from any context (except NMI due to semaphore limitations).
I hate boot consoles because they don't use the Linux driver model and
rely fully on external synchronization. This has made the rework very
difficult and is actually the core reason why our work was reverted back
in 5.19-rc4. But for debugging early boot problems, they are quite
useful.
I have a new proposal. What if we allow boot consoles to always print in
the caller context (for any preemption model)? I hacked a quick test
together by providing console_flush_all() an extra argument for printing
_only_ on boot consoles. Then in vprintk_emit(), I always do a
boot-console-only-flush (under console_trylock) after storing the record
in the ringbuffer.
For PREEMPT_RT this is horrible. But I am OK with mandating that RT
scheduling cannot be guaranteed with boot consoles (just as it is
currently mandated that RT scheduling cannot be guaranteed in emergency
situations). Since the boot consoles are lockless, they pose no deadlock
threat to RT.
This has some nice features:
- We get early debugging in all preemption models.
- We get true synchronous printing when using boot consoles (which
should make peterz happy).
- Boot consoles are then horrible enough that options such as
"keep_bootcon" will really be avoided unless debugging kernel issues.
From the tests I have run so far, it looks good.
Looking to the future, I think this would also provide an excellent
foundation for the "sync" console option I would like. For nbcon
consoles with the "sync" option specified, it would work the same way,
flushing boot consoles and nbcon consoles directly in vprintk_emit().
Thoughts?
John
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-06-28 15:56 ` John Ogness
@ 2024-07-01 15:33 ` Petr Mladek
2024-07-01 21:01 ` John Ogness
0 siblings, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-07-01 15:33 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Fri 2024-06-28 18:02:19, John Ogness wrote:
> Hi Petr,
>
> Your comments are sending me into deep thought about this
> situation. Some more comments from me...
>
> On 2024-06-28, John Ogness <john.ogness@linutronix.de> wrote:
> > On PREEMPT_RT, legacy consoles are not allowed to print from
> > non-preemptible contexts because they use spinlocks (rtmutexes).
>
> The above statement is not true for legacy _boot_ consoles
> (earlycon/earlyprintk). These are lockless and are intended to execute
> from any context (except NMI due to semaphore limitations).
>
> I hate boot consoles because they don't use the Linux driver model and
> rely fully on external synchronization. This has made the rework very
> difficult and is actually the core reason why our work was reverted back
> in 5.19-rc4. But for debugging early boot problems, they are quite
> useful.
>
> I have a new proposal. What if we allow boot consoles to always print in
> the caller context (for any preemption model)? I hacked a quick test
> together by providing console_flush_all() an extra argument for printing
> _only_ on boot consoles. Then in vprintk_emit(), I always do a
> boot-console-only-flush (under console_trylock) after storing the record
> in the ringbuffer.
>
> For PREEMPT_RT this is horrible. But I am OK with mandating that RT
> scheduling cannot be guaranteed with boot consoles (just as it is
> currently mandated that RT scheduling cannot be guaranteed in emergency
> situations). Since the boot consoles are lockless, they pose no deadlock
> threat to RT.
Is this really the case for all boot consoles?
I had the feeling that some boot consoles actually used port->lock.
And for example, register_earlycon() is initializing this spin lock.
> This has some nice features:
>
> - We get early debugging in all preemption models.
It would be great.
> - We get true synchronous printing when using boot consoles (which
> should make peterz happy).
Well, Peter's mode is really special because it is done without
the console_lock(). Every printk() is flushing its own message
to the console. Parallel printk()'s are busy waiting for each other.
> - Boot consoles are then horrible enough that options such as
> "keep_bootcon" will really be avoided unless debugging kernel issues.
>
> >From the tests I have run so far, it looks good.
>
> Looking to the future, I think this would also provide an excellent
> foundation for the "sync" console option I would like. For nbcon
> consoles with the "sync" option specified, it would work the same way,
> flushing boot consoles and nbcon consoles directly in vprintk_emit().
The sync mode would be nice.
Just to be sure. I guess that you are talking about a sync mode
using some trylock mechanism where the current owner would be
responsible for flushing everything.
Peter Zijlstra's mode (serialized printk()) is easy to implement
and might be needed in some situations. But I am not sure if
it would be good enough for most other users preferring the
current "synchronous" output.
Well, let's see what people request after they get some experience
with the first nbcon consoles and kthreads.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-07-01 15:33 ` Petr Mladek
@ 2024-07-01 21:01 ` John Ogness
2024-07-02 9:11 ` Petr Mladek
0 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-07-01 21:01 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On 2024-07-01, Petr Mladek <pmladek@suse.com> wrote:
>> I have a new proposal. What if we allow boot consoles to always print
>> in the caller context (for any preemption model)? I hacked a quick
>> test together by providing console_flush_all() an extra argument for
>> printing _only_ on boot consoles. Then in vprintk_emit(), I always do
>> a boot-console-only-flush (under console_trylock) after storing the
>> record in the ringbuffer.
>>
>> For PREEMPT_RT this is horrible. But I am OK with mandating that RT
>> scheduling cannot be guaranteed with boot consoles (just as it is
>> currently mandated that RT scheduling cannot be guaranteed in
>> emergency situations). Since the boot consoles are lockless, they
>> pose no deadlock threat to RT.
>
> Is this really the case for all boot consoles?
>
> I had the feeling that some boot consoles actually used port->lock.
> And for example, register_earlycon() is initializing this spin lock.
Doing a naive search, it seems like there are a few that do some spin
locking:
- alpha/kernel/srmcons.c
- tty/hvc/hvc_xen.c
- tty/mips_ejtag_fdc.c
- usb/early/xhci-dbc.c
I will need to look at these in more detail.
>> - We get true synchronous printing when using boot consoles (which
>> should make peterz happy).
>
> Well, Peter's mode is really special because it is done without
> the console_lock(). Every printk() is flushing its own message
> to the console. Parallel printk()'s are busy waiting for each other.
This is also what I would like to see. For debugging purposes (which is
the purpose of CON_BOOT) this mode of operation is probably preferred.
>> Looking to the future, I think this would also provide an excellent
>> foundation for the "sync" console option I would like. For nbcon
>> consoles with the "sync" option specified, it would work the same
>> way, flushing boot consoles and nbcon consoles directly in
>> vprintk_emit().
>
> The sync mode would be nice.
>
> Just to be sure. I guess that you are talking about a sync mode
> using some trylock mechanism where the current owner would be
> responsible for flushing everything.
>
> Peter Zijlstra's mode (serialized printk()) is easy to implement
> and might be needed in some situations. But I am not sure if
> it would be good enough for most other users preferring the
> current "synchronous" output.
I have always envisioned it being a true sync mode. Each console has its
own nbcon state. So if it was set to sync mode, vprintk_emit() could
local_irq_save() and then busy-wait until it acquires the nbcon state
for that console. There would be no handovers and no relying on another
context to print your message. It would be straight forward and truly
synchronous (for that console). Back to the roots of printk().
> Well, let's see what people request after they get some experience
> with the first nbcon consoles and kthreads.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-07-01 21:01 ` John Ogness
@ 2024-07-02 9:11 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-07-02 9:11 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Mon 2024-07-01 23:07:23, John Ogness wrote:
> On 2024-07-01, Petr Mladek <pmladek@suse.com> wrote:
> >> I have a new proposal. What if we allow boot consoles to always print
> >> in the caller context (for any preemption model)? I hacked a quick
> >> test together by providing console_flush_all() an extra argument for
> >> printing _only_ on boot consoles. Then in vprintk_emit(), I always do
> >> a boot-console-only-flush (under console_trylock) after storing the
> >> record in the ringbuffer.
> >>
> >> For PREEMPT_RT this is horrible. But I am OK with mandating that RT
> >> scheduling cannot be guaranteed with boot consoles (just as it is
> >> currently mandated that RT scheduling cannot be guaranteed in
> >> emergency situations). Since the boot consoles are lockless, they
> >> pose no deadlock threat to RT.
> >
> > Is this really the case for all boot consoles?
> >
> > I had the feeling that some boot consoles actually used port->lock.
> > And for example, register_earlycon() is initializing this spin lock.
>
> Doing a naive search, it seems like there are a few that do some spin
> locking:
>
> - alpha/kernel/srmcons.c
> - tty/hvc/hvc_xen.c
> - tty/mips_ejtag_fdc.c
> - usb/early/xhci-dbc.c
>
> I will need to look at these in more detail.
This looks like a non-trivial thing. I assume that you would do this later.
We probably do not want to complicate/delay the current patchset by this.
> >> - We get true synchronous printing when using boot consoles (which
> >> should make peterz happy).
> >
> > Well, Peter's mode is really special because it is done without
> > the console_lock(). Every printk() is flushing its own message
> > to the console. Parallel printk()'s are busy waiting for each other.
>
> This is also what I would like to see. For debugging purposes (which is
> the purpose of CON_BOOT) this mode of operation is probably preferred.
Makes sense. And it would be great when it was enough.
Well, I could also imagine that a non-serialized printk() might be
useful to debug some races in the code before the printk
kthreads are started.
Another scenario where a synchronized printk might be useful is
a sudden death. In this case, the "synchronous" printk would
be needed all the time. People might prefer the current
legacy mode where printk works synchronously most of the time
but it is not fully serialized.
Honestly, I do not know what people want. There always will be
usecase where some mode would be needed or work better.
We should be prepared for the situation when people would
want to keep a mode which has been used last 25+ years.
> >> Looking to the future, I think this would also provide an excellent
> >> foundation for the "sync" console option I would like. For nbcon
> >> consoles with the "sync" option specified, it would work the same
> >> way, flushing boot consoles and nbcon consoles directly in
> >> vprintk_emit().
> >
> > The sync mode would be nice.
> >
> > Just to be sure. I guess that you are talking about a sync mode
> > using some trylock mechanism where the current owner would be
> > responsible for flushing everything.
> >
> > Peter Zijlstra's mode (serialized printk()) is easy to implement
> > and might be needed in some situations. But I am not sure if
> > it would be good enough for most other users preferring the
> > current "synchronous" output.
>
> I have always envisioned it being a true sync mode. Each console has its
> own nbcon state. So if it was set to sync mode, vprintk_emit() could
> local_irq_save() and then busy-wait until it acquires the nbcon state
> for that console. There would be no handovers and no relying on another
> context to print your message. It would be straight forward and truly
> synchronous (for that console). Back to the roots of printk().
I see this as yet another feature which might be introduced later.
In this patchset, I would keep the legacy behavior when boot
consoles are registered.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-06-28 14:11 ` John Ogness
2024-06-28 15:56 ` John Ogness
@ 2024-07-01 14:50 ` Petr Mladek
2024-07-02 9:30 ` Petr Mladek
1 sibling, 1 reply; 63+ messages in thread
From: Petr Mladek @ 2024-07-01 14:50 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Fri 2024-06-28 16:17:13, John Ogness wrote:
> On 2024-06-28, Petr Mladek <pmladek@suse.com> wrote:
> > On Fri 2024-06-28 14:28:47, John Ogness wrote:
> >> On 2024-06-28, Petr Mladek <pmladek@suse.com> wrote:
> >> >> @@ -1494,7 +1508,9 @@ void nbcon_cpu_emergency_exit(void)
> >> >> * to handle it.
> >> >> */
> >> >> do_trigger_flush = true;
> >> >> - if (printing_via_unlock && !is_printk_deferred()) {
> >> >> + if (!force_printkthreads() &&
> >> >
> >> > Is this correct? We still need to flush the messages directly
> >> > when the legacy kthread is not ready yet.
> >>
> >> No! If force_printkthreads() is set, messages will never flush directly
> >> except for console_flush_on_panic().
> >
> > But this is an _emergency_ context! This would be inconsistent with
> > the nbcon consoles where the messages are flushed directly.
> >
> > Is RT sheduling quaranteed when the system reached emergency state?
>
> No.
>
> > In fact, we probably should not check force_printkthreads() here at
> > all. It would be only for NBCON_PRIO_NORMAL context.
>
> On PREEMPT_RT, legacy consoles are not allowed to print from
> non-preemptible contexts because they use spinlocks (rtmutexes).
Ah, I see.
> This is a disadvantage for legacy consoles on PREEMPT_RT. If people want
> a legacy console to gain the reliability attribute on PREEMPT_RT, then
> that console _must_ be converted to nbcon.
Makes sense.
Thanks a lot for explanation.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles
2024-07-01 14:50 ` Petr Mladek
@ 2024-07-02 9:30 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-07-02 9:30 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Mon 2024-07-01 16:50:31, Petr Mladek wrote:
> On Fri 2024-06-28 16:17:13, John Ogness wrote:
> > On 2024-06-28, Petr Mladek <pmladek@suse.com> wrote:
> > > On Fri 2024-06-28 14:28:47, John Ogness wrote:
> > >> On 2024-06-28, Petr Mladek <pmladek@suse.com> wrote:
> > >> >> @@ -1494,7 +1508,9 @@ void nbcon_cpu_emergency_exit(void)
> > >> >> * to handle it.
> > >> >> */
> > >> >> do_trigger_flush = true;
> > >> >> - if (printing_via_unlock && !is_printk_deferred()) {
> > >> >> + if (!force_printkthreads() &&
> > >> >
> > >> > Is this correct? We still need to flush the messages directly
> > >> > when the legacy kthread is not ready yet.
> > >>
> > >> No! If force_printkthreads() is set, messages will never flush directly
> > >> except for console_flush_on_panic().
> > >
> > > But this is an _emergency_ context! This would be inconsistent with
> > > the nbcon consoles where the messages are flushed directly.
> > >
> > > Is RT sheduling quaranteed when the system reached emergency state?
> >
> > No.
> >
> > > In fact, we probably should not check force_printkthreads() here at
> > > all. It would be only for NBCON_PRIO_NORMAL context.
> >
> > On PREEMPT_RT, legacy consoles are not allowed to print from
> > non-preemptible contexts because they use spinlocks (rtmutexes).
>
> Ah, I see.
>
> > This is a disadvantage for legacy consoles on PREEMPT_RT. If people want
> > a legacy console to gain the reliability attribute on PREEMPT_RT, then
> > that console _must_ be converted to nbcon.
>
> Makes sense.
>
> Thanks a lot for explanation.
It is clear that the legacy consoles could not be flushed directly in
nbcon_cpu_emergency_exit() with PREEMPT_RT. I thought whether we
could/should allow this for other preemption modes.
My opinion. Let's keep it simple and behave the same in all preemption
modes. force_printkthreads() is enabled only in PREEMPT_RT by default.
Users would get the "expected" legacy behavior in other preemption
modes by default.
By other words, let's keep "force_printkthreads()" to really simulate
RT behavior also in other preemption modes.
That said, I still suggest to rename the variable/function to
"force_legacy_thread". It does not affect the behavior of
the kthreads for nbcon consoles.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 13/18] proc: consoles: Add notation to c_start/c_stop
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (11 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 12/18] printk: Add kthread for all legacy consoles John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-07-01 15:43 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 14/18] proc: Add nbcon support for /proc/consoles John Ogness
` (5 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
linux-fsdevel
fs/proc/consoles.c:78:13: warning: context imbalance in 'c_start'
- wrong count at exit
fs/proc/consoles.c:104:13: warning: context imbalance in 'c_stop'
- unexpected unlock
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
fs/proc/consoles.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index e0758fe7936d..7036fdfa0bec 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -68,6 +68,7 @@ static int show_console_dev(struct seq_file *m, void *v)
}
static void *c_start(struct seq_file *m, loff_t *pos)
+ __acquires(&console_mutex)
{
struct console *con;
loff_t off = 0;
@@ -94,6 +95,7 @@ static void *c_next(struct seq_file *m, void *v, loff_t *pos)
}
static void c_stop(struct seq_file *m, void *v)
+ __releases(&console_mutex)
{
console_list_unlock();
}
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH printk v2 14/18] proc: Add nbcon support for /proc/consoles
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (12 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 13/18] proc: consoles: Add notation to c_start/c_stop John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-07-01 15:47 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 15/18] tty: sysfs: Add nbcon support for 'active' John Ogness
` (4 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
linux-fsdevel
Update /proc/consoles output to show 'W' if an nbcon write
callback is implemented (write_atomic or write_thread).
Also update /proc/consoles output to show 'N' if it is an
nbcon console.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
fs/proc/consoles.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index 7036fdfa0bec..c3c01ec2273c 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -21,12 +21,14 @@ static int show_console_dev(struct seq_file *m, void *v)
{ CON_ENABLED, 'E' },
{ CON_CONSDEV, 'C' },
{ CON_BOOT, 'B' },
+ { CON_NBCON, 'N' },
{ CON_PRINTBUFFER, 'p' },
{ CON_BRL, 'b' },
{ CON_ANYTIME, 'a' },
};
char flags[ARRAY_SIZE(con_flags) + 1];
struct console *con = v;
+ char con_write = '-';
unsigned int a;
dev_t dev = 0;
@@ -57,9 +59,15 @@ static int show_console_dev(struct seq_file *m, void *v)
seq_setwidth(m, 21 - 1);
seq_printf(m, "%s%d", con->name, con->index);
seq_pad(m, ' ');
- seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-',
- con->write ? 'W' : '-', con->unblank ? 'U' : '-',
- flags);
+ if (con->flags & CON_NBCON) {
+ if (con->write_atomic || con->write_thread)
+ con_write = 'W';
+ } else {
+ if (con->write)
+ con_write = 'W';
+ }
+ seq_printf(m, "%c%c%c (%s)", con->read ? 'R' : '-', con_write,
+ con->unblank ? 'U' : '-', flags);
if (dev)
seq_printf(m, " %4d:%d", MAJOR(dev), MINOR(dev));
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* [PATCH printk v2 15/18] tty: sysfs: Add nbcon support for 'active'
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (13 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 14/18] proc: Add nbcon support for /proc/consoles John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-06-04 11:48 ` Greg Kroah-Hartman
2024-07-01 15:50 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 16/18] printk: Provide threadprintk boot argument John Ogness
` (3 subsequent siblings)
18 siblings, 2 replies; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman, Jiri Slaby, linux-serial
Allow the 'active' attribute to list nbcon consoles.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
drivers/tty/tty_io.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 407b0d87b7c1..c9c914bc033c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3567,8 +3567,13 @@ static ssize_t show_cons_active(struct device *dev,
for_each_console(c) {
if (!c->device)
continue;
- if (!c->write)
- continue;
+ if (c->flags & CON_NBCON) {
+ if (!c->write_atomic && !c->write_thread)
+ continue;
+ } else {
+ if (!c->write)
+ continue;
+ }
if ((c->flags & CON_ENABLED) == 0)
continue;
cs[i++] = c;
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 15/18] tty: sysfs: Add nbcon support for 'active'
2024-06-03 23:24 ` [PATCH printk v2 15/18] tty: sysfs: Add nbcon support for 'active' John Ogness
@ 2024-06-04 11:48 ` Greg Kroah-Hartman
2024-07-01 15:50 ` Petr Mladek
1 sibling, 0 replies; 63+ messages in thread
From: Greg Kroah-Hartman @ 2024-06-04 11:48 UTC (permalink / raw)
To: John Ogness
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner,
linux-kernel, Jiri Slaby, linux-serial
On Tue, Jun 04, 2024 at 01:30:50AM +0206, John Ogness wrote:
> Allow the 'active' attribute to list nbcon consoles.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> drivers/tty/tty_io.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 15/18] tty: sysfs: Add nbcon support for 'active'
2024-06-03 23:24 ` [PATCH printk v2 15/18] tty: sysfs: Add nbcon support for 'active' John Ogness
2024-06-04 11:48 ` Greg Kroah-Hartman
@ 2024-07-01 15:50 ` Petr Mladek
1 sibling, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-07-01 15:50 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman, Jiri Slaby, linux-serial
On Tue 2024-06-04 01:30:50, John Ogness wrote:
> Allow the 'active' attribute to list nbcon consoles.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 16/18] printk: Provide threadprintk boot argument
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (14 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 15/18] tty: sysfs: Add nbcon support for 'active' John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-07-02 12:12 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 17/18] printk: Avoid false positive lockdep report for legacy printing John Ogness
` (2 subsequent siblings)
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Jonathan Corbet, Paul E. McKenney, Josh Poimboeuf,
Borislav Petkov (AMD), Xiongwei Song, linux-doc
For PREEMPT_RT, legacy console printing is performed in a dedicated
kthread. However, this behavior can also be interesting for other
preemption models as it minimizes the duration of printk() calls by
deferring all printing.
Provide a new boot argument "threadprintk" that will create the
dedicated kthread for legacy console printing for !PREEMPT_RT
systems.
The implementation is the same as the "threadirqs" boot argument.
Users should be aware that if this option is enabled, the shutdown,
reboot, and panic messages probably will not be visible on the
legacy consoles.
Non-legacy consoles (NBCON) already have their own dedicated kernel
threads for printing and reliable shutdown, reboot, and panic
printing. This option really only applies to legacy consoles.
Users can view /proc/consoles to see if their console driver is
legacy or not. NBCON console drivers are shown with 'N'.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
Documentation/admin-guide/kernel-parameters.txt | 12 ++++++++++++
kernel/printk/internal.h | 4 +++-
kernel/printk/printk.c | 11 +++++++++++
3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 45d95614ec44..17977dd4fafa 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -6572,6 +6572,18 @@
Force threading of all interrupt handlers except those
marked explicitly IRQF_NO_THREAD.
+ threadprintk [KNL]
+ Force threaded printing of all legacy consoles. Be
+ aware that with this option, the shutdown, reboot, and
+ panic messages may not be printed on the legacy
+ consoles. Also, earlycon/earlyprintk printing will be
+ delayed until a regular console or the kthread is
+ available.
+
+ Users can view /proc/consoles to see if their console
+ driver is legacy or not. Non-legacy (NBCON) console
+ drivers are already threaded and are shown with 'N'.
+
topology= [S390,EARLY]
Format: {off | on}
Specify if the kernel should make use of the cpu
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index b66dfa865591..48c3564f95eb 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -3,6 +3,7 @@
* internal.h - printk internal definitions
*/
#include <linux/console.h>
+#include <linux/jump_label.h>
#include <linux/percpu.h>
#include <linux/types.h>
@@ -24,7 +25,8 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
#ifdef CONFIG_PREEMPT_RT
# define force_printkthreads() (true)
#else
-# define force_printkthreads() (false)
+DECLARE_STATIC_KEY_FALSE(force_printkthreads_key);
+# define force_printkthreads() (static_branch_unlikely(&force_printkthreads_key))
#endif
#ifdef CONFIG_PRINTK
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 1c63fd0c1166..ea2d66152256 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -195,6 +195,17 @@ static int __init control_devkmsg(char *str)
}
__setup("printk.devkmsg=", control_devkmsg);
+#if !defined(CONFIG_PREEMPT_RT)
+DEFINE_STATIC_KEY_FALSE(force_printkthreads_key);
+
+static int __init setup_forced_printkthreads(char *arg)
+{
+ static_branch_enable(&force_printkthreads_key);
+ return 0;
+}
+early_param("threadprintk", setup_forced_printkthreads);
+#endif
+
char devkmsg_log_str[DEVKMSG_STR_MAX_SIZE] = "ratelimit";
#if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 16/18] printk: Provide threadprintk boot argument
2024-06-03 23:24 ` [PATCH printk v2 16/18] printk: Provide threadprintk boot argument John Ogness
@ 2024-07-02 12:12 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-07-02 12:12 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Jonathan Corbet, Paul E. McKenney, Josh Poimboeuf,
Borislav Petkov (AMD), Xiongwei Song, linux-doc
On Tue 2024-06-04 01:30:51, John Ogness wrote:
> For PREEMPT_RT, legacy console printing is performed in a dedicated
> kthread. However, this behavior can also be interesting for other
> preemption models as it minimizes the duration of printk() calls by
> deferring all printing.
>
> Provide a new boot argument "threadprintk" that will create the
> dedicated kthread for legacy console printing for !PREEMPT_RT
> systems.
>
> The implementation is the same as the "threadirqs" boot argument.
>
> Users should be aware that if this option is enabled, the shutdown,
> reboot, and panic messages probably will not be visible on the
> legacy consoles.
printk() is _heavily_ limited in this mode. Users would see the
messages only when the system is running well.
I think that this is a _big_ difference against "threadirqs".
It still allows to handle some critical IRQ handlers directly
by using IRQF_NO_THREAD.
> Non-legacy consoles (NBCON) already have their own dedicated kernel
> threads for printing and reliable shutdown, reboot, and panic
> printing. This option really only applies to legacy consoles.
OK, the NBCON consoles might make it a bit more useful. But there
will be only one at the beginning. And it won't work with boot
consoles.
All I want to say is that this mode has big limitations
and kind of weird semantic. This semantic is basically
needed only with PREEMPT_RT.
I would prefer to remove this patch for now. It would give us
more time to think about the threaded and sync modes.
Or I would at least use "force" in the name to make it more clear
that it forces some non-default (non-optimal) behavior.
I would call it "printk_thread_force" or "printk_legacy_thread_force".
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 17/18] printk: Avoid false positive lockdep report for legacy printing
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (15 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 16/18] printk: Provide threadprintk boot argument John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-07-02 13:26 ` Petr Mladek
2024-06-03 23:24 ` [PATCH printk v2 18/18] printk: nbcon: Add function for printers to reacquire ownership John Ogness
2024-06-04 13:31 ` [PATCH printk v2 00/18] add threaded printing + the rest Juri Lelli
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
Legacy console printing from printk() caller context may invoke
the console driver from atomic context. This leads to a lockdep
splat because the console driver will acquire a sleeping lock
and the caller may already hold a spinning lock. This is noticed
by lockdep on !PREEMPT_RT configurations because it will lead to
a problem on PREEMPT_RT.
However, on PREEMPT_RT the printing path from atomic context is
always avoided and the console driver is always invoked from a
dedicated thread. Thus the lockdep splat on !PREEMPT_RT is a
false positive.
For !PREEMPT_RT override the lock-context before invoking the
console driver to avoid the false positive.
Do not override the lock-context for PREEMPT_RT in order to
allow lockdep to catch any real locking context issues related
to the write callback usage.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/printk.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index ea2d66152256..5e9778f69206 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2980,6 +2980,33 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
return true;
}
+/*
+ * Legacy console printing from printk() caller context does not respect
+ * raw_spinlock/spinlock nesting. For !PREEMPT_RT the lockdep warning is a
+ * false positive. For PREEMPT_RT the false positive condition does not
+ * occur.
+ *
+ * This map is used to establish LD_WAIT_SLEEP context for the console write
+ * callbacks when legacy printing to avoid false positive lockdep complaints,
+ * thus allowing lockdep to continue to function for real issues.
+ */
+#ifdef CONFIG_PREEMPT_RT
+static inline void printk_legacy_lock_map_acquire_try(void) { }
+static inline void printk_legacy_lock_map_release(void) { }
+#else
+static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
+
+static inline void printk_legacy_lock_map_acquire_try(void)
+{
+ lock_map_acquire_try(&printk_legacy_map);
+}
+
+static inline void printk_legacy_lock_map_release(void)
+{
+ lock_map_release(&printk_legacy_map);
+}
+#endif /* CONFIG_PREEMPT_RT */
+
/*
* Used as the printk buffers for non-panic, serialized console printing.
* This is for legacy (!CON_NBCON) as well as all boot (CON_BOOT) consoles.
@@ -3035,7 +3062,7 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
/*
* With forced threading this function is either in a thread
* or panic context. So there is no need for concern about
- * printk reentrance or handovers.
+ * printk reentrance, handovers, or lockdep complaints.
*/
con->write(con, outbuf, pmsg.outbuf_len);
@@ -3057,7 +3084,9 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
/* Do not trace print latency. */
stop_critical_timings();
+ printk_legacy_lock_map_acquire_try();
con->write(con, outbuf, pmsg.outbuf_len);
+ printk_legacy_lock_map_release();
start_critical_timings();
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 17/18] printk: Avoid false positive lockdep report for legacy printing
2024-06-03 23:24 ` [PATCH printk v2 17/18] printk: Avoid false positive lockdep report for legacy printing John Ogness
@ 2024-07-02 13:26 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-07-02 13:26 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Tue 2024-06-04 01:30:52, John Ogness wrote:
> Legacy console printing from printk() caller context may invoke
> the console driver from atomic context. This leads to a lockdep
> splat because the console driver will acquire a sleeping lock
> and the caller may already hold a spinning lock. This is noticed
> by lockdep on !PREEMPT_RT configurations because it will lead to
> a problem on PREEMPT_RT.
>
> However, on PREEMPT_RT the printing path from atomic context is
> always avoided and the console driver is always invoked from a
> dedicated thread. Thus the lockdep splat on !PREEMPT_RT is a
> false positive.
>
> For !PREEMPT_RT override the lock-context before invoking the
> console driver to avoid the false positive.
>
> Do not override the lock-context for PREEMPT_RT in order to
> allow lockdep to catch any real locking context issues related
> to the write callback usage.
Could you please provide more information?
Is this anything new?
How did it work so far?
See below.
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> ---
> kernel/printk/printk.c | 31 ++++++++++++++++++++++++++++++-
> 1 file changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index ea2d66152256..5e9778f69206 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2980,6 +2980,33 @@ bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
> return true;
> }
>
> +/*
> + * Legacy console printing from printk() caller context does not respect
> + * raw_spinlock/spinlock nesting. For !PREEMPT_RT the lockdep warning is a
> + * false positive. For PREEMPT_RT the false positive condition does not
> + * occur.
> + *
> + * This map is used to establish LD_WAIT_SLEEP context for the console write
> + * callbacks when legacy printing to avoid false positive lockdep complaints,
> + * thus allowing lockdep to continue to function for real issues.
> + */
> +#ifdef CONFIG_PREEMPT_RT
> +static inline void printk_legacy_lock_map_acquire_try(void) { }
> +static inline void printk_legacy_lock_map_release(void) { }
> +#else
> +static DEFINE_WAIT_OVERRIDE_MAP(printk_legacy_map, LD_WAIT_SLEEP);
> +
> +static inline void printk_legacy_lock_map_acquire_try(void)
> +{
> + lock_map_acquire_try(&printk_legacy_map);
> +}
> +
> +static inline void printk_legacy_lock_map_release(void)
> +{
> + lock_map_release(&printk_legacy_map);
> +}
> +#endif /* CONFIG_PREEMPT_RT */
> +
> /*
> * Used as the printk buffers for non-panic, serialized console printing.
> * This is for legacy (!CON_NBCON) as well as all boot (CON_BOOT) consoles.
> @@ -3035,7 +3062,7 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
> /*
> * With forced threading this function is either in a thread
> * or panic context. So there is no need for concern about
> - * printk reentrance or handovers.
> + * printk reentrance, handovers, or lockdep complaints.
> */
>
> con->write(con, outbuf, pmsg.outbuf_len);
> @@ -3057,7 +3084,9 @@ static bool console_emit_next_record(struct console *con, bool *handover, int co
> /* Do not trace print latency. */
> stop_critical_timings();
>
> + printk_legacy_lock_map_acquire_try();
I am not familiar with lockdep in these depths. The comment in
include/kernel/lockdep_types.h says that LD_WAIT_SLEEP is
a type used for sleeping locks. And this code says that
it is acquire_try(). I would assume that similar lockdep
map is set also by "console_trylock()".
Do we need this for a new callpath without console_trylock()?
Or do I miss anything?
> con->write(con, outbuf, pmsg.outbuf_len);
> + printk_legacy_lock_map_release();
>
> start_critical_timings();
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH printk v2 18/18] printk: nbcon: Add function for printers to reacquire ownership
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (16 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 17/18] printk: Avoid false positive lockdep report for legacy printing John Ogness
@ 2024-06-03 23:24 ` John Ogness
2024-07-02 14:31 ` Petr Mladek
2024-06-04 13:31 ` [PATCH printk v2 00/18] add threaded printing + the rest Juri Lelli
18 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-03 23:24 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
Since ownership can be lost at any time due to handover or
takeover, a printing context _must_ be prepared to back out
immediately and carefully. However, there are scenarios where
the printing context must reacquire ownership in order to
finalize or revert hardware changes.
One such example is when interrupts are disabled during
printing. No other context will automagically re-enable the
interrupts. For this case, the disabling context _must_
reacquire nbcon ownership so that it can re-enable the
interrupts.
Provide nbcon_reacquire() for exactly this purpose. It allows a
printing context to reacquire ownership using the same priority
as its previous ownership.
Note that after a successful reacquire the printing context
will have no output buffer because that has been lost. This
function cannot be used to resume printing.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
include/linux/console.h | 6 ++++++
kernel/printk/nbcon.c | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+)
diff --git a/include/linux/console.h b/include/linux/console.h
index 9927f08ac054..96c0923d023b 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -372,6 +372,10 @@ struct console {
*
* The callback should allow the takeover whenever it is safe. It
* increases the chance to see messages when the system is in trouble.
+ * If the driver must reacquire ownership in order to finalize or
+ * revert hardware changes, nbcon_reacquire() can be used. However,
+ * on reacquire the buffer content is no longer available. A
+ * reacquire cannot be used to resume printing.
*
* The callback can be called from any context (including NMI).
* Therefore it must avoid usage of any locking and instead rely
@@ -591,6 +595,7 @@ extern void nbcon_cpu_emergency_flush(void);
extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
+extern void nbcon_reacquire(struct nbcon_write_context *wctxt);
#else
static inline void nbcon_cpu_emergency_enter(void) { }
static inline void nbcon_cpu_emergency_exit(void) { }
@@ -598,6 +603,7 @@ static inline void nbcon_cpu_emergency_flush(void) { }
static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
+static inline void nbcon_reacquire(struct nbcon_write_context *wctxt) { }
#endif
extern int console_set_on_cmdline;
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index e8060b5abdf8..4b9645e7ed70 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -838,6 +838,38 @@ bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt)
}
EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
+/**
+ * nbcon_reacquire - Reacquire a console after losing ownership while printing
+ * @wctxt: The write context that was handed to the write callback
+ *
+ * Since ownership can be lost at any time due to handover or takeover, a
+ * printing context _must_ be prepared to back out immediately and
+ * carefully. However, there are scenarios where the printing context must
+ * reacquire ownership in order to finalize or revert hardware changes.
+ *
+ * This function allows a printing context to reacquire ownership using the
+ * same priority as its previous ownership.
+ *
+ * Note that after a successful reacquire the printing context will have no
+ * output buffer because that has been lost. This function cannot be used to
+ * resume printing.
+ */
+void nbcon_reacquire(struct nbcon_write_context *wctxt)
+{
+ struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+ struct console *con = ctxt->console;
+ struct nbcon_state cur;
+
+ while (!nbcon_context_try_acquire(ctxt))
+ cpu_relax();
+
+ wctxt->outbuf = NULL;
+ wctxt->len = 0;
+ nbcon_state_read(con, &cur);
+ wctxt->unsafe_takeover = cur.unsafe_takeover;
+}
+EXPORT_SYMBOL_GPL(nbcon_reacquire);
+
/**
* nbcon_emit_next_record - Emit a record in the acquired context
* @wctxt: The write context that will be handed to the write function
@@ -945,6 +977,15 @@ static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt, bool use_a
return false;
}
+ if (!wctxt->outbuf) {
+ /*
+ * Ownership was lost and reacquired by the driver.
+ * Handle it as if ownership was lost.
+ */
+ nbcon_context_release(ctxt);
+ return false;
+ }
+
/*
* Since any dropped message was successfully output, reset the
* dropped count for the console.
--
2.39.2
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 18/18] printk: nbcon: Add function for printers to reacquire ownership
2024-06-03 23:24 ` [PATCH printk v2 18/18] printk: nbcon: Add function for printers to reacquire ownership John Ogness
@ 2024-07-02 14:31 ` Petr Mladek
0 siblings, 0 replies; 63+ messages in thread
From: Petr Mladek @ 2024-07-02 14:31 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Tue 2024-06-04 01:30:53, John Ogness wrote:
> Since ownership can be lost at any time due to handover or
> takeover, a printing context _must_ be prepared to back out
> immediately and carefully. However, there are scenarios where
> the printing context must reacquire ownership in order to
> finalize or revert hardware changes.
>
> One such example is when interrupts are disabled during
> printing. No other context will automagically re-enable the
> interrupts. For this case, the disabling context _must_
> reacquire nbcon ownership so that it can re-enable the
> interrupts.
>
> Provide nbcon_reacquire() for exactly this purpose. It allows a
> printing context to reacquire ownership using the same priority
> as its previous ownership.
>
> Note that after a successful reacquire the printing context
> will have no output buffer because that has been lost. This
> function cannot be used to resume printing.
>
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -838,6 +838,38 @@ bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt)
> }
> EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
>
> +/**
> + * nbcon_reacquire - Reacquire a console after losing ownership while printing
> + * @wctxt: The write context that was handed to the write callback
> + *
> + * Since ownership can be lost at any time due to handover or takeover, a
> + * printing context _must_ be prepared to back out immediately and
> + * carefully. However, there are scenarios where the printing context must
> + * reacquire ownership in order to finalize or revert hardware changes.
> + *
> + * This function allows a printing context to reacquire ownership using the
> + * same priority as its previous ownership.
> + *
> + * Note that after a successful reacquire the printing context will have no
> + * output buffer because that has been lost. This function cannot be used to
> + * resume printing.
> + */
> +void nbcon_reacquire(struct nbcon_write_context *wctxt)
I think about calling it "nbcon_reacquire_nobuf" to make it clear
that it is not a complete recovery.
> +{
> + struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> + struct console *con = ctxt->console;
> + struct nbcon_state cur;
> +
> + while (!nbcon_context_try_acquire(ctxt))
> + cpu_relax();
> +
> + wctxt->outbuf = NULL;
> + wctxt->len = 0;
> + nbcon_state_read(con, &cur);
> + wctxt->unsafe_takeover = cur.unsafe_takeover;
The nbcon_state_read() makes it a bit tricky. I would hide it into
a helper script:
void nbcon_write_context_set_buf(struct nbcon_write_context *wctxt,
char *buf, unsigned int len)
{
struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
struct console *con = ctxt->console;
struct nbcon_state cur;
wctxt->outbuf = buf;
wctxt->len = len;
nbcon_state_read(con, &cur);
wctxt->unsafe_takeover = cur.unsafe_takeover;
}
It would help to keep it in sync with nbcon_emit_next_record().
> +}
> +EXPORT_SYMBOL_GPL(nbcon_reacquire);
Otherwise, it looks good. I do not resist on the proposed changes.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH printk v2 00/18] add threaded printing + the rest
2024-06-03 23:24 [PATCH printk v2 00/18] add threaded printing + the rest John Ogness
` (17 preceding siblings ...)
2024-06-03 23:24 ` [PATCH printk v2 18/18] printk: nbcon: Add function for printers to reacquire ownership John Ogness
@ 2024-06-04 13:31 ` Juri Lelli
2024-06-05 8:09 ` John Ogness
18 siblings, 1 reply; 63+ messages in thread
From: Juri Lelli @ 2024-06-04 13:31 UTC (permalink / raw)
To: John Ogness
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner,
linux-kernel, Jonathan Corbet, Greg Kroah-Hartman, Jiri Slaby,
Sreenath Vijayan, Shimoyashiki Taichi, Tomas Mudrunka, linux-doc,
linux-serial, linux-fsdevel, Paul E. McKenney, Josh Poimboeuf,
Borislav Petkov (AMD), Xiongwei Song
Hi John,
On 04/06/24 01:30, John Ogness wrote:
> Hi,
>
> This is v2 of a series to implement threaded console printing as well
> as some other minor pieces (such as proc and sysfs support). This
> series is only a subset of the original v1 [0]. In particular, this
> series represents patches 11, 12, 15 of the v1 series. For information
> about the motivation of the nbcon consoles, please read the cover
> letter of v1.
>
> This series provides the remaining pieces of the printk rework. All
> other components are either already mainline or are currently in
> linux-next. In particular this series does:
Our QE reported something like the following while testing the latest
rt-devel branch (I then could reproduce with this set applied on top of
linux-next).
---
... kernel: INFO: task khugepaged:351 blocked for more than 1 seconds.
... kernel: Not tainted 6.9.0-thrdprintk+ #3
... kernel: "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
... kernel: task:khugepaged state:D stack:0 pid:351 tgid:351 ppid:2 flags:0x00004000
... kernel: Call Trace:
... kernel: <TASK>
... kernel: __schedule+0x2bd/0x7f0
... kernel: ? __lock_release.isra.0+0x5e/0x170
... kernel: schedule+0x3d/0x100
... kernel: schedule_timeout+0x1ca/0x1f0
... kernel: ? mark_held_locks+0x49/0x80
... kernel: ? _raw_spin_unlock_irq+0x24/0x50
... kernel: ? lockdep_hardirqs_on+0x77/0x100
... kernel: __wait_for_common+0xb7/0x220
... kernel: ? __pfx_schedule_timeout+0x10/0x10
... kernel: __flush_work+0x70/0x90
... kernel: ? __pfx_wq_barrier_func+0x10/0x10
... kernel: __lru_add_drain_all+0x179/0x210
... kernel: khugepaged+0x73/0x200
... kernel: ? lockdep_hardirqs_on+0x77/0x100
... kernel: ? _raw_spin_unlock_irqrestore+0x38/0x60
... kernel: ? __pfx_khugepaged+0x10/0x10
... kernel: kthread+0xec/0x120
... kernel: ? __pfx_kthread+0x10/0x10
... kernel: ret_from_fork+0x2d/0x50
... kernel: ? __pfx_kthread+0x10/0x10
... kernel: ret_from_fork_asm+0x1a/0x30
... kernel: </TASK>
... kernel:
... Showing all locks held in the system:
... kernel: 1 lock held by khungtaskd/345:
... kernel: #0: ffffffff8cbff1c0 (rcu_read_lock){....}-{1:2}, at: debug_show_all_locks+0x32/0x1d0
... kernel: BUG: using smp_processor_id() in preemptible [00000000] code: khungtaskd/345
... kernel: caller is nbcon_get_cpu_emergency_nesting+0x25/0x40
... kernel: CPU: 30 PID: 345 Comm: khungtaskd Kdump: loaded Not tainted 6.9.0-thrdprintk+ #3
... kernel: Hardware name: Dell Inc. PowerEdge R740/04FC42, BIOS 2.10.2 02/24/2021
... kernel: Call Trace:
... kernel: <TASK>
... kernel: dump_stack_lvl+0x7f/0xa0
... kernel: check_preemption_disabled+0xbf/0xe0
... kernel: nbcon_get_cpu_emergency_nesting+0x25/0x40
... kernel: nbcon_cpu_emergency_flush+0xa/0x60
... kernel: debug_show_all_locks+0x9d/0x1d0
... kernel: check_hung_uninterruptible_tasks+0x4f0/0x540
... kernel: ? check_hung_uninterruptible_tasks+0x185/0x540
... kernel: ? __pfx_watchdog+0x10/0x10
... kernel: watchdog+0x99/0xa0
... kernel: kthread+0xec/0x120
... kernel: ? __pfx_kthread+0x10/0x10
... kernel: ret_from_fork+0x2d/0x50
... kernel: ? __pfx_kthread+0x10/0x10
... kernel: ret_from_fork_asm+0x1a/0x30
... kernel: </TASK>
---
It requires DEBUG_PREEMPT and LOCKDEP enabled, sched_rt_runtime_us = -1
and a while(1) loop running at FIFO for some time (I also set sysctl
kernel.hung_task_timeout_secs=1 to speed up reproduction).
Looks like check_hung_uninterruptible_tasks() requires some care as you
did already in linux-next for panic, rcu and lockdep ("Make emergency
sections ...")?
Thanks,
Juri
^ permalink raw reply [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 00/18] add threaded printing + the rest
2024-06-04 13:31 ` [PATCH printk v2 00/18] add threaded printing + the rest Juri Lelli
@ 2024-06-05 8:09 ` John Ogness
2024-06-05 9:32 ` Juri Lelli
0 siblings, 1 reply; 63+ messages in thread
From: John Ogness @ 2024-06-05 8:09 UTC (permalink / raw)
To: Juri Lelli
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner,
linux-kernel, Jonathan Corbet, Greg Kroah-Hartman, Jiri Slaby,
Sreenath Vijayan, Shimoyashiki Taichi, Tomas Mudrunka, linux-doc,
linux-serial, linux-fsdevel, Paul E. McKenney, Josh Poimboeuf,
Borislav Petkov (AMD), Xiongwei Song
Hi Juri,
On 2024-06-04, Juri Lelli <juri.lelli@redhat.com> wrote:
> Our QE reported something like the following while testing the latest
> rt-devel branch (I then could reproduce with this set applied on top of
> linux-next).
>
> ---
> ... kernel: INFO: task khugepaged:351 blocked for more than 1 seconds.
> ... kernel: Not tainted 6.9.0-thrdprintk+ #3
> ... kernel: "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> ... kernel: task:khugepaged state:D stack:0 pid:351 tgid:351 ppid:2 flags:0x00004000
> ... kernel: Call Trace:
> ... kernel: <TASK>
> ... kernel: __schedule+0x2bd/0x7f0
> ... kernel: ? __lock_release.isra.0+0x5e/0x170
> ... kernel: schedule+0x3d/0x100
> ... kernel: schedule_timeout+0x1ca/0x1f0
> ... kernel: ? mark_held_locks+0x49/0x80
> ... kernel: ? _raw_spin_unlock_irq+0x24/0x50
> ... kernel: ? lockdep_hardirqs_on+0x77/0x100
> ... kernel: __wait_for_common+0xb7/0x220
> ... kernel: ? __pfx_schedule_timeout+0x10/0x10
> ... kernel: __flush_work+0x70/0x90
> ... kernel: ? __pfx_wq_barrier_func+0x10/0x10
> ... kernel: __lru_add_drain_all+0x179/0x210
> ... kernel: khugepaged+0x73/0x200
> ... kernel: ? lockdep_hardirqs_on+0x77/0x100
> ... kernel: ? _raw_spin_unlock_irqrestore+0x38/0x60
> ... kernel: ? __pfx_khugepaged+0x10/0x10
> ... kernel: kthread+0xec/0x120
> ... kernel: ? __pfx_kthread+0x10/0x10
> ... kernel: ret_from_fork+0x2d/0x50
> ... kernel: ? __pfx_kthread+0x10/0x10
> ... kernel: ret_from_fork_asm+0x1a/0x30
> ... kernel: </TASK>
> ... kernel:
> ... Showing all locks held in the system:
> ... kernel: 1 lock held by khungtaskd/345:
> ... kernel: #0: ffffffff8cbff1c0 (rcu_read_lock){....}-{1:2}, at: debug_show_all_locks+0x32/0x1d0
> ... kernel: BUG: using smp_processor_id() in preemptible [00000000] code: khungtaskd/345
> ... kernel: caller is nbcon_get_cpu_emergency_nesting+0x25/0x40
> ... kernel: CPU: 30 PID: 345 Comm: khungtaskd Kdump: loaded Not tainted 6.9.0-thrdprintk+ #3
> ... kernel: Hardware name: Dell Inc. PowerEdge R740/04FC42, BIOS 2.10.2 02/24/2021
> ... kernel: Call Trace:
> ... kernel: <TASK>
> ... kernel: dump_stack_lvl+0x7f/0xa0
> ... kernel: check_preemption_disabled+0xbf/0xe0
> ... kernel: nbcon_get_cpu_emergency_nesting+0x25/0x40
> ... kernel: nbcon_cpu_emergency_flush+0xa/0x60
> ... kernel: debug_show_all_locks+0x9d/0x1d0
> ... kernel: check_hung_uninterruptible_tasks+0x4f0/0x540
> ... kernel: ? check_hung_uninterruptible_tasks+0x185/0x540
> ... kernel: ? __pfx_watchdog+0x10/0x10
> ... kernel: watchdog+0x99/0xa0
> ... kernel: kthread+0xec/0x120
> ... kernel: ? __pfx_kthread+0x10/0x10
> ... kernel: ret_from_fork+0x2d/0x50
> ... kernel: ? __pfx_kthread+0x10/0x10
> ... kernel: ret_from_fork_asm+0x1a/0x30
> ... kernel: </TASK>
> ---
>
> It requires DEBUG_PREEMPT and LOCKDEP enabled, sched_rt_runtime_us = -1
> and a while(1) loop running at FIFO for some time (I also set sysctl
> kernel.hung_task_timeout_secs=1 to speed up reproduction).
>
> Looks like check_hung_uninterruptible_tasks() requires some care as you
> did already in linux-next for panic, rcu and lockdep ("Make emergency
> sections ...")?
Yes, that probably is a good candidate for emergency mode.
However, your report is also identifying a real issue:
nbcon_cpu_emergency_flush() was implemented to be callable from
non-emergency contexts (in which case it should do nothing). However, in
order to check if it is an emergency context, migration needs to be
disabled.
Perhaps the below change can be made for v2 of this series?
John
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 4b9645e7ed70..eeaf8465f492 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1581,8 +1581,19 @@ void nbcon_cpu_emergency_exit(void)
*/
void nbcon_cpu_emergency_flush(void)
{
+ bool is_emergency;
+
+ /*
+ * If the current context is not an emergency context, preemption
+ * might be enabled. To be sure, disable preemption when checking
+ * if this is an emergency context.
+ */
+ preempt_disable();
+ is_emergency = (*nbcon_get_cpu_emergency_nesting() != 0);
+ preempt_enable();
+
/* The explicit flush is needed only in the emergency context. */
- if (*(nbcon_get_cpu_emergency_nesting()) == 0)
+ if (!is_emergency)
return;
nbcon_atomic_flush_pending();
^ permalink raw reply related [flat|nested] 63+ messages in thread* Re: [PATCH printk v2 00/18] add threaded printing + the rest
2024-06-05 8:09 ` John Ogness
@ 2024-06-05 9:32 ` Juri Lelli
0 siblings, 0 replies; 63+ messages in thread
From: Juri Lelli @ 2024-06-05 9:32 UTC (permalink / raw)
To: John Ogness
Cc: Petr Mladek, Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner,
linux-kernel, Jonathan Corbet, Greg Kroah-Hartman, Jiri Slaby,
Sreenath Vijayan, Shimoyashiki Taichi, Tomas Mudrunka, linux-doc,
linux-serial, linux-fsdevel, Paul E. McKenney, Josh Poimboeuf,
Borislav Petkov (AMD), Xiongwei Song
On 05/06/24 10:15, John Ogness wrote:
...
> Yes, that probably is a good candidate for emergency mode.
>
> However, your report is also identifying a real issue:
> nbcon_cpu_emergency_flush() was implemented to be callable from
> non-emergency contexts (in which case it should do nothing). However, in
> order to check if it is an emergency context, migration needs to be
> disabled.
I see.
> Perhaps the below change can be made for v2 of this series?
Yes, this seems to cure it.
Thanks for the super quick reply and patch!
Best,
Juri
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 4b9645e7ed70..eeaf8465f492 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1581,8 +1581,19 @@ void nbcon_cpu_emergency_exit(void)
> */
> void nbcon_cpu_emergency_flush(void)
> {
> + bool is_emergency;
> +
> + /*
> + * If the current context is not an emergency context, preemption
> + * might be enabled. To be sure, disable preemption when checking
> + * if this is an emergency context.
> + */
> + preempt_disable();
> + is_emergency = (*nbcon_get_cpu_emergency_nesting() != 0);
> + preempt_enable();
> +
> /* The explicit flush is needed only in the emergency context. */
> - if (*(nbcon_get_cpu_emergency_nesting()) == 0)
> + if (!is_emergency)
> return;
>
> nbcon_atomic_flush_pending();
>
^ permalink raw reply [flat|nested] 63+ messages in thread