Once the kthread is running and available
(i.e. @printk_kthreads_running is set), the kthread becomes
responsible for flushing any pending messages which are added
in NBCON_PRIO_NORMAL context. Namely the legacy
console_flush_all() and device_release() no longer flush the
console. And nbcon_atomic_flush_pending() used by
nbcon_cpu_emergency_exit() no longer flushes messages added
after the emergency messages.

The console context is safe when used by the kthread only when
one of the following conditions are true:

  1. Other caller acquires the console context with
     NBCON_PRIO_NORMAL with preemption disabled. It will
     release the context before rescheduling.

  2. Other caller acquires the console context with
     NBCON_PRIO_NORMAL under the device_lock.

  3. The kthread is the only context which acquires the console
     with NBCON_PRIO_NORMAL.

This is satisfied for all atomic printing call sites:

nbcon_legacy_emit_next_record() (#1)

nbcon_atomic_flush_pending_con() (#1)

nbcon_device_release() (#2)

It is even double guaranteed when @printk_kthreads_running
is set because then _only_ the kthread will print for
NBCON_PRIO_NORMAL. (#3)

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/internal.h | 26 ++++++++++++++++++++++++
 kernel/printk/nbcon.c    | 17 ++++++++++------
 kernel/printk/printk.c   | 43 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index a96d4114a1db..8166e24f8780 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -113,6 +113,13 @@ static inline bool console_is_usable(struct console *con, short flags, bool use_
 		/* The write_atomic() callback is optional. */
 		if (use_atomic && !con->write_atomic)
 			return false;
+
+		/*
+		 * For the !use_atomic case, @printk_kthreads_running is not
+		 * checked because the write_thread() callback is also used
+		 * via the legacy loop when the printer threads are not
+		 * available.
+		 */
 	} else {
 		if (!con->write)
 			return false;
@@ -176,6 +183,7 @@ static inline void nbcon_atomic_flush_pending(void) { }
 static inline bool nbcon_legacy_emit_next_record(struct console *con, bool *handover,
 						 int cookie, bool use_atomic) { return false; }
 static inline void nbcon_kthread_wake(struct console *con) { }
+static inline void nbcon_kthreads_wake(void) { }
 
 static inline bool console_is_usable(struct console *con, short flags,
 				     bool use_atomic) { return false; }
@@ -190,6 +198,7 @@ extern bool legacy_allow_panic_sync;
 /**
  * struct console_flush_type - Define available console flush methods
  * @nbcon_atomic:	Flush directly using nbcon_atomic() callback
+ * @nbcon_offload:	Offload flush to printer thread
  * @legacy_direct:	Call the legacy loop in this context
  * @legacy_offload:	Offload the legacy loop into IRQ
  *
@@ -197,6 +206,7 @@ extern bool legacy_allow_panic_sync;
  */
 struct console_flush_type {
 	bool	nbcon_atomic;
+	bool	nbcon_offload;
 	bool	legacy_direct;
 	bool	legacy_offload;
 };
@@ -211,6 +221,22 @@ static inline void printk_get_console_flush_type(struct console_flush_type *ft)
 
 	switch (nbcon_get_default_prio()) {
 	case NBCON_PRIO_NORMAL:
+		if (have_nbcon_console && !have_boot_console) {
+			if (printk_kthreads_running)
+				ft->nbcon_offload = true;
+			else
+				ft->nbcon_atomic = true;
+		}
+
+		/* Legacy consoles are flushed directly when possible. */
+		if (have_legacy_console || have_boot_console) {
+			if (!is_printk_legacy_deferred())
+				ft->legacy_direct = true;
+			else
+				ft->legacy_offload = true;
+		}
+		break;
+
 	case NBCON_PRIO_EMERGENCY:
 		if (have_nbcon_console && !have_boot_console)
 			ft->nbcon_atomic = true;
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 623f3a29bfc6..c36473fbbf89 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1486,6 +1486,7 @@ static int __nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq,
 static void nbcon_atomic_flush_pending_con(struct console *con, u64 stop_seq,
 					   bool allow_unsafe_takeover)
 {
+	struct console_flush_type ft;
 	unsigned long flags;
 	int err;
 
@@ -1515,10 +1516,12 @@ 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)) {
+	printk_get_console_flush_type(&ft);
+	if (!ft.nbcon_offload &&
+	    prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
 		stop_seq = prb_next_reserve_seq(prb);
 		goto again;
 	}
@@ -1745,17 +1748,19 @@ void nbcon_device_release(struct console *con)
 
 	/*
 	 * This context must flush any new records added while the console
-	 * was locked. The console_srcu_read_lock must be taken to ensure
-	 * the console is usable throughout flushing.
+	 * was locked if the printer thread is not available to do it. The
+	 * console_srcu_read_lock must be taken to ensure the console is
+	 * usable throughout flushing.
 	 */
 	cookie = console_srcu_read_lock();
+	printk_get_console_flush_type(&ft);
 	if (console_is_usable(con, console_srcu_read_flags(con), true) &&
+	    !ft.nbcon_offload &&
 	    prb_read_valid(prb, nbcon_seq_read(con), NULL)) {
 		/*
 		 * If nbcon_atomic flushing is not available, fallback to
 		 * using the legacy loop.
 		 */
-		printk_get_console_flush_type(&ft);
 		if (ft.nbcon_atomic) {
 			__nbcon_atomic_flush_pending_con(con, prb_next_reserve_seq(prb), false);
 		} else if (ft.legacy_direct) {
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 55d75db00042..0d45d82a7c79 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2384,6 +2384,9 @@ asmlinkage int vprintk_emit(int facility, int level,
 	if (ft.nbcon_atomic)
 		nbcon_atomic_flush_pending();
 
+	if (ft.nbcon_offload)
+		nbcon_kthreads_wake();
+
 	if (ft.legacy_direct) {
 		/*
 		 * The caller may be holding system-critical or
@@ -2732,6 +2735,7 @@ void suspend_console(void)
 
 void resume_console(void)
 {
+	struct console_flush_type ft;
 	struct console *con;
 
 	if (!console_suspend_enabled)
@@ -2749,6 +2753,10 @@ void resume_console(void)
 	 */
 	synchronize_srcu(&console_srcu);
 
+	printk_get_console_flush_type(&ft);
+	if (ft.nbcon_offload)
+		nbcon_kthreads_wake();
+
 	pr_flush(1000, true);
 }
 
@@ -3060,6 +3068,7 @@ static inline void printk_kthreads_check_locked(void) { }
  */
 static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
 {
+	struct console_flush_type ft;
 	bool any_usable = false;
 	struct console *con;
 	bool any_progress;
@@ -3071,12 +3080,21 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 	do {
 		any_progress = false;
 
+		printk_get_console_flush_type(&ft);
+
 		cookie = console_srcu_read_lock();
 		for_each_console_srcu(con) {
 			short flags = console_srcu_read_flags(con);
 			u64 printk_seq;
 			bool progress;
 
+			/*
+			 * console_flush_all() is only for legacy consoles when
+			 * the nbcon consoles have their printer threads.
+			 */
+			if ((flags & CON_NBCON) && ft.nbcon_offload)
+				continue;
+
 			if (!console_is_usable(con, flags, !do_cond_resched))
 				continue;
 			any_usable = true;
@@ -3387,9 +3405,25 @@ EXPORT_SYMBOL(console_stop);
 
 void console_start(struct console *console)
 {
+	struct console_flush_type ft;
+	bool is_nbcon;
+
 	console_list_lock();
 	console_srcu_write_flags(console, console->flags | CON_ENABLED);
+	is_nbcon = console->flags & CON_NBCON;
 	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);
+
+	printk_get_console_flush_type(&ft);
+	if (is_nbcon && ft.nbcon_offload)
+		nbcon_kthread_wake(console);
+
 	__pr_flush(console, 1000, true);
 }
 EXPORT_SYMBOL(console_start);
@@ -4152,8 +4186,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, true))
+			if (!console_is_usable(c, flags, true) &&
+			    !console_is_usable(c, flags, false)) {
 				continue;
+			}
 
 			if (flags & CON_NBCON) {
 				printk_seq = nbcon_seq_read(c);
@@ -4629,8 +4665,13 @@ EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
  */
 void console_try_replay_all(void)
 {
+	struct console_flush_type ft;
+
+	printk_get_console_flush_type(&ft);
 	if (console_trylock()) {
 		__console_rewind_all();
+		if (ft.nbcon_offload)
+			nbcon_kthreads_wake();
 		/* Consoles are flushed as part of console_unlock(). */
 		console_unlock();
 	}
-- 
2.39.2

On Tue 2024-08-27 06:49:25, John Ogness wrote:
> Once the kthread is running and available
> (i.e. @printk_kthreads_running is set), the kthread becomes
> responsible for flushing any pending messages which are added
> in NBCON_PRIO_NORMAL context. Namely the legacy
> console_flush_all() and device_release() no longer flush the
> console. And nbcon_atomic_flush_pending() used by
> nbcon_cpu_emergency_exit() no longer flushes messages added
> after the emergency messages.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2749,6 +2753,10 @@ void resume_console(void)
>  	 */
>  	synchronize_srcu(&console_srcu);
>  
> +	printk_get_console_flush_type(&ft);
> +	if (ft.nbcon_offload)
> +		nbcon_kthreads_wake();

Who would flush the nbcon consoles in the following case?

  ft.nbcon_atomic == true && ft.legacy_direct == false

pr_flush() won't call the legacy loop in this case.

We should probably update pr_flush() to call nbcon_atomic_flush_pending()
in this case.

> +
>  	pr_flush(1000, true);
>  }
>  
> @@ -3387,9 +3405,25 @@ EXPORT_SYMBOL(console_stop);
>  
>  void console_start(struct console *console)
>  {
> +	struct console_flush_type ft;
> +	bool is_nbcon;
> +
>  	console_list_lock();
>  	console_srcu_write_flags(console, console->flags | CON_ENABLED);
> +	is_nbcon = console->flags & CON_NBCON;
>  	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);
> +
> +	printk_get_console_flush_type(&ft);
> +	if (is_nbcon && ft.nbcon_offload)
> +		nbcon_kthread_wake(console);

Same here.

>  	__pr_flush(console, 1000, true);
>  }
>  EXPORT_SYMBOL(console_start);
> @@ -4629,8 +4665,13 @@ EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>   */
>  void console_try_replay_all(void)
>  {
> +	struct console_flush_type ft;
> +
> +	printk_get_console_flush_type(&ft);
>  	if (console_trylock()) {
>  		__console_rewind_all();
> +		if (ft.nbcon_offload)
> +			nbcon_kthreads_wake();

And here. We in this case, we should likely call add here:

		if (ft.nbcon_atomic)
			nbcon_atomic_flush_pending()
			
>  		/* Consoles are flushed as part of console_unlock(). */
>  		console_unlock();
>  	}

Best Regards,
Petr

On 2024-08-27, Petr Mladek <pmladek@suse.com> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2749,6 +2753,10 @@ void resume_console(void)
>>  	 */
>>  	synchronize_srcu(&console_srcu);
>>  
>> +	printk_get_console_flush_type(&ft);
>> +	if (ft.nbcon_offload)
>> +		nbcon_kthreads_wake();
>
> Who would flush the nbcon consoles in the following case?
>
>   ft.nbcon_atomic == true && ft.legacy_direct == false
>
> pr_flush() won't call the legacy loop in this case.
>
> We should probably update pr_flush() to call nbcon_atomic_flush_pending()
> in this case.

Agreed.

>> @@ -3387,9 +3405,25 @@ EXPORT_SYMBOL(console_stop);
>>  
>>  void console_start(struct console *console)
>>  {
>> +	struct console_flush_type ft;
>> +	bool is_nbcon;
>> +
>>  	console_list_lock();
>>  	console_srcu_write_flags(console, console->flags | CON_ENABLED);
>> +	is_nbcon = console->flags & CON_NBCON;
>>  	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);
>> +
>> +	printk_get_console_flush_type(&ft);
>> +	if (is_nbcon && ft.nbcon_offload)
>> +		nbcon_kthread_wake(console);
>
> Same here.
>
>
>>  	__pr_flush(console, 1000, true);

Agreed, via adding nbcon_atomic_flush_pending() within __pr_flush().

>> @@ -4629,8 +4665,13 @@ EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>>   */
>>  void console_try_replay_all(void)
>>  {
>> +	struct console_flush_type ft;
>> +
>> +	printk_get_console_flush_type(&ft);
>>  	if (console_trylock()) {
>>  		__console_rewind_all();
>> +		if (ft.nbcon_offload)
>> +			nbcon_kthreads_wake();
>
> And here. We in this case, we should likely call add here:
>
> 		if (ft.nbcon_atomic)
> 			nbcon_atomic_flush_pending()

Agreed.

You once mentioned having a generic flush function that simply evaluates
the flush types and then executes them appropriately. It would be nice
to get rid of all/most these individual eval/flush snippets.

At a quick glance, the significant variations are:

- for legacy_direct: console_trylock() vs. console_lock() usage

- only performing direct/atomic flushing

- only performing offload flushing

- vprintk_emit(), console_start(), nbcon_device_release() are special

The first three variations could all be covered by arguments to the
generic flush function:

void console_flush(bool do_direct, bool do_offload, bool may_sleep);

or by packing it into bits/macros for readability, with call examples
such as:

console_flush(CON_FLUSH_OFFLOAD);
console_flush(CON_FLUSH_DIRECT | CON_FLUSH_OFFLOAD);
console_flush(CON_FLUSH_DIRECT | CON_FLUSH_MAY_SLEEP);

Such a function would have avoided the issues you found in this review.

John Ogness