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

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

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