On 2025-06-13, Petr Mladek <pmla...@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/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.
--
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
