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 <john.ogness@linutronix.de>]

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