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

[Marcos Paulo de Souza]

On Fri, 2025-06-13 at 17:20 +0200, Petr Mladek wrote:
> On Fri 2025-06-06 23:53:44, Marcos Paulo de Souza wrote:
> 
> 
> 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.
> 

I like this idea. Also, thanks a lot for explaining why the current
version won't work.

I also liked John's proposal to use a a bitmask on console_is_usable,
but I'll think a little on it once I restart working on it this week.

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

Much better, I'll adapt the code as you suggested.

> 
> 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@xxxxxxxxxxxxxxx/
>     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.

I agree, such a context would even help the reviewers that would spend
some time reading the code and thinking themselves that some code is
being readded for some reason.