Re: [PATCH printk v2 09/18] printk: nbcon: Start printing threads

[Petr Mladek <pmladek@suse.com>]

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