Boot consoles are not serialized with the nbcon consoles via the nbcon
console context or con->device_lock(). The serialization is possible only
via the legacy console_lock().

The decision whether nbcon_atomic_flush_pending() should and can be
called safely is similar and closely related to the decision
whether the legacy loop should be used.

Define printing_via_context_safe symmetrically with printing_via_unlock.
Allow to call nbcon_atomic_flush_pending() only when it is needed and safe.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/internal.h | 10 ++++++++++
 kernel/printk/nbcon.c    |  9 +++++++--
 kernel/printk/printk.c   |  7 ++++---
 3 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 38680c6b2b39..bafec0a27da3 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -182,6 +182,7 @@ static inline bool console_is_usable(struct console *con, short flags) { return
 #endif /* CONFIG_PRINTK */
 
 extern bool have_boot_console;
+extern bool have_nbcon_console;
 extern bool have_legacy_console;
 
 /*
@@ -192,6 +193,15 @@ extern bool have_legacy_console;
  */
 #define printing_via_unlock (have_legacy_console || have_boot_console)
 
+/*
+ * Specifies if printing on nbcon consoles is needed and also safe
+ * when serialized only by the nbcon context. If @have_boot_console
+ * is true, the nbcon consoles must be serialized with the boot
+ * consoles using the legacy console_lock().
+ */
+#define printing_via_context_safe (have_nbcon_console || !have_boot_console)
+
+
 extern struct printk_buffers printk_shared_pbufs;
 
 /**
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 89b340ca303c..9b2df848718c 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1404,7 +1404,8 @@ void nbcon_cpu_emergency_exit(void)
 	 * for the emergency messages is NBCON_PRIO_EMERGENCY.
 	 */
 	if (*cpu_emergency_nesting == 1) {
-		nbcon_atomic_flush_pending();
+		if (printing_via_context_safe)
+			nbcon_atomic_flush_pending();
 
 		/*
 		 * Safely attempt to flush the legacy consoles in this
@@ -1446,7 +1447,8 @@ void nbcon_cpu_emergency_flush(void)
 	if (*(nbcon_get_cpu_emergency_nesting()) == 0)
 		return;
 
-	nbcon_atomic_flush_pending();
+	if (printing_via_context_safe)
+		nbcon_atomic_flush_pending();
 
 	if (printing_via_unlock && !is_printk_deferred()) {
 		if (console_trylock())
@@ -1637,6 +1639,9 @@ void nbcon_device_release(struct console *con)
 	 * was locked. The console_srcu_read_lock must be taken to ensure
 	 * the console is usable throughout flushing.
 	 */
+	if (!printing_via_context_safe)
+		return;
+
 	cookie = console_srcu_read_lock();
 	if (console_is_usable(con, console_srcu_read_flags(con)) &&
 	    !con->kthread &&
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 137bd9a721c4..3183db5b4180 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -475,7 +475,7 @@ bool have_legacy_console;
  * synchronous printing of legacy consoles will not occur during panic until
  * the backtrace has been stored to the ringbuffer.
  */
-static bool have_nbcon_console;
+bool have_nbcon_console;
 
 /*
  * Specifies if a boot console is registered. If boot consoles are present,
@@ -2370,7 +2370,7 @@ asmlinkage int vprintk_emit(int facility, int level,
 
 	printed_len = vprintk_store(facility, level, dev_info, fmt, args);
 
-	if (have_nbcon_console && !have_boot_console) {
+	if (printing_via_context_safe) {
 		bool is_panic_context = this_cpu_in_panic();
 
 		/*
@@ -3283,7 +3283,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
 	if (mode == CONSOLE_REPLAY_ALL)
 		__console_rewind_all();
 
-	nbcon_atomic_flush_pending();
+	if (printing_via_context_safe)
+		nbcon_atomic_flush_pending();
 
 	if (printing_via_unlock)
 		console_flush_all(false, &next_seq, &handover);
-- 
2.45.2

On 2024-06-13, Petr Mladek <pmladek@suse.com> wrote:
> Boot consoles are not serialized with the nbcon consoles via the nbcon
> console context or con->device_lock(). The serialization is possible only
> via the legacy console_lock().
>
> The decision whether nbcon_atomic_flush_pending() should and can be
> called safely is similar and closely related to the decision
> whether the legacy loop should be used.
>
> Define printing_via_context_safe symmetrically with printing_via_unlock.
> Allow to call nbcon_atomic_flush_pending() only when it is needed and safe.

This patch, along with other comments you made about the many different
checks to see when it is allowed to do what, forced me to take a step
back and look at the big picture. Indeed, by the end of this series we
have many boolean variables that influence decisions about how to handle
threads and how to print.

I am thinking it makes more sense to incorporate these booleans into a
single variable (printk_state?). The variable would only be changed
under the console_list_lock, which could allow lockless users to
leverage the console_srcu_read_lock for consistency.

The different orthogonal bits of the variable would be:


* have_boot_console - true if a boot console is registered

* have_legacy_console - true if a non-nbcon console is registered

* have_nbcon_console - true if an nbcon console is registered

* printk_threads_allowed - true when it is OK to have threads
  (set/cleared by notifiers, and more or less represents
  "system_state == SYSTEM_SCHEDULING || system_state == SYSTEM_RUNNING")

* printk_threads_available - true while all printing threads are running
  (and implies at least 1 thread is running)


We could provide macros to interpret this multi-flag value for various
scenarios:

#define nbcon_may_create_threads()
  (printk_threads_allowed)

#define nbcon_may_rely_on_threads()
  (have_nbcon_console && !have_boot_console && printk_threads_available)

#define nbcon_may_flush_atomic()
  (have_nbcon_console && !have_boot_console)

#define nbcon_must_flush_atomic()
  (have_nbcon_console && !have_boot_console && !printk_threads_available)

#define nbcon_must_flush_via_unlock()
  (have_nbcon_console && have_boot_console)

#define printing_via_unlock()
  (have_legacy_console || have_boot_console)

Of course, the exact naming of these macros gives us a big shed to
paint. But I think this could help to simplify/unify the checks for the
various scenarios.

John Ogness

On Tue 2024-06-25 22:59:34, John Ogness wrote:
> On 2024-06-13, Petr Mladek <pmladek@suse.com> wrote:
> > Boot consoles are not serialized with the nbcon consoles via the nbcon
> > console context or con->device_lock(). The serialization is possible only
> > via the legacy console_lock().
> >
> > The decision whether nbcon_atomic_flush_pending() should and can be
> > called safely is similar and closely related to the decision
> > whether the legacy loop should be used.
> >
> > Define printing_via_context_safe symmetrically with printing_via_unlock.
> > Allow to call nbcon_atomic_flush_pending() only when it is needed and safe.
> 
> This patch, along with other comments you made about the many different
> checks to see when it is allowed to do what, forced me to take a step
> back and look at the big picture. Indeed, by the end of this series we
> have many boolean variables that influence decisions about how to handle
> threads and how to print.
> 
> I am thinking it makes more sense to incorporate these booleans into a
> single variable (printk_state?). The variable would only be changed
> under the console_list_lock, which could allow lockless users to
> leverage the console_srcu_read_lock for consistency.
> 
> The different orthogonal bits of the variable would be:
> 
> 
> * have_boot_console - true if a boot console is registered
> 
> * have_legacy_console - true if a non-nbcon console is registered
> 
> * have_nbcon_console - true if an nbcon console is registered
> 
> * printk_threads_allowed - true when it is OK to have threads
>   (set/cleared by notifiers, and more or less represents
>   "system_state == SYSTEM_SCHEDULING || system_state == SYSTEM_RUNNING")
> 
> * printk_threads_available - true while all printing threads are running
>   (and implies at least 1 thread is running)

I would call the variable "printk_threads_running". It seems to have
more clear meaning.

12th patch adds:

  * force_printkthread - which triggers offloading even the legacy
	into a thread. Or does it force using kthreads even in
	emergency?


> We could provide macros to interpret this multi-flag value for various
> scenarios:
> 
> #define nbcon_may_create_threads()
>   (printk_threads_allowed)

The force_printkthread involves also legacy consoles. As a result,
the "nbcon_" prefix does not look important.

> #define nbcon_may_rely_on_threads()
>   (have_nbcon_console && !have_boot_console && printk_threads_available)
> 
> #define nbcon_may_flush_atomic()
>   (have_nbcon_console && !have_boot_console)
> 
> #define nbcon_must_flush_atomic()
>   (have_nbcon_console && !have_boot_console && !printk_threads_available)
> 
> #define nbcon_must_flush_via_unlock()
>   (have_nbcon_console && have_boot_console)
> 
> #define printing_via_unlock()
>   (have_legacy_console || have_boot_console)

I like this "may" vs "must" variants.

Good names and good macros might help, definitely. But I think that
there are more problems. There are also several variants of functions
flushing the consoles. Are they are orthogonal as well:

One aspect are the console callbacks:

    + write()		# legacy consoles
    + write_atomic()    # nbcon consoles in atomic or unknown context
    + write_thread      # nbcon consoles in task context

Another aspect is the serialization:

    + console_lock()			  # legacy loop
    + con->driver_lock() + nbcon_context  # nbcon consoles in normal context
    + nbcon_context			  # nbcon consoles in emergency
					  # or panic context
Caller:

    + printk()
    + explicit flush()

Context priority:

    + normal
    + emergency
    + panic


I still have to think if we could somehow improve the situation,
for example, by using some systematic names.

Well, the conditions are more or less straightforward in most
situations with three exceptions:

   + vprintk_emit()
   + nbcon_cpu_emergency_exit()
   + nbcon_cpu_emergency_flush()

, where:

  +  The rules for the direct flush are the same in both
     nbcon_cpu_emergency_exit() and nbcon_cpu_emergency_flush().

     But only nbcon_cpu_emergency_exit() calls the trigger-offload
     part.


  +  The rules in vprintk_emit() are much more complicated
     by the special handling in:

       + NBCON_PRIO_PANIC and legacy_allow_panic_sync
       + NBCON_PRIO_EMERGENCY

IMHO, it would help to:

  + avoid code duplication in nbcon_cpu_emergency_exit()/flush()

  + make the code in vprintk_emit() as compact as possible,
    especially avoid updating "do_trylock_unlock" all over

  + make the code in vprintk_emit as similar to
    nbcon_cpu_emergency_exit()/flush() as possible.


I have tried various variants and it always became complicated.
So I decided to make the logic as compact as possible in the
following two functions:


/**
 * struct console_flush_type - Define how to flush the consoles.
 * @nbcon_atomic: Flush directly using nbcon_atomic() callback
 * @nbcon_thread: Offload the flush to the kthread
 * @legacy_direct: Call the legacy loop in this context
 * @legacy_offload: Offload the legacy loop into IRQ or kthread
 */
struct console_flush_type {
	bool	nbcon_atomic;
	bool	nbcon_thread;
	bool	legacy_direct;
	bool	legacy_offload;
};

/*
 * Decide how the messages should get flushed from printk().
 * It is supposed to be called in vprintk_emit()
 *
 * Variant per console type
 */
void printk_set_console_flush_type(struct console_flush *flush)
{
	enum nbcon_prio nbcon_prio;

	memset(flush, 0, sizeof(*flush));

	/*
	 * nbcon_get_default_prio() can be read safely even in premptible
	 * context. NBCON_PRIO_PANIC is used only on panic-CPU.
	 * NBCON_PRIO_EMERGENCY is set only in context with CPU migragtion
	 * disabled.
	 */
	nbcon_prio = nbcon_get_default_prio();

	/*
	 * Skip it in EMERGENCY priority. The console will be
	 * explicitly flushed when exiting the emergency section.
	 */
	 if (nbcon_prio == NBCON_PRIO_EMERGENCY)
			return;

	/* How to flush nbcon consoles without legacy loop. */
	if (have_nbcon_console && !have_boot_console) {
		if (nbcon_prio == NBCON_PRIO_PANIC) {
			flush->nbcon_atomic = true;

			/* In panic, the legacy consoles are not allowed
			 * to print from the printk calling context unless
			 * explicitly allowed. This gives the safe nbcon
			 * consoles a chance to print out all the panic
			 * messages first. This restriction only applies
			 * if there are nbcon consoles registered.
			 */
			if (!legacy_allow_panic_sync)
				return;

		/* Only NBCON_PRIO_NORMAL left. */
		} else if (nbcon_kthreads_running) {
			flush->nbcon_thread = true;
		} else {
			flush->nbcon_atomic = true;
		}
	}

	/* How to do the legacy loop. */
	if (have_legacy_console || have_boot_console) {
		if (nbcon_prio == NBCON_PRIO_PANIC &&
		    legacy_allow_panic_sync) {
			flush->nbcon_direct = true;

		/* Only NBCON_PRIO_NORMAL left. */
		} if (is_printk_deferred() || console_legacy_thread)) {
			flush->legacy_thread = true;
		} else {
			flush->legacy_direct = true;
		}
	}
}

/*
 * Decide how the messages should get flushed from printk().
 * It is supposed to be called in vprintk_emit()
 *
 * Variant per nbcon prio
 */
void printk_set_console_flush_type(struct console_flush *flush)
{
	enum nbcon_prio nbcon_prio;

	memset(flush, 0, sizeof(*flush));

	/*
	 * nbcon_get_default_prio() can be read safely even in premptible
	 * context. NBCON_PRIO_PANIC is used only on panic-CPU.
	 * NBCON_PRIO_EMERGENCY is set only in context with CPU migragtion
	 * disabled.
	 */
	nbcon_prio = nbcon_get_default_prio();

	switch(nbcon_prio):
	NBCON_PRIO_NORMAL:
		if (have_nbcon_console && !have_boot_console) {
			if (nbcon_kthreads_running)
				flush->nbcon_thread = true;
			else
				flush->nbcon_atomic = true;
		}

		if (have_legacy_console || have_boot_console ) {
			if (is_printk_deferred() || console_legacy_thread))
				flush->legacy_thread = true;
			else
				flush->legacy_direct = true;
		}
		break;

	NBCON_PRIO_EMERGENCY:
		/*
		 * Skip it in EMERGENCY priority. The console will be
		 * explicitly flushed when exiting the emergency section.
		 */
		 break;

	NBCON_PRIO_PANIC:
		if (have_nbcon_console && !have_boot_console) {
			flush->nbcon_atomic = true;

			/* In panic, the legacy consoles are not allowed
			 * to print from the printk calling context unless
			 * explicitly allowed. This gives the safe nbcon
			 * consoles a chance to print out all the panic
			 * messages first. This restriction only applies
			 * if there are nbcon consoles registered.
			 */
			if (!legacy_allow_panic_sync)
				break;
		}

		if ((have_legacy_console || have_boot_console) &&
			flush->nbcon_direct = true;

		break;
	default:
		WARN_ON_ONCE(1, "This should never happen\n");
	}
}

/*
 * Decide how the messages should get flushed from emergency context.
 * This is called when we really want to flush the emergency messages.
 *
 * FIXME: Emergency messages should always get flushed directly, except
 *        when it is not safe.
 */
void get_console_emergency_flush_type(struct console_flush_type *flush)
{
	enum nbcon_prio nbcon_prio = nbcon_get_default_prio();

	memset(flush, 0, sizeof(*flush));

	WARN_ON_ONCE(nbcon_prio != NBCON_PRIO_EMERGENCY);

	if (have_nbcon_console && !have_boot_console)
		flush->nbcon_atomic = true;


	if (have_legacy_console || have_boot_console) {
		/* FIXME: add force_legacy_kthread? */
		if (is_printk_deferred())
			flush->legacy_thread = true;
		} else {
			flush->legacy_direct = true;
		}
	}
}

