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

Marcos Paulo de Souza
2025-06-23T18:53:22Z

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

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.