Re: [PATCH 2/7] printk: Use consoles_suspended flag when suspending/resuming all consoles

[John Ogness <john.ogness@linutronix.de>]

On 2025-06-13, Petr Mladek <pmladek@suse.com> wrote:
>> 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()

Yes.

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

[...]

> 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);
> 	}

Agreed.

>    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.

Or it could become a bitmask of printing types to check:

#define ATOMIC_PRINTING 0x1
#define NONATOMIC_PRINTING 0x2

and then __pr_flush() looks like:

if (!console_is_usable(c, flags, ATOMIC_PRINTING|NONATOMIC_PRINTING)

>       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.

A bit tricky, but you are right.

>
>
> 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.

-- 
John Ogness
Linutronix GmbH | Bahnhofstraße 3 | D-88690 Uhldingen-Mühlhofen
Phone: +49 7556 25 999 20; Fax.: +49 7556 25 999 99

Hinweise zum Datenschutz finden Sie hier (Information on data privacy
can be found here): https://linutronix.de/legal/data-protection.php

Linutronix GmbH | Firmensitz (Registered Office): Uhldingen-Mühlhofen |
Registergericht (Registration Court): Amtsgericht Freiburg i.Br., HRB700
806 | Geschäftsführer (Managing Directors): Harry Demas, Heinz Egger,
Thomas Gleixner, Yin Sorrell, Jeffrey Schneiderman