The first function might be used in vprintk_emit as:

asmlinkage int vprintk_emit(int facility, int level,
			    const struct dev_printk_info *dev_info,
			    const char *fmt, va_list args)
{
	struct console_flush_type flush;
	bool do_trylock_unlock = !force_printkthreads() &&
				 printing_via_unlock;
	int printed_len;

	/* Suppress unimportant messages after panic happens */
	if (unlikely(suppress_printk))
		return 0;

	/*
	 * The messages on the panic CPU are the most important. If
	 * non-panic CPUs are generating any messages, they will be
	 * silently dropped.
	 */
	if (other_cpu_in_panic())
		return 0;

	if (level == LOGLEVEL_SCHED) {
		level = LOGLEVEL_DEFAULT;
		/* If called from the scheduler, we can not call up(). */
		do_trylock_unlock = false;
	}

	printk_delay(level);

	printed_len = vprintk_store(facility, level, dev_info, fmt, args);

	printk_set_console_flush_type(&flush);

	if (flush.nbcon_direct)
		nbcon_atomic_flush_pending();

	if (flush.nbcon_thread)
		nbcon_wake_threads();

	if (flush.legacy_direct) {
		/*
		 * The caller may be holding system-critical or
		 * timing-sensitive locks. Disable preemption during
		 * printing of all remaining records to all consoles so that
		 * this context can return as soon as possible. Hopefully
		 * another printk() caller will take over the printing.
		 *
		 * Also, nbcon_get_default_prio() requires migration disabled.
		 */
		preempt_disable();

		/*
		 * Try to acquire and then immediately release the console
		 * semaphore. The release will print out buffers. With the
		 * spinning variant, this context tries to take over the
		 * printing from another printing context.
		 */
		if (console_trylock_spinning())
			console_unlock();

		preempt_enable();
	}

	if (flush.legacy_thread)
		defer_console_output();
	else
		wake_up_klogd();

	return printed_len;
}
EXPORT_SYMBOL(vprintk_emit);


