Message-ID: <aExBo-8cVOy6GegR@pathway.suse.cz>
Date: Fri, 13 Jun 2025 17:20:03 +0200
From: Petr Mladek <pmladek@...e.com>
To: Marcos Paulo de Souza <mpdesouza@...e.com>
Cc: Steven Rostedt <rostedt@...dmis.org>,
	John Ogness <john.ogness@...utronix.de>,
	Sergey Senozhatsky <senozhatsky@...omium.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Jiri Slaby <jirislaby@...nel.org>,
	Jason Wessel <jason.wessel@...driver.com>,
	Daniel Thompson <danielt@...nel.org>,
	Douglas Anderson <dianders@...omium.org>,
	Richard Weinberger <richard@....at>,
	Anton Ivanov <anton.ivanov@...bridgegreys.com>,
	Johannes Berg <johannes@...solutions.net>,
	linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org,
	kgdb-bugreport@...ts.sourceforge.net, linux-um@...ts.infradead.org
Subject: Re: [PATCH 2/7] printk: Use consoles_suspended flag when
 suspending/resuming all consoles

On Fri 2025-06-06 23:53:44, Marcos Paulo de Souza wrote:
> Instead of update a per-console CON_SUSPENDED flag, use the console_list
> locks to protect this flag. This is also applied to console_is_usable
> functions, which now also checks if consoles_suspend is set.
> 
> Signed-off-by: Marcos Paulo de Souza <mpdesouza@...e.com>
> ---
>  kernel/printk/internal.h |  7 ++++++-
>  kernel/printk/nbcon.c    |  8 ++++----
>  kernel/printk/printk.c   | 23 ++++++++++-------------
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index 48a24e7b309db20fdd7419f7aeda68ea7c79fd80..752101904f44b13059b6a922519d88e24c9f32c0 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -118,8 +118,12 @@ void nbcon_kthreads_wake(void);
>   * which can also play a role in deciding if @con can be used to print
>   * records.
>   */
> -static inline bool console_is_usable(struct console *con, short flags, bool use_atomic)
> +static inline bool console_is_usable(struct console *con, short flags,
> +				     bool use_atomic, bool consoles_suspended)
>  {
> +	if (consoles_suspended)
> +		return false;
> +
>  	if (!(flags & CON_ENABLED))
>  		return false;
>  
> @@ -212,6 +216,7 @@ extern bool have_boot_console;
>  extern bool have_nbcon_console;
>  extern bool have_legacy_console;
>  extern bool legacy_allow_panic_sync;
> +extern bool consoles_suspended;
>  
>  /**
>   * struct console_flush_type - Define available console flush methods
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index fd12efcc4aeda8883773d9807bc215f6e5cdf71a..72de12396e6f1bc5234acfdf6dcc393acf88d216 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -1147,7 +1147,7 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex
>  	cookie = console_srcu_read_lock();
>  
>  	flags = console_srcu_read_flags(con);
> -	if (console_is_usable(con, flags, false)) {
> +	if (console_is_usable(con, flags, false, consoles_suspended)) {

The new global console_suspended value has the be synchronized the
same way as the current CON_SUSPENDED per-console flag.
It means that the value must be:

  + updated only under console_list_lock together with
    synchronize_rcu().

  + read using READ_ONCE() under console_srcu_read_lock()


I am going to propose more solutions because no one is obviously
the best one.

Variant A:
=========

Create a helper functions, similar to
console_srcu_read_flags() and console_srcu_write_flags():

Something like:

static inline bool console_srcu_read_consoles_suspended()
{
	WARN_ON_ONCE(!console_srcu_read_lock_is_held());

	/*
	 * The READ_ONCE() matches the WRITE_ONCE() when the value
	 * is modified console_srcu_write_consoles_suspended().
	 */
	return data_race(READ_ONCE(consoles_suspended));
}

static inline void console_srcu_write_consoles_suspended(bool suspended)
{
	lockdep_assert_console_list_lock_held();

	/* This matches the READ_ONCE() in console_srcu_read_consoles_suspended(). */
	WRITE_ONCE(consoles_suspended, suspended);
}

This has the drawback that most console_is_usable() callers would need
to get and pass both variables, for example:

--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1137,6 +1137,7 @@ static bool nbcon_emit_one(struct nbcon_write_context *wctxt, bool use_atomic)
  */
 static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_context *ctxt)
 {
+	bool cons_suspended;
 	bool ret = false;
 	short flags;
 	int cookie;
@@ -1147,7 +1148,8 @@ static bool nbcon_kthread_should_wakeup(struct console *con, struct nbcon_contex
 	cookie = console_srcu_read_lock();
 
 	flags = console_srcu_read_flags(con);
-	if (console_is_usable(con, flags, false)) {
+	cons_suspended = console_srcu_read_consoles_suspended();
+	if (console_is_usable(con, flags, false, cons_suspended)) {
 		/* Bring the sequence in @ctxt up to date */
 		ctxt->seq = nbcon_seq_read(con);

Pros:

   + always correct

Cons:

   + not user friendly



Variant B:
==========

Do not pass @consoles_suspended as a parameter. Instead, read it
in console_us_usable() directly.

I do not like this because it is not consistent with the con->flags
handling and it is not clear why.


Variant C:
==========

Remove even @flags parameter from console_is_usable() and read both
values there directly.

Many callers read @flags only because they call console_is_usable().
The change would simplify the code.

But there are few exceptions:

  1. __nbcon_atomic_flush_pending(), console_flush_all(),
     and legacy_kthread_should_wakeup() pass @flags to
     console_is_usable() and also check CON_NBCON flag.

     But CON_NBCON flag is special. It is statically initialized
     and never set/cleared at runtime. It can be checked without
     READ_ONCE(). Well, we still might want to be sure that
     the struct console can't disappear.

     IMHO, this can be solved by a helper function:

	/**
	 * console_srcu_is_nbcon - Locklessly check whether the console is nbcon
	 * @con:	struct console pointer of console to check
	 *
	 * Requires console_srcu_read_lock to be held, which implies that @con might
	 * be a registered console. The purpose of holding console_srcu_read_lock is
	 * to guarantee that no exit/cleanup routines will run if the console
	 * is currently undergoing unregistration.
	 *
	 * If the caller is holding the console_list_lock or it is _certain_ that
	 * @con is not and will not become registered, the caller may read
	 * @con->flags directly instead.
	 *
	 * Context: Any context.
	 * Return: True when CON_NBCON flag is set.
	 */
	static inline bool console_is_nbcon(const struct console *con)
	{
		WARN_ON_ONCE(!console_srcu_read_lock_is_held());

		/*
		 * The CON_NBCON flag is statically initialized and is never
		 * set or cleared at runtime.
		return data_race(con->flags & CON_NBCON);
	}


   2. Another exception is __pr_flush() where console_is_usable() is
      called twice with @use_atomic set "true" and "false".

      We would want to read "con->flags" only once here. A solution
      would be to add a parameter to check both con->write_atomic
      and con->write_thread in a single call.

      But it might actually be enough to check is with the "false"
      value because "con->write_thread()" is mandatory for nbcon
      consoles. And legacy consoles do not distinguish atomic mode.


Variant D:
==========

We need to distinguish the global and per-console "suspended" flag
because they might be nested. But we could use a separate flag
for the global setting.

I mean that:

    + console_suspend() would set CON_SUSPENDED flag
    + console_suspend_all() would set CON_SUSPENDED_ALL flag

They both will be in con->flags.

Pros:

    + It is easy to implement.

Cons:

    + It feels a bit ugly.


My opinion:
===========

I personally prefer the variant C because:

  + Removes one parameter from console_is_usable().

  + The lockless synchronization of both global and per-console
    flags is hidden in console_is_usable().

  + The global console_suspended flag will be stored in global
    variable (in compare with variant D).

What do you think, please?

Best Regards,
Petr


PS: The commit message and the cover letter should better explain
    the background of this change.

    It would be great if the cover letter described the bigger
    picture, especially the history of the console_suspended,
    CON_SUSPENDED, and CON_ENABLED flags. It might use info
    from
    https://lore.kernel.org/lkml/ZyoNZfLT6tlVAWjO@pathway.suse.cz/
    and maybe even this link.

    Also this commit message should mention that it partly reverts
    the commit 9e70a5e109a4a233678 ("printk: Add per-console
    suspended state"). But it is not simple revert because
    we need to preserve the synchronization using
    the console_list_lock for writing and SRCU for reading.

Powered by blists - more mailing lists