* [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
@ 2023-07-28 0:02 ` John Ogness
2023-07-28 14:47 ` Petr Mladek
2023-07-28 0:02 ` [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging John Ogness
` (7 subsequent siblings)
8 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-07-28 0:02 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
From: Thomas Gleixner <tglx@linutronix.de>
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 <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
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
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure
2023-07-28 0:02 ` [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure John Ogness
@ 2023-07-28 14:47 ` Petr Mladek
2023-07-28 20:51 ` John Ogness
0 siblings, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-07-28 14:47 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-07-28 02:08:26, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> 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/20230717194607.145135-3-john.ogness@linutronix.de
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/20230717194607.145135-5-john.ogness@linutronix.de/
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.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure
2023-07-28 14:47 ` Petr Mladek
@ 2023-07-28 20:51 ` John Ogness
2023-07-31 15:39 ` Petr Mladek
0 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-07-28 20:51 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
Hi Petr,
On 2023-07-28, Petr Mladek <pmladek@suse.com> 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/20230717194607.145135-3-john.ogness@linutronix.de
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure
2023-07-28 20:51 ` John Ogness
@ 2023-07-31 15:39 ` Petr Mladek
2023-07-31 20:39 ` John Ogness
0 siblings, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-07-31 15:39 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-07-28 22:57:58, John Ogness wrote:
> Hi Petr,
>
> On 2023-07-28, Petr Mladek <pmladek@suse.com> 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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure
2023-07-31 15:39 ` Petr Mladek
@ 2023-07-31 20:39 ` John Ogness
2023-08-01 10:35 ` Petr Mladek
0 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-07-31 20:39 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2023-07-31, Petr Mladek <pmladek@suse.com> 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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure
2023-07-31 20:39 ` John Ogness
@ 2023-08-01 10:35 ` Petr Mladek
0 siblings, 0 replies; 37+ messages in thread
From: Petr Mladek @ 2023-08-01 10:35 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Mon 2023-07-31 22:45:00, John Ogness wrote:
> On 2023-07-31, Petr Mladek <pmladek@suse.com> 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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
2023-07-28 0:02 ` [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure John Ogness
@ 2023-07-28 0:02 ` John Ogness
2023-07-28 9:52 ` John Ogness
2023-07-28 15:22 ` Petr Mladek
2023-07-28 0:02 ` [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic John Ogness
` (6 subsequent siblings)
8 siblings, 2 replies; 37+ messages in thread
From: John Ogness @ 2023-07-28 0:02 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
To debug and validate nbcon ownership transitions, provide a new
macro debug_store() (enabled with DEBUG_NBCON) to log transition
information to the ringbuffer. If enabled, this macro only logs
to the ringbuffer and does not trigger any printing. This is to
avoid triggering recursive printing in the nbcon consoles.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/printk_nbcon.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
index bb379a4f6263..f9462b088439 100644
--- a/kernel/printk/printk_nbcon.c
+++ b/kernel/printk/printk_nbcon.c
@@ -10,6 +10,35 @@
* the legacy style console_lock mechanism.
*/
+/*
+ * Define DEBUG_NBCON to allow for nbcon ownership transitions to be logged
+ * to the ringbuffer. The debug_store() macro only logs to the lockless
+ * ringbuffer and does not trigger any printing.
+ */
+#undef DEBUG_NBCON
+
+#ifdef DEBUG_NBCON
+/* Only write to ringbuffer. */
+int __debug_store(const char *fmt, ...)
+{
+ va_list args;
+ int r;
+
+ va_start(args, fmt);
+ r = vprintk_store(2, 7, NULL, fmt, args);
+ va_end(args);
+
+ return r;
+}
+#define debug_store(cond, fmt, ...) \
+ do { \
+ if (cond) \
+ __debug_store(pr_fmt("DEBUG: " fmt), ##__VA_ARGS__) \
+ } while (0)
+#else
+#define debug_store(cond, fmt, ...)
+#endif
+
/**
* nbcon_state_set - Helper function to set the console state
* @con: Console to update
--
2.39.2
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging
2023-07-28 0:02 ` [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging John Ogness
@ 2023-07-28 9:52 ` John Ogness
2023-07-28 15:22 ` Petr Mladek
1 sibling, 0 replies; 37+ messages in thread
From: John Ogness @ 2023-07-28 9:52 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On 2023-07-28, John Ogness <john.ogness@linutronix.de> wrote:
> +/*
> + * Define DEBUG_NBCON to allow for nbcon ownership transitions to be logged
> + * to the ringbuffer. The debug_store() macro only logs to the lockless
> + * ringbuffer and does not trigger any printing.
> + */
> +#undef DEBUG_NBCON
> +
> +#ifdef DEBUG_NBCON
> +/* Only write to ringbuffer. */
> +int __debug_store(const char *fmt, ...)
> +{
> + va_list args;
> + int r;
> +
> + va_start(args, fmt);
> + r = vprintk_store(2, 7, NULL, fmt, args);
> + va_end(args);
> +
> + return r;
> +}
> +#define debug_store(cond, fmt, ...) \
> + do { \
> + if (cond) \
> + __debug_store(pr_fmt("DEBUG: " fmt), ##__VA_ARGS__) \
Missing a semi-colon here. Wrapping this with a do-while was a
last-minute change requested by checkpatch.pl. Probably nobody would
notice because you must manually define DEBUG_NBCON by changing the
source code. Fixed for v3 (assuming Petr allows me to keep this
debugging code in place).
> + } while (0)
> +#else
> +#define debug_store(cond, fmt, ...)
> +#endif
John
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging
2023-07-28 0:02 ` [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging John Ogness
2023-07-28 9:52 ` John Ogness
@ 2023-07-28 15:22 ` Petr Mladek
2023-07-28 21:01 ` John Ogness
1 sibling, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-07-28 15:22 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Fri 2023-07-28 02:08:27, John Ogness wrote:
> To debug and validate nbcon ownership transitions, provide a new
> macro debug_store() (enabled with DEBUG_NBCON) to log transition
> information to the ringbuffer. If enabled, this macro only logs
> to the ringbuffer and does not trigger any printing. This is to
> avoid triggering recursive printing in the nbcon consoles.
>
> --- a/kernel/printk/printk_nbcon.c
> +++ b/kernel/printk/printk_nbcon.c
> @@ -10,6 +10,35 @@
> * the legacy style console_lock mechanism.
> */
>
> +/*
> + * Define DEBUG_NBCON to allow for nbcon ownership transitions to be logged
> + * to the ringbuffer. The debug_store() macro only logs to the lockless
> + * ringbuffer and does not trigger any printing.
> + */
> +#undef DEBUG_NBCON
> +
> +#ifdef DEBUG_NBCON
> +/* Only write to ringbuffer. */
> +int __debug_store(const char *fmt, ...)
> +{
> + va_list args;
> + int r;
> +
> + va_start(args, fmt);
> + r = vprintk_store(2, 7, NULL, fmt, args);
I have never used the facility number before. It seems that they are
defined in /usr/include/sys/syslog.h, for example, see
https://sites.uclouvain.be/SystInfo/usr/include/sys/syslog.h.html
They are even somehow documented in "man 3 syslog", for example, see
https://linux.die.net/man/3/syslog
We should probably use one of the LOG_LOCALX numbers, e.g.
#define LOG_LOCAL0 (16<<3)
Also, please use LOGLEVEL_DEBUG instead of the hard coded "7".
> + va_end(args);
> +
> + return r;
> +}
> +#define debug_store(cond, fmt, ...) \
> + do { \
> + if (cond) \
> + __debug_store(pr_fmt("DEBUG: " fmt), ##__VA_ARGS__) \
> + } while (0)
> +#else
> +#define debug_store(cond, fmt, ...)
> +#endif
> +
> /**
> * nbcon_state_set - Helper function to set the console state
> * @con: Console to update
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging
2023-07-28 15:22 ` Petr Mladek
@ 2023-07-28 21:01 ` John Ogness
2023-07-31 15:43 ` Petr Mladek
0 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-07-28 21:01 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On 2023-07-28, Petr Mladek <pmladek@suse.com> wrote:
>> + r = vprintk_store(2, 7, NULL, fmt, args);
>
> We should probably use one of the LOG_LOCALX numbers, e.g.
>
> #define LOG_LOCAL0 (16<<3)
>
> Also, please use LOGLEVEL_DEBUG instead of the hard coded "7".
OK.
I am also open to dropping the function and its use. When developing the
ringbuffer I also had a similar function but never attempted to publish
it. It is useful for developing/testing/debugging, but once everything
works as it should it has no real purpose. I have no problems keeping
the debug stuff hidden in my own private toolbox.
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging
2023-07-28 21:01 ` John Ogness
@ 2023-07-31 15:43 ` Petr Mladek
0 siblings, 0 replies; 37+ messages in thread
From: Petr Mladek @ 2023-07-31 15:43 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Fri 2023-07-28 23:07:08, John Ogness wrote:
> On 2023-07-28, Petr Mladek <pmladek@suse.com> wrote:
> >> + r = vprintk_store(2, 7, NULL, fmt, args);
> >
> > We should probably use one of the LOG_LOCALX numbers, e.g.
> >
> > #define LOG_LOCAL0 (16<<3)
> >
> > Also, please use LOGLEVEL_DEBUG instead of the hard coded "7".
>
> OK.
>
> I am also open to dropping the function and its use. When developing the
> ringbuffer I also had a similar function but never attempted to publish
> it. It is useful for developing/testing/debugging, but once everything
> works as it should it has no real purpose. I have no problems keeping
> the debug stuff hidden in my own private toolbox.
I do not mind to add this patch. It is actually pretty interesting
function. I wonder if it might inspire anyone for using this approach
for some other reasons.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
2023-07-28 0:02 ` [PATCH printk v2 1/8] printk: Add non-BKL (nbcon) console basic infrastructure John Ogness
2023-07-28 0:02 ` [PATCH printk v2 2/8] printk: Provide debug_store() for nbcon debugging John Ogness
@ 2023-07-28 0:02 ` John Ogness
2023-08-09 10:20 ` Petr Mladek
2023-08-09 12:44 ` hostile takeover: " Petr Mladek
2023-07-28 0:02 ` [PATCH printk v2 4/8] printk: nbcon: Add buffer management John Ogness
` (5 subsequent siblings)
8 siblings, 2 replies; 37+ messages in thread
From: John Ogness @ 2023-07-28 0:02 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
From: Thomas Gleixner <tglx@linutronix.de>
Add per console acquire/release functionality. The console 'locked'
state is a combination of multiple state fields:
- The 'prio' field contains the severity of the context that owns
the console. This field is used for decisions whether to attempt
friendly handovers and also prevents takeovers from a less
severe context, e.g. to protect the panic CPU. A value of 0
(NBCON_PRIO_NONE) means the console is not locked.
- The 'cpu' field denotes on which CPU the console is locked.
The acquire mechanism comes with several flavours:
- Straight forward acquire when the console is not contended.
- Friendly handover mechanism based on a request/grant handshake.
The requesting context:
1) Sets its priority into the 'req_prio' field.
2) Waits (with a timeout) for the owning context to unlock the
console.
3) Sets the 'prio' field and clears the 'req_prio' field.
The owning context:
1) Observes the 'req_prio' field set.
2) Gives up console ownership by clearing the 'prio' field.
- Hostile takeover
The new owner takes the console over without 'req_prio'
handshake.
This is required when friendly handovers are not possible,
i.e. the higher priority context interrupted the owning
context on the same CPU or the owning context is not able
to make progress on a remote CPU.
The release is the counterpart which either releases the console
directly or yields it gracefully over to a requester.
All operations on console::nbcon_state are atomic cmpxchg based to
handle concurrency.
The acquire/release functions implement only minimal policies:
- Preference for higher priority contexts.
- Protection of the panic CPU.
All other policy decisions have to be made at the call sites:
- What is marked as an unsafe section.
- Whether to spinwait if there is already an owner.
- Whether to attempt a hostile takeover when safe.
- Whether to attempt a hostile takeover when unsafe.
The design allows to implement the well known:
acquire()
output_one_line()
release()
algorithm, but also allows to avoid the per line acquire/release for
e.g. panic situations by doing the acquire once and then relying on
the panic CPU protection for the rest.
Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
include/linux/console.h | 59 +++++
kernel/printk/printk_nbcon.c | 447 +++++++++++++++++++++++++++++++++++
2 files changed, 506 insertions(+)
diff --git a/include/linux/console.h b/include/linux/console.h
index c99265d82b98..e06cd1ce3e82 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -175,13 +175,28 @@ enum cons_flags {
* struct nbcon_state - console state for nbcon consoles
* @atom: Compound of the state fields for atomic operations
*
+ * @req_prio: The priority of a handover request
+ * @prio: The priority of the current usage
+ * @unsafe: Console is busy in a non takeover region
+ * @hostile_unsafe: The @unsafe value before a hostile takeover
+ * @cpu: The CPU on which the owner runs
+ *
* To be used for reading and preparing of the value stored in the nbcon
* state variable @console.nbcon_state.
+ *
+ * The @prio and @req_prio fields are particularly important to allow
+ * spin-waiting to timeout and give up without the risk of a waiter being
+ * assigned the lock after giving up.
*/
struct nbcon_state {
union {
unsigned int atom;
struct {
+ unsigned int prio : 2;
+ unsigned int req_prio : 2;
+ unsigned int unsafe : 1;
+ unsigned int hostile_unsafe : 1;
+ unsigned int cpu : 24;
};
};
};
@@ -194,6 +209,50 @@ struct nbcon_state {
*/
static_assert(sizeof(struct nbcon_state) <= sizeof(int));
+/**
+ * nbcon_prio - console owner priority for nbcon consoles
+ * @NBCON_PRIO_NONE: Unused
+ * @NBCON_PRIO_NORMAL: Normal (non-emergency) usage
+ * @NBCON_PRIO_EMERGENCY: Emergency output (WARN/OOPS...)
+ * @NBCON_PRIO_PANIC: Panic output
+ * @NBCON_PRIO_MAX: The number of priority levels
+ *
+ * A context wanting to produce emergency output can carefully takeover the
+ * console, even without consent of the owner. Ideally such a takeover is only
+ * when @nbcon_state::unsafe is not set. However, a context wanting to produce
+ * panic output can ignore the unsafe flag as a last resort. If panic output
+ * is active, no takeover is possible until the panic output releases the
+ * console.
+ */
+enum nbcon_prio {
+ NBCON_PRIO_NONE = 0,
+ NBCON_PRIO_NORMAL,
+ NBCON_PRIO_EMERGENCY,
+ NBCON_PRIO_PANIC,
+ NBCON_PRIO_MAX,
+};
+
+struct console;
+
+/**
+ * struct nbcon_context - Context for console acquire/release
+ * @console: The associated console
+ * @spinwait_max_us: Limit for spinwait acquire
+ * @prio: Priority of the context
+ * @unsafe: This context is in an unsafe section
+ * @hostile: Acquire console by hostile takeover
+ * @takeover_unsafe: Acquire console by hostile takeover even if unsafe
+ */
+struct nbcon_context {
+ /* members set by caller */
+ struct console *console;
+ unsigned int spinwait_max_us;
+ enum nbcon_prio prio;
+ unsigned int unsafe : 1;
+ unsigned int hostile : 1;
+ unsigned int takeover_unsafe : 1;
+};
+
/**
* struct console - The console descriptor structure
* @name: The name of the console driver
diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
index f9462b088439..b0acde0cb949 100644
--- a/kernel/printk/printk_nbcon.c
+++ b/kernel/printk/printk_nbcon.c
@@ -4,10 +4,38 @@
#include <linux/kernel.h>
#include <linux/console.h>
+#include <linux/delay.h>
#include "internal.h"
/*
* Printk console printing implementation for consoles that do not depend on
* the legacy style console_lock mechanism.
+ *
+ * Console is locked on a CPU when @nbcon_state::prio is set and
+ * @nbcon_state:cpu == current CPU. This is valid for the current execution
+ * context.
+ *
+ * Nesting execution contexts on the same CPU can carefully take over if
+ * the driver allows reentrancy via nbcon_state::unsafe = false. When the
+ * interrupted context resumes it checks the state before entering an unsafe
+ * region and aborts the operation if it detects a takeover.
+ *
+ * In case of panic the nesting context can take over the console forcefully.
+ *
+ * A concurrent writer on a different CPU with a higher priority can request
+ * to take over the console by:
+ *
+ * 1) Carefully writing the desired priority into @nbcon_state::req_prio
+ * if there is no higher priority request pending.
+ *
+ * 2) Carefully spin on @nbcon_state::prio until it is no longer locked.
+ *
+ * 3) Attempt to lock the console by carefully writing the desired
+ * priority to @nbcon_state::prio and carefully removing the desired
+ * priority from @nbcon_state::req_prio.
+ *
+ * In case that the owner does not react on the request and does not make
+ * observable progress, the waiter will timeout and can then decide to do
+ * a hostile takeover.
*/
/*
@@ -76,6 +104,425 @@ static inline bool nbcon_state_try_cmpxchg(struct console *con, struct nbcon_sta
return atomic_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_state), &cur->atom, new->atom);
}
+/**
+ * nbcon_context_try_acquire_direct - Try to acquire directly
+ * @ctxt: The context of the caller
+ * @cur: The current console state
+ *
+ * Return: 0 on success and @cur is updated to the new console state.
+ * Otherwise an error code on failure.
+ *
+ * Errors:
+ *
+ * -EPERM: A panic is in progress an this is not the panic CPU.
+ * Retrying the acquire will not be immediately useful.
+ *
+ * -EBUSY: The console already has an owner. Retrying the
+ * acquire will not be immediately useful.
+ *
+ * -EAGAIN: An unexpected state change occurred. @cur has been
+ * updated. The caller should retry the acquire.
+ *
+ * The general procedure is to change @prio from unowned to owned.
+ */
+static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
+ struct nbcon_state *cur)
+{
+ unsigned int cpu = smp_processor_id();
+ struct console *con = ctxt->console;
+ struct nbcon_state new = {
+ .cpu = cpu,
+ .prio = ctxt->prio,
+ .unsafe = cur->hostile_unsafe | ctxt->unsafe,
+ .hostile_unsafe = cur->hostile_unsafe,
+ };
+
+ if (other_cpu_in_panic())
+ return -EPERM;
+
+ /*
+ * Direct not possible if owner in unsafe region. If there
+ * was previously a hostile takeover, the console may be left
+ * in an unsafe state, even if it is now unowned.
+ */
+ if (cur->unsafe)
+ return -EBUSY;
+
+ /* Direct is only possible if it is unowned. */
+ if (cur->prio != NBCON_PRIO_NONE)
+ return -EBUSY;
+
+ /* Higher priority waiters are allowed to keep waiting. */
+ if (cur->req_prio > ctxt->prio)
+ new.req_prio = cur->req_prio;
+ else
+ new.req_prio = 0;
+
+ /* All looks good. Try to take ownership. */
+
+ if (!nbcon_state_try_cmpxchg(con, cur, &new))
+ return -EAGAIN;
+
+ cur->atom = new.atom;
+
+ return 0;
+}
+
+/**
+ * nbcon_context_try_acquire_handover - Try to acquire via handover
+ * @ctxt: The context of the caller
+ * @cur: The current console state
+ *
+ * Return: 0 on success and @cur is updated to the new console state.
+ * Otherwise an error code on failure.
+ *
+ * Errors:
+ *
+ * -EPERM: A panic is in progress an this is not the panic CPU.
+ * Retrying the acquire will not be immediately useful.
+ *
+ * -EBUSY: The current owner or waiter is such that this context
+ * is not able to execute a handover. Retrying the
+ * acquire will not be immediately useful.
+ *
+ * -EAGAIN: An unexpected state change occurred. @cur has been
+ * updated. The caller should retry the acquire.
+ *
+ * The general procedure is to set @req_prio and wait until unowned. Then
+ * set @prio (claiming ownership) and clearing @req_prio.
+ */
+static int nbcon_context_try_acquire_handover(struct nbcon_context *ctxt,
+ struct nbcon_state *cur)
+{
+ unsigned int cpu = smp_processor_id();
+ struct console *con = ctxt->console;
+ struct nbcon_state new;
+ int timeout;
+ int err = 0;
+
+ /* Cannot request handover if owner has same or higher priority. */
+ if (cur->prio >= ctxt->prio)
+ return -EBUSY;
+
+ /* Cannot request handover if a waiter has same or higher priority. */
+ if (cur->req_prio >= ctxt->prio)
+ return -EBUSY;
+
+ /*
+ * If there is no owner, a handover may be in progress and this
+ * context (having a higher priority) could jump in directly.
+ */
+ if (cur->prio == NBCON_PRIO_NONE)
+ return nbcon_context_try_acquire_direct(ctxt, cur);
+
+ /* Cannot handover on same CPU. */
+ if (cur->cpu == cpu)
+ return -EBUSY;
+
+ /* Cannot request handover if caller unwilling to wait. */
+ if (ctxt->spinwait_max_us == 0)
+ return -EBUSY;
+
+ /* Set request for handover. */
+ new.atom = cur->atom;
+ new.req_prio = ctxt->prio;
+ if (!nbcon_state_try_cmpxchg(con, cur, &new))
+ return -EAGAIN;
+
+ cur->atom = new.atom;
+
+ debug_store(1, "handover: cpu%d SPINNING to acquire for prio%d\n", cpu, ctxt->prio);
+
+ /* Wait until there is no owner and then acquire directly. */
+ for (timeout = ctxt->spinwait_max_us; timeout >= 0; timeout--) {
+ /* On successful acquire, this request is cleared. */
+ err = nbcon_context_try_acquire_direct(ctxt, cur);
+ if (!err)
+ return 0;
+
+ /*
+ * If another CPU is in panic, the request must be removed
+ * before returning to the caller.
+ */
+ if (err == -EPERM)
+ break;
+
+ /* Continue spinwaiting on -EAGAIN and -EBUSY. */
+
+ udelay(1);
+
+ /* Re-read the state because some time has passed. */
+ nbcon_state_read(con, cur);
+
+ /*
+ * If the waiter unexpectedly changed, this request is
+ * no longer set. Have the caller try again rather than
+ * guessing what has happened.
+ */
+ if (cur->req_prio != ctxt->prio)
+ return -EAGAIN;
+ }
+
+ /* Timeout. Safely remove handover request and report failure. */
+ do {
+ /* No need to remove request if there is another waiter. */
+ if (cur->req_prio != ctxt->prio) {
+ if (err == -EPERM)
+ return err;
+ return -EBUSY;
+ }
+
+ /* Unset request for handover. */
+ new.atom = cur->atom;
+ new.req_prio = NBCON_PRIO_NONE;
+ if (nbcon_state_try_cmpxchg(con, cur, &new)) {
+ debug_store(1, "handover: cpu%d TIMEOUT to acquire for prio%d\n",
+ cpu, ctxt->prio);
+ /*
+ * Request successfully unset. Report failure of
+ * acquiring via handover.
+ */
+ cur->atom = new.atom;
+ return -EBUSY;
+ }
+
+ /*
+ * Unable to remove request. Try direct acquire. If
+ * successful, this request is also cleared.
+ */
+ err = nbcon_context_try_acquire_direct(ctxt, cur);
+ } while (err);
+
+ /* Direct acquire at timeout succeeded! */
+ return 0;
+}
+
+/**
+ * nbcon_context_try_acquire_hostile - Try to acquire via hostile takeover
+ * @ctxt: The context of the caller
+ * @cur: The current console state
+ *
+ * Return: 0 on success and @cur is updated to the new console state.
+ * Otherwise an error code on failure.
+ *
+ * Errors:
+ *
+ * -EPERM: A panic is in progress an this is not the panic CPU
+ * or this context is not the single panic context.
+ * Retrying the acquire will not be immediately useful.
+ *
+ * -EBUSY: The current owner is in an unsafe region and
+ * @takeover_unsafe was not set. Retrying the acquire
+ * is only immediately useful if @takeover_unsafe is
+ * set.
+ *
+ * -EAGAIN: An unexpected state change occurred. @cur has been
+ * updated. The caller should retry the acquire.
+ *
+ * The general procedure is to set @prio (forcing ownership). This method
+ * must only be used as a final attempt during panic.
+ */
+static int nbcon_context_try_acquire_hostile(struct nbcon_context *ctxt,
+ struct nbcon_state *cur)
+{
+ unsigned int cpu = smp_processor_id();
+ struct console *con = ctxt->console;
+ struct nbcon_state new = {
+ .cpu = cpu,
+ .prio = ctxt->prio,
+ .unsafe = cur->hostile_unsafe | ctxt->unsafe,
+ .hostile_unsafe = cur->hostile_unsafe | cur->unsafe,
+ };
+
+ if (other_cpu_in_panic())
+ return -EPERM;
+
+ /* Hostile takeovers must only be in the single panic context! */
+ if (WARN_ON_ONCE(ctxt->prio != NBCON_PRIO_PANIC || cur->prio == NBCON_PRIO_PANIC))
+ return -EPERM;
+
+ /*
+ * If a hostile takeover in unsafe regions is wanted,
+ * this is additionally requested.
+ */
+ if (cur->unsafe && !ctxt->takeover_unsafe)
+ return -EBUSY;
+
+ if (!nbcon_state_try_cmpxchg(con, cur, &new))
+ return -EAGAIN;
+
+ cur->atom = new.atom;
+
+ return 0;
+}
+
+/**
+ * nbcon_context_try_acquire - Try to acquire nbcon console
+ * @ctxt: The context of the caller
+ *
+ * Return: True if the console was acquired. False otherwise.
+ *
+ * The attempts to acquire always begin with the safest methods and only
+ * upon failure move to more aggressive methods.
+ *
+ * If the caller requested a hostile takeover, on success @ctxt is
+ * updated so that the caller can see if indeed a hostile (and
+ * possibly unsafe) takeover occurred.
+ */
+__maybe_unused
+static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
+{
+ __maybe_unused
+ unsigned int cpu = smp_processor_id();
+ struct console *con = ctxt->console;
+ struct nbcon_state cur;
+ bool hostile;
+ int err;
+
+ /* @takeover_unsafe is only valid when hostile takeover requested. */
+ WARN_ON_ONCE(ctxt->takeover_unsafe && !ctxt->hostile);
+
+ nbcon_state_read(con, &cur);
+
+try_again:
+ hostile = false;
+
+ /* ACQUIRE DIRECT */
+
+ debug_store(ctxt->prio > NBCON_PRIO_NORMAL,
+ "direct: cpu%d trying DIRECT acquire prio%d\n", cpu, ctxt->prio);
+ err = nbcon_context_try_acquire_direct(ctxt, &cur);
+ if (!err) {
+ debug_store(ctxt->prio > NBCON_PRIO_NORMAL,
+ "direct: cpu%d SUCCESS prio%d\n", cpu, ctxt->prio);
+ goto success;
+ } else if (err == -EPERM) {
+ debug_store(1, "direct: cpu%d FAILED prio%d (non-panic cpu)\n",
+ cpu, ctxt->prio);
+ return false;
+ } else if (err == -EAGAIN) {
+ debug_store(1, "direct: cpu%d FAILED prio%d (state race)\n",
+ cpu, ctxt->prio);
+ goto try_again;
+ } else {
+ debug_store(1, "direct: cpu%d FAILED prio%d (already owned by cpu%d)\n",
+ cpu, ctxt->prio, cur.cpu);
+ /* Continue to next method. */
+ }
+
+ /* ACQUIRE VIA HANDOVER */
+
+ debug_store(1, "handover: cpu%d REQUESTING prio%d from cpu%d\n",
+ cpu, ctxt->prio, cur.cpu);
+ err = nbcon_context_try_acquire_handover(ctxt, &cur);
+ if (!err) {
+ debug_store(1, "handover: cpu%d SUCCESS prio%d\n",
+ cpu, ctxt->prio);
+ goto success;
+ } else if (err == -EPERM) {
+ debug_store(1, "handover: cpu%d FAILED requesting prio%d (non-panic cpu)\n",
+ cpu, ctxt->prio);
+ return false;
+ } else if (err == -EAGAIN) {
+ debug_store(1, "handover: cpu%d FAILED requesting prio%d (state race)\n",
+ cpu, ctxt->prio);
+ goto try_again;
+ } else {
+ debug_store(1, "handover: cpu%d FAILED requesting prio%d (cpu%d/prio%d/timeout)\n",
+ cpu, ctxt->prio, cur.cpu, cur.prio);
+ /* Continue to next method. */
+ }
+
+ /* ACQUIRE VIA HOSTILE TAKEOVER */
+
+ /* Only attempt hostile takeover if explicitly requested. */
+ if (!ctxt->hostile)
+ return false;
+
+ debug_store(1,
+ "hostile: cpu%d trying HOSTILE acquire for prio%d from cpu%d (takeover_unsafe=%d)\n",
+ cpu, ctxt->prio, cur.cpu, ctxt->takeover_unsafe);
+ err = nbcon_context_try_acquire_hostile(ctxt, &cur);
+ if (!err) {
+ debug_store(1, "hostile: cpu%d SUCCESS prio%d\n", cpu, ctxt->prio);
+ /* Let caller know if the takeover was unsafe. */
+ ctxt->takeover_unsafe = cur.hostile_unsafe;
+ hostile = true;
+ goto success;
+ } else if (err == -EPERM) {
+ debug_store(1, "hostile: cpu%d FAILED acquire prio%d (non-panic cpu)\n",
+ cpu, ctxt->prio);
+ return false;
+ } else if (err == -EAGAIN) {
+ debug_store(1, "hostile: cpu%d FAILED acquire prio%d (state race)\n",
+ cpu, ctxt->prio);
+ goto try_again;
+ } else {
+ debug_store(1, "hostile: cpu%d FAILED acquire prio%d (unsafe)\n",
+ cpu, ctxt->prio);
+ /* Continue to next method. */
+ }
+
+ /* No methods left to try. */
+ return false;
+success:
+ if (!hostile) {
+ /* Let caller know it was not a hostile takeover. */
+ ctxt->hostile = 0;
+ ctxt->takeover_unsafe = 0;
+ }
+ return true;
+}
+
+static bool nbcon_owner_matches(struct nbcon_state *cur, int expected_cpu,
+ int expected_prio)
+{
+ /*
+ * Since consoles can only be acquired by higher priorities,
+ * owning contexts are uniquely identified by @prio. However,
+ * since contexts can unexpectedly lose ownership, it is
+ * possible that later another owner appears with the same
+ * priority. For this reason @cpu is also needed.
+ */
+
+ if (cur->prio != expected_prio)
+ return false;
+
+ if (cur->cpu != expected_cpu)
+ return false;
+
+ return true;
+}
+
+/**
+ * nbcon_context_release - Release the console
+ * @ctxt: The nbcon context from nbcon_context_try_acquire()
+ */
+__maybe_unused
+static void nbcon_context_release(struct nbcon_context *ctxt)
+{
+ unsigned int cpu = smp_processor_id();
+ struct console *con = ctxt->console;
+ struct nbcon_state cur;
+ struct nbcon_state new;
+
+ nbcon_state_read(con, &cur);
+ do {
+ if (!nbcon_owner_matches(&cur, cpu, ctxt->prio))
+ return;
+
+ new.atom = cur.atom;
+ new.prio = NBCON_PRIO_NONE;
+ new.unsafe = cur.hostile_unsafe;
+
+ /*
+ * If @hostile_unsafe is set, it is kept set so that
+ * the state remains permanently unsafe.
+ */
+
+ } while (!nbcon_state_try_cmpxchg(con, &cur, &new));
+}
+
/**
* nbcon_init - Initialize the nbcon console specific data
* @con: Console to initialize
--
2.39.2
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic
2023-07-28 0:02 ` [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic John Ogness
@ 2023-08-09 10:20 ` Petr Mladek
2023-08-29 9:43 ` John Ogness
2023-08-09 12:44 ` hostile takeover: " Petr Mladek
1 sibling, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-08-09 10:20 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-07-28 02:08:28, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Add per console acquire/release functionality. The console 'locked'
> state is a combination of multiple state fields:
I scratched my head on this patch many days and I am still not sure
how to deal with it. I propose some changes but ...
My proposal is a kind of inverted logic. It allows only limited number
of scenarios. It reduces backward loops. And it is more clear what
is possible in which part of the code.
I kind of like it this way.
The original approach allows what makes sense. Restart the entire
try_acquire loop when something has changed.
It might reduce code duplication. But it allows too many combinations
to my taste so that I am not able keep them in head. From my POV,
it might require more test+debug+fix loops. And the debug part
might be painful.
But it is possible that I just look at it from a wrong angle.
About this mail:
It is long. I updated it several days. I did not know how to
split it because most comments are somehow related.
I do not expect that you would answer every detail. Top level view
is more important. And also situations where I missed some context.
> --- a/kernel/printk/printk_nbcon.c
> +++ b/kernel/printk/printk_nbcon.c
> @@ -76,6 +104,425 @@ static inline bool nbcon_state_try_cmpxchg(struct console *con, struct nbcon_sta
> return atomic_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_state), &cur->atom, new->atom);
> }
>
> +/**
> + * nbcon_context_try_acquire_direct - Try to acquire directly
> + * @ctxt: The context of the caller
> + * @cur: The current console state
> + *
> + * Return: 0 on success and @cur is updated to the new console state.
> + * Otherwise an error code on failure.
> + *
> + * Errors:
It might be because I am not a native speaker. But I was a bit
confused by the below description. I think that I know what
it means but only because I "know" the context.
Especially using the same "Retrying the acquire will not be
immediately useful." does not help to understand the difference
of what the caller should do.
> + *
> + * -EPERM: A panic is in progress an this is not the panic CPU.
> + * Retrying the acquire will not be immediately useful.
I would write something like:
* -EPERM: A panic is in progress an this is not the panic CPU.
* This context is not allowed to take the lock.
But I would use is also when the priority is not sufficient. In this
case, any waiting does not make sense either. So, I would write:
* -EPERM: The console is already owned by a context with
* the same or higher priority. Or a panic is in
* progress an this is not the panic CPU. Any waiting
* does not make sense in this case.
> + *
> + * -EBUSY: The console already has an owner. Retrying the
> + * acquire will not be immediately useful.
I would write something like this:
* -EBUSY: The console already has an owner with a lower
* The caller might try handover.
> + *
> + * -EAGAIN: An unexpected state change occurred. @cur has been
> + * updated. The caller should retry the acquire.
The word "unexpected" sounds like it should never happen. It might
rare but it is a perfectly fine race. I would write:
"@cur has changed in the meaning. The caller should retry."
> + *
> + * The general procedure is to change @prio from unowned to owned.
> + */
> +static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
> + struct nbcon_state *cur)
> +{
> + unsigned int cpu = smp_processor_id();
> + struct console *con = ctxt->console;
> + struct nbcon_state new = {
> + .cpu = cpu,
> + .prio = ctxt->prio,
> + .unsafe = cur->hostile_unsafe | ctxt->unsafe,
> + .hostile_unsafe = cur->hostile_unsafe,
> + };
> +
> + if (other_cpu_in_panic())
> + return -EPERM;
> +
> + /*
> + * Direct not possible if owner in unsafe region. If there
> + * was previously a hostile takeover, the console may be left
> + * in an unsafe state, even if it is now unowned.
> + */
> + if (cur->unsafe)
> + return -EBUSY;
I would expect that this function is able to take the lock when there
is no owner and no waiter.
The harsh takeover will be used as the last attempt in panic().
But what about messages printed after this flush? They should
be able to reach the console as well.
It might be enough when the information that there was a harsh
takeover in the past will stay stored in nbcon_state and
copied to nbcon_ctxt. If the console driver used a special
code after a harsh takeover, it would still know that it should
use it.
> + /* Direct is only possible if it is unowned. */
> + if (cur->prio != NBCON_PRIO_NONE)
> + return -EBUSY;
> +
> + /* Higher priority waiters are allowed to keep waiting. */
> + if (cur->req_prio > ctxt->prio)
> + new.req_prio = cur->req_prio;
> + else
> + new.req_prio = 0;
I am confused here.
1. I would expect that we bail out when there already is a request
with a higher priority.
It seems like this code tries to steal the lock that was
released for the higher priority waiter. Honestly, I do
not see any good reason for this. Higher priority
request should always get the lock ASAP!
If we do not want an extra delay then I would replace
udelay(1) with cpu_relax(). The lock is supposed to
wait the same way as a spinlock.
Or is this somehow important for RT?
I guess that the kthread might afford rescheduling here. But
will it reschedule also in EMERGENCY context? It will not
reschedule in PANIC for sure.
Well, if we allow rescheduling then we need to guarantee
that the process stays on the same CPU. I am not sure what
is the plan here.
2. I would expect that we bail out even when there already is a request
with the same priority.
OK, the spinning context will detect this because nbcon_state_try_cmpxchg()
will fail and it will restart everything with -EAGAIN. And it will
later bail out in nbcon_context_try_acquire_handover().
So, this likely works but it is pretty hairy to me. It is far from
obvious and not documented. It took me long time to put
all these pieces together.
> + /* All looks good. Try to take ownership. */
> +
> + if (!nbcon_state_try_cmpxchg(con, cur, &new))
> + return -EAGAIN;
> +
> + cur->atom = new.atom;
> +
> + return 0;
> +}
> +
> +/**
> + * nbcon_context_try_acquire_handover - Try to acquire via handover
> + * @ctxt: The context of the caller
> + * @cur: The current console state
> + *
> + * Return: 0 on success and @cur is updated to the new console state.
> + * Otherwise an error code on failure.
> + *
> + * Errors:
> + *
> + * -EPERM: A panic is in progress an this is not the panic CPU.
> + * Retrying the acquire will not be immediately useful.
> + *
> + * -EBUSY: The current owner or waiter is such that this context
> + * is not able to execute a handover. Retrying the
> + * acquire will not be immediately useful.
> + *
> + * -EAGAIN: An unexpected state change occurred. @cur has been
> + * updated. The caller should retry the acquire.
> + *
I have the same problems to understand the meaning of the error codes
as in nbcon_context_try_acquire_direct(). See an alternative text below.
> + * The general procedure is to set @req_prio and wait until unowned. Then
> + * set @prio (claiming ownership) and clearing @req_prio.
> + */
> +static int nbcon_context_try_acquire_handover(struct nbcon_context *ctxt,
> + struct nbcon_state *cur)
> +{
> + unsigned int cpu = smp_processor_id();
> + struct console *con = ctxt->console;
> + struct nbcon_state new;
> + int timeout;
> + int err = 0;
> +
> + /* Cannot request handover if owner has same or higher priority. */
> + if (cur->prio >= ctxt->prio)
> + return -EBUSY;
> +
I was surprised by the -EBUSY. Insufficient priority sounds more like
a permission problem.
I guess that you use -EBUSY so that it would try the harsh takeover.
It is not a big deal. But it should be explained in a comment.
But I would personally use -EPERM.
> + /* Cannot request handover if a waiter has same or higher priority. */
> + if (cur->req_prio >= ctxt->prio)
> + return -EBUSY;
Same here.
> +
> + /*
> + * If there is no owner, a handover may be in progress and this
> + * context (having a higher priority) could jump in directly.
> + */
> + if (cur->prio == NBCON_PRIO_NONE)
> + return nbcon_context_try_acquire_direct(ctxt, cur);
This looks superfluous. We are here because
nbcon_context_try_acquire_direct() failed.
> + /* Cannot handover on same CPU. */
> + if (cur->cpu == cpu)
> + return -EBUSY;
-EBUSY looks natural in this case.
> + /* Cannot request handover if caller unwilling to wait. */
> + if (ctxt->spinwait_max_us == 0)
> + return -EBUSY;
> +
> + /* Set request for handover. */
> + new.atom = cur->atom;
> + new.req_prio = ctxt->prio;
> + if (!nbcon_state_try_cmpxchg(con, cur, &new))
> + return -EAGAIN;
> +
> + cur->atom = new.atom;
> +
> + debug_store(1, "handover: cpu%d SPINNING to acquire for prio%d\n", cpu, ctxt->prio);
> +
> + /* Wait until there is no owner and then acquire directly. */
> + for (timeout = ctxt->spinwait_max_us; timeout >= 0; timeout--) {
> + /* On successful acquire, this request is cleared. */
> + err = nbcon_context_try_acquire_direct(ctxt, cur);
I guess that this is the reason why nbcon_context_try_acquire_direct()
tries to get the lock even when there is a request with the same
priority. But it means that more CPUs with the same priority
might get the lock via nbcon_context_try_acquire_direct().
Only one might succeed.
This made my head spin a lot. And it took me long time to understand
if the race is ok, if there will be more waiters with the same
priority spinning, ...
OK, callers with the same priority should not spin because
they will bail out at the beginning of this function. So, there should
be only one waiter, except, see below.
> + if (!err)
> + return 0;
> +
> + /*
> + * If another CPU is in panic, the request must be removed
> + * before returning to the caller.
> + */
> + if (err == -EPERM)
> + break;
> +
> + /* Continue spinwaiting on -EAGAIN and -EBUSY. */
> +
> + udelay(1);
> +
> + /* Re-read the state because some time has passed. */
> + nbcon_state_read(con, cur);
> +
> + /*
> + * If the waiter unexpectedly changed, this request is
> + * no longer set. Have the caller try again rather than
> + * guessing what has happened.
> + */
> + if (cur->req_prio != ctxt->prio)
IMHO, the following might happen:
CPU0 CPU1 CPU2
# EMERGENCY PRIO)
nbcon_context_try_acquire_handover()
nbcon_context_try_acquire_handover for (timeout...
udelay()
# current owner releases the lock
# another try_acquire in EMERGENCY_PRIO
nbcon_context_try_acquire_direct()
# SUCCESS, the lock was free
# req_prio is clear
# another try_acquire in EMERGENCY_PRIO
nbcon_context_try_acquire_direct()
# -EBUSY, lock owned by CPU1
# nbcon_context_try_acquire_handover()
# see clear req_prio
# set req_prio to and is spinning
nbcon_state_read(con, cur);
if (cur->req_prio != ctxt->prio)
# fails because req_prio is the same even
# when the owner is different
RESULT: Two CPUs are happily spinning.
Maybe, this particular race is acceptable. But is it the only race?
My feeling:
It took me quite some time to make mental mode of the code.
Then I had to do many spins with various variants to find
the above race.
And I am not sure if this is the only one. My brain is hurting.
Update after several days:
I thought about this many days. (Partly because there was a weekend
and I was busy with other things). I was not sure if I wanted to avoid
these races because of my intuition (might be a good reason) or because
I wanted to do it my way (would be a bad reason).
And I do not feel comfortable with this patch even after many days.
The stealing and the many continue/restart paths create to many
combinations. It might be perfectly fine if everything works.
But it will create many complications when there is a problem.
And the question is not only whether the lock work as expected.
The problem is that the stealing also makes the result less
predictable. It would add more possible "races" also to
the try_acquire() callers.
IMHO, the semantic of this lock would be much easier when it
guarantees that higher priority request will block any lower
priority one. And that only one EMERGENCY context will wait
and get the lock.
So, I would prefer to make this more straightforward:
1. nbcon_context_try_acquire_direct() should take the lock only
when the lock is free and there is not pending request
with the same or higher priority.
if (cur->req_prio >= ctxt->prio)
return -EPERM;
2. nbcon_context_try_acquire_handover() might duplicate some code
with _direct() variant. But it should guarantee that there will
be only one waiter a straightforward way.
I would suggest something like this for the waiting loop:
err = -EBUSY;
for (timeout = ctxt->spinwait_max_us; timeout >= 0; timeout--) {
/*
* We are here because the lock is not free.
*
* In theory, the following race might happen during udelay().
* The lock was taken and released by a higher priority
* context. Then the lock was taken with a lower priority
* context and another CPU with our priority started handover.
* As a result, two CPUs might be cycling now.
*
* It would not be a big deal. Only one CPU
* would succeed with taking over the lock. The other
* would bail out.
*
* But in practice, this could not happen because
* messages with NORMAL prio are serialized either by
* console_lock() or by the kthread. All messages with
* PANIC priority are handled on panic CPU. And panic
* context overrides any other context and the
* priority never goes down.
*
* An ultimate solution would be adding req_cpu to
* check that someone else starter spinning on another
* CPU. But it is not worth it as described above.
*/
udelay(1); or cpu_relax() ?
/* Re-read the state because some time has passed. */
nbcon_state_read(con, cur);
/* See below how this is implemented. */
err = nbcon_context_try_acquire_request(ctxt, cur);
while (err = -EBUSY);
/* Was it timeout? */
if (err != -EBUSY)
return err;
/* Timed out. Carefully remove the request. */
do {
new.atom = cur->atom;
new.req_prio = NBCON_PRIO_NONE;
if (nbcon_state_try_cmpxchg(con, cur, &new)) {
debug_store(1, "handover: cpu%d TIMEOUT to acquire for prio%d\n",
cpu, ctxt->prio);
/*
* Request successfully unset. Report failure of
* acquiring via handover.
*/
cur->atom = new.atom;
return -EBUSY;
}
/*
* Something has changed. Maybe the lock is free or
* taken over by a higher priority task. Or maybe
* just some other meta data has changed.
*
* We would get the lock when it is free and the
* request is still ours. And we will get -EBUSY
* when the request is still ours.
*/
err = nbcon_context_try_acquire_request(ctxt, cur);
while (err = -EBUSY);
return err;
}
, where
/*
* Try to take the lock when it is reserved for us. It is called for
* a re-read @cur state.
*
* Returns:
*
* 0: successfully acquired the lock.
*
* -EPERM: The console has been taken by a context with a higher
* priority. Or a panic is in progress an this is not
* the panic CPU. Waiting does not make sense any longer.
*
* -EBUSY: The lock is still blocked and reserved for us. It makes
* sense to continue waiting until the timeout.
*/
static int nbcon_context_try_acquire_request(struct nbcon_context *ctxt,
struct nbcon_state *cur)
{
struct nbcon_state new;
if (other_cpu_in_panic())
return -EPERM;
/* A higher priority context took over the request/lock */
if (cur->req_prio != ctxt->prio)
return -EPERM;
/* Request is ours. But is the lock still owned? */
if (cur->prio != NBCON_PRIO_NONE)
return -EBUSY;
/* The lock is available and is reserved for us. Try to get it. */
new.atom = cur->atom;
new.cpu = smp_processor_id();
new.prio = ctxt->prio;
new.req_prio = 0;
new.unsafe = cur->hostile_unsafe | ctxt->unsafe;
if (!nbcon_state_try_cmpxchg(con, cur, &new))
return 0;
/*
* Someone else manipulated the state. But the lock was free
* and blocked for us. Only a higher priority context was
* allowed to take over it.
*/
return -EPERM;
}
Just for comparison. The _direct() variant might look like:
/*
* Try to get the lock when it is free not requested by another
* context with the same or higher priority.
*
* Returns:
*
* 0: successfully acquired the lock.
*
* -EPERM: The console is already owned by a context with
* the same or higher priority. Or a panic is in
* progress an this is not the panic CPU. Any waiting
* does not make sense in this case.
*
* -EBUSY: The console already has an owner with a lower
* The caller might try handover.
*
*/
static int nbcon_context_try_acquire_directly(struct nbcon_context *ctxt,
struct nbcon_state *cur)
{
struct nbcon_state new;
try_again:
if (other_cpu_in_panic())
return -EPERM;
/* Could override only request from a lower priority context. */
if (ctxt->prio <= cur->req_prio)
return -EPERM;
if (cur->prio != NBCON_PRIO_NONE)
return -EBUSY;
/* The lock is available and we are free to take it. */
new.atom = cur->atom;
new.cpu = smp_processor_id();
new.prio = ctxt->prio;
/* Clear request from a lower priority context. */
new.req_prio = 0;
new.unsafe = cur->hostile_unsafe | ctxt->unsafe;
/*
* The state might have changed when anyone else took the lock.
* Check again if we could busy wait or give up.
*/
if (nbcon_state_try_cmpxchg(con, cur, &new))
goto try_again;
return 0;
}
So, there is some code duplication but it is clear what is going on.
And there are only two error codes.
> + }
> +
> + /* Timeout. Safely remove handover request and report failure. */
> + do {
> + /* No need to remove request if there is another waiter. */
> + if (cur->req_prio != ctxt->prio) {
> + if (err == -EPERM)
> + return err;
> + return -EBUSY;
> + }
> +
> + /* Unset request for handover. */
> + new.atom = cur->atom;
> + new.req_prio = NBCON_PRIO_NONE;
> + if (nbcon_state_try_cmpxchg(con, cur, &new)) {
> + debug_store(1, "handover: cpu%d TIMEOUT to acquire for prio%d\n",
> + cpu, ctxt->prio);
> + /*
> + * Request successfully unset. Report failure of
> + * acquiring via handover.
> + */
> + cur->atom = new.atom;
> + return -EBUSY;
> + }
> +
> + /*
> + * Unable to remove request. Try direct acquire. If
> + * successful, this request is also cleared.
> + */
> + err = nbcon_context_try_acquire_direct(ctxt, cur);
> + } while (err);
> +
> + /* Direct acquire at timeout succeeded! */
> + return 0;
> +}
> +
> +/**
> + * nbcon_context_try_acquire_hostile - Try to acquire via hostile takeover
> + * @ctxt: The context of the caller
> + * @cur: The current console state
> + *
> + * Return: 0 on success and @cur is updated to the new console state.
> + * Otherwise an error code on failure.
> + *
> + * Errors:
> + *
> + * -EPERM: A panic is in progress an this is not the panic CPU
> + * or this context is not the single panic context.
> + * Retrying the acquire will not be immediately useful.
> + *
> + * -EBUSY: The current owner is in an unsafe region and
> + * @takeover_unsafe was not set. Retrying the acquire
> + * is only immediately useful if @takeover_unsafe is
> + * set.
> + *
> + * -EAGAIN: An unexpected state change occurred. @cur has been
> + * updated. The caller should retry the acquire.
> + *
> + * The general procedure is to set @prio (forcing ownership). This method
> + * must only be used as a final attempt during panic.
> + */
> +static int nbcon_context_try_acquire_hostile(struct nbcon_context *ctxt,
> + struct nbcon_state *cur)
> +{
> + unsigned int cpu = smp_processor_id();
> + struct console *con = ctxt->console;
> + struct nbcon_state new = {
> + .cpu = cpu,
> + .prio = ctxt->prio,
> + .unsafe = cur->hostile_unsafe | ctxt->unsafe,
> + .hostile_unsafe = cur->hostile_unsafe | cur->unsafe,
We do hostile only when it is unsafe. So, I would expect:
.unsafe = true;
.hostile_unsafe = true;
> + };
> +
> + if (other_cpu_in_panic())
> + return -EPERM;
> +
> + /* Hostile takeovers must only be in the single panic context! */
> + if (WARN_ON_ONCE(ctxt->prio != NBCON_PRIO_PANIC || cur->prio == NBCON_PRIO_PANIC))
> + return -EPERM;
I wondered if we actually might want to take over the lock from
a nested PANIC context on the panic() CPU.
For example, when panic() was in normal process context
and NMI came when flushing a message to the console.
Should the NMI context take over the lock a harsh way?
But it actually does not make sense. The harsh takeover
is allowed only in the final flush_in_panic() which is
called from the same context as the panic() function.
> +
> + /*
> + * If a hostile takeover in unsafe regions is wanted,
> + * this is additionally requested.
> + */
> + if (cur->unsafe && !ctxt->takeover_unsafe)
> + return -EBUSY;
I would return -EPERM because we simply can't take the lock
in this context now.
> + if (!nbcon_state_try_cmpxchg(con, cur, &new))
> + return -EAGAIN;
Do we really need to start from the very beginning?
If try_cmpxchg fails it means that a non-panic CPU
still owns the lock and is manipulating the state.
I would just repeat the cmpxchg inside this function.
I would just revisit the values of the .unsafe and
.takeover_unsafe flags. The forced takeover might
be safe now.
> +
> + cur->atom = new.atom;
> +
> + return 0;
> +}
> +
> +/**
> + * nbcon_context_try_acquire - Try to acquire nbcon console
> + * @ctxt: The context of the caller
> + *
> + * Return: True if the console was acquired. False otherwise.
> + *
> + * The attempts to acquire always begin with the safest methods and only
> + * upon failure move to more aggressive methods.
> + *
> + * If the caller requested a hostile takeover, on success @ctxt is
> + * updated so that the caller can see if indeed a hostile (and
> + * possibly unsafe) takeover occurred.
> + */
> +__maybe_unused
> +static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
> +{
> + __maybe_unused
> + unsigned int cpu = smp_processor_id();
> + struct console *con = ctxt->console;
> + struct nbcon_state cur;
> + bool hostile;
> + int err;
> +
> + /* @takeover_unsafe is only valid when hostile takeover requested. */
> + WARN_ON_ONCE(ctxt->takeover_unsafe && !ctxt->hostile);
> +
> + nbcon_state_read(con, &cur);
> +
> +try_again:
> + hostile = false;
> +
> + /* ACQUIRE DIRECT */
> +
> + debug_store(ctxt->prio > NBCON_PRIO_NORMAL,
> + "direct: cpu%d trying DIRECT acquire prio%d\n", cpu, ctxt->prio);
> + err = nbcon_context_try_acquire_direct(ctxt, &cur);
> + if (!err) {
> + debug_store(ctxt->prio > NBCON_PRIO_NORMAL,
> + "direct: cpu%d SUCCESS prio%d\n", cpu, ctxt->prio);
> + goto success;
> + } else if (err == -EPERM) {
> + debug_store(1, "direct: cpu%d FAILED prio%d (non-panic cpu)\n",
> + cpu, ctxt->prio);
> + return false;
> + } else if (err == -EAGAIN) {
> + debug_store(1, "direct: cpu%d FAILED prio%d (state race)\n",
> + cpu, ctxt->prio);
> + goto try_again;
> + } else {
> + debug_store(1, "direct: cpu%d FAILED prio%d (already owned by cpu%d)\n",
> + cpu, ctxt->prio, cur.cpu);
> + /* Continue to next method. */
> + }
Hmm, the many debug_store() calls are a bit disturbing here.
Is the custom message really importnat? Would be enough to do?:
debug_store(ctxt->prio > NBCON_PRIO_NORMAL,
"direct: cpu%d trying DIRECT acquire prio%d\n", cpu, ctxt->prio);
err = nbcon_context_try_acquire_direct(ctxt, &cur);
debug_store(ctxt->prio > NBCON_PRIO_NORMAL,
"direct: cpu%d DIRECT acquire prio%d returned %pe\n", cpu, ctxt->prio, ERR_PTR(err));
if (!err)
goto success;
if (err == -EPERM)
return -false;
WARN_ON_ONCE(err != -EBUSY);
/* ACQUIRE VIA HANDOVER */
...
> + debug_store(1, "handover: cpu%d REQUESTING prio%d from cpu%d\n",
> + cpu, ctxt->prio, cur.cpu);
> + err = nbcon_context_try_acquire_handover(ctxt, &cur);
> + if (!err) {
> + debug_store(1, "handover: cpu%d SUCCESS prio%d\n",
> + cpu, ctxt->prio);
> + goto success;
> + } else if (err == -EPERM) {
> + debug_store(1, "handover: cpu%d FAILED requesting prio%d (non-panic cpu)\n",
> + cpu, ctxt->prio);
> + return false;
> + } else if (err == -EAGAIN) {
> + debug_store(1, "handover: cpu%d FAILED requesting prio%d (state race)\n",
> + cpu, ctxt->prio);
> + goto try_again;
> + } else {
> + debug_store(1, "handover: cpu%d FAILED requesting prio%d (cpu%d/prio%d/timeout)\n",
> + cpu, ctxt->prio, cur.cpu, cur.prio);
> + /* Continue to next method. */
> + }
> +
> + /* ACQUIRE VIA HOSTILE TAKEOVER */
> +
> + /* Only attempt hostile takeover if explicitly requested. */
> + if (!ctxt->hostile)
> + return false;
> +
> + debug_store(1,
> + "hostile: cpu%d trying HOSTILE acquire for prio%d from cpu%d (takeover_unsafe=%d)\n",
> + cpu, ctxt->prio, cur.cpu, ctxt->takeover_unsafe);
> + err = nbcon_context_try_acquire_hostile(ctxt, &cur);
> + if (!err) {
> + debug_store(1, "hostile: cpu%d SUCCESS prio%d\n", cpu, ctxt->prio);
> + /* Let caller know if the takeover was unsafe. */
> + ctxt->takeover_unsafe = cur.hostile_unsafe;
> + hostile = true;
> + goto success;
> + } else if (err == -EPERM) {
> + debug_store(1, "hostile: cpu%d FAILED acquire prio%d (non-panic cpu)\n",
> + cpu, ctxt->prio);
> + return false;
> + } else if (err == -EAGAIN) {
> + debug_store(1, "hostile: cpu%d FAILED acquire prio%d (state race)\n",
> + cpu, ctxt->prio);
> + goto try_again;
> + } else {
> + debug_store(1, "hostile: cpu%d FAILED acquire prio%d (unsafe)\n",
> + cpu, ctxt->prio);
> + /* Continue to next method. */
> + }
> +
> + /* No methods left to try. */
> + return false;
> +success:
> + if (!hostile) {
> + /* Let caller know it was not a hostile takeover. */
> + ctxt->hostile = 0;
> + ctxt->takeover_unsafe = 0;
nbcon_context_try_acquire_direct() sets new.unsafe when
cur->hostile_unsafe is set.
My understanding is that any acquire is unsafe when there
was an unsafe hostile takeover in the past. IMHO, ctxt should
be aware of this and we should not blindly clear these variables.
> + }
> + return true;
> +}
> +/**
> + * nbcon_context_release - Release the console
> + * @ctxt: The nbcon context from nbcon_context_try_acquire()
> + */
> +__maybe_unused
> +static void nbcon_context_release(struct nbcon_context *ctxt)
> +{
> + unsigned int cpu = smp_processor_id();
> + struct console *con = ctxt->console;
> + struct nbcon_state cur;
> + struct nbcon_state new;
> +
> + nbcon_state_read(con, &cur);
> + do {
> + if (!nbcon_owner_matches(&cur, cpu, ctxt->prio))
> + return;
> +
> + new.atom = cur.atom;
> + new.prio = NBCON_PRIO_NONE;
> + new.unsafe = cur.hostile_unsafe;
> +
> + /*
> + * If @hostile_unsafe is set, it is kept set so that
> + * the state remains permanently unsafe.
> + */
This comment should be above the new.unsafe assignment.
> +
> + } while (!nbcon_state_try_cmpxchg(con, &cur, &new));
> +}
> +
> /**
> * nbcon_init - Initialize the nbcon console specific data
> * @con: Console to initialize
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic
2023-08-09 10:20 ` Petr Mladek
@ 2023-08-29 9:43 ` John Ogness
0 siblings, 0 replies; 37+ messages in thread
From: John Ogness @ 2023-08-29 9:43 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2023-08-09, Petr Mladek <pmladek@suse.com> wrote:
> My proposal is a kind of inverted logic. It allows only limited number
> of scenarios. It reduces backward loops. And it is more clear what
> is possible in which part of the code.
I agree with your suggestions for simplification. Thanks.
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* hostile takeover: Re: [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic
2023-07-28 0:02 ` [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic John Ogness
2023-08-09 10:20 ` Petr Mladek
@ 2023-08-09 12:44 ` Petr Mladek
2023-08-29 10:22 ` John Ogness
1 sibling, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-08-09 12:44 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
I have actually forgot one thing.
On Fri 2023-07-28 02:08:28, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Add per console acquire/release functionality. The console 'locked'
> state is a combination of multiple state fields:
>
> - Hostile takeover
>
> The new owner takes the console over without 'req_prio'
> handshake.
>
> This is required when friendly handovers are not possible,
> i.e. the higher priority context interrupted the owning
> context on the same CPU or the owning context is not able
> to make progress on a remote CPU.
I always expected that there would be only one hostile takeover.
It would allow to take the lock a harsh way when the friendly
way fails.
> All other policy decisions have to be made at the call sites:
>
> - What is marked as an unsafe section.
> - Whether to spinwait if there is already an owner.
> - Whether to attempt a hostile takeover when safe.
> - Whether to attempt a hostile takeover when unsafe.
But there seems to be actually two variants. How they are
supposed to be used, please?
I would expect that a higher priority context would always
be able to takeover the lock when it is in a safe context.
By other words, "hostile takeover when safe" would be
the standard behavior for context with a higher priority.
And if I get it correctly then nbcon_enter_unsafe() returns
"false" when there is a pending request with a higher priority.
And only requests with a higher priority are allowed.
By other words, the difference between normal takeover and
"hostile takeover when safe" is that the 1st one has to
wait until the current owner calls nbcon_enter_unsafe().
But the result is the same. The current owner might
prematurely end after calling nbcon_enter_unsafe().
Maybe, this another relic from the initial more generic approach?
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -175,13 +175,28 @@ enum cons_flags {
> * struct nbcon_state - console state for nbcon consoles
> * @atom: Compound of the state fields for atomic operations
> *
> + * @req_prio: The priority of a handover request
> + * @prio: The priority of the current usage
> + * @unsafe: Console is busy in a non takeover region
> + * @hostile_unsafe: The @unsafe value before a hostile takeover
> + * @cpu: The CPU on which the owner runs
> + *
> * To be used for reading and preparing of the value stored in the nbcon
> * state variable @console.nbcon_state.
> + *
> + * The @prio and @req_prio fields are particularly important to allow
> + * spin-waiting to timeout and give up without the risk of a waiter being
> + * assigned the lock after giving up.
> */
> struct nbcon_state {
> union {
> unsigned int atom;
> struct {
> + unsigned int prio : 2;
> + unsigned int req_prio : 2;
> + unsigned int unsafe : 1;
> + unsigned int hostile_unsafe : 1;
> + unsigned int cpu : 24;
> };
> };
> };
> @@ -194,6 +209,50 @@ struct nbcon_state {
> */
> static_assert(sizeof(struct nbcon_state) <= sizeof(int));
>
> +/**
> + * nbcon_prio - console owner priority for nbcon consoles
> + * @NBCON_PRIO_NONE: Unused
> + * @NBCON_PRIO_NORMAL: Normal (non-emergency) usage
> + * @NBCON_PRIO_EMERGENCY: Emergency output (WARN/OOPS...)
> + * @NBCON_PRIO_PANIC: Panic output
> + * @NBCON_PRIO_MAX: The number of priority levels
> + *
> + * A context wanting to produce emergency output can carefully takeover the
> + * console, even without consent of the owner. Ideally such a takeover is only
> + * when @nbcon_state::unsafe is not set. However, a context wanting to produce
> + * panic output can ignore the unsafe flag as a last resort. If panic output
> + * is active, no takeover is possible until the panic output releases the
> + * console.
> + */
> +enum nbcon_prio {
> + NBCON_PRIO_NONE = 0,
> + NBCON_PRIO_NORMAL,
> + NBCON_PRIO_EMERGENCY,
> + NBCON_PRIO_PANIC,
> + NBCON_PRIO_MAX,
> +};
> +
> +struct console;
> +
> +/**
> + * struct nbcon_context - Context for console acquire/release
> + * @console: The associated console
> + * @spinwait_max_us: Limit for spinwait acquire
> + * @prio: Priority of the context
> + * @unsafe: This context is in an unsafe section
This seems to be an input value for try_acquire(). It is
controversial.
I guess that it removes the need to call nbcon_enter_unsafe()
right after try_acquire_console().
Hmm, this semantic is problematic:
1. The result would be non-paired enter_unsafe()/exit_unsafe()
calls.
2. I would personally expect that this is an output value
set by try_acquire() so that the context might know
how it got the lock.
3. Strictly speaking, as an input value, it would mean that
try_acquire() is called when the console is in "unsafe" state.
But the caller does not know in which state the console
is until it acquires the lock.
> + * @hostile: Acquire console by hostile takeover
> + * @takeover_unsafe: Acquire console by hostile takeover even if unsafe
> + */
> +struct nbcon_context {
> + /* members set by caller */
> + struct console *console;
> + unsigned int spinwait_max_us;
> + enum nbcon_prio prio;
> + unsigned int unsafe : 1;
> + unsigned int hostile : 1;
> + unsigned int takeover_unsafe : 1;
> +};
The names make sense. But there are 4 names used struct nbcon_state
and struct nbcon_context. One is used twice:
state.unsafe
state.hostile_unsafe
ctxt.unsafe
ctxt.hostile
ctxt.takeover_unsafe
For me it is not easy to remember which permutation is used where ;-)
It would be easier if we remove the "hostile when safe" variant.
Then 3 variables might be enough. I suggest something like:
state.unsafe Console is busy and takeover is not safe.
state.unsafe_takeover A hostile takeover in an unsafe state happened
in the past. The console can't be safe until
re-initialized.
ctxt.allow_unsafe_takeover Allow hostile takeover even if unsafe.
Can be used only with PANIC prio. Might cause
a system freeze when the console is used later.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: hostile takeover: Re: [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic
2023-08-09 12:44 ` hostile takeover: " Petr Mladek
@ 2023-08-29 10:22 ` John Ogness
2023-09-07 15:40 ` Petr Mladek
0 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-08-29 10:22 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2023-08-09, Petr Mladek <pmladek@suse.com> wrote:
>> Add per console acquire/release functionality. The console 'locked'
>> state is a combination of multiple state fields:
>>
>> - Hostile takeover
>>
>> The new owner takes the console over without 'req_prio'
>> handshake.
>>
>> This is required when friendly handovers are not possible,
>> i.e. the higher priority context interrupted the owning
>> context on the same CPU or the owning context is not able
>> to make progress on a remote CPU.
>
> I always expected that there would be only one hostile takeover.
> It would allow to take the lock a harsh way when the friendly
> way fails.
You are referring to the hostile takeover when unsafe. A hostile
takeover when safe is still considered a hostile takeover. (At least,
until now that is how it was considered. More below...)
>> All other policy decisions have to be made at the call sites:
>>
>> - What is marked as an unsafe section.
>> - Whether to spinwait if there is already an owner.
>> - Whether to attempt a hostile takeover when safe.
>> - Whether to attempt a hostile takeover when unsafe.
>
> But there seems to be actually two variants. How they are
> supposed to be used, please?
>
> I would expect that a higher priority context would always
> be able to takeover the lock when it is in a safe context.
>
> By other words, "hostile takeover when safe" would be
> the standard behavior for context with a higher priority.
The difference is that with "hostile takeover when safe" there is a
foreign CPU that still thinks it has the lock, even though it does not.
> By other words, the difference between normal takeover and
> "hostile takeover when safe" is that the 1st one has to
> wait until the current owner calls nbcon_enter_unsafe().
> But the result is the same. The current owner might
> prematurely end after calling nbcon_enter_unsafe().
>
> Maybe, this another relic from the initial more generic approach?
I suppose so. But then why not try the "hostile takeover when safe"
first and only use the handover request as a fallback? Like this:
1. try direct
2. try hostile takeover when safe
3. try handover
4. try hostile takeover when unsafe
Then we should remove the "hostile" label for #2 and just call it:
1. try direct
2. try safe takeover
3. try handover
4. try hostile takeover
(I guess this is how you imagined things should be.)
>> +/**
>> + * struct nbcon_context - Context for console acquire/release
>> + * @console: The associated console
>> + * @spinwait_max_us: Limit for spinwait acquire
>> + * @prio: Priority of the context
>> + * @unsafe: This context is in an unsafe section
>
> This seems to be an input value for try_acquire(). It is
> controversial.
>
> I guess that it removes the need to call nbcon_enter_unsafe()
> right after try_acquire_console().
Yes. It removes the "safe window" between try_acquire_console() and
nbcon_enter_unsafe() by allowing to acquire and mark unsafe
atomically. For example, this would be useful for the port_lock()
wrapper we discussed. But it is not strictly necessary. I can remove it.
Below I answer your comments anyway.
> Hmm, this semantic is problematic:
>
> 1. The result would be non-paired enter_unsafe()/exit_unsafe()
> calls.
The paired calls are either:
try_acquire(unsafe=1) -> release()
or
try_acquire(unsafe=0) + enter_unsafe() -> exit_unsafe() + release()
> 2. I would personally expect that this is an output value
> set by try_acquire() so that the context might know
> how it got the lock.
The state structure is used to communicate this information.
> 3. Strictly speaking, as an input value, it would mean that
> try_acquire() is called when the console is in "unsafe" state.
> But the caller does not know in which state the console
> is until it acquires the lock.
This is not about the current state. The calling _context_ wants to set
a certain state. The caller wants to acquire the lock and atomically
mark this as the beginning of an unsafe section. But as I wrote, this is
not strictly necessary. Only an efficient convenience.
> For me it is not easy to remember which permutation is used where ;-)
> It would be easier if we remove the "hostile when safe" variant.
> Then 3 variables might be enough. I suggest something like:
>
> state.unsafe Console is busy and takeover is not safe.
>
> state.unsafe_takeover A hostile takeover in an unsafe state happened
> in the past. The console can't be safe until
> re-initialized.
>
> ctxt.allow_unsafe_takeover Allow hostile takeover even if unsafe.
> Can be used only with PANIC prio. Might cause
> a system freeze when the console is used later.
Since "safe takeovers" should be handled equivalent to "handovers" then
I agree with your suggestion.
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: hostile takeover: Re: [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic
2023-08-29 10:22 ` John Ogness
@ 2023-09-07 15:40 ` Petr Mladek
2023-09-08 14:48 ` John Ogness
0 siblings, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-09-07 15:40 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Tue 2023-08-29 12:28:10, John Ogness wrote:
> On 2023-08-09, Petr Mladek <pmladek@suse.com> wrote:
> >> Add per console acquire/release functionality. The console 'locked'
> >> state is a combination of multiple state fields:
> >>
> >> - Hostile takeover
> >>
> >> The new owner takes the console over without 'req_prio'
> >> handshake.
> >>
> >> This is required when friendly handovers are not possible,
> >> i.e. the higher priority context interrupted the owning
> >> context on the same CPU or the owning context is not able
> >> to make progress on a remote CPU.
> >
> > I always expected that there would be only one hostile takeover.
> > It would allow to take the lock a harsh way when the friendly
> > way fails.
>
> You are referring to the hostile takeover when unsafe. A hostile
> takeover when safe is still considered a hostile takeover. (At least,
> until now that is how it was considered. More below...)
Yes.
Note: I did not distinguish much the "takeover" and "handover"
words when thinking about the logic. They sound almost
the same to me. I think that I have some partial dyslexia[*]
or so.
Also I had the words "hostile" and "harsh" connected with
the "takeover when unsafe".
As a result, I made a simplified mental model where
"handover" and "takeover when safe" were equivalent.
I am sorry if it caused some confusion. I see
the difference now.
[*] This might be the reason why I am so sensitive on naming.
> >> All other policy decisions have to be made at the call sites:
> >>
> >> - What is marked as an unsafe section.
> >> - Whether to spinwait if there is already an owner.
> >> - Whether to attempt a hostile takeover when safe.
> >> - Whether to attempt a hostile takeover when unsafe.
> >
> > But there seems to be actually two variants. How they are
> > supposed to be used, please?
> >
> > I would expect that a higher priority context would always
> > be able to takeover the lock when it is in a safe context.
> >
> > By other words, "hostile takeover when safe" would be
> > the standard behavior for context with a higher priority.
>
> The difference is that with "hostile takeover when safe" there is a
> foreign CPU that still thinks it has the lock, even though it does not.
Strictly speaking, the foreign CPU should not expect that it owns
the lock when it is in a safe region. The "safe" means that
the hostile takeover is safe. And the new owner could use
the console without any risk.
My understanding was:
+ The lock allows to emit one record atomically.
+ The "safe region under the lock" marks code when the console
is in a consistent state and any synchronized resources
are not accessed.
+ Only a higher priority context might take over the lock
in the "safe region". Then it might use the console device
a safe way. And it is supposed to re-emit the interrupted
record.
I think that the catch is in the "synchronized resources".
It is not only the console device but also the printk buffer.
I though that we needed the extra printk buffer in panic only
because of the harsh takeover when unsafe when the message is being
read and formatted.
And I though that the console drivers would handle characters
one by one. Then it might be safe to read one character
even in "safe" state.
Hmm, I am actually not sure if nbcon_emit_next_record()
calls printk_get_next_message() and con->write_atomic(con, wctxt)
in "safe" or "unsafe" context.
I mean, I am not sure if the access to the printk buffer
is synchronized by the console lock or not. It would be nice
to document it and maybe add some assert_context_is_safe()/unsafe().
> > By other words, the difference between normal takeover and
> > "hostile takeover when safe" is that the 1st one has to
> > wait until the current owner calls nbcon_enter_unsafe().
> > But the result is the same. The current owner might
> > prematurely end after calling nbcon_enter_unsafe().
OK, there is one difference:
+ The "handover" passes the lock on well defined locations.
+ The "takeover when safe" can happen anywhere in the "safe"
region.
And the "takeover when safe" is currently allowed only in
console_flush_on_panic(). It means that the lock is
passed to a higher priority context on well defined
locations most of the time. Which is a more conservative approach.
> > Maybe, this another relic from the initial more generic approach?
>
> I suppose so. But then why not try the "hostile takeover when safe"
> first and only use the handover request as a fallback? Like this:
>
> 1. try direct
> 2. try hostile takeover when safe
> 3. try handover
> 4. try hostile takeover when unsafe
>
> Then we should remove the "hostile" label for #2 and just call it:
>
> 1. try direct
> 2. try safe takeover
> 3. try handover
> 4. try hostile takeover
I rather meant:
1. try direct
2. try handover +
try safe takeover in every waiting cycle
3. try hostile takeover
But then it won't be a handover anymore.
Anyway, I would keep it as is for now. As mentioned above,
the current handover is more conservative approach because
the lock is passed on well defined locations.
> (I guess this is how you imagined things should be.)
>
> >> +/**
> >> + * struct nbcon_context - Context for console acquire/release
> >> + * @console: The associated console
> >> + * @spinwait_max_us: Limit for spinwait acquire
> >> + * @prio: Priority of the context
> >> + * @unsafe: This context is in an unsafe section
> >
> > This seems to be an input value for try_acquire(). It is
> > controversial.
> >
> > I guess that it removes the need to call nbcon_enter_unsafe()
> > right after try_acquire_console().
>
> Yes. It removes the "safe window" between try_acquire_console() and
> nbcon_enter_unsafe() by allowing to acquire and mark unsafe
> atomically. For example, this would be useful for the port_lock()
> wrapper we discussed. But it is not strictly necessary. I can remove it.
>
> Below I answer your comments anyway.
>
> > Hmm, this semantic is problematic:
> >
> > 1. The result would be non-paired enter_unsafe()/exit_unsafe()
> > calls.
>
> The paired calls are either:
>
> try_acquire(unsafe=1) -> release()
We should not create such an API. It would be confusing. And it would
create strange code.
People, should be allowed enter/exit unsafe inside the lock.
If the lock sets unsafe by default then the code would
look like:
try_acquire(unsafe=1)
#do something synchronized
exit_unsafe()
# do something innocent
enter_unsafe()
#do something synchronized again
release()
In this case, it would make more sense to implement the
opposite enter_safe() and exit_safe().
But this would be ugly. It would create the felling that
the code in this section might do anything because it is
safe. But it is actually the other way around.
The code in the "safe" section must be very careful because
it must be safe for takeover.
> or
>
> try_acquire(unsafe=0) + enter_unsafe() -> exit_unsafe() + release()
I am not sure what is the difference between "+" and "->"
Anyway, IMHO, try_acquire() should always return with unsafe=0
(except for unsafe_takeover).
It would allow use enter_unsafe()/exit_unsafe() around
the sensitive code. They both will be in the same function
and "enter" will be before "exit".
BTW: The name enter_unsafe() somehow looks more acceptable than
the reverted enter_safe(). It makes the feeling that we
do something unsafe in this section which is actually true.
I though about a better name then "unsafe" but I did
not found any.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: hostile takeover: Re: [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic
2023-09-07 15:40 ` Petr Mladek
@ 2023-09-08 14:48 ` John Ogness
0 siblings, 0 replies; 37+ messages in thread
From: John Ogness @ 2023-09-08 14:48 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2023-09-07, Petr Mladek <pmladek@suse.com> wrote:
> Hmm, I am actually not sure if nbcon_emit_next_record()
> calls printk_get_next_message() and con->write_atomic(con, wctxt)
> in "safe" or "unsafe" context.
It does not! In the early versions of this code it was not a problem
because we had a per-priority buffer for each console. But now we only
have one buffer that is shared by the NORMAL and EMERGENCY priorities
per console. For v4 I am creating a safe region around
printk_get_next_message() and console_prepend_dropped(). That will fix
the issue. Nice catch!
For the callbacks write_atomic() (and later, write_thread()) it should
not be called in unsafe. It is up to the drivers to decide what is safe
and unsafe.
>> 1. try direct
>> 2. try safe takeover
>> 3. try handover
>> 4. try hostile takeover
>
> I rather meant:
>
> 1. try direct
> 2. try handover +
> try safe takeover in every waiting cycle
> 3. try hostile takeover
>
> But then it won't be a handover anymore.
>
> Anyway, I would keep it as is for now. As mentioned above,
> the current handover is more conservative approach because
> the lock is passed on well defined locations.
For v3 I made the change as I suggested above. So it will perform a
direct takeover if the priority is higher and it is safe. Yes, waiting
first might seem more polite and conservative. But if the driver is in a
safe state, I see no reason to make a higher priority context wait. If
it would be a problem for the first context to suddenly lose ownership,
then it should be marking it an unsafe region.
For fun I implemented it such that it only directly takes over an owner
after having waited. But it adds quite a bit of ugliness to the routine
and I don't think it is worth it.
For v4 I will keep it the same as v3: a direct takeover of an existing
owner when safe before trying the handover.
John Ogness
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH printk v2 4/8] printk: nbcon: Add buffer management
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
` (2 preceding siblings ...)
2023-07-28 0:02 ` [PATCH printk v2 3/8] printk: nbcon: Add acquire/release logic John Ogness
@ 2023-07-28 0:02 ` John Ogness
2023-08-09 14:06 ` Petr Mladek
2023-07-28 0:02 ` [PATCH printk v2 5/8] printk: nbcon: Add sequence handling John Ogness
` (4 subsequent siblings)
8 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-07-28 0:02 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
From: Thomas Gleixner <tglx@linutronix.de>
In case of hostile takeovers it must be ensured that the previous
owner cannot scribble over the output buffer of the emergency/panic
context. This is achieved by:
- Adding a global output buffer instance for the panic context.
This is the only situation where hostile takeovers can occur and
there is always at most 1 panic context.
- Allocating an output buffer per console upon console
registration. This buffer is used by the console owner when not
in panic context.
- Choosing the appropriate buffer is handled in the acquire/release
functions.
Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
include/linux/console.h | 7 +++++++
kernel/printk/internal.h | 6 ++++++
kernel/printk/printk.c | 6 ------
kernel/printk/printk_nbcon.c | 26 ++++++++++++++++++++++++--
4 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index e06cd1ce3e82..d2bcd2c190a7 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -233,6 +233,7 @@ enum nbcon_prio {
};
struct console;
+struct printk_buffers;
/**
* struct nbcon_context - Context for console acquire/release
@@ -242,6 +243,7 @@ struct console;
* @unsafe: This context is in an unsafe section
* @hostile: Acquire console by hostile takeover
* @takeover_unsafe: Acquire console by hostile takeover even if unsafe
+ * @pbufs: Pointer to the text buffer for this context
*/
struct nbcon_context {
/* members set by caller */
@@ -251,6 +253,9 @@ struct nbcon_context {
unsigned int unsafe : 1;
unsigned int hostile : 1;
unsigned int takeover_unsafe : 1;
+
+ /* members set by acquire */
+ struct printk_buffers *pbufs;
};
/**
@@ -274,6 +279,7 @@ struct nbcon_context {
* @node: hlist node for the console list
*
* @nbcon_state: State for nbcon consoles
+ * @pbufs: Pointer to nbcon private buffer
*/
struct console {
char name[16];
@@ -296,6 +302,7 @@ struct console {
/* nbcon console specific members */
atomic_t __private nbcon_state;
+ struct printk_buffers *pbufs;
};
#ifdef CONFIG_LOCKDEP
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 655810f2976e..0f2be350600e 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -13,6 +13,12 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
#define printk_sysctl_init() do { } while (0)
#endif
+#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__)
+
#ifdef CONFIG_PRINTK
#ifdef CONFIG_PRINTK_CALLER
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 98b4854c81ea..582552a96c57 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3359,12 +3359,6 @@ static void try_enable_default_console(struct console *newcon)
newcon->flags |= CON_CONSDEV;
}
-#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)
{
struct console *con;
diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
index b0acde0cb949..39fa64891ec6 100644
--- a/kernel/printk/printk_nbcon.c
+++ b/kernel/printk/printk_nbcon.c
@@ -5,6 +5,7 @@
#include <linux/kernel.h>
#include <linux/console.h>
#include <linux/delay.h>
+#include <linux/slab.h>
#include "internal.h"
/*
* Printk console printing implementation for consoles that do not depend on
@@ -20,6 +21,9 @@
* region and aborts the operation if it detects a takeover.
*
* In case of panic the nesting context can take over the console forcefully.
+ * If the interrupted context touches the assigned record buffer after
+ * takeover, it does not cause harm because the interrupting single panic
+ * context is assigned its own panic record buffer.
*
* A concurrent writer on a different CPU with a higher priority can request
* to take over the console by:
@@ -356,6 +360,8 @@ static int nbcon_context_try_acquire_hostile(struct nbcon_context *ctxt,
return 0;
}
+static struct printk_buffers panic_nbcon_pbufs;
+
/**
* nbcon_context_try_acquire - Try to acquire nbcon console
* @ctxt: The context of the caller
@@ -372,7 +378,6 @@ static int nbcon_context_try_acquire_hostile(struct nbcon_context *ctxt,
__maybe_unused
static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
{
- __maybe_unused
unsigned int cpu = smp_processor_id();
struct console *con = ctxt->console;
struct nbcon_state cur;
@@ -471,6 +476,13 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
ctxt->hostile = 0;
ctxt->takeover_unsafe = 0;
}
+
+ /* Assign the appropriate buffer for this context. */
+ if (atomic_read(&panic_cpu) == cpu)
+ ctxt->pbufs = &panic_nbcon_pbufs;
+ else
+ ctxt->pbufs = con->pbufs;
+
return true;
}
@@ -509,7 +521,7 @@ static void nbcon_context_release(struct nbcon_context *ctxt)
nbcon_state_read(con, &cur);
do {
if (!nbcon_owner_matches(&cur, cpu, ctxt->prio))
- return;
+ break;
new.atom = cur.atom;
new.prio = NBCON_PRIO_NONE;
@@ -521,6 +533,8 @@ static void nbcon_context_release(struct nbcon_context *ctxt)
*/
} while (!nbcon_state_try_cmpxchg(con, &cur, &new));
+
+ ctxt->pbufs = NULL;
}
/**
@@ -534,6 +548,12 @@ bool nbcon_init(struct console *con)
{
struct nbcon_state state = { };
+ con->pbufs = kmalloc(sizeof(*con->pbufs), GFP_KERNEL);
+ if (!con->pbufs) {
+ con_printk(KERN_ERR, con, "failed to allocate printing buffer\n");
+ return false;
+ }
+
nbcon_state_set(con, &state);
return true;
}
@@ -547,4 +567,6 @@ void nbcon_cleanup(struct console *con)
struct nbcon_state state = { };
nbcon_state_set(con, &state);
+ kfree(con->pbufs);
+ con->pbufs = NULL;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 4/8] printk: nbcon: Add buffer management
2023-07-28 0:02 ` [PATCH printk v2 4/8] printk: nbcon: Add buffer management John Ogness
@ 2023-08-09 14:06 ` Petr Mladek
0 siblings, 0 replies; 37+ messages in thread
From: Petr Mladek @ 2023-08-09 14:06 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-07-28 02:08:29, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> In case of hostile takeovers it must be ensured that the previous
> owner cannot scribble over the output buffer of the emergency/panic
> context. This is achieved by:
>
> - Adding a global output buffer instance for the panic context.
> This is the only situation where hostile takeovers can occur and
> there is always at most 1 panic context.
>
> - Allocating an output buffer per console upon console
> registration. This buffer is used by the console owner when not
> in panic context.
>
> - Choosing the appropriate buffer is handled in the acquire/release
> functions.
>
> Co-developed-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
Looks good to me:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH printk v2 5/8] printk: nbcon: Add sequence handling
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
` (3 preceding siblings ...)
2023-07-28 0:02 ` [PATCH printk v2 4/8] printk: nbcon: Add buffer management John Ogness
@ 2023-07-28 0:02 ` John Ogness
2023-07-28 1:33 ` kernel test robot
2023-08-10 9:28 ` Petr Mladek
2023-07-28 0:02 ` [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions John Ogness
` (3 subsequent siblings)
8 siblings, 2 replies; 37+ messages in thread
From: John Ogness @ 2023-07-28 0:02 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
From: Thomas Gleixner <tglx@linutronix.de>
Add an atomic_long_t field @nbcon_seq to the console struct to
store the sequence number for nbcon consoles. For nbcon consoles
this will be used instead of the non-atomic @seq field. The new
field allows for safe atomic sequence number updates without
requiring any locking.
On 64bit systems the new field stores the full sequence number.
On 32bit systems the new field stores the lower 32 bits of the
sequence number, which are expanded to 64bit as needed by
folding the values based on the sequence numbers available in
the ringbuffer.
For 32bit systems, having a 32bit representation in the console
is sufficient. If a console ever gets more than 2^31 records
behind the ringbuffer then this is the least of the problems.
Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
include/linux/console.h | 4 ++
kernel/printk/internal.h | 6 ++
kernel/printk/printk.c | 39 +++++++++---
kernel/printk/printk_nbcon.c | 114 +++++++++++++++++++++++++++++++++++
4 files changed, 153 insertions(+), 10 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index d2bcd2c190a7..a50eaa3b8420 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -244,6 +244,7 @@ struct printk_buffers;
* @hostile: Acquire console by hostile takeover
* @takeover_unsafe: Acquire console by hostile takeover even if unsafe
* @pbufs: Pointer to the text buffer for this context
+ * @seq: The sequence number to print for this context
*/
struct nbcon_context {
/* members set by caller */
@@ -256,6 +257,7 @@ struct nbcon_context {
/* members set by acquire */
struct printk_buffers *pbufs;
+ u64 seq;
};
/**
@@ -279,6 +281,7 @@ struct nbcon_context {
* @node: hlist node for the console list
*
* @nbcon_state: State for nbcon consoles
+ * @nbcon_seq: Sequence number of the next record for nbcon to print
* @pbufs: Pointer to nbcon private buffer
*/
struct console {
@@ -302,6 +305,7 @@ struct console {
/* nbcon console specific members */
atomic_t __private nbcon_state;
+ atomic_long_t __private nbcon_seq;
struct printk_buffers *pbufs;
};
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 0f2be350600e..1baccddf83b8 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -4,6 +4,7 @@
*/
#include <linux/percpu.h>
#include <linux/console.h>
+#include "printk_ringbuffer.h"
#if defined(CONFIG_PRINTK) && defined(CONFIG_SYSCTL)
void __init printk_sysctl_init(void);
@@ -42,6 +43,7 @@ enum printk_info_flags {
LOG_CONT = 8, /* text is a fragment of a continuation line */
};
+extern struct printk_ringbuffer *prb;
extern bool have_legacy_console;
extern bool have_boot_console;
@@ -72,6 +74,8 @@ void defer_console_output(void);
u16 printk_parse_prefix(const char *text, int *level,
enum printk_info_flags *flags);
+u64 nbcon_seq_read(struct console *con);
+void nbcon_seq_force(struct console *con, u64 seq);
bool nbcon_init(struct console *con);
void nbcon_cleanup(struct console *con);
@@ -90,6 +94,8 @@ void nbcon_cleanup(struct console *con);
#define printk_safe_exit_irqrestore(flags) local_irq_restore(flags)
static inline bool printk_percpu_data_ready(void) { return false; }
+static inline u64 nbcon_seq_read(struct console *con) { return 0; }
+static inline void nbcon_seq_force(struct console *con, u64 seq) { }
static inline bool nbcon_init(struct console *con) { return true; }
static inline void nbcon_cleanup(struct console *con) { }
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 582552a96c57..64e404aacbc4 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -512,7 +512,7 @@ _DEFINE_PRINTKRB(printk_rb_static, CONFIG_LOG_BUF_SHIFT - PRB_AVGBITS,
static struct printk_ringbuffer printk_rb_dynamic;
-static struct printk_ringbuffer *prb = &printk_rb_static;
+struct printk_ringbuffer *prb = &printk_rb_static;
/*
* We cannot access per-CPU data (e.g. per-CPU flush irq_work) before
@@ -3171,8 +3171,27 @@ void console_unblank(void)
*/
void console_flush_on_panic(enum con_flush_mode mode)
{
+ struct console *c;
bool handover;
- u64 next_seq;
+ short flags;
+ int cookie;
+ u64 seq;
+
+ seq = prb_first_valid_seq(prb);
+
+ /*
+ * Safely handle the atomic consoles before trying to flush any
+ * legacy consoles.
+ */
+ if (mode == CONSOLE_REPLAY_ALL) {
+ cookie = console_srcu_read_lock();
+ for_each_console_srcu(c) {
+ flags = console_srcu_read_flags(c);
+ if (flags & CON_NBCON)
+ nbcon_seq_force(c, seq);
+ }
+ console_srcu_read_unlock(cookie);
+ }
if (!serialized_printing)
return;
@@ -3195,12 +3214,6 @@ void console_flush_on_panic(enum con_flush_mode mode)
console_may_schedule = 0;
if (mode == CONSOLE_REPLAY_ALL) {
- struct console *c;
- int cookie;
- u64 seq;
-
- seq = prb_first_valid_seq(prb);
-
cookie = console_srcu_read_lock();
for_each_console_srcu(c) {
/*
@@ -3212,7 +3225,7 @@ void console_flush_on_panic(enum con_flush_mode mode)
console_srcu_read_unlock(cookie);
}
- console_flush_all(false, &next_seq, &handover);
+ console_flush_all(false, &seq, &handover);
}
/*
@@ -3795,6 +3808,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;
+ short flags;
bool locked;
int cookie;
u64 diff;
@@ -3821,6 +3835,9 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
for_each_console_srcu(c) {
if (con && con != c)
continue;
+
+ flags = console_srcu_read_flags(c);
+
/*
* If consoles are not usable, it cannot be expected
* that they make forward progress, so only increment
@@ -3829,7 +3846,9 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
if (!console_is_usable(c))
continue;
- if (locked)
+ if (flags & CON_NBCON)
+ printk_seq = nbcon_seq_read(c);
+ else if (locked)
printk_seq = c->seq;
else
continue;
diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
index 39fa64891ec6..8229a0a00d5b 100644
--- a/kernel/printk/printk_nbcon.c
+++ b/kernel/printk/printk_nbcon.c
@@ -108,6 +108,116 @@ static inline bool nbcon_state_try_cmpxchg(struct console *con, struct nbcon_sta
return atomic_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_state), &cur->atom, new->atom);
}
+/* Convert sequence from u64 to unsigned long. */
+static inline unsigned long __nbcon_seq_to_stored(u64 full_seq)
+{
+ /* On 32bit systems only the lower 32 bits are stored. */
+ return (unsigned long)full_seq;
+}
+
+/* Convert sequence from unsigned long to u64. */
+static inline u64 __nbcon_seq_to_full(unsigned long stored_seq)
+{
+#ifdef CONFIG_64BIT
+ return stored_seq;
+#else
+ u64 full_seq;
+ u64 rb_seq;
+
+ /*
+ * The provided sequence is only the lower 32 bits of the ringbuffer
+ * sequence. It needs to be expanded to 64bit. Get the next sequence
+ * number from the ringbuffer and fold it.
+ */
+ rb_seq = prb_next_seq(prb);
+ full_seq = rb_seq - ((u32)rb_seq - stored_seq);
+
+ return full_seq;
+#endif
+}
+
+/**
+ * nbcon_seq_init - Helper function to initialize the console sequence
+ * @con: Console to work on
+ *
+ * Set @con->nbcon_seq to the starting record (specified with con->seq).
+ * If the starting record no longer exists, the oldest available record
+ * is chosen. This is because on 32bit systems only the lower 32 bits of
+ * the sequence number are stored. The upper 32 bits are derived from the
+ * sequence numbers available in the ringbuffer.
+ *
+ * For init only. Do not use for runtime updates.
+ */
+static void nbcon_seq_init(struct console *con)
+{
+ u64 seq = max_t(u64, con->seq, prb_first_valid_seq(prb));
+
+ atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), __nbcon_seq_to_stored(seq));
+
+ /* Clear con->seq since nbcon consoles use con->nbcon_seq instead. */
+ con->seq = 0;
+}
+
+/**
+ * nbcon_read_seq - Read the current console sequence
+ * @con: Console to read the sequence of
+ *
+ * Return: Sequence number of the next record to print on @con.
+ */
+u64 nbcon_seq_read(struct console *con)
+{
+ unsigned long seq = atomic_long_read(&ACCESS_PRIVATE(con, nbcon_seq));
+
+ return __nbcon_seq_to_full(seq);
+}
+
+/**
+ * nbcon_seq_force - Force console sequence to a specific value
+ * @con: Console to work on
+ *
+ * Only to be used in extreme situations (such as panic with
+ * CONSOLE_REPLAY_ALL).
+ */
+void nbcon_seq_force(struct console *con, u64 seq)
+{
+ atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), __nbcon_seq_to_stored(seq));
+}
+
+/**
+ * nbcon_seq_try_update - Try to update the console sequence number
+ * @ctxt: Pointer to an acquire context that contains
+ * all information about the acquire mode
+ *
+ * Return: True if the console sequence was updated, false otherwise.
+ *
+ * On 32bit the sequence in con->nbcon_seq is only the lower 32 bits.
+ * Therefore it must be expanded to 64bit upon a failed cmpxchg in
+ * order to correctly verify that the new sequence (ctxt->seq) is
+ * larger.
+ *
+ * In case of fail the console has been likely taken over. However, the
+ * caller must still assume it has ownership and decide how to proceed.
+ */
+__maybe_unused
+static bool nbcon_seq_try_update(struct nbcon_context *ctxt)
+{
+ struct console *con = ctxt->console;
+ u64 con_seq = nbcon_seq_read(con);
+
+ while (con_seq < ctxt->seq) {
+ unsigned long seq = __nbcon_seq_to_stored(con_seq);
+
+ if (atomic_long_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_seq), &seq,
+ __nbcon_seq_to_stored(ctxt->seq))) {
+ return true;
+ }
+
+ /* Expand new @seq value for comparing. */
+ con_seq = __nbcon_seq_to_full(seq);
+ }
+ return false;
+}
+
/**
* nbcon_context_try_acquire_direct - Try to acquire directly
* @ctxt: The context of the caller
@@ -483,6 +593,9 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
else
ctxt->pbufs = con->pbufs;
+ /* Set the record sequence for this context to print. */
+ ctxt->seq = nbcon_seq_read(ctxt->console);
+
return true;
}
@@ -554,6 +667,7 @@ bool nbcon_init(struct console *con)
return false;
}
+ nbcon_seq_init(con);
nbcon_state_set(con, &state);
return true;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 5/8] printk: nbcon: Add sequence handling
2023-07-28 0:02 ` [PATCH printk v2 5/8] printk: nbcon: Add sequence handling John Ogness
@ 2023-07-28 1:33 ` kernel test robot
2023-07-28 9:00 ` John Ogness
2023-08-10 9:28 ` Petr Mladek
1 sibling, 1 reply; 37+ messages in thread
From: kernel test robot @ 2023-07-28 1:33 UTC (permalink / raw)
To: John Ogness, Petr Mladek
Cc: oe-kbuild-all, Sergey Senozhatsky, Steven Rostedt,
Thomas Gleixner, linux-kernel, Greg Kroah-Hartman
Hi John,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 132a90d1527fedba2d95085c951ccf00dbbebe41]
url: https://github.com/intel-lab-lkp/linux/commits/John-Ogness/printk-Add-non-BKL-nbcon-console-basic-infrastructure/20230728-080545
base: 132a90d1527fedba2d95085c951ccf00dbbebe41
patch link: https://lore.kernel.org/r/20230728000233.50887-6-john.ogness%40linutronix.de
patch subject: [PATCH printk v2 5/8] printk: nbcon: Add sequence handling
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20230728/202307280939.4IPb76dh-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230728/202307280939.4IPb76dh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307280939.4IPb76dh-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> kernel/printk/printk_nbcon.c:168: warning: expecting prototype for nbcon_read_seq(). Prototype was for nbcon_seq_read() instead
kernel/printk/printk_nbcon.c:182: warning: Function parameter or member 'seq' not described in 'nbcon_seq_force'
vim +168 kernel/printk/printk_nbcon.c
160
161 /**
162 * nbcon_read_seq - Read the current console sequence
163 * @con: Console to read the sequence of
164 *
165 * Return: Sequence number of the next record to print on @con.
166 */
167 u64 nbcon_seq_read(struct console *con)
> 168 {
169 unsigned long seq = atomic_long_read(&ACCESS_PRIVATE(con, nbcon_seq));
170
171 return __nbcon_seq_to_full(seq);
172 }
173
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 5/8] printk: nbcon: Add sequence handling
2023-07-28 1:33 ` kernel test robot
@ 2023-07-28 9:00 ` John Ogness
0 siblings, 0 replies; 37+ messages in thread
From: John Ogness @ 2023-07-28 9:00 UTC (permalink / raw)
To: kernel test robot, Petr Mladek
Cc: oe-kbuild-all, Sergey Senozhatsky, Steven Rostedt,
Thomas Gleixner, linux-kernel, Greg Kroah-Hartman
On 2023-07-28, kernel test robot <lkp@intel.com> wrote:
>>> kernel/printk/printk_nbcon.c:168: warning: expecting prototype for nbcon_read_seq(). Prototype was for nbcon_seq_read() instead
> kernel/printk/printk_nbcon.c:182: warning: Function parameter or member 'seq' not described in 'nbcon_seq_force'
Thanks kernel test robot for spotting the 2 kerneldoc errors. I will fix
them for v3.
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH printk v2 5/8] printk: nbcon: Add sequence handling
2023-07-28 0:02 ` [PATCH printk v2 5/8] printk: nbcon: Add sequence handling John Ogness
2023-07-28 1:33 ` kernel test robot
@ 2023-08-10 9:28 ` Petr Mladek
2023-08-29 11:51 ` John Ogness
1 sibling, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-08-10 9:28 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-07-28 02:08:30, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Add an atomic_long_t field @nbcon_seq to the console struct to
> store the sequence number for nbcon consoles. For nbcon consoles
> this will be used instead of the non-atomic @seq field. The new
> field allows for safe atomic sequence number updates without
> requiring any locking.
>
> On 64bit systems the new field stores the full sequence number.
> On 32bit systems the new field stores the lower 32 bits of the
> sequence number, which are expanded to 64bit as needed by
> folding the values based on the sequence numbers available in
> the ringbuffer.
>
> For 32bit systems, having a 32bit representation in the console
> is sufficient. If a console ever gets more than 2^31 records
> behind the ringbuffer then this is the least of the problems.
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3171,8 +3171,27 @@ void console_unblank(void)
> */
> void console_flush_on_panic(enum con_flush_mode mode)
> {
> + struct console *c;
> bool handover;
> - u64 next_seq;
> + short flags;
> + int cookie;
> + u64 seq;
> +
> + seq = prb_first_valid_seq(prb);
> +
> + /*
> + * Safely handle the atomic consoles before trying to flush any
> + * legacy consoles.
> + */
This is a bit weird because the loop below just sets sequence
number for NBCON consoles. But they are not really flushed.
I think that you already agreed with it for v3. But let me mention
it here for completeness.
I would prefer to just add the API and use it later when some
particular action get supported. And the flush could not do
anything until nbcon_write() callback is added.
As is it is now, this patch adds nbcon_read()/write() into random
locations and it is not clear how they will be used and if
it is enough.
That said, it makes sense to update the init() path.
> + if (mode == CONSOLE_REPLAY_ALL) {
> + cookie = console_srcu_read_lock();
> + for_each_console_srcu(c) {
> + flags = console_srcu_read_flags(c);
> + if (flags & CON_NBCON)
> + nbcon_seq_force(c, seq);
> + }
> + console_srcu_read_unlock(cookie);
> + }
> if (!serialized_printing)
> return;
[...]
> @@ -3829,7 +3846,9 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
> if (!console_is_usable(c))
> continue;
>
> - if (locked)
> + if (flags & CON_NBCON)
> + printk_seq = nbcon_seq_read(c);
> + else if (locked)
> printk_seq = c->seq;
> else
> continue;
I think that I mentioned this already in a previous patch. The "else
continue" path is bad. It allows quietly skip messages for classic
consoles when "locked" is false. I know that it should not happen
but...
A solution would be to add WARN_ON_ONCE() before the continue.
> diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
> index 39fa64891ec6..8229a0a00d5b 100644
> --- a/kernel/printk/printk_nbcon.c
> +++ b/kernel/printk/printk_nbcon.c
> @@ -108,6 +108,116 @@ static inline bool nbcon_state_try_cmpxchg(struct console *con, struct nbcon_sta
> return atomic_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_state), &cur->atom, new->atom);
> }
>
> +/* Convert sequence from u64 to unsigned long. */
> +static inline unsigned long __nbcon_seq_to_stored(u64 full_seq)
> +{
> + /* On 32bit systems only the lower 32 bits are stored. */
> + return (unsigned long)full_seq;
> +}
> +
> +/* Convert sequence from unsigned long to u64. */
> +static inline u64 __nbcon_seq_to_full(unsigned long stored_seq)
> +{
> +#ifdef CONFIG_64BIT
> + return stored_seq;
> +#else
> + u64 full_seq;
> + u64 rb_seq;
> +
> + /*
> + * The provided sequence is only the lower 32 bits of the ringbuffer
> + * sequence. It needs to be expanded to 64bit. Get the next sequence
> + * number from the ringbuffer and fold it.
> + */
> + rb_seq = prb_next_seq(prb);
> + full_seq = rb_seq - ((u32)rb_seq - stored_seq);
> +
> + return full_seq;
> +#endif
> +}
I would personally do it this way:
#ifdef CONFIG_64BIT
$define __seq_to_nbcon_seq(seq) seq
$define __nbcon_seq_to_seq(seq) seq
#else /* CONFIG_64BIT */
$define __seq_to_nbcon_seq(seq) ((u32)seq)
static inline u64 __nbcon_seq_to_seq(u32 nbcon_seq)
{
u64 seq;
u64 rb_next_seq;
/*
* The provided sequence is only the lower 32 bits of the ringbuffer
* sequence. It needs to be expanded to 64bit. Get the next sequence
* number from the ringbuffer and fold it.
*
* Having a 32bit representation in the console is sufficient.
* If a console ever gets more than 2^31 records behind
* the ringbuffer then this is the least of the problems.
*
* Also the access to the ring buffer is always safe.
*/
rb_next_seq = prb_next_seq(prb);
seq = rb_next_seq - ((u32)rb_next_seq - nbcon_seq);
return seq;
}
#endif /* CONFIG_64BIT */
It looks more clear to me.
> +
> +/**
> + * nbcon_seq_init - Helper function to initialize the console sequence
> + * @con: Console to work on
> + *
> + * Set @con->nbcon_seq to the starting record (specified with con->seq).
> + * If the starting record no longer exists, the oldest available record
> + * is chosen. This is because on 32bit systems only the lower 32 bits of
> + * the sequence number are stored. The upper 32 bits are derived from the
> + * sequence numbers available in the ringbuffer.
It makes sense even on 64-bit systems. I would do:
s/This is because on 32bit systems/This is especially important on 32bit systems/
> + *
> + * For init only. Do not use for runtime updates.
> + */
> +static void nbcon_seq_init(struct console *con)
> +{
> + u64 seq = max_t(u64, con->seq, prb_first_valid_seq(prb));
> +
> + atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), __nbcon_seq_to_stored(seq));
> +
> + /* Clear con->seq since nbcon consoles use con->nbcon_seq instead. */
> + con->seq = 0;
> +}
[...]
> +/**
> + * nbcon_seq_try_update - Try to update the console sequence number
> + * @ctxt: Pointer to an acquire context that contains
> + * all information about the acquire mode
> + *
> + * Return: True if the console sequence was updated, false otherwise.
> + *
> + * On 32bit the sequence in con->nbcon_seq is only the lower 32 bits.
> + * Therefore it must be expanded to 64bit upon a failed cmpxchg in
> + * order to correctly verify that the new sequence (ctxt->seq) is
> + * larger.
> + *
> + * In case of fail the console has been likely taken over. However, the
> + * caller must still assume it has ownership and decide how to proceed.
> + */
> +__maybe_unused
> +static bool nbcon_seq_try_update(struct nbcon_context *ctxt)
> +{
> + struct console *con = ctxt->console;
> + u64 con_seq = nbcon_seq_read(con);
> +
> + while (con_seq < ctxt->seq) {
What if anyone called nbcon_seq_force() to reply the entire log
in the meantime?
IMHO, we should remember the original nbcon_seq before
the context handle a line. And this function should update
nbcon_seq only when it has not been changed by other context
in the meantime.
> + unsigned long seq = __nbcon_seq_to_stored(con_seq);
> +
> + if (atomic_long_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_seq), &seq,
> + __nbcon_seq_to_stored(ctxt->seq))) {
> + return true;
> + }
> +
> + /* Expand new @seq value for comparing. */
> + con_seq = __nbcon_seq_to_full(seq);
> + }
> + return false;
> +}
> +
> /**
> * nbcon_context_try_acquire_direct - Try to acquire directly
> * @ctxt: The context of the caller
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 5/8] printk: nbcon: Add sequence handling
2023-08-10 9:28 ` Petr Mladek
@ 2023-08-29 11:51 ` John Ogness
0 siblings, 0 replies; 37+ messages in thread
From: John Ogness @ 2023-08-29 11:51 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2023-08-10, Petr Mladek <pmladek@suse.com> wrote:
>> void console_flush_on_panic(enum con_flush_mode mode)
>> {
>> + struct console *c;
>> bool handover;
>> - u64 next_seq;
>> + short flags;
>> + int cookie;
>> + u64 seq;
>> +
>> + seq = prb_first_valid_seq(prb);
>> +
>> + /*
>> + * Safely handle the atomic consoles before trying to flush any
>> + * legacy consoles.
>> + */
>
> This is a bit weird because the loop below just sets sequence
> number for NBCON consoles. But they are not really flushed.
>
> I think that you already agreed with it for v3. But let me mention
> it here for completeness.
>
> I would prefer to just add the API and use it later when some
> particular action get supported. And the flush could not do
> anything until nbcon_write() callback is added.
Actually I agreed with this for v2, but didn't take it far enough. I now
also agree that console_flush_on_panic() should not be modified at all
until the actual flushing functions are available.
>> @@ -3829,7 +3846,9 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
>> if (!console_is_usable(c))
>> continue;
>>
>> - if (locked)
>> + if (flags & CON_NBCON)
>> + printk_seq = nbcon_seq_read(c);
>> + else if (locked)
>> printk_seq = c->seq;
>> else
>> continue;
>
> I think that I mentioned this already in a previous patch. The "else
> continue" path is bad. It allows quietly skip messages for classic
> consoles when "locked" is false. I know that it should not happen
> but...
>
> A solution would be to add WARN_ON_ONCE() before the continue.
I like the WARN_ON_ONCE() suggestion. For v3 I will do:
if (flags & CON_NBCON) {
printk_seq = nbcon_seq_read(c);
} else {
WARN_ON_ONCE(!locked);
printk_seq = c->seq;
}
>> +static bool nbcon_seq_try_update(struct nbcon_context *ctxt)
>> +{
>> + struct console *con = ctxt->console;
>> + u64 con_seq = nbcon_seq_read(con);
>> +
>> + while (con_seq < ctxt->seq) {
>
> What if anyone called nbcon_seq_force() to reply the entire log
> in the meantime?
>
> IMHO, we should remember the original nbcon_seq before
> the context handle a line. And this function should update
> nbcon_seq only when it has not been changed by other context
> in the meantime.
Makes sense. I will do that for v3.
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
` (4 preceding siblings ...)
2023-07-28 0:02 ` [PATCH printk v2 5/8] printk: nbcon: Add sequence handling John Ogness
@ 2023-07-28 0:02 ` John Ogness
2023-08-10 12:56 ` Petr Mladek
2023-07-28 0:02 ` [PATCH printk v2 7/8] printk: nbcon: Add emit function and callback function for atomic printing John Ogness
` (2 subsequent siblings)
8 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-07-28 0:02 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
From: Thomas Gleixner <tglx@linutronix.de>
Provide functions that are related to the safe handover mechanism
and allow console drivers to dynamically specify unsafe regions:
- nbcon_context_can_proceed()
Invoked by a console owner to check whether a handover request
is pending or whether the console was taken over in a hostile
fashion. If a handover request is pending, this function will
also perform the handover, thus cancelling its own ownership.
- nbcon_context_update_unsafe()
Invoked by a console owner to denote that the driver is about
to enter or leave a critical region where a hostile take over
is unsafe. This function is also a cancellation point where
loss of ownership can occur.
The unsafe state is stored in the console state and allows a
new context to make informed decisions whether to attempt a
takeover of such a console. The unsafe state is also available
to the driver so that it can make informed decisions about the
required actions or take a special emergency path.
Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
kernel/printk/printk_nbcon.c | 113 ++++++++++++++++++++++++++++++++++-
1 file changed, 112 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
index 8229a0a00d5b..e41f2eff5ef6 100644
--- a/kernel/printk/printk_nbcon.c
+++ b/kernel/printk/printk_nbcon.c
@@ -623,7 +623,6 @@ static bool nbcon_owner_matches(struct nbcon_state *cur, int expected_cpu,
* nbcon_context_release - Release the console
* @ctxt: The nbcon context from nbcon_context_try_acquire()
*/
-__maybe_unused
static void nbcon_context_release(struct nbcon_context *ctxt)
{
unsigned int cpu = smp_processor_id();
@@ -650,6 +649,118 @@ static void nbcon_context_release(struct nbcon_context *ctxt)
ctxt->pbufs = NULL;
}
+/**
+ * nbcon_context_can_proceed - Check whether ownership can proceed
+ * @ctxt: The nbcon context from nbcon_context_try_acquire()
+ * @cur: The current console state
+ *
+ * Return: True if the state is correct. False if ownership was
+ * handed over or taken.
+ *
+ * Must be invoked after the record was dumped into the assigned buffer
+ * and at appropriate safe places in the driver.
+ *
+ * When this function returns false then the calling context is no longer
+ * the owner and is no longer allowed to go forward. In this case it must
+ * back out immediately and carefully. The buffer content is also no longer
+ * trusted since it no longer belongs to the calling context.
+ */
+static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_state *cur)
+{
+ unsigned int cpu = smp_processor_id();
+
+ /* Make sure this context is still the owner. */
+ if (!nbcon_owner_matches(cur, cpu, ctxt->prio)) {
+ debug_store(1, "handover: cpu%d DETECTED HOSTILE takeover\n", cpu);
+ return false;
+ }
+
+ /* The console owner can proceed if there is no waiter. */
+ if (cur->req_prio == NBCON_PRIO_NONE)
+ return true;
+
+ /*
+ * A console owner within an unsafe region is always allowed to
+ * proceed, even if there are waiters. It can perform a handover
+ * when exiting the unsafe region. Otherwise the waiter will
+ * need to perform an unsafe hostile takeover.
+ */
+ if (cur->unsafe) {
+ debug_store(cur->req_prio > cur->prio,
+ "handover: cpu%d IGNORING HANDOVER prio%d -> prio%d (unsafe)\n",
+ cpu, cur->prio, cur->req_prio);
+ return true;
+ }
+
+ /* Waiters always have higher priorities than owners. */
+ WARN_ON_ONCE(cur->req_prio <= cur->prio);
+
+ debug_store(1, "handover: cpu%d HANDING OVER prio%d -> prio%d\n",
+ cpu, cur->prio, cur->req_prio);
+
+
+ /*
+ * Having a safe point for take over and eventually a few
+ * duplicated characters or a full line is way better than a
+ * hostile takeover. Post processing can take care of the garbage.
+ * Release and hand over.
+ */
+ nbcon_context_release(ctxt);
+
+ /*
+ * It is not known whether the handover succeeded. The outermost
+ * callsite has to make the final decision whether printing
+ * should proceed or not (via reacquire, possibly hostile). The
+ * console is now unlocked so go back all the way instead of
+ * trying to implement heuristics in tons of places.
+ */
+ return false;
+}
+
+/**
+ * nbcon_context_update_unsafe - Update the unsafe bit in @con->nbcon_state
+ * @ctxt: The nbcon context from nbcon_context_try_acquire()
+ * @unsafe: The new value for the unsafe bit
+ *
+ * Return: True if the state is correct. False if ownership was
+ * handed over or taken.
+ *
+ * Typically the unsafe bit is set during acquire. This function allows
+ * modifying the unsafe status without releasing ownership.
+ *
+ * When this function returns false then the calling context is no longer
+ * the owner and is no longer allowed to go forward. In this case it must
+ * back out immediately and carefully. The buffer content is also no longer
+ * trusted since it no longer belongs to the calling context.
+ *
+ * Internal helper to avoid duplicated code
+ */
+__maybe_unused
+static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
+{
+ struct console *con = ctxt->console;
+ struct nbcon_state cur;
+ struct nbcon_state new;
+
+ nbcon_state_read(con, &cur);
+
+ /* The unsafe bit must not be cleared if @hostile_unsafe is set. */
+ if (!unsafe && cur.hostile_unsafe)
+ return nbcon_context_can_proceed(ctxt, &cur);
+
+ do {
+ if (!nbcon_context_can_proceed(ctxt, &cur))
+ return false;
+
+ new.atom = cur.atom;
+ new.unsafe = unsafe;
+ } while (!nbcon_state_try_cmpxchg(con, &cur, &new));
+
+ ctxt->unsafe = unsafe;
+
+ return true;
+}
+
/**
* nbcon_init - Initialize the nbcon console specific data
* @con: Console to initialize
--
2.39.2
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions
2023-07-28 0:02 ` [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions John Ogness
@ 2023-08-10 12:56 ` Petr Mladek
2023-08-29 12:23 ` John Ogness
0 siblings, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-08-10 12:56 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Fri 2023-07-28 02:08:31, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Provide functions that are related to the safe handover mechanism
> and allow console drivers to dynamically specify unsafe regions:
> --- a/kernel/printk/printk_nbcon.c
> +++ b/kernel/printk/printk_nbcon.c
> @@ -650,6 +649,118 @@ static void nbcon_context_release(struct nbcon_context *ctxt)
> ctxt->pbufs = NULL;
> }
>
> +/**
> + * nbcon_context_can_proceed - Check whether ownership can proceed
> + * @ctxt: The nbcon context from nbcon_context_try_acquire()
> + * @cur: The current console state
> + *
> + * Return: True if the state is correct. False if ownership was
> + * handed over or taken.
> + *
> + * Must be invoked after the record was dumped into the assigned buffer
> + * and at appropriate safe places in the driver.
> + *
> + * When this function returns false then the calling context is no longer
> + * the owner and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context.
> + */
> +static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_state *cur)
> +{
[...]
> + /*
> + * A console owner within an unsafe region is always allowed to
> + * proceed, even if there are waiters. It can perform a handover
> + * when exiting the unsafe region. Otherwise the waiter will
> + * need to perform an unsafe hostile takeover.
> + */
> + if (cur->unsafe) {
> + debug_store(cur->req_prio > cur->prio,
> + "handover: cpu%d IGNORING HANDOVER prio%d -> prio%d (unsafe)\n",
> + cpu, cur->prio, cur->req_prio);
> + return true;
> + }
[...]
> +}
> +
> +/**
> + * nbcon_context_update_unsafe - Update the unsafe bit in @con->nbcon_state
> + * @ctxt: The nbcon context from nbcon_context_try_acquire()
> + * @unsafe: The new value for the unsafe bit
> + *
> + * Return: True if the state is correct. False if ownership was
> + * handed over or taken.
> + *
> + * Typically the unsafe bit is set during acquire. This function allows
> + * modifying the unsafe status without releasing ownership.
> + *
> + * When this function returns false then the calling context is no longer
> + * the owner and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context.
> + *
> + * Internal helper to avoid duplicated code
> + */
> +__maybe_unused
> +static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
> +{
> + struct console *con = ctxt->console;
> + struct nbcon_state cur;
> + struct nbcon_state new;
> +
> + nbcon_state_read(con, &cur);
> +
> + /* The unsafe bit must not be cleared if @hostile_unsafe is set. */
> + if (!unsafe && cur.hostile_unsafe)
> + return nbcon_context_can_proceed(ctxt, &cur);
> +
> + do {
> + if (!nbcon_context_can_proceed(ctxt, &cur))
> + return false;
nbcon_context_can_proceed() returns "true" even when there
is a pending request. It happens when the current state is "unsafe".
But see below.
> +
> + new.atom = cur.atom;
> + new.unsafe = unsafe;
> + } while (!nbcon_state_try_cmpxchg(con, &cur, &new));
If the new state is "safe" and there is a pending request
then we should release the lock and return false here.
It does not make sense to block the waiter just to realize
that we can't enter "unsafe" state again.
> + ctxt->unsafe = unsafe;
> +
> + return true;
An easy solution would be to do here:
ctxt->unsafe = unsafe;
return nbcon_context_can_proceed(ctxt, &cur);
> +}
But maybe, we can change the logic a bit. Something like:
/**
* nbcon_context_update_unsafe - Update the unsafe bit in @con->nbcon_state
* @ctxt: The nbcon context from nbcon_context_try_acquire()
* @unsafe: The new value for the unsafe bit
*
* Return: True if the state is correct. False if ownership was
* handed over or taken.
*
* When this function returns false then the calling context is no longer
* the owner and is no longer allowed to go forward. In this case it must
* back out immediately and carefully. The buffer content is also no longer
* trusted since it no longer belongs to the calling context.
*
* Internal helper to avoid duplicated code
*/
static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
{
struct console *con = ctxt->console;
struct nbcon_state cur;
struct nbcon_state new;
bool updated, can_proceed;
if (!nbcon_context_can_proceed(ctxt, &cur))
return false;
/* The unsafe bit must not be cleared if @hostile_unsafe is set. */
if (cur.hostile_unsafe)
unsafe = true;
if (cur.unsafe == unsafe)
return true;
do {
new.atom = cur.atom;
new.unsafe = unsafe;
updated = nbcon_state_try_cmpxchg(con, &cur, &new));
/*
* The state has changed. Either there is a new
* request lor there was a hostile takeover.
*/
can_proceed = nbcon_context_can_proceed(ctxt, &cur);
} while (!updated && can_proceed);
if (updated)
ctxt->unsafe = unsafe;
return can_proceed;
}
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions
2023-08-10 12:56 ` Petr Mladek
@ 2023-08-29 12:23 ` John Ogness
0 siblings, 0 replies; 37+ messages in thread
From: John Ogness @ 2023-08-29 12:23 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On 2023-08-10, Petr Mladek <pmladek@suse.com> wrote:
>> +static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
>> +{
>> + struct console *con = ctxt->console;
>> + struct nbcon_state cur;
>> + struct nbcon_state new;
>> +
>> + nbcon_state_read(con, &cur);
>> +
>> + /* The unsafe bit must not be cleared if @hostile_unsafe is set. */
>> + if (!unsafe && cur.hostile_unsafe)
>> + return nbcon_context_can_proceed(ctxt, &cur);
>> +
>> + do {
>> + if (!nbcon_context_can_proceed(ctxt, &cur))
>> + return false;
>
> nbcon_context_can_proceed() returns "true" even when there
> is a pending request. It happens when the current state is "unsafe".
Correct.
>> +
>> + new.atom = cur.atom;
>> + new.unsafe = unsafe;
>> + } while (!nbcon_state_try_cmpxchg(con, &cur, &new));
>
> If the new state is "safe" and there is a pending request
> then we should release the lock and return false here.
The function does release the lock and return false... after it clears
state.unsafe.
> It does not make sense to block the waiter just to realize
> that we can't enter "unsafe" state again.
Sorry, I do not understand what you mean.
>> + ctxt->unsafe = unsafe;
>> +
>> + return true;
>
> An easy solution would be to do here:
>
> ctxt->unsafe = unsafe;
> return nbcon_context_can_proceed(ctxt, &cur);
ctxt->unsafe is an input. It does not need to be updated anyway. (And
besides, it will be removed for v3.)
> But maybe, we can change the logic a bit. Something like:
I think with ctxt->unsafe removed, this v2 implementation is ok.
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH printk v2 7/8] printk: nbcon: Add emit function and callback function for atomic printing
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
` (5 preceding siblings ...)
2023-07-28 0:02 ` [PATCH printk v2 6/8] printk: nbcon: Add ownership state functions John Ogness
@ 2023-07-28 0:02 ` John Ogness
2023-08-11 13:37 ` Petr Mladek
2023-07-28 0:02 ` [PATCH printk v2 8/8] printk: nbcon: Add functions for drivers to mark unsafe regions John Ogness
2023-08-11 13:56 ` [PATCH printk v2 0/8] wire up nbcon consoles Petr Mladek
8 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-07-28 0:02 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
From: Thomas Gleixner <tglx@linutronix.de>
Implement an emit function for nbcon consoles to output printk
messages. It utilizes the lockless printk_get_next_message() and
console_prepend_dropped() functions to retrieve/build the output
message. The emit function includes the required safety points to
check for handover/takeover and calls a new write_atomic callback
of the console driver to output the message. It also includes
proper handling for updating the nbcon console sequence number.
A new nbcon_write_context struct is introduced. This is provided
to the write_atomic callback and includes only the information
necessary for performing atomic writes.
Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
include/linux/console.h | 27 ++++++++
kernel/printk/internal.h | 6 ++
kernel/printk/printk.c | 9 +--
kernel/printk/printk_nbcon.c | 121 ++++++++++++++++++++++++++++++++++-
4 files changed, 156 insertions(+), 7 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index a50eaa3b8420..3d129b2b70a1 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -243,6 +243,7 @@ struct printk_buffers;
* @unsafe: This context is in an unsafe section
* @hostile: Acquire console by hostile takeover
* @takeover_unsafe: Acquire console by hostile takeover even if unsafe
+ * @backlog: Ringbuffer has pending records
* @pbufs: Pointer to the text buffer for this context
* @seq: The sequence number to print for this context
*/
@@ -255,11 +256,28 @@ struct nbcon_context {
unsigned int hostile : 1;
unsigned int takeover_unsafe : 1;
+ /* members set by emit */
+ unsigned int backlog : 1;
+
/* members set by acquire */
struct printk_buffers *pbufs;
u64 seq;
};
+/**
+ * struct nbcon_write_context - Context handed to the nbcon write callbacks
+ * @ctxt: The core console context
+ * @outbuf: Pointer to the text buffer for output
+ * @len: Length to write
+ * @hostile_unsafe: The @unsafe value before a hostile takeover
+ */
+struct nbcon_write_context {
+ struct nbcon_context __private ctxt;
+ char *outbuf;
+ unsigned int len;
+ bool hostile_unsafe;
+};
+
/**
* struct console - The console descriptor structure
* @name: The name of the console driver
@@ -280,6 +298,7 @@ struct nbcon_context {
* @data: Driver private data
* @node: hlist node for the console list
*
+ * @write_atomic: Write callback for atomic context
* @nbcon_state: State for nbcon consoles
* @nbcon_seq: Sequence number of the next record for nbcon to print
* @pbufs: Pointer to nbcon private buffer
@@ -304,6 +323,8 @@ struct console {
struct hlist_node node;
/* nbcon console specific members */
+ bool (*write_atomic)(struct console *con,
+ struct nbcon_write_context *wctxt);
atomic_t __private nbcon_state;
atomic_long_t __private nbcon_seq;
struct printk_buffers *pbufs;
@@ -433,6 +454,12 @@ static inline bool console_is_registered(const struct console *con)
lockdep_assert_console_list_lock_held(); \
hlist_for_each_entry(con, &console_list, node)
+#ifdef CONFIG_PRINTK
+extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
+#else
+static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
+#endif
+
extern int console_set_on_cmdline;
extern struct console *early_console;
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 1baccddf83b8..96dcb2cc05b1 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -128,3 +128,9 @@ struct printk_message {
};
bool other_cpu_in_panic(void);
+bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
+ bool is_extended, bool may_supress);
+
+#ifdef CONFIG_PRINTK
+void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped);
+#endif
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 64e404aacbc4..0124c433da9d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -716,9 +716,6 @@ static ssize_t msg_print_ext_body(char *buf, size_t size,
return len;
}
-static bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
- bool is_extended, bool may_supress);
-
/* /dev/kmsg - userspace message inject/listen interface */
struct devkmsg_user {
atomic64_t seq;
@@ -2751,7 +2748,7 @@ static void __console_unlock(void)
* If @pmsg->pbufs->outbuf is modified, @pmsg->outbuf_len is updated.
*/
#ifdef CONFIG_PRINTK
-static void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
+void console_prepend_dropped(struct printk_message *pmsg, unsigned long dropped)
{
struct printk_buffers *pbufs = pmsg->pbufs;
const size_t scratchbuf_sz = sizeof(pbufs->scratchbuf);
@@ -2805,8 +2802,8 @@ static void console_prepend_dropped(struct printk_message *pmsg, unsigned long d
* of @pmsg are valid. (See the documentation of struct printk_message
* for information about the @pmsg fields.)
*/
-static bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
- bool is_extended, bool may_suppress)
+bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
+ bool is_extended, bool may_suppress)
{
static int panic_console_dropped;
diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
index e41f2eff5ef6..e1f0e4278ffa 100644
--- a/kernel/printk/printk_nbcon.c
+++ b/kernel/printk/printk_nbcon.c
@@ -198,7 +198,6 @@ void nbcon_seq_force(struct console *con, u64 seq)
* In case of fail the console has been likely taken over. However, the
* caller must still assume it has ownership and decide how to proceed.
*/
-__maybe_unused
static bool nbcon_seq_try_update(struct nbcon_context *ctxt)
{
struct console *con = ctxt->console;
@@ -717,6 +716,33 @@ static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_s
return false;
}
+/**
+ * nbcon_can_proceed - Check whether ownership can proceed
+ * @wctxt: The write context that was handed to the write function
+ *
+ * Return: True if the state is correct. False if ownership was
+ * handed over or taken.
+ *
+ * Must be invoked after the record was dumped into the assigned buffer
+ * and at appropriate safe places in the driver.
+ *
+ * When this function returns false then the calling context is no longer
+ * the owner and is no longer allowed to go forward. In this case it must
+ * back out immediately and carefully. The buffer content is also no longer
+ * trusted since it no longer belongs to the calling context.
+ */
+bool nbcon_can_proceed(struct nbcon_write_context *wctxt)
+{
+ struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+ struct console *con = ctxt->console;
+ struct nbcon_state cur;
+
+ nbcon_state_read(con, &cur);
+
+ return nbcon_context_can_proceed(ctxt, &cur);
+}
+EXPORT_SYMBOL_GPL(nbcon_can_proceed);
+
/**
* nbcon_context_update_unsafe - Update the unsafe bit in @con->nbcon_state
* @ctxt: The nbcon context from nbcon_context_try_acquire()
@@ -761,6 +787,99 @@ static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
return true;
}
+/**
+ * nbcon_emit_next_record - Emit a record in the acquired context
+ * @wctxt: The write context that will be handed to the write function
+ *
+ * Return: True if the state is correct. False if ownership was
+ * handed over or taken.
+ *
+ * When this function returns false then the calling context is no longer
+ * the owner and is no longer allowed to go forward. In this case it must
+ * back out immediately and carefully. The buffer content is also no longer
+ * trusted since it no longer belongs to the calling context. If the caller
+ * wants to do more it must reacquire the console first.
+ *
+ * When true is returned, @wctxt->ctxt.backlog indicates whether there are
+ * still records pending in the ringbuffer,
+ */
+__maybe_unused
+static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
+{
+ struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+ struct console *con = ctxt->console;
+ bool is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
+ struct printk_message pmsg = {
+ .pbufs = ctxt->pbufs,
+ };
+ unsigned long con_dropped;
+ struct nbcon_state cur;
+ unsigned long dropped;
+ bool done;
+
+ ctxt->backlog = printk_get_next_message(&pmsg, ctxt->seq, is_extended, true);
+ if (!ctxt->backlog)
+ return true;
+
+ /*
+ * @con->dropped is not protected in case of hostile takeovers. In
+ * that situation the update can be racy so annotate it accordingly.
+ */
+ con_dropped = data_race(READ_ONCE(con->dropped));
+
+ dropped = con_dropped + pmsg.dropped;
+ if (dropped && !is_extended)
+ console_prepend_dropped(&pmsg, dropped);
+
+ /* Safety point. Do not touch state in case of takeover. */
+ nbcon_state_read(con, &cur);
+ if (!nbcon_context_can_proceed(ctxt, &cur))
+ return false;
+
+ /* For skipped records just update seq/dropped in @con. */
+ if (pmsg.outbuf_len == 0)
+ goto update_con;
+
+ /* Set the write context before calling write callback. */
+ wctxt->hostile_unsafe = cur.hostile_unsafe;
+ wctxt->len = pmsg.outbuf_len;
+ if (wctxt->len)
+ wctxt->outbuf = &pmsg.pbufs->outbuf[0];
+ else
+ wctxt->outbuf = NULL;
+
+ if (con->write_atomic) {
+ done = con->write_atomic(con, wctxt);
+ } else {
+ nbcon_context_release(ctxt);
+ WARN_ON_ONCE(1);
+ done = false;
+ }
+
+ /* If not done, the emit was aborted. */
+ if (!done)
+ return false;
+
+ /*
+ * Since any dropped message was successfully output, reset the
+ * dropped count for the console.
+ */
+ dropped = 0;
+update_con:
+ if (dropped != con_dropped) {
+ /* Counterpart to the READ_ONCE() above. */
+ WRITE_ONCE(con->dropped, dropped);
+ }
+
+ ctxt->seq = pmsg.seq + 1;
+ if (!nbcon_seq_try_update(ctxt)) {
+ /* This context probably lost ownership. Check. */
+ return nbcon_can_proceed(wctxt);
+ }
+
+ return true;
+}
+
/**
* nbcon_init - Initialize the nbcon console specific data
* @con: Console to initialize
--
2.39.2
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 7/8] printk: nbcon: Add emit function and callback function for atomic printing
2023-07-28 0:02 ` [PATCH printk v2 7/8] printk: nbcon: Add emit function and callback function for atomic printing John Ogness
@ 2023-08-11 13:37 ` Petr Mladek
2023-08-29 13:08 ` John Ogness
0 siblings, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-08-11 13:37 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-07-28 02:08:32, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Implement an emit function for nbcon consoles to output printk
> messages. It utilizes the lockless printk_get_next_message() and
> console_prepend_dropped() functions to retrieve/build the output
> message. The emit function includes the required safety points to
> check for handover/takeover and calls a new write_atomic callback
> of the console driver to output the message. It also includes
> proper handling for updating the nbcon console sequence number.
>
> A new nbcon_write_context struct is introduced. This is provided
> to the write_atomic callback and includes only the information
> necessary for performing atomic writes.
>
> --- a/kernel/printk/printk_nbcon.c
> +++ b/kernel/printk/printk_nbcon.c
> @@ -761,6 +787,99 @@ static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
> return true;
> }
>
> +/**
> + * nbcon_emit_next_record - Emit a record in the acquired context
> + * @wctxt: The write context that will be handed to the write function
> + *
> + * Return: True if the state is correct. False if ownership was
> + * handed over or taken.
The meaning of "True if the state is correct" is not clear. I would
write something like:
* Return: True if this context still owns the console. False
* if the ownership has been handed over or taken.
> + *
> + * When this function returns false then the calling context is no longer
> + * the owner and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context. If the caller
> + * wants to do more it must reacquire the console first.
> + *
> + * When true is returned, @wctxt->ctxt.backlog indicates whether there are
> + * still records pending in the ringbuffer,
> + */
> +__maybe_unused
> +static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
> +{
> + struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> + struct console *con = ctxt->console;
> + bool is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
> + struct printk_message pmsg = {
> + .pbufs = ctxt->pbufs,
> + };
> + unsigned long con_dropped;
> + struct nbcon_state cur;
> + unsigned long dropped;
> + bool done;
> +
> + ctxt->backlog = printk_get_next_message(&pmsg, ctxt->seq, is_extended, true);
> + if (!ctxt->backlog)
> + return true;
> +
> + /*
> + * @con->dropped is not protected in case of hostile takeovers. In
> + * that situation the update can be racy so annotate it accordingly.
> + */
> + con_dropped = data_race(READ_ONCE(con->dropped));
> +
> + dropped = con_dropped + pmsg.dropped;
> + if (dropped && !is_extended)
> + console_prepend_dropped(&pmsg, dropped);
> +
> + /* Safety point. Do not touch state in case of takeover. */
> + nbcon_state_read(con, &cur);
> + if (!nbcon_context_can_proceed(ctxt, &cur))
> + return false;
> +
> + /* For skipped records just update seq/dropped in @con. */
> + if (pmsg.outbuf_len == 0)
> + goto update_con;
> +
> + /* Set the write context before calling write callback. */
> + wctxt->hostile_unsafe = cur.hostile_unsafe;
Is there any reason why wctxt need to know about cur.hostile_usafe?
Will it use another console->*write*() callback or so?
Or will it do another decision based on this value?
My concern is that this value could change anytime. If we
copy this value then it might get out-of_date easily.
When thinking more about it. The value should be the same as long
as this context owns the console. If we really need to have
the value in struct nbcon_wctxt then we should set it when
the structure is initialized under the lock.
Honestly, it looks weird to set it in nbcon_emit_next_record().
It looks like a random place to me. And it means the value is
invalid before nbcon_emit_next_record() was called.
Do we guarantee that it won't be used in other code paths?
> + wctxt->len = pmsg.outbuf_len;
> + if (wctxt->len)
> + wctxt->outbuf = &pmsg.pbufs->outbuf[0];
These two values are actually well defined for each wctxt.
It depends where nbcon_emit_next_record() is called from.
It might be:
+ console_unlock => the global "pbufs"
+ kthread or emergency printk() => con->pbufs
+ panic() => panic_nbcon_pbufs()
IMHO, the value should stay the same as long as the context
is used/exists.
It might make sense to do initialization of these values
with some actions. wxtxt might be initialized:
+ when defined according to the context where
it is defined.
+ when ctxt is acquired if the values might change
according to the current state of the system.
+ before each con->*write*() when the values
might change even when the context is still
acquired.
Important:
All the above ideas and concers come to my head because
I do not know the full context in which the API will
be used. I see only the pieces that will be later
put together.
I could live with this code when this is the only
location where the values are set and used. We could
clean up the logic later when I see the entire puzzle.
This "detail" most likely is not worth delaying
the entire patchset. This probably fails into
the category "perfection is enemy of good".
> + else
> + wctxt->outbuf = NULL;
It looks weird to call con->write_atomic() when outbuf() is NULL.
It actually could never happen because of the above used:
if (pmsg.outbuf_len == 0)
goto update_con;
> + if (con->write_atomic) {
> + done = con->write_atomic(con, wctxt);
> + } else {
> + nbcon_context_release(ctxt);
> + WARN_ON_ONCE(1);
> + done = false;
> + }
> +
> + /* If not done, the emit was aborted. */
> + if (!done)
> + return false;
> +
> + /*
> + * Since any dropped message was successfully output, reset the
> + * dropped count for the console.
> + */
> + dropped = 0;
> +update_con:
> + if (dropped != con_dropped) {
> + /* Counterpart to the READ_ONCE() above. */
> + WRITE_ONCE(con->dropped, dropped);
> + }
I would personally call nbcon_seq_try_update(ctxt) before
updating con->dropped.
If we are not able to udpate con->dropped then the current owner
will see the same lost messages. Or it is a REPLY_ALL in which
case the number is misleading anyway.
That said, both ways are racy. And it is not worth too big
complexity.
> + ctxt->seq = pmsg.seq + 1;
> + if (!nbcon_seq_try_update(ctxt)) {
> + /* This context probably lost ownership. Check. */
> + return nbcon_can_proceed(wctxt);
> + }
I can't imagine a valid situation where nbcon_seq_try_update(ctxt)
fails and we could continue. If this happened then it would be
suspicious.
I would do:
/*
* Since any dropped message was successfully output, reset the
* dropped count for the console.
*/
dropped = 0;
update_con:
ctxt->seq = pmsg.seq + 1;
if (!nbcon_seq_try_update(ctxt) {
/*
* Anyone else manipulated the sequnce number.
* It should not happen without loosing access
* to the console.
*/
if (!nbcon_can_proceed(wctxt)
return false;
WARN_ON_ONCE(1);
}
if (dropped != con_dropped) {
/* Counterpart to the READ_ONCE() above. */
data_race(WRITE_ONCE(con->dropped, dropped));
}
> +
> + return true;
> +}
> +
> /**
> * nbcon_init - Initialize the nbcon console specific data
> * @con: Console to initialize
> --
> 2.39.2
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 7/8] printk: nbcon: Add emit function and callback function for atomic printing
2023-08-11 13:37 ` Petr Mladek
@ 2023-08-29 13:08 ` John Ogness
0 siblings, 0 replies; 37+ messages in thread
From: John Ogness @ 2023-08-29 13:08 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2023-08-11, Petr Mladek <pmladek@suse.com> wrote:
>> +static bool nbcon_emit_next_record(struct nbcon_write_context *wctxt)
>> +{
>> + struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
>> + struct console *con = ctxt->console;
>> + bool is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
>> + struct printk_message pmsg = {
>> + .pbufs = ctxt->pbufs,
>> + };
>> + unsigned long con_dropped;
>> + struct nbcon_state cur;
>> + unsigned long dropped;
>> + bool done;
>> +
>> + ctxt->backlog = printk_get_next_message(&pmsg, ctxt->seq, is_extended, true);
>> + if (!ctxt->backlog)
>> + return true;
>> +
>> + /*
>> + * @con->dropped is not protected in case of hostile takeovers. In
>> + * that situation the update can be racy so annotate it accordingly.
>> + */
>> + con_dropped = data_race(READ_ONCE(con->dropped));
>> +
>> + dropped = con_dropped + pmsg.dropped;
>> + if (dropped && !is_extended)
>> + console_prepend_dropped(&pmsg, dropped);
>> +
>> + /* Safety point. Do not touch state in case of takeover. */
>> + nbcon_state_read(con, &cur);
>> + if (!nbcon_context_can_proceed(ctxt, &cur))
>> + return false;
>> +
>> + /* For skipped records just update seq/dropped in @con. */
>> + if (pmsg.outbuf_len == 0)
>> + goto update_con;
>> +
>> + /* Set the write context before calling write callback. */
>> + wctxt->hostile_unsafe = cur.hostile_unsafe;
>
> Is there any reason why wctxt need to know about cur.hostile_usafe?
> Will it use another console->*write*() callback or so?
> Or will it do another decision based on this value?
wctxt exists so that the driver understands its context. If
wctxt->hostile_unsafe is set, the driver knows that an unsafe hostile
takeover took place. This can certainly help the driver to make
decisions about what it wants to do.
> My concern is that this value could change anytime. If we
> copy this value then it might get out-of_date easily.
If this value changes, it means the owner changed. So the driver will
abort due to can_proceed() failing anyway.
> When thinking more about it. The value should be the same as long
> as this context owns the console. If we really need to have
> the value in struct nbcon_wctxt then we should set it when
> the structure is initialized under the lock.
This code _is_ initializing wctxt.
> Honestly, it looks weird to set it in nbcon_emit_next_record().
> It looks like a random place to me. And it means the value is
> invalid before nbcon_emit_next_record() was called.
> Do we guarantee that it won't be used in other code paths?
The wctxt structure only exists to serve the write callback. IMHO
initializing it just before calling the callbacks is the correct
placement.
> It might make sense to do initialization of these values
> with some actions. wxtxt might be initialized:
>
> + when defined according to the context where
> it is defined.
This is not appropriate because the same wctxt is recycled for multiple
consoles. (The associated @console is initialized during acquire.)
> + when ctxt is acquired if the values might change
> according to the current state of the system.
wctxt->len would still need to be set in nbcon_emit_next_record(). So we
would have multiple locations that are initializing different parts of
wctxt. I would prefer to have all the initialization in one place.
> + before each con->*write*() when the values
> might change even when the context is still
> acquired.
This is how it is in v2. If the state values change, the driver bails
out due to can_proceed() failing.
>> + else
>> + wctxt->outbuf = NULL;
>
> It looks weird to call con->write_atomic() when outbuf() is NULL.
> It actually could never happen because of the above used:
Right. I will remove that for v3.
>> +update_con:
>> + if (dropped != con_dropped) {
>> + /* Counterpart to the READ_ONCE() above. */
>> + WRITE_ONCE(con->dropped, dropped);
>> + }
>
> I would personally call nbcon_seq_try_update(ctxt) before
> updating con->dropped.
>
> If we are not able to udpate con->dropped then the current owner
> will see the same lost messages. Or it is a REPLY_ALL in which
> case the number is misleading anyway.
>
> That said, both ways are racy. And it is not worth too big
> complexity.
I also debated this for a short time. Keep in mind that we are talking
about the following situation:
A context successfully pushed out a dropped message and the printk record.
The race is when the console has a hostile takeover and the new owner
starts printing before or during @dropped and @seq being updated.
As v2 is written, you will see either:
- the dropped message and printk record a second time
- only the printk record a second time
With your suggestion, you will see either:
- the dropped message and printk record a second time
- only the dropped message a second time
IMHO it is better to see the printk record a second time because it
makes it clear that there was a repeat. By seeing only the dropped
message a second time, a user might think that the number of records was
really dropped twice. Also, it is strange to see the dropped message
twice but not the printk record, since the dropped message preceeds the
printk record.
> I can't imagine a valid situation where nbcon_seq_try_update(ctxt)
> fails and we could continue. If this happened then it would be
> suspicious.
>
> I would do:
>
> if (!nbcon_seq_try_update(ctxt) {
> /*
> * Anyone else manipulated the sequnce number.
> * It should not happen without loosing access
> * to the console.
> */
> if (!nbcon_can_proceed(wctxt)
> return false;
> WARN_ON_ONCE(1);
> }
OK.
John
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH printk v2 8/8] printk: nbcon: Add functions for drivers to mark unsafe regions
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
` (6 preceding siblings ...)
2023-07-28 0:02 ` [PATCH printk v2 7/8] printk: nbcon: Add emit function and callback function for atomic printing John Ogness
@ 2023-07-28 0:02 ` John Ogness
2023-08-11 13:50 ` Petr Mladek
2023-08-11 13:56 ` [PATCH printk v2 0/8] wire up nbcon consoles Petr Mladek
8 siblings, 1 reply; 37+ messages in thread
From: John Ogness @ 2023-07-28 0:02 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
From: Thomas Gleixner <tglx@linutronix.de>
For the write_atomic callback, the console driver may have unsafe
regions that need to be appropriately marked. Provide functions
that accept the nbcon_write_context struct to allow for the driver
to enter and exit unsafe regions.
Co-developed-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Thomas Gleixner (Intel) <tglx@linutronix.de>
---
include/linux/console.h | 4 ++++
kernel/printk/printk_nbcon.c | 41 +++++++++++++++++++++++++++++++++++-
2 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 3d129b2b70a1..e4ce1015627c 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -456,8 +456,12 @@ static inline bool console_is_registered(const struct console *con)
#ifdef CONFIG_PRINTK
extern bool nbcon_can_proceed(struct nbcon_write_context *wctxt);
+extern bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt);
+extern bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt);
#else
static inline bool nbcon_can_proceed(struct nbcon_write_context *wctxt) { return false; }
+static inline bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt) { return false; }
+static inline bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt) { return false; }
#endif
extern int console_set_on_cmdline;
diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
index e1f0e4278ffa..57b7539bbbb2 100644
--- a/kernel/printk/printk_nbcon.c
+++ b/kernel/printk/printk_nbcon.c
@@ -761,7 +761,6 @@ EXPORT_SYMBOL_GPL(nbcon_can_proceed);
*
* Internal helper to avoid duplicated code
*/
-__maybe_unused
static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
{
struct console *con = ctxt->console;
@@ -787,6 +786,46 @@ static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
return true;
}
+/**
+ * nbcon_enter_unsafe - Enter an unsafe region in the driver
+ * @wctxt: The write context that was handed to the write function
+ *
+ * Return: True if the state is correct. False if ownership was
+ * handed over or taken.
+ *
+ * When this function returns false then the calling context is no longer
+ * the owner and is no longer allowed to go forward. In this case it must
+ * back out immediately and carefully. The buffer content is also no longer
+ * trusted since it no longer belongs to the calling context.
+ */
+bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt)
+{
+ struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+
+ return nbcon_context_update_unsafe(ctxt, true);
+}
+EXPORT_SYMBOL_GPL(nbcon_enter_unsafe);
+
+/**
+ * nbcon_exit_unsafe - Exit an unsafe region in the driver
+ * @wctxt: The write context that was handed to the write function
+ *
+ * Return: True if the state is correct. False if ownership was
+ * handed over or taken.
+ *
+ * When this function returns false then the calling context is no longer
+ * the owner and is no longer allowed to go forward. In this case it must
+ * back out immediately and carefully. The buffer content is also no longer
+ * trusted since it no longer belongs to the calling context.
+ */
+bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt)
+{
+ struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
+
+ return nbcon_context_update_unsafe(ctxt, false);
+}
+EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
+
/**
* nbcon_emit_next_record - Emit a record in the acquired context
* @wctxt: The write context that will be handed to the write function
--
2.39.2
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 8/8] printk: nbcon: Add functions for drivers to mark unsafe regions
2023-07-28 0:02 ` [PATCH printk v2 8/8] printk: nbcon: Add functions for drivers to mark unsafe regions John Ogness
@ 2023-08-11 13:50 ` Petr Mladek
2023-08-29 13:13 ` John Ogness
0 siblings, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-08-11 13:50 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-07-28 02:08:33, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> For the write_atomic callback, the console driver may have unsafe
> regions that need to be appropriately marked. Provide functions
> that accept the nbcon_write_context struct to allow for the driver
> to enter and exit unsafe regions.
>
> diff --git a/kernel/printk/printk_nbcon.c b/kernel/printk/printk_nbcon.c
> index e1f0e4278ffa..57b7539bbbb2 100644
> --- a/kernel/printk/printk_nbcon.c
> +++ b/kernel/printk/printk_nbcon.c
> @@ -761,7 +761,6 @@ EXPORT_SYMBOL_GPL(nbcon_can_proceed);
> *
> * Internal helper to avoid duplicated code
> */
> -__maybe_unused
> static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
> {
> struct console *con = ctxt->console;
It might be better to define nbcon_context_update_unsafe()
in this patch. I guess that nbcon_enter_unsafe()/nbcon_exit_unsafe()
are the only callers.
> @@ -787,6 +786,46 @@ static bool nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsafe)
> return true;
> }
>
> +/**
> + * nbcon_enter_unsafe - Enter an unsafe region in the driver
> + * @wctxt: The write context that was handed to the write function
> + *
> + * Return: True if the state is correct. False if ownership was
> + * handed over or taken.
Same as in 7th patch. I would write:
* Return: True if this context still owns the console. False
if the ownership has been handed over or taken.
> + *
> + * When this function returns false then the calling context is no longer
> + * the owner and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context.
> + */
> +bool nbcon_enter_unsafe(struct nbcon_write_context *wctxt)
> +{
> + struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +
> + return nbcon_context_update_unsafe(ctxt, true);
> +}
> +EXPORT_SYMBOL_GPL(nbcon_enter_unsafe);
> +
> +/**
> + * nbcon_exit_unsafe - Exit an unsafe region in the driver
> + * @wctxt: The write context that was handed to the write function
> + *
> + * Return: True if the state is correct. False if ownership was
> + * handed over or taken.
Same here.
> + * When this function returns false then the calling context is no longer
> + * the owner and is no longer allowed to go forward. In this case it must
> + * back out immediately and carefully. The buffer content is also no longer
> + * trusted since it no longer belongs to the calling context.
> + */
> +bool nbcon_exit_unsafe(struct nbcon_write_context *wctxt)
> +{
> + struct nbcon_context *ctxt = &ACCESS_PRIVATE(wctxt, ctxt);
> +
> + return nbcon_context_update_unsafe(ctxt, false);
> +}
> +EXPORT_SYMBOL_GPL(nbcon_exit_unsafe);
> +
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH printk v2 0/8] wire up nbcon consoles
2023-07-28 0:02 [PATCH printk v2 0/8] wire up nbcon consoles John Ogness
` (7 preceding siblings ...)
2023-07-28 0:02 ` [PATCH printk v2 8/8] printk: nbcon: Add functions for drivers to mark unsafe regions John Ogness
@ 2023-08-11 13:56 ` Petr Mladek
2023-08-29 13:21 ` John Ogness
8 siblings, 1 reply; 37+ messages in thread
From: Petr Mladek @ 2023-08-11 13:56 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-07-28 02:08:25, John Ogness wrote:
> Hi,
>
> This is v2 of a series to introduce the new non-BKL (nbcon)
> consoles. This series is only a subset of the original
> v1 [0]. In particular, this series represents patches 5-10 of
> the v1 series. For information about the motivation of the
> atomic consoles, please read the cover letter of v1.
>
> This series focuses on wiring up the printk subsystem to
> be able to use the nbcon consoles and implement their ownership
> interfaces and rules. This series does _not_ include threaded
> printing, atomic printing regions, or nbcon drivers. Those
> features will be added in separate follow-up series.
>
>
> include/linux/console.h | 132 +++++
> kernel/printk/Makefile | 2 +-
> kernel/printk/internal.h | 29 ++
> kernel/printk/printk.c | 156 ++++--
> kernel/printk/printk_nbcon.c | 955 +++++++++++++++++++++++++++++++++++
Nit: Is there still any chance to rename this to kernel/printk/nbcon.c ?
I am sorry that I did not suggested this earlier. I think that
we should have omitted the "printk_" prefix even for
the "ringbuffer.*" files.
I think that it came from "printk_safe.c". But it made some sense.
"printk_safe_*" was also an API.
But in general, "printk_" prefix is superfluous in "printk" directory.
Best Regards,
Petr
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH printk v2 0/8] wire up nbcon consoles
2023-08-11 13:56 ` [PATCH printk v2 0/8] wire up nbcon consoles Petr Mladek
@ 2023-08-29 13:21 ` John Ogness
0 siblings, 0 replies; 37+ messages in thread
From: John Ogness @ 2023-08-29 13:21 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2023-08-11, Petr Mladek <pmladek@suse.com> wrote:
> Nit: Is there still any chance to rename this to kernel/printk/nbcon.c ?
Sure. Will do for v3.
> I am sorry that I did not suggested this earlier. I think that
> we should have omitted the "printk_" prefix even for
> the "ringbuffer.*" files.
For the ringbuffer I actually wanted "printk" everywhere to make it
clear that it is not a generic ringbuffer. The struct is actually named
"printk_ringbuffer".
John
^ permalink raw reply [flat|nested] 37+ messages in thread