On Fri 2025-06-06 23:53:44, Marcos Paulo de Souza wrote:
> Instead of update a per-console CON_SUSPENDED flag, use the console_list
> locks to protect this flag. This is also applied to console_is_usable
> functions, which now also checks if consoles_suspend is set.
>
> Signed-off-by: Marcos Paulo de Souza <mpdeso...@suse.com>
> ---
> kernel/printk/internal.h | 7 ++++++-
> kernel/printk/nbcon.c | 8 ++++----
> kernel/printk/printk.c | 23 ++++++++++-------------
> 3 files changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
> index
> 48a24e7b309db20fdd7419f7aeda68ea7c79fd80..752101904f44b13059b6a922519d88e24c9f32c0
> 100644
> --- a/kernel/printk/internal.h
> +++ b/kernel/printk/internal.h
> @@ -118,8 +118,12 @@ void nbcon_kthreads_wake(void);
> * which can also play a role in deciding if @con can be used to print
> * records.
> */
> -static inline bool console_is_usable(struct console *con, short flags, bool
> use_atomic)
> +static inline bool console_is_usable(struct console *con, short flags,
> + bool use_atomic, bool consoles_suspended)
> {
> + if (consoles_suspended)
> + return false;
> +
> if (!(flags & CON_ENABLED))
> return false;
>
> @@ -212,6 +216,7 @@ extern bool have_boot_console;
> extern bool have_nbcon_console;
> extern bool have_legacy_console;
> extern bool legacy_allow_panic_sync;
> +extern bool consoles_suspended;
>
> /**
> * struct console_flush_type - Define available console flush methods
> 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()
I am going to propose more solutions because no one is obviously
the best one.
Variant A:
=========
Create a helper functions, similar to
console_srcu_read_flags() and console_srcu_write_flags():
Something like:
static inline bool console_srcu_read_consoles_suspended()
{
WARN_ON_ONCE(!console_srcu_read_lock_is_held());
/*
* The READ_ONCE() matches the WRITE_ONCE() when the value
* is modified console_srcu_write_consoles_suspended().
*/
return data_race(READ_ONCE(consoles_suspended));
}
static inline void console_srcu_write_consoles_suspended(bool suspended)
{
lockdep_assert_console_list_lock_held();
/* This matches the READ_ONCE() in
console_srcu_read_consoles_suspended(). */
WRITE_ONCE(consoles_suspended, suspended);
}
This has the drawback that most console_is_usable() callers would need
to get and pass both variables, for example:
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -1137,6 +1137,7 @@ static bool nbcon_emit_one(struct nbcon_write_context
*wctxt, bool use_atomic)
*/
static bool nbcon_kthread_should_wakeup(struct console *con, struct
nbcon_context *ctxt)
{
+ bool cons_suspended;
bool ret = false;
short flags;
int cookie;
@@ -1147,7 +1148,8 @@ 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)) {
+ cons_suspended = console_srcu_read_consoles_suspended();
+ if (console_is_usable(con, flags, false, cons_suspended)) {
/* Bring the sequence in @ctxt up to date */
ctxt->seq = nbcon_seq_read(con);
Pros:
+ always correct
Cons:
+ not user friendly
Variant B:
==========
Do not pass @consoles_suspended as a parameter. Instead, read it
in console_us_usable() directly.
I do not like this because it is not consistent with the con->flags
handling and it is not clear why.
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.
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.
