From: Thomas Gleixner <[email protected]>
The current console/printk subsystem is protected by a Big Kernel Lock,
(aka console_lock) which has ill defined semantics and is more or less
stateless. This puts severe limitations on the console subsystem and
makes forced takeover and output in emergency and panic situations a
fragile endeavour that is based on try and pray.
The goal of non-BKL (nbcon) consoles is to break out of the console lock
jail and to provide a new infrastructure that avoids the pitfalls and
allows console drivers to be gradually converted over.
The proposed infrastructure aims for the following properties:
- Per console locking instead of global locking
- Per console state that allows to make informed decisions
- Stateful handover and takeover
As a first step, state is added to struct console. The per console state
is an atomic_t using a 32bit bit field.
Reserve state bits, which will be populated later in the series. Wire
it up into the console register/unregister functionality and exclude
such consoles from being handled in the legacy console mechanisms. Since
the nbcon consoles will not depend on the console lock/unlock dance
for printing, only perform said dance if a legacy console is registered.
The decision to use a bitfield was made as using a plain u32 with
mask/shift operations turned out to result in uncomprehensible code.
Note that nbcon consoles are not able to print simultaneously with boot
consoles because it is not possible to know if they are using the same
hardware. For this reason, nbcon consoles are handled as legacy consoles
as long as a boot console is registered.
Co-developed-by: John Ogness <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Signed-off-by: Thomas Gleixner (Intel) <[email protected]>
---
include/linux/console.h | 31 ++++++++++
kernel/printk/Makefile | 2 +-
kernel/printk/internal.h | 11 ++++
kernel/printk/printk.c | 112 +++++++++++++++++++++++++++++++----
kernel/printk/printk_nbcon.c | 74 +++++++++++++++++++++++
5 files changed, 216 insertions(+), 14 deletions(-)
create mode 100644 kernel/printk/printk_nbcon.c
diff --git a/include/linux/console.h b/include/linux/console.h
index 7de11c763eb3..c99265d82b98 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -156,6 +156,8 @@ static inline int con_debug_leave(void)
* /dev/kmesg which requires a larger output buffer.
* @CON_SUSPENDED: Indicates if a console is suspended. If true, the
* printing callbacks must not be called.
+ * @CON_NBCON: Console can operate outside of the legacy style console_lock
+ * constraints.
*/
enum cons_flags {
CON_PRINTBUFFER = BIT(0),
@@ -166,8 +168,32 @@ enum cons_flags {
CON_BRL = BIT(5),
CON_EXTENDED = BIT(6),
CON_SUSPENDED = BIT(7),
+ CON_NBCON = BIT(8),
};
+/**
+ * struct nbcon_state - console state for nbcon consoles
+ * @atom: Compound of the state fields for atomic operations
+ *
+ * To be used for reading and preparing of the value stored in the nbcon
+ * state variable @console.nbcon_state.
+ */
+struct nbcon_state {
+ union {
+ unsigned int atom;
+ struct {
+ };
+ };
+};
+
+/*
+ * The nbcon_state struct is used to easily create and interpret values that
+ * are stored in the console.nbcon_state variable. Make sure this struct stays
+ * within the size boundaries of that atomic variable's underlying type in
+ * order to avoid any accidental truncation.
+ */
+static_assert(sizeof(struct nbcon_state) <= sizeof(int));
+
/**
* struct console - The console descriptor structure
* @name: The name of the console driver
@@ -187,6 +213,8 @@ enum cons_flags {
* @dropped: Number of unreported dropped ringbuffer records
* @data: Driver private data
* @node: hlist node for the console list
+ *
+ * @nbcon_state: State for nbcon consoles
*/
struct console {
char name[16];
@@ -206,6 +234,9 @@ struct console {
unsigned long dropped;
void *data;
struct hlist_node node;
+
+ /* nbcon console specific members */
+ atomic_t __private nbcon_state;
};
#ifdef CONFIG_LOCKDEP
diff --git a/kernel/printk/Makefile b/kernel/printk/Makefile
index f5b388e810b9..552525edf562 100644
--- a/kernel/printk/Makefile
+++ b/kernel/printk/Makefile
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-y = printk.o
-obj-$(CONFIG_PRINTK) += printk_safe.o
+obj-$(CONFIG_PRINTK) += printk_safe.o printk_nbcon.o
obj-$(CONFIG_A11Y_BRAILLE_CONSOLE) += braille.o
obj-$(CONFIG_PRINTK_INDEX) += index.o
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 7d4979d5c3ce..655810f2976e 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -3,6 +3,7 @@
* internal.h - printk internal definitions
*/
#include <linux/percpu.h>
+#include <linux/console.h>
#if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
void __init printk_sysctl_init(void);
@@ -35,6 +36,9 @@ enum printk_info_flags {
LOG_CONT = 8, /* text is a fragment of a continuation line */
};
+extern bool have_legacy_console;
+extern bool have_boot_console;
+
__printf(4, 0)
int vprintk_store(int facility, int level,
const struct dev_printk_info *dev_info,
@@ -61,6 +65,10 @@ void defer_console_output(void);
u16 printk_parse_prefix(const char *text, int *level,
enum printk_info_flags *flags);
+
+bool nbcon_init(struct console *con);
+void nbcon_cleanup(struct console *con);
+
#else
#define PRINTK_PREFIX_MAX 0
@@ -76,6 +84,9 @@ u16 printk_parse_prefix(const char *text, int *level,
#define printk_safe_exit_irqrestore(flags) local_irq_restore(flags)
static inline bool printk_percpu_data_ready(void) { return false; }
+static inline bool nbcon_init(struct console *con) { return true; }
+static inline void nbcon_cleanup(struct console *con) { }
+
#endif /* CONFIG_PRINTK */
/**
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8787d3a72114..98b4854c81ea 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -442,6 +442,26 @@ static int console_msg_format = MSG_FORMAT_DEFAULT;
/* syslog_lock protects syslog_* variables and write access to clear_seq. */
static DEFINE_MUTEX(syslog_lock);
+/*
+ * Specifies if a legacy console is registered. See serialized_printing
+ * for details.
+ */
+bool have_legacy_console;
+
+/*
+ * Specifies if a boot console is registered. See serialized_printing
+ * for details.
+ */
+bool have_boot_console;
+
+/*
+ * Specifies if the console lock/unlock dance is needed for console
+ * printing. If @have_boot_console is true, the nbcon consoles will
+ * be printed serially along with the legacy consoles because nbcon
+ * consoles cannot print simultaneously with boot consoles.
+ */
+#define serialized_printing (have_legacy_console || have_boot_console)
+
#ifdef CONFIG_PRINTK
DECLARE_WAIT_QUEUE_HEAD(log_wait);
/* All 3 protected by @syslog_lock. */
@@ -2286,7 +2306,7 @@ asmlinkage int vprintk_emit(int facility, int level,
printed_len = vprintk_store(facility, level, dev_info, fmt, args);
/* If called from the scheduler, we can not call up(). */
- if (!in_sched) {
+ if (!in_sched && serialized_printing) {
/*
* The caller may be holding system-critical or
* timing-sensitive locks. Disable preemption during
@@ -2603,7 +2623,7 @@ void resume_console(void)
*/
static int console_cpu_notify(unsigned int cpu)
{
- if (!cpuhp_tasks_frozen) {
+ if (!cpuhp_tasks_frozen && serialized_printing) {
/* If trylock fails, someone else is doing the printing */
if (console_trylock())
console_unlock();
@@ -2955,8 +2975,17 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
cookie = console_srcu_read_lock();
for_each_console_srcu(con) {
+ short flags = console_srcu_read_flags(con);
bool progress;
+ /*
+ * console_flush_all() is only for legacy consoles,
+ * unless a boot console is registered. See
+ * serialized_printing for details.
+ */
+ if ((flags & CON_NBCON) && !have_boot_console)
+ continue;
+
if (!console_is_usable(con))
continue;
any_usable = true;
@@ -3075,6 +3104,9 @@ void console_unblank(void)
struct console *c;
int cookie;
+ if (!serialized_printing)
+ return;
+
/*
* First check if there are any consoles implementing the unblank()
* callback. If not, there is no reason to continue and take the
@@ -3142,6 +3174,9 @@ void console_flush_on_panic(enum con_flush_mode mode)
bool handover;
u64 next_seq;
+ if (!serialized_printing)
+ return;
+
/*
* Ignore the console lock and flush out the messages. Attempting a
* trylock would not be useful because:
@@ -3324,9 +3359,10 @@ static void try_enable_default_console(struct console *newcon)
newcon->flags |= CON_CONSDEV;
}
-#define con_printk(lvl, con, fmt, ...) \
- printk(lvl pr_fmt("%sconsole [%s%d] " fmt), \
- (con->flags & CON_BOOT) ? "boot" : "", \
+#define con_printk(lvl, con, fmt, ...) \
+ printk(lvl pr_fmt("%s%sconsole [%s%d] " fmt), \
+ (con->flags & CON_NBCON) ? "" : "legacy ", \
+ (con->flags & CON_BOOT) ? "boot" : "", \
con->name, con->index, ##__VA_ARGS__)
static void console_init_seq(struct console *newcon, bool bootcon_registered)
@@ -3486,6 +3522,15 @@ void register_console(struct console *newcon)
newcon->dropped = 0;
console_init_seq(newcon, bootcon_registered);
+ if (!(newcon->flags & CON_NBCON)) {
+ have_legacy_console = true;
+ } else if (!nbcon_init(newcon)) {
+ goto unlock;
+ }
+
+ if (newcon->flags & CON_BOOT)
+ have_boot_console = true;
+
/*
* Put this console in the list - keep the
* preferred driver at the head of the list.
@@ -3538,6 +3583,9 @@ EXPORT_SYMBOL(register_console);
/* Must be called under console_list_lock(). */
static int unregister_console_locked(struct console *console)
{
+ bool is_legacy_con = !(console->flags & CON_NBCON);
+ bool is_boot_con = (console->flags & CON_BOOT);
+ struct console *c;
int res;
lockdep_assert_console_list_lock_held();
@@ -3577,11 +3625,34 @@ static int unregister_console_locked(struct console *console)
*/
synchronize_srcu(&console_srcu);
+ if (console->flags & CON_NBCON)
+ nbcon_cleanup(console);
+
console_sysfs_notify();
if (console->exit)
res = console->exit(console);
+ /*
+ * If the current console was a boot and/or legacy console, the
+ * related global flags might need to be updated.
+ */
+ if (is_boot_con || is_legacy_con) {
+ bool found_boot_con = false;
+ bool found_legacy_con = false;
+
+ for_each_console(c) {
+ if (c->flags & CON_BOOT)
+ found_boot_con = true;
+ if (!(c->flags & CON_NBCON))
+ found_legacy_con = true;
+ }
+ if (!found_boot_con)
+ have_boot_console = false;
+ if (!found_legacy_con)
+ have_legacy_console = false;
+ }
+
return res;
}
@@ -3730,6 +3801,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
struct console *c;
u64 last_diff = 0;
u64 printk_seq;
+ bool locked;
int cookie;
u64 diff;
u64 seq;
@@ -3739,13 +3811,17 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
seq = prb_next_seq(prb);
for (;;) {
+ locked = false;
diff = 0;
- /*
- * Hold the console_lock to guarantee safe access to
- * console->seq.
- */
- console_lock();
+ if (serialized_printing) {
+ /*
+ * Hold the console_lock to guarantee safe access to
+ * console->seq.
+ */
+ console_lock();
+ locked = true;
+ }
cookie = console_srcu_read_lock();
for_each_console_srcu(c) {
@@ -3758,7 +3834,12 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
*/
if (!console_is_usable(c))
continue;
- printk_seq = c->seq;
+
+ if (locked)
+ printk_seq = c->seq;
+ else
+ continue;
+
if (printk_seq < seq)
diff += seq - printk_seq;
}
@@ -3767,7 +3848,8 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
if (diff != last_diff && reset_on_progress)
remaining = timeout_ms;
- console_unlock();
+ if (locked)
+ console_unlock();
/* Note: @diff is 0 if there are no usable consoles. */
if (diff == 0 || remaining == 0)
@@ -3893,7 +3975,11 @@ void defer_console_output(void)
* New messages may have been added directly to the ringbuffer
* using vprintk_store(), so wake any waiters as well.
*/
- __wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
+ int val = PRINTK_PENDING_WAKEUP;
+
+ if (serialized_printing)
+ val |= PRINTK_PENDING_OUTPUT;
+ __wake_up_klogd(val);
}
void printk_trigger_flush(void)
diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
new file mode 100644
index 000000000000..bb379a4f6263
--- /dev/null
+++ b/kernel/printk/printk_nbcon.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright (C) 2022 Linutronix GmbH, John Ogness
+// Copyright (C) 2022 Intel, Thomas Gleixner
+
+#include <linux/kernel.h>
+#include <linux/console.h>
+#include "internal.h"
+/*
+ * Printk console printing implementation for consoles that do not depend on
+ * the legacy style console_lock mechanism.
+ */
+
+/**
+ * nbcon_state_set - Helper function to set the console state
+ * @con: Console to update
+ * @new: The new state to write
+ *
+ * Only to be used when the console is not yet or no longer visible in the
+ * system. Otherwise use nbcon_state_try_cmpxchg().
+ */
+static inline void nbcon_state_set(struct console *con, struct nbcon_state *new)
+{
+ atomic_set(&ACCESS_PRIVATE(con, nbcon_state), new->atom);
+}
+
+/**
+ * nbcon_state_read - Helper function to read the console state
+ * @con: Console to read
+ * @state: The state to store the result
+ */
+static inline void nbcon_state_read(struct console *con, struct nbcon_state *state)
+{
+ state->atom = atomic_read(&ACCESS_PRIVATE(con, nbcon_state));
+}
+
+/**
+ * nbcon_state_try_cmpxchg() - Helper function for atomic_try_cmpxchg() on console state
+ * @con: Console to update
+ * @cur: Old/expected state
+ * @new: New state
+ *
+ * Return: True on success. False on fail and @cur is updated.
+ */
+static inline bool nbcon_state_try_cmpxchg(struct console *con, struct nbcon_state *cur,
+ struct nbcon_state *new)
+{
+ return atomic_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_state), &cur->atom, new->atom);
+}
+
+/**
+ * nbcon_init - Initialize the nbcon console specific data
+ * @con: Console to initialize
+ *
+ * Return: True on success. False otherwise and the console cannot
+ * be used.
+ */
+bool nbcon_init(struct console *con)
+{
+ struct nbcon_state state = { };
+
+ nbcon_state_set(con, &state);
+ return true;
+}
+
+/**
+ * nbcon_cleanup - Cleanup the nbcon console specific data
+ * @con: Console to cleanup
+ */
+void nbcon_cleanup(struct console *con)
+{
+ struct nbcon_state state = { };
+
+ nbcon_state_set(con, &state);
+}
--
2.39.2
On Fri 2023-07-28 02:08:26, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> The current console/printk subsystem is protected by a Big Kernel Lock,
> (aka console_lock) which has ill defined semantics and is more or less
> stateless. This puts severe limitations on the console subsystem and
> makes forced takeover and output in emergency and panic situations a
> fragile endeavour that is based on try and pray.
>
> The goal of non-BKL (nbcon) consoles is to break out of the console lock
> jail and to provide a new infrastructure that avoids the pitfalls and
> allows console drivers to be gradually converted over.
>
> The proposed infrastructure aims for the following properties:
>
> - Per console locking instead of global locking
> - Per console state that allows to make informed decisions
> - Stateful handover and takeover
>
> As a first step, state is added to struct console. The per console state
> is an atomic_t using a 32bit bit field.
>
> Reserve state bits, which will be populated later in the series. Wire
> it up into the console register/unregister functionality and exclude
> such consoles from being handled in the legacy console mechanisms. Since
> the nbcon consoles will not depend on the console lock/unlock dance
> for printing, only perform said dance if a legacy console is registered.
>
> The decision to use a bitfield was made as using a plain u32 with
> mask/shift operations turned out to result in uncomprehensible code.
The is nice explanation for adding the CON_NBCON, struct nbcon_state,
nbcon_init(), nbcon_cleanup() and the API for setting nbcon_state.
> Note that nbcon consoles are not able to print simultaneously with boot
> consoles because it is not possible to know if they are using the same
> hardware. For this reason, nbcon consoles are handled as legacy consoles
> as long as a boot console is registered.
But the patch does many more "unclear" things and only some are explained
by the above paragraph.
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -442,6 +442,26 @@ static int console_msg_format = MSG_FORMAT_DEFAULT;
> /* syslog_lock protects syslog_* variables and write access to clear_seq. */
> static DEFINE_MUTEX(syslog_lock);
>
> +/*
> + * Specifies if a legacy console is registered. See serialized_printing
> + * for details.
> + */
> +bool have_legacy_console;
> +
> +/*
> + * Specifies if a boot console is registered. See serialized_printing
> + * for details.
> + */
> +bool have_boot_console;
> +
> +/*
> + * Specifies if the console lock/unlock dance is needed for console
> + * printing. If @have_boot_console is true, the nbcon consoles will
> + * be printed serially along with the legacy consoles because nbcon
> + * consoles cannot print simultaneously with boot consoles.
> + */
> +#define serialized_printing (have_legacy_console || have_boot_console)
"serialized_printing" is a bit ambiguous name. We need serialized
printing also in panic(), ...
What about?
#define have_serialized_console (have_legacy_console || have_boot_console)
Or maybe have just this one variable.
> +
> #ifdef CONFIG_PRINTK
> DECLARE_WAIT_QUEUE_HEAD(log_wait);
> /* All 3 protected by @syslog_lock. */
> @@ -2286,7 +2306,7 @@ asmlinkage int vprintk_emit(int facility, int level,
> printed_len = vprintk_store(facility, level, dev_info, fmt, args);
>
> /* If called from the scheduler, we can not call up(). */
> - if (!in_sched) {
> + if (!in_sched && serialized_printing) {
> /*
> * The caller may be holding system-critical or
> * timing-sensitive locks. Disable preemption during
> @@ -2603,7 +2623,7 @@ void resume_console(void)
> */
> static int console_cpu_notify(unsigned int cpu)
> {
> - if (!cpuhp_tasks_frozen) {
> + if (!cpuhp_tasks_frozen && serialized_printing) {
It would be worth adding a comment why this does something only when
serialized_printing is set.
My understanding is that others will be handled by the respective
kthreads which are not blocked by a hotplug of particular CPU.
> /* If trylock fails, someone else is doing the printing */
> if (console_trylock())
> console_unlock();
> @@ -2955,8 +2975,17 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>
> cookie = console_srcu_read_lock();
> for_each_console_srcu(con) {
> + short flags = console_srcu_read_flags(con);
> bool progress;
>
> + /*
> + * console_flush_all() is only for legacy consoles,
> + * unless a boot console is registered. See
> + * serialized_printing for details.
> + */
> + if ((flags & CON_NBCON) && !have_boot_console)
> + continue;
This makes sense when console_flush_all() is called from
console_unlock() but not when it is called from console_init_seq().
Well, it is fine even in console_init_seq() because it is used there
only when there are boot consoles. Except that it checks "bootcon_registered"
instead of "have_boot_console". So that it is far from clear.
I suggest to:
+ Update console_flush_all() description. Mention that it flushes
only serialized consoles
+ Add a comment into console_init_seq() about that flushing only
serialized consoles is enough. All consoles are serialized
when there is a boot console registered.
+ (Optional) Rename console_flush_all() to console_flush_all_serialized()
to make it more clear. But the updated comment might be enough.
+ (Future) Get rid of @bootcon_registered. It seems that
"have_boot_console" would be enough. Well, it should be
done in a separate patch and could be done later.
> +
> if (!console_is_usable(con))
> continue;
> any_usable = true;
> @@ -3075,6 +3104,9 @@ void console_unblank(void)
> struct console *c;
> int cookie;
>
> + if (!serialized_printing)
> + return;
This looks strange. Even nbcon might need to get unblanked.
I guess that you do this because you want to avoid taking
console_lock() in the panic() when there are not consoles
which would implement unblank().
But we actually handled this a better way in a previous patch, see
https://lore.kernel.org/r/[email protected]
So, I would remove this hunk.
> +
> /*
> * First check if there are any consoles implementing the unblank()
> * callback. If not, there is no reason to continue and take the
> @@ -3142,6 +3174,9 @@ void console_flush_on_panic(enum con_flush_mode mode)
> bool handover;
> u64 next_seq;
>
> + if (!serialized_printing)
> + return;
Honestly, this does not make much sense. console_flush_on_panic()
should try to flush all consoles. The kthreads do not work
in panic().
I guess that the motivation is the same as in console_unblank().
Note that console_lock() has already been removed, see
https://lore.kernel.org/all/[email protected]/
I would remove this hunk as well. Or it would deserve some explanation.
> +
> /*
> * Ignore the console lock and flush out the messages. Attempting a
> * trylock would not be useful because:
> @@ -3486,6 +3522,15 @@ void register_console(struct console *newcon)
> newcon->dropped = 0;
> console_init_seq(newcon, bootcon_registered);
>
> + if (!(newcon->flags & CON_NBCON)) {
> + have_legacy_console = true;
> + } else if (!nbcon_init(newcon)) {
> + goto unlock;
In case of err, we should revert the changes done above:
+ clear CONSOLE_ENABLED and CON_CONSDEV flags
+ call newcon->exit() as a counter part to newcon->setup()
> + }
> +
> + if (newcon->flags & CON_BOOT)
> + have_boot_console = true;
> +
> /*
> * Put this console in the list - keep the
> * preferred driver at the head of the list.
> @@ -3577,11 +3625,34 @@ static int unregister_console_locked(struct console *console)
> */
> synchronize_srcu(&console_srcu);
>
> + if (console->flags & CON_NBCON)
> + nbcon_cleanup(console);
> +
> console_sysfs_notify();
>
> if (console->exit)
> res = console->exit(console);
>
> + /*
> + * If the current console was a boot and/or legacy console, the
> + * related global flags might need to be updated.
> + */
> + if (is_boot_con || is_legacy_con) {
> + bool found_boot_con = false;
> + bool found_legacy_con = false;
> +
> + for_each_console(c) {
> + if (c->flags & CON_BOOT)
> + found_boot_con = true;
> + if (!(c->flags & CON_NBCON))
> + found_legacy_con = true;
> + }
> + if (!found_boot_con)
> + have_boot_console = false;
> + if (!found_legacy_con)
> + have_legacy_console = false;
> + }
Just thinking loudly:
This is a bit racy in situations where this value is checked
without the console_list_lock, e.g. in vprintk_emit().
I think that it is actually OK because they are only setting "false".
As a result, other code might try to flush the consoles the serialized
way even when not really needed.
I think that it is a race which we could ignore. The chance is
super-small that it might hit us in the future. (Famous last words :-)
> +
> return res;
> }
>
> @@ -3730,6 +3801,7 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> struct console *c;
> u64 last_diff = 0;
> u64 printk_seq;
> + bool locked;
> int cookie;
> u64 diff;
> u64 seq;
> @@ -3739,13 +3811,17 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> seq = prb_next_seq(prb);
>
> for (;;) {
> + locked = false;
> diff = 0;
>
> - /*
> - * Hold the console_lock to guarantee safe access to
> - * console->seq.
> - */
> - console_lock();
> + if (serialized_printing) {
> + /*
> + * Hold the console_lock to guarantee safe access to
> + * console->seq.
> + */
> + console_lock();
> + locked = true;
> + }
>
> cookie = console_srcu_read_lock();
> for_each_console_srcu(c) {
> @@ -3758,7 +3834,12 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> */
> if (!console_is_usable(c))
> continue;
> - printk_seq = c->seq;
> +
> + if (locked)
> + printk_seq = c->seq;
> + else
> + continue;
This is strange. It basically means that __pr_flush() is a NOP when
serialized_printing is false.
We should optimize the console_lock() handling after we implement
the path for nbcons.
In each case, we should not skip any usable console here.
> +
> if (printk_seq < seq)
> diff += seq - printk_seq;
> }
> @@ -3767,7 +3848,8 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> if (diff != last_diff && reset_on_progress)
> remaining = timeout_ms;
>
> - console_unlock();
> + if (locked)
> + console_unlock();
>
> /* Note: @diff is 0 if there are no usable consoles. */
> if (diff == 0 || remaining == 0)
> @@ -3893,7 +3975,11 @@ void defer_console_output(void)
> * New messages may have been added directly to the ringbuffer
> * using vprintk_store(), so wake any waiters as well.
> */
> - __wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
> + int val = PRINTK_PENDING_WAKEUP;
> +
> + if (serialized_printing)
> + val |= PRINTK_PENDING_OUTPUT;
> + __wake_up_klogd(val);
This would deserve an explanation why PRINTK_PENDING_WAKEUP is enough.
I know that it is because it will be done by kthreads. But I know it
only because I know the wide context, plans, ...
> }
>
> void printk_trigger_flush(void)
I would prefer if we split this patch into two:
+ 1st adding the nbcon_state-related API and logic
+ 2nd adding have_serialized_console and related stuff
The various cases where the have_{legacy,boot,serialized}_console variables are
set/used would deserve some explanation. At least, we should
mention that they will be handled by a kthread. Some hunks might be even
be better moved to a patch adding the alternative code path for
threaded/atomic consoles.
Best Regards,
Petr
PS: I am sorry that I did not comment this in v1. Everything was new
at that time. And this somehow fallen through cracks.
Hi Petr,
On 2023-07-28, Petr Mladek <[email protected]> wrote:
> The is nice explanation for adding the CON_NBCON, struct nbcon_state,
> nbcon_init(), nbcon_cleanup() and the API for setting nbcon_state.
>
>> Note that nbcon consoles are not able to print simultaneously with
>> boot consoles because it is not possible to know if they are using
>> the same hardware. For this reason, nbcon consoles are handled as
>> legacy consoles as long as a boot console is registered.
>
> But the patch does many more "unclear" things and only some are
> explained by the above paragraph.
I must admit that this first patch is tricky. I am wiring up printk.c
for nbcon consoles (consoles that will have threaded printing and their
own synchronized atomic printing), yet those pieces are not there
yet. So you end up with a lot of code paths where it seems like there
are strange NOP paths added.
However, it is important to understand that those new paths will never
be taken until actual nbcon consoles exist, which won't be until the end
of the rework. The motivation for adding the new paths now is to
demonstrate that we are not breaking any of the legacy stuff.
Would it be a better approach to fully implement nbcon consoles and then
as a final step wire it into printk.c? That is how we integrated the
ringbuffer. (Spoiler alert: At the end of the email I decide that this
is also the way I want to go for nbcon consoles.)
>> +/*
>> + * Specifies if the console lock/unlock dance is needed for console
>> + * printing. If @have_boot_console is true, the nbcon consoles will
>> + * be printed serially along with the legacy consoles because nbcon
>> + * consoles cannot print simultaneously with boot consoles.
>> + */
>> +#define serialized_printing (have_legacy_console || have_boot_console)
>
> "serialized_printing" is a bit ambiguous name. We need serialized
> printing also in panic(), ...
>
> What about?
>
> #define have_serialized_console (have_legacy_console || have_boot_console)
This macro is not about having a serialized console. The first sentence
in the comment describes it best. It is just to signal if we need to do
the console lock/unlock dance to generate console output.
Something like "need_bkl_dance" would be a better name, but it doesn't
sound very technical.
>> @@ -2603,7 +2623,7 @@ void resume_console(void)
>> */
>> static int console_cpu_notify(unsigned int cpu)
>> {
>> - if (!cpuhp_tasks_frozen) {
>> + if (!cpuhp_tasks_frozen && serialized_printing) {
>
> It would be worth adding a comment why this does something only when
> serialized_printing is set.
OK.
> My understanding is that others will be handled by the respective
> kthreads which are not blocked by a hotplug of particular CPU.
Correct. This function is only important when the bkl dance is needed
(which is also the only thing this function does).
>> @@ -2955,8 +2975,17 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
>>
>> cookie = console_srcu_read_lock();
>> for_each_console_srcu(con) {
>> + short flags = console_srcu_read_flags(con);
>> bool progress;
>>
>> + /*
>> + * console_flush_all() is only for legacy consoles,
>> + * unless a boot console is registered. See
>> + * serialized_printing for details.
>> + */
>> + if ((flags & CON_NBCON) && !have_boot_console)
>> + continue;
>
> I suggest to:
>
> + Update console_flush_all() description. Mention that it flushes
> only serialized consoles
Agreed. It is only responsible for bkl dance flushing.
> + Add a comment into console_init_seq() about that flushing only
> serialized consoles is enough. All consoles are serialized
> when there is a boot console registered.
OK.
> + (Optional) Rename console_flush_all() to console_flush_all_serialized()
> to make it more clear. But the updated comment might be enough.
I guess "serialized" is not really a good name. I'll think about
this. But I agree that it should no longer be called
console_flush_all().
> + (Future) Get rid of @bootcon_registered. It seems that
> "have_boot_console" would be enough. Well, it should be
> done in a separate patch and could be done later.
Agreed. I will add a patch for this.
>> @@ -3075,6 +3104,9 @@ void console_unblank(void)
>> struct console *c;
>> int cookie;
>>
>> + if (!serialized_printing)
>> + return;
>
> This looks strange. Even nbcon might need to get unblanked.
Honestly, I never thought that an nbcon console would implement
unblank. I suppose the unblank() callback for nbcon consoles will have
the requirement that it cannot assume it is not also within its write
callbacks.
> But we actually handled this a better way in a previous patch, see
> https://lore.kernel.org/r/[email protected]
I will change it so that nbcon consoles are also allowed to implement
unblank.
>> @@ -3142,6 +3174,9 @@ void console_flush_on_panic(enum con_flush_mode mode)
>> bool handover;
>> u64 next_seq;
>>
>> + if (!serialized_printing)
>> + return;
>
> Honestly, this does not make much sense. console_flush_on_panic()
> should try to flush all consoles. The kthreads do not work
> in panic().
The atomic console flushing will appear above this statement. But with
this first patch that infrastructure is not in place yet. Keep in mind
that after this patch @serialized_printing will _always_ be true. The
series will gradually insert additional nbcon pieces.
>> @@ -3486,6 +3522,15 @@ void register_console(struct console *newcon)
>> newcon->dropped = 0;
>> console_init_seq(newcon, bootcon_registered);
>>
>> + if (!(newcon->flags & CON_NBCON)) {
>> + have_legacy_console = true;
>> + } else if (!nbcon_init(newcon)) {
>> + goto unlock;
>
> In case of err, we should revert the changes done above:
>
> + clear CONSOLE_ENABLED and CON_CONSDEV flags
> + call newcon->exit() as a counter part to newcon->setup()
Agreed. That is a bit ugly. Perhaps I will split nbcon_init() into 2
pieces. The part that can fail (memory allocation) can happen before
@newcon is touched. And the part that will always succeed (initializing
structures and setting the sequence number) can happen here.
>> @@ -3577,11 +3625,34 @@ static int unregister_console_locked(struct console *console)
>> */
>> synchronize_srcu(&console_srcu);
>>
>> + if (console->flags & CON_NBCON)
>> + nbcon_cleanup(console);
>> +
>> console_sysfs_notify();
>>
>> if (console->exit)
>> res = console->exit(console);
>>
>> + /*
>> + * If the current console was a boot and/or legacy console, the
>> + * related global flags might need to be updated.
>> + */
>> + if (is_boot_con || is_legacy_con) {
>> + bool found_boot_con = false;
>> + bool found_legacy_con = false;
>> +
>> + for_each_console(c) {
>> + if (c->flags & CON_BOOT)
>> + found_boot_con = true;
>> + if (!(c->flags & CON_NBCON))
>> + found_legacy_con = true;
>> + }
>> + if (!found_boot_con)
>> + have_boot_console = false;
>> + if (!found_legacy_con)
>> + have_legacy_console = false;
>> + }
>
> This is a bit racy in situations where this value is checked
> without the console_list_lock, e.g. in vprintk_emit().
You are correct. I can move this above the synchronize_srcu(), then
there are no races because the variables are checked under the
console_srcu_read_lock. The kthreads won't be started until after the
synchronize_srcu(). (Although it wouldn't be an issue anyway if an nbcon
is simultaneously accessed from a console_unlock context and a kthread
context. nbcon consoles do not require any serialization.)
>> @@ -3739,13 +3811,17 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>> seq = prb_next_seq(prb);
>>
>> for (;;) {
>> + locked = false;
>> diff = 0;
>>
>> - /*
>> - * Hold the console_lock to guarantee safe access to
>> - * console->seq.
>> - */
>> - console_lock();
>> + if (serialized_printing) {
>> + /*
>> + * Hold the console_lock to guarantee safe access to
>> + * console->seq.
>> + */
>> + console_lock();
>> + locked = true;
>> + }
>>
>> cookie = console_srcu_read_lock();
>> for_each_console_srcu(c) {
>> @@ -3758,7 +3834,12 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>> */
>> if (!console_is_usable(c))
>> continue;
>> - printk_seq = c->seq;
>> +
>> + if (locked)
>> + printk_seq = c->seq;
>> + else
>> + continue;
>
> This is strange. It basically means that __pr_flush() is a NOP when
> serialized_printing is false.
But at this point in the rework @serialized_printing can never be
false. The important thing at this point is that we are not breaking the
legacy consoles. When @serialized_printing is true, everything works as
before.
>> @@ -3893,7 +3975,11 @@ void defer_console_output(void)
>> * New messages may have been added directly to the ringbuffer
>> * using vprintk_store(), so wake any waiters as well.
>> */
>> - __wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
>> + int val = PRINTK_PENDING_WAKEUP;
>> +
>> + if (serialized_printing)
>> + val |= PRINTK_PENDING_OUTPUT;
>> + __wake_up_klogd(val);
>
> This would deserve an explanation why PRINTK_PENDING_WAKEUP is enough.
>
> I know that it is because it will be done by kthreads. But I know it
> only because I know the wide context, plans, ...
Right.
> I would prefer if we split this patch into two:
>
> + 1st adding the nbcon_state-related API and logic
> + 2nd adding have_serialized_console and related stuff
>
> The various cases where the have_{legacy,boot,serialized}_console
> variables are set/used would deserve some explanation. At least, we
> should mention that they will be handled by a kthread. Some hunks
> might be even be better moved to a patch adding the alternative code
> path for threaded/atomic consoles.
Then perhaps I will remove all changes to printk.c until the end of the
rework. Only necessary minor changes due to shared code (like making
shared functions in printk.c non-static) would be made.
This has the advantage that when I do modify printk.c, I could
immediately add all explanations about the nbcon threaded and atomic
paths and they would make sense because you would see the threaded and
atomic functions being called in those paths.
Taking PREEMPT_RT as an example, the current total remaining rework
diffstat (including this series) for printk.c is:
kernel/printk/printk.c | 419 ++++++++++++++++++++++++-------
1 file changed, 329 insertions(+), 90 deletions(-)
And probably that could be split into several patches.
For v3 I will only touch printk.c to expose shared code. (But your
comments are still important because you pointed out several things that
need to be different when I do get around to modifying printk.c.)
John
On Fri 2023-07-28 22:57:58, John Ogness wrote:
> Hi Petr,
>
> On 2023-07-28, Petr Mladek <[email protected]> wrote:
> > The is nice explanation for adding the CON_NBCON, struct nbcon_state,
> > nbcon_init(), nbcon_cleanup() and the API for setting nbcon_state.
> >
> >> Note that nbcon consoles are not able to print simultaneously with
> >> boot consoles because it is not possible to know if they are using
> >> the same hardware. For this reason, nbcon consoles are handled as
> >> legacy consoles as long as a boot console is registered.
> >
> > But the patch does many more "unclear" things and only some are
> > explained by the above paragraph.
>
> I must admit that this first patch is tricky. I am wiring up printk.c
> for nbcon consoles (consoles that will have threaded printing and their
> own synchronized atomic printing), yet those pieces are not there
> yet. So you end up with a lot of code paths where it seems like there
> are strange NOP paths added.
>
> However, it is important to understand that those new paths will never
> be taken until actual nbcon consoles exist, which won't be until the end
> of the rework. The motivation for adding the new paths now is to
> demonstrate that we are not breaking any of the legacy stuff.
I know that splitting many changes into pieces is not an easy task.
And I am not sure what is a reasonable approach.
> Would it be a better approach to fully implement nbcon consoles and then
> as a final step wire it into printk.c? That is how we integrated the
> ringbuffer. (Spoiler alert: At the end of the email I decide that this
> is also the way I want to go for nbcon consoles.)
I am not sure how exactly you plan it. From my POV, it is perfectly
fine to prepare the infrastructure for nbcons. I would just add
the nbcon specific handling step by step. There is no need to add
NOP path now when there will be a real code later.
> >> +/*
> >> + * Specifies if the console lock/unlock dance is needed for console
> >> + * printing. If @have_boot_console is true, the nbcon consoles will
> >> + * be printed serially along with the legacy consoles because nbcon
> >> + * consoles cannot print simultaneously with boot consoles.
> >> + */
> >> +#define serialized_printing (have_legacy_console || have_boot_console)
> >
> > "serialized_printing" is a bit ambiguous name. We need serialized
> > printing also in panic(), ...
> >
> > What about?
> >
> > #define have_serialized_console (have_legacy_console || have_boot_console)
>
> This macro is not about having a serialized console. The first sentence
> in the comment describes it best. It is just to signal if we need to do
> the console lock/unlock dance to generate console output.
>
> Something like "need_bkl_dance" would be a better name, but it doesn't
> sound very technical.
I see.
Question: Will console_lock() serialize the early-boot handling
of non-BKL conosles? I mean the direct flush in vprintk_emit().
At lest, the v1 patch set called cons_atomic_flush() in vprintk_emit()
without taking outside console_lock().
If console_lock never serializes non-BKL consoles then I rather would define:
#define serialized_nbcon (have_nbcon && have_boot_console)
and use:
+ "have_legacy_console" when console lock/unlock dance is neded.
+ "serialize_nbcon" the non-BKL consoles need to be serialized
IMHO, it will be more clear what is going on.
But I might be wrong. Maybe, we should really introduce these
variables in the patchset which would add the corresponding
code paths for non-BKL consoles.
> >> @@ -2955,8 +2975,17 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
> >>
> >> cookie = console_srcu_read_lock();
> >> for_each_console_srcu(con) {
> >> + short flags = console_srcu_read_flags(con);
> >> bool progress;
> >>
> >> + /*
> >> + * console_flush_all() is only for legacy consoles,
> >> + * unless a boot console is registered. See
> >> + * serialized_printing for details.
> >> + */
> >> + if ((flags & CON_NBCON) && !have_boot_console)
> >> + continue;
> >
> > I suggest to:
> >
> > + Update console_flush_all() description. Mention that it flushes
> > only serialized consoles
>
> Agreed. It is only responsible for bkl dance flushing.
Will it flush only legacy consoles? Or will it flush also non-BKL
consoles during the early boot?
> > + Add a comment into console_init_seq() about that flushing only
> > serialized consoles is enough. All consoles are serialized
> > when there is a boot console registered.
>
> OK.
>
> > + (Optional) Rename console_flush_all() to console_flush_all_serialized()
> > to make it more clear. But the updated comment might be enough.
>
> I guess "serialized" is not really a good name. I'll think about
> this. But I agree that it should no longer be called
> console_flush_all().
I would call it _legacy() when it will be used only for legacy
consoles.
And somthing like _directly() when it is used for flushing
all consoles directly.
> > + (Future) Get rid of @bootcon_registered. It seems that
> > "have_boot_console" would be enough. Well, it should be
> > done in a separate patch and could be done later.
>
> Agreed. I will add a patch for this.
Great. Feel free to postpone it.
> >> @@ -3486,6 +3522,15 @@ void register_console(struct console *newcon)
> >> newcon->dropped = 0;
> >> console_init_seq(newcon, bootcon_registered);
> >>
> >> + if (!(newcon->flags & CON_NBCON)) {
> >> + have_legacy_console = true;
> >> + } else if (!nbcon_init(newcon)) {
> >> + goto unlock;
> >
> > In case of err, we should revert the changes done above:
> >
> > + clear CONSOLE_ENABLED and CON_CONSDEV flags
> > + call newcon->exit() as a counter part to newcon->setup()
>
> Agreed. That is a bit ugly. Perhaps I will split nbcon_init() into 2
> pieces. The part that can fail (memory allocation) can happen before
> @newcon is touched. And the part that will always succeed (initializing
> structures and setting the sequence number) can happen here.
Whatever looks reasonable.
> >> @@ -3577,11 +3625,34 @@ static int unregister_console_locked(struct console *console)
> >> */
> >> synchronize_srcu(&console_srcu);
> >>
> >> + if (console->flags & CON_NBCON)
> >> + nbcon_cleanup(console);
> >> +
> >> console_sysfs_notify();
> >>
> >> if (console->exit)
> >> res = console->exit(console);
> >>
> >> + /*
> >> + * If the current console was a boot and/or legacy console, the
> >> + * related global flags might need to be updated.
> >> + */
> >> + if (is_boot_con || is_legacy_con) {
> >> + bool found_boot_con = false;
> >> + bool found_legacy_con = false;
> >> +
> >> + for_each_console(c) {
> >> + if (c->flags & CON_BOOT)
> >> + found_boot_con = true;
> >> + if (!(c->flags & CON_NBCON))
> >> + found_legacy_con = true;
> >> + }
> >> + if (!found_boot_con)
> >> + have_boot_console = false;
> >> + if (!found_legacy_con)
> >> + have_legacy_console = false;
> >> + }
> >
> > This is a bit racy in situations where this value is checked
> > without the console_list_lock, e.g. in vprintk_emit().
>
> You are correct. I can move this above the synchronize_srcu(), then
> there are no races because the variables are checked under the
> console_srcu_read_lock. The kthreads won't be started until after the
> synchronize_srcu().
I would rather keep it as it is now. The current version makes sure
that no SRCU walk will see a boot console when "have_boot_console"
is already cleared.
> (Although it wouldn't be an issue anyway if an nbcon
> is simultaneously accessed from a console_unlock context and a kthread
> context. nbcon consoles do not require any serialization.)
Yup.
> >> @@ -3758,7 +3834,12 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> >> */
> >> if (!console_is_usable(c))
> >> continue;
> >> - printk_seq = c->seq;
> >> +
> >> + if (locked)
> >> + printk_seq = c->seq;
> >> + else
> >> + continue;
> >
> > This is strange. It basically means that __pr_flush() is a NOP when
> > serialized_printing is false.
>
> But at this point in the rework @serialized_printing can never be
> false. The important thing at this point is that we are not breaking the
> legacy consoles. When @serialized_printing is true, everything works as
> before.
I think that it is wrong even after adding the nbcon check. The code
looks like this at the end of this patchset:
/*
* If consoles are not usable, it cannot be expected
* that they make forward progress, so only increment
* @diff for usable consoles.
*/
if (!console_is_usable(c))
continue;
if (flags & CON_NBCON)
printk_seq = nbcon_seq_read(c);
else if (locked)
printk_seq = c->seq;
else
continue;
I guess that the "else-continue" path will never happen. But when
it happens then pr_flush() would ignore a usable console and it looks
wrong.
> >> @@ -3893,7 +3975,11 @@ void defer_console_output(void)
> >> * New messages may have been added directly to the ringbuffer
> >> * using vprintk_store(), so wake any waiters as well.
> >> */
> >> - __wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
> >> + int val = PRINTK_PENDING_WAKEUP;
> >> +
> >> + if (serialized_printing)
> >> + val |= PRINTK_PENDING_OUTPUT;
> >> + __wake_up_klogd(val);
> >
> > This would deserve an explanation why PRINTK_PENDING_WAKEUP is enough.
> >
> > I know that it is because it will be done by kthreads. But I know it
> > only because I know the wide context, plans, ...
>
> Right.
>
> > I would prefer if we split this patch into two:
> >
> > + 1st adding the nbcon_state-related API and logic
> > + 2nd adding have_serialized_console and related stuff
> >
> > The various cases where the have_{legacy,boot,serialized}_console
> > variables are set/used would deserve some explanation. At least, we
> > should mention that they will be handled by a kthread. Some hunks
> > might be even be better moved to a patch adding the alternative code
> > path for threaded/atomic consoles.
>
> Then perhaps I will remove all changes to printk.c until the end of the
> rework. Only necessary minor changes due to shared code (like making
> shared functions in printk.c non-static) would be made.
>
> This has the advantage that when I do modify printk.c, I could
> immediately add all explanations about the nbcon threaded and atomic
> paths and they would make sense because you would see the threaded and
> atomic functions being called in those paths.
It looks like a better approach. I hope that it will not add you
too much work. But it will help with the review definitely because
it won't leave these non-answered questions in the common code.
Thanks a lot for the effort.
Best Regards,
Petr
On 2023-07-31, Petr Mladek <[email protected]> wrote:
>>> #define have_serialized_console (have_legacy_console || have_boot_console)
>>
>> This macro is not about having a serialized console. The first
>> sentence in the comment describes it best. It is just to signal if we
>> need to do the console lock/unlock dance to generate console output.
>>
>> Something like "need_bkl_dance" would be a better name, but it
>> doesn't sound very technical.
>
> I see.
>
> Question: Will console_lock() serialize the early-boot handling
> of non-BKL conosles? I mean the direct flush in vprintk_emit().
Initially there will be no nbcon consoles that support CON_BOOT. This
means that there are no nbcon consoles in "early boot". The only reason
that nbcon consoles and legacy boot consoles would co-exist (aside from
the brief moment before boot consoles are unregistered) is if
keep_bootcon is used.
As long as a boot console is registered, nbcon consoles are also bound
to console_lock() serialization. We have no choice until we can safely
link boot consoles to regular consoles.
I think this will be ok for the first release. The 8250 will not become
less reliable in early boot. And once the boot console is unregistered,
the 8250 nbcon console will be able to fly.
> At lest, the v1 patch set called cons_atomic_flush() in vprintk_emit()
> without taking outside console_lock().
Yes. But in the v1 patch set, console_is_usable() returns false for
nbcon consoles if there is a boot console registered. So the
cons_atomic_flush() in vprintk_emit() would not end up printing
anything.
As per your v1 feedback, that check will no longer be in
console_is_usable(), but instead will be further out in higher level
code.
> If console_lock never serializes non-BKL consoles then I rather would define:
>
> #define serialized_nbcon (have_nbcon && have_boot_console)
> and use:
>
> + "have_legacy_console" when console lock/unlock dance is neded.
>
> + "serialize_nbcon" the non-BKL consoles need to be serialized
>
> IMHO, it will be more clear what is going on.
We have 3 scenarios that I would like to easily identify using global
variable(s).
1. There are only nbcon consoles. The console lock never needs to be
taken.
2. There are nbcon consoles and regular legacy consoles. The console
lock must be taken to serialize only the regular legacy consoles. There
are separate code paths (without the console lock) that will take care
of nbcon atomic printing and nbcon threaded printing.
3. There are nbcon consoles and boot consoles. The console lock must be
taken to serialize all consoles.
Perhaps rather than using 2 booleans and a macro, we just use a single
atomic_t that describes the console serialization mode? The effect is
the same, but maybe it makes the intention of the code a bit easier to
understand?
SERMOD_BOOTCON = 0,
SERMOD_WITH_LEGACY = 1,
SERMOD_ONLY_NBCON = 2,
Or maybe describe the modes based on their behavior rather than their
condition:
SERMOD_ONLY_CONSOLE_LOCK = 0,
SERMOD_ALSO_CONSOLE_LOCK = 1,
SERMOD_NO_CONSOLE_LOCK = 2,
>>> + Update console_flush_all() description. Mention that it flushes
>>> only serialized consoles
>>
>> Agreed. It is only responsible for bkl dance flushing.
>
> Will it flush only legacy consoles? Or will it flush also non-BKL
> consoles during the early boot?
It will also flush nbcon consoles if a boot console is registered.
> I think that it is wrong even after adding the nbcon check. The code
> looks like this at the end of this patchset:
>
> /*
> * If consoles are not usable, it cannot be expected
> * that they make forward progress, so only increment
> * @diff for usable consoles.
> */
> if (!console_is_usable(c))
> continue;
>
> if (flags & CON_NBCON)
> printk_seq = nbcon_seq_read(c);
> else if (locked)
> printk_seq = c->seq;
> else
> continue;
>
> I guess that the "else-continue" path will never happen. But when
> it happens then pr_flush() would ignore a usable console and it looks
> wrong.
My reason for keeping the "if locked" was to remind the developer that
the console lock must be held in order to safely read @console->seq. But
you are right that it makes things look awkward. I will just change the
code to:
if (flags & CON_NBCON)
printk_seq = nbcon_seq_read(c);
else
printk_seq = c->seq;
There is already a comment at the console_lock() explaining why it is
taken. That is enough.
John
On Mon 2023-07-31 22:45:00, John Ogness wrote:
> On 2023-07-31, Petr Mladek <[email protected]> wrote:
> >>> #define have_serialized_console (have_legacy_console || have_boot_console)
> >>
> >> This macro is not about having a serialized console. The first
> >> sentence in the comment describes it best. It is just to signal if we
> >> need to do the console lock/unlock dance to generate console output.
> >>
> >> Something like "need_bkl_dance" would be a better name, but it
> >> doesn't sound very technical.
> >
> > I see.
> >
> > Question: Will console_lock() serialize the early-boot handling
> > of non-BKL conosles? I mean the direct flush in vprintk_emit().
>
> Initially there will be no nbcon consoles that support CON_BOOT. This
> means that there are no nbcon consoles in "early boot". The only reason
> that nbcon consoles and legacy boot consoles would co-exist (aside from
> the brief moment before boot consoles are unregistered) is if
> keep_bootcon is used.
>
> As long as a boot console is registered, nbcon consoles are also bound
> to console_lock() serialization. We have no choice until we can safely
> link boot consoles to regular consoles.
>
> I think this will be ok for the first release. The 8250 will not become
> less reliable in early boot. And once the boot console is unregistered,
> the 8250 nbcon console will be able to fly.
Makes sense. Thanks a lot for explanation.
> > At lest, the v1 patch set called cons_atomic_flush() in vprintk_emit()
> > without taking outside console_lock().
>
> Yes. But in the v1 patch set, console_is_usable() returns false for
> nbcon consoles if there is a boot console registered. So the
> cons_atomic_flush() in vprintk_emit() would not end up printing
> anything.
>
> As per your v1 feedback, that check will no longer be in
> console_is_usable(), but instead will be further out in higher level
> code.
I see.
> We have 3 scenarios that I would like to easily identify using global
> variable(s).
>
> 1. There are only nbcon consoles. The console lock never needs to be
> taken.
>
> 2. There are nbcon consoles and regular legacy consoles. The console
> lock must be taken to serialize only the regular legacy consoles. There
> are separate code paths (without the console lock) that will take care
> of nbcon atomic printing and nbcon threaded printing.
>
> 3. There are nbcon consoles and boot consoles. The console lock must be
> taken to serialize all consoles.
>
> Perhaps rather than using 2 booleans and a macro, we just use a single
> atomic_t that describes the console serialization mode? The effect is
> the same, but maybe it makes the intention of the code a bit easier to
> understand?
>
> SERMOD_BOOTCON = 0,
> SERMOD_WITH_LEGACY = 1,
> SERMOD_ONLY_NBCON = 2,
IMHO, this is not ideal. Most locations would need to do the console
lock/unlock dance in both '0' and '1' mode. It can be solved by
"sermode <= SERMOD_WITH_LEGACY" but then it would not be clear
what are the modes below '1'.
> Or maybe describe the modes based on their behavior rather than their
> condition:
>
> SERMOD_ONLY_CONSOLE_LOCK = 0,
> SERMOD_ALSO_CONSOLE_LOCK = 1,
> SERMOD_NO_CONSOLE_LOCK = 2,
The might be more practical. But I think that the original variables
were better after all. Well, what about renaming the macro
#define need_legacy_console_flush (have_legacy_console || have_boot_console)
or
#define need_console_lock (have_legacy_console || have_boot_console)
I personally prefer "need_legacy_console_flush". Well, I am not sure
if it would fit all use cases.
> >>> + Update console_flush_all() description. Mention that it flushes
> >>> only serialized consoles
> >>
> >> Agreed. It is only responsible for bkl dance flushing.
> >
> > Will it flush only legacy consoles? Or will it flush also non-BKL
> > consoles during the early boot?
>
> It will also flush nbcon consoles if a boot console is registered.
>
> > I think that it is wrong even after adding the nbcon check. The code
> > looks like this at the end of this patchset:
> >
> > /*
> > * If consoles are not usable, it cannot be expected
> > * that they make forward progress, so only increment
> > * @diff for usable consoles.
> > */
> > if (!console_is_usable(c))
> > continue;
> >
> > if (flags & CON_NBCON)
> > printk_seq = nbcon_seq_read(c);
> > else if (locked)
> > printk_seq = c->seq;
> > else
> > continue;
> >
> > I guess that the "else-continue" path will never happen. But when
> > it happens then pr_flush() would ignore a usable console and it looks
> > wrong.
>
> My reason for keeping the "if locked" was to remind the developer that
> the console lock must be held in order to safely read @console->seq.
I see.
> But you are right that it makes things look awkward. I will just change the
> code to:
>
> if (flags & CON_NBCON)
> printk_seq = nbcon_seq_read(c);
> else
> printk_seq = c->seq;
>
> There is already a comment at the console_lock() explaining why it is
> taken. That is enough.
This looks better to me.
Best Regards,
Petr