I do not say that this is the best solution.

My view:

1. I like the 2nd variant of printk_set_console_flush_type() where the
   code is primary split by nbcon_prio.

   And I like that the rather complicated rules are as compact as
   possible and quite clear for each scenario.


2. The situation is much easier in get_console_emergency_flush_type().

   Note that it never sets "nbcon_thread". Maybe, we could flush the
   consoles in this function. And just return whether it is needed
   to offload the legacy loop.


Best Regards,
Petr

On Thu 2024-06-13 14:52:24, Petr Mladek wrote:
> Boot consoles are not serialized with the nbcon consoles via the nbcon
> console context or con->device_lock(). The serialization is possible only
> via the legacy console_lock().
> 
> The decision whether nbcon_atomic_flush_pending() should and can be
> called safely is similar and closely related to the decision
> whether the legacy loop should be used.
> 
> Define printing_via_context_safe symmetrically with printing_via_unlock.
> Allow to call nbcon_atomic_flush_pending() only when it is needed and safe.
> 
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -192,6 +193,15 @@ extern bool have_legacy_console;
>   */
>  #define printing_via_unlock (have_legacy_console || have_boot_console)
>  
> +/*
> + * Specifies if printing on nbcon consoles is needed and also safe
> + * when serialized only by the nbcon context. If @have_boot_console
> + * is true, the nbcon consoles must be serialized with the boot
> + * consoles using the legacy console_lock().
> + */
> +#define printing_via_context_safe (have_nbcon_console || !have_boot_console)

Oops, this should be:

#define printing_via_context_safe (have_nbcon_console && !have_boot_console)


Best Regards,
Petr