Re: [PATCH RFC 0/3] printk: Don't flush messages using write_atomic during console registration if kthreads have not been started yet.

John Ogness
2025-06-03T16:09:50Z

On 2025-06-03, Petr Mladek <pmladek@suse.com> wrote:
> I thought whether we could avoid introducing yet another variable
> and still keep the code sane. And I came with the following.
> The commit messages describes the idea.
>
> I hope that I have covered all the cases. Note that I haven't tested
> it with nbcon console though.
>
> What do you think, please?
>
> From 5768ff7e9d944bb904344341a2a447d2f101e6ba Mon Sep 17 00:00:00 2001
> From: Petr Mladek <pmladek@suse.com>
> Date: Tue, 3 Jun 2025 14:19:00 +0200
> Subject: [PATCH] printk: Allow to use the printk kthread immediately even for
> 1st nbcon
>
> The kthreads for nbcon consoles are created by nbcon_alloc() at the beginning
> of the console registration. But it currently works only for the 2nd or
> later nbcon console because the code checks @printk_kthreads_running.
>
> The kthread for the 1st registered nbcon console is created at the very
> end of register_console() by printk_kthreads_check_locked(). As a result,
> the entire log is replayed synchronously when the "enabled" message
> gets printed. It might block the boot for a long time with a slow serial
> console.
>
> Prevent the synchronous flush by creating the kthread even for the 1st
> nbcon console when it is safe (kthreads ready and no boot consoles).
>
> Also inform printk() to use the kthread by setting @printk_kthreads_running.
> Note that the kthreads already must be running when it is safe and this
> is not the 1st nbcon console.
>
> Symmetrically, clear @printk_kthreads_running when the last nbcon
> console was unregistered by nbcon_free(). This requires updating
> @have_nbcon_console before nbcon_free() gets called.
>
> Note that there is _no_ problem when the 1st nbcon console replaces boot
> consoles. In this case, the kthread will be started at the end
> of registration after the boot consoles are removed. But the console
> does not reply the entire log buffer in this case. Note that
> the flag CON_PRINTBUFFER is always cleared when the boot consoles are
> removed and vice versa.
>
> Closes: https://lore.kernel.org/r/20250514173514.2117832-1-mcobb@thegoodpenguin.co.uk
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> kernel/printk/internal.h | 2 ++
> kernel/printk/nbcon.c | 17 +++++++++++++++--
> kernel/printk/printk.c | 19 ++++++++++---------
> 3 files changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 48a24e7b309d..567c9e100d47 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -64,6 +64,7 @@ struct dev_printk_info;
>
> extern struct printk_ringbuffer *prb;
> extern bool printk_kthreads_running;
> +extern bool printk_kthreads_ready;
> extern bool debug_non_panic_cpus;
>
> __printf(4, 0)
> @@ -180,6 +181,7 @@ static inline void nbcon_kthread_wake(struct console *con)
> #define PRINTKRB_RECORD_MAX 0
>
> #define printk_kthreads_running (false)
> +#define printk_kthreads_ready (false)
>
> /*
> * In !PRINTK builds we still export console_sem
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index fd12efcc4aed..7519d09c20e7 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1671,6 +1671,9 @@ bool nbcon_alloc(struct console *con)
> {
> struct nbcon_state state = { };
>
> + /* Synchronize the kthread start. */
> + lockdep_assert_console_list_lock_held();
> +
> /* The write_thread() callback is mandatory. */
> if (WARN_ON(!con->write_thread))
> return false;
> @@ -1701,12 +1704,15 @@ bool nbcon_alloc(struct console *con)
> return false;
> }
>
> - if (printk_kthreads_running) {
> + if (printk_kthreads_ready && !have_boot_console) {
> if (!nbcon_kthread_create(con)) {
> kfree(con->pbufs);
> con->pbufs = NULL;
> return false;
> }
> +
> + /* Might be the first kthread. */
> + printk_kthreads_running = true;
> }
> }
>
> @@ -1721,8 +1727,15 @@ void nbcon_free(struct console *con)
> {
> struct nbcon_state state = { };
>
> - if (printk_kthreads_running)
> + /* Synchronize the kthread stop. */
> + lockdep_assert_console_list_lock_held();
> +
> + if (printk_kthreads_running) {
> nbcon_kthread_stop(con);
> + /* Might be the last nbcon console */

Super small nit... add a period at the end of the comment to be
consistent with the one one-liners.

> + if (!have_nbcon_console)
> + printk_kthreads_running = false;
> + }

This is pretty tricky. We should have a comment here (and possibly in
the function description of nbcon_free()) mentioning that nbcon_free()
must be called _after_ @have_nbcon_console has been updated for the
removal of this console. Generally it would not matter because
printk_kthreads_check_locked() is called at the end of
unregister. However, if in register CON_BRL is set, then nbcon_free() is
called without ever calling printk_kthreads_check_locked(). So this new
code in nbcon_free() is necessary to correctly reset
@printk_kthreads_running in that case.

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 1eea80d0648e..af6e4f0e8e22 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3574,7 +3574,7 @@ EXPORT_SYMBOL(console_resume);
> static int unregister_console_locked(struct console *console);
>
> /* True when system boot is far enough to create printer threads. */
> -static bool printk_kthreads_ready __ro_after_init;
> +bool printk_kthreads_ready __ro_after_init;
>
> static struct task_struct *printk_legacy_kthread;
>
> @@ -3713,6 +3713,7 @@ static void printk_kthreads_check_locked(void)
> if (!printk_kthreads_ready)
> return;
>
> + /* Start or stop the legacy kthread when needed. */
> if (have_legacy_console || have_boot_console) {
> if (!printk_legacy_kthread &&
> force_legacy_kthread() &&
> @@ -4204,14 +4205,6 @@ static int unregister_console_locked(struct console *console)
> */
> synchronize_srcu(&console_srcu);
>
> - if (console->flags & CON_NBCON)
> - nbcon_free(console);
> -
> - console_sysfs_notify();
> -
> - if (console->exit)
> - res = console->exit(console);
> -
> /*
> * With this console gone, the global flags tracking registered
> * console types may have changed. Update them.
> @@ -4232,6 +4225,14 @@ static int unregister_console_locked(struct console *console)
> if (!found_nbcon_con)
> have_nbcon_console = found_nbcon_con;
>

Maybe also a small comment here that it can be freed because
@have_nbcon_console has been updated. Just to leave a little hint for
future developers that its location is important.

> + if (console->flags & CON_NBCON)
> + nbcon_free(console);
> +
> + console_sysfs_notify();
> +
> + if (console->exit)
> + res = console->exit(console);
> +
> /* Changed console list, may require printer threads to start/stop. */
> printk_kthreads_check_locked();

Aside from documenting these new subtle relationships, I think this is
a good solution.

Note that LKML is not CC. I can offer my reviewed-by when the patch is
posted on LKML.

John Ogness