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