* [PATCH printk v4 1/8] printk: Add non-BKL (nbcon) console basic infrastructure
2023-09-08 18:50 [PATCH printk v4 0/8] provide nbcon base John Ogness
@ 2023-09-08 18:50 ` John Ogness
2023-09-08 18:50 ` [PATCH printk v4 2/8] printk: nbcon: Add acquire/release logic John Ogness
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: John Ogness @ 2023-09-08 18:50 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
also 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.
It was decided to use a bitfield because using a plain u32 with
mask/shift operations resulted in uncomprehensible code.
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>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
include/linux/console.h | 31 ++++++++++++++++++
kernel/printk/Makefile | 2 +-
kernel/printk/internal.h | 8 +++++
kernel/printk/nbcon.c | 70 ++++++++++++++++++++++++++++++++++++++++
kernel/printk/printk.c | 13 ++++++--
5 files changed, 120 insertions(+), 4 deletions(-)
create mode 100644 kernel/printk/nbcon.c
diff --git a/include/linux/console.h b/include/linux/console.h
index 7de11c763eb3..a2d37a7a98a8 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. Ensure this struct stays
+ * within the size boundaries of the 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..39a2b61c7232 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 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..2ca0ab78802c 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);
@@ -61,6 +62,10 @@ void defer_console_output(void);
u16 printk_parse_prefix(const char *text, int *level,
enum printk_info_flags *flags);
+
+void nbcon_init(struct console *con);
+void nbcon_cleanup(struct console *con);
+
#else
#define PRINTK_PREFIX_MAX 0
@@ -76,6 +81,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 void nbcon_init(struct console *con) { }
+static inline void nbcon_cleanup(struct console *con) { }
+
#endif /* CONFIG_PRINTK */
/**
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
new file mode 100644
index 000000000000..10266d3e7883
--- /dev/null
+++ b/kernel/printk/nbcon.c
@@ -0,0 +1,70 @@
+// 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
+ */
+void nbcon_init(struct console *con)
+{
+ struct nbcon_state state = { };
+
+ nbcon_state_set(con, &state);
+}
+
+/**
+ * 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);
+}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8787d3a72114..c0246093ea41 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3324,9 +3324,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 +3487,9 @@ void register_console(struct console *newcon)
newcon->dropped = 0;
console_init_seq(newcon, bootcon_registered);
+ if (newcon->flags & CON_NBCON)
+ nbcon_init(newcon);
+
/*
* Put this console in the list - keep the
* preferred driver at the head of the list.
@@ -3577,6 +3581,9 @@ 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)
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH printk v4 2/8] printk: nbcon: Add acquire/release logic
2023-09-08 18:50 [PATCH printk v4 0/8] provide nbcon base John Ogness
2023-09-08 18:50 ` [PATCH printk v4 1/8] printk: Add non-BKL (nbcon) console basic infrastructure John Ogness
@ 2023-09-08 18:50 ` John Ogness
2023-09-14 13:16 ` Petr Mladek
2023-09-08 18:50 ` [PATCH printk v4 3/8] printk: Make static printk buffers available to nbcon John Ogness
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: John Ogness @ 2023-09-08 18:50 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 priority 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:
- Direct acquire when the console is not owned or is owned by a
lower priority context and the console is in a safe state.
- Friendly handover mechanism based on a request/grant handshake.
The requesting context must have a higher priority than the
current owner.
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.
- Unsafe hostile takeover
The new owner takes the console over without 'req_prio'
handshake. The new owner must have a higher priority than
the previous owner.
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 that 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 spin-wait if there is already an owner and the
console is in an unsafe state.
- Whether to attempt an unsafe hostile takeover.
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/nbcon.c | 432 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 491 insertions(+)
diff --git a/include/linux/console.h b/include/linux/console.h
index a2d37a7a98a8..eeebb82d6d07 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -175,13 +175,29 @@ 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 owner
+ * @unsafe: Console is busy in a non takeover region
+ * @unsafe_takeover: A hostile takeover in an unsafe state happened in the
+ * past. The console cannot be safe until re-initialized.
+ * @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 unsafe_takeover : 1;
+ unsigned int cpu : 24;
};
};
};
@@ -194,6 +210,49 @@ 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 spin-wait acquire
+ * @prio: Priority of the context
+ * @allow_unsafe_takeover: Allow performing takeover even if unsafe. Can
+ * be used only with NBCON_PRIO_PANIC @prio. It
+ * might cause a system freeze when the console
+ * is used later.
+ */
+struct nbcon_context {
+ /* members set by caller */
+ struct console *console;
+ unsigned int spinwait_max_us;
+ enum nbcon_prio prio;
+ unsigned int allow_unsafe_takeover : 1;
+};
+
/**
* struct console - The console descriptor structure
* @name: The name of the console driver
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 10266d3e7883..57ddfb7f0994 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -4,10 +4,43 @@
#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 directly
+ * take over if the console is not in an unsafe state by carefully writing
+ * its priority into @nbcon_state::prio.
+ *
+ * If the console is in an unsafe state, the concurrent writer 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 the owner does not react on the request and does not make
+ * observable progress, the waiter will timeout and can then decide to do
+ * an unsafe hostile takeover. Upon unsafe hostile takeover, the console
+ * is kept permanently in the unsafe state.
*/
/**
@@ -47,6 +80,405 @@ 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 and this is not the panic CPU
+ * or this context does not have a priority above the
+ * current owner or waiter. No acquire method can be
+ * successful for this context.
+ *
+ * -EBUSY: The console is in an unsafe state. The caller should
+ * try using the handover acquire method.
+ *
+ * The general procedure is to change @nbcon_state::prio from unowned to
+ * owned. Or, if the console is not in the unsafe state, to change
+ * @nbcon_state::prio to a higher priority owner.
+ */
+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;
+
+ do {
+ if (other_cpu_in_panic())
+ return -EPERM;
+
+ if (ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio)
+ return -EPERM;
+
+ if (cur->unsafe)
+ return -EBUSY;
+
+ /*
+ * The console should never be safe for a direct acquire
+ * if an unsafe hostile takeover has ever happened.
+ */
+ WARN_ON_ONCE(cur->unsafe_takeover);
+
+ new.atom = cur->atom;
+ new.prio = ctxt->prio;
+ new.req_prio = NBCON_PRIO_NONE;
+ new.unsafe = cur->unsafe_takeover;
+ new.cpu = cpu;
+
+ } while (!nbcon_state_try_cmpxchg(con, cur, &new));
+
+ cur->atom = new.atom;
+
+ return 0;
+}
+
+static bool nbcon_waiter_matches(struct nbcon_state *cur, int expected_prio)
+{
+ /*
+ * The request context is well defined by the @req_prio because:
+ *
+ * - Only a context with a higher priority can take over the request.
+ * - There are only three priorities.
+ * - Only one CPU is allowed to request PANIC priority.
+ * - Lower priorities are ignored during panic() until reboot.
+ *
+ * As a result, the following scenario is *not* possible:
+ *
+ * 1. Another context with a higher priority directly takes ownership.
+ * 2. The higher priority context releases the ownership.
+ * 3. A lower priority context takes the ownership.
+ * 4. Another context with the same priority as this context
+ * creates a request and starts waiting.
+ */
+
+ return (cur->req_prio == expected_prio);
+}
+
+/**
+ * nbcon_context_try_acquire_requested - Try to acquire after having
+ * requested a 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 and this is not the panic CPU
+ * or this context is no longer the waiter. For the
+ * former case, the caller must carefully remove the
+ * request before aborting the acquire.
+ *
+ * -EBUSY: The console is still locked. The caller should
+ * continue waiting.
+ *
+ * This is a helper function for nbcon_context_try_acquire_handover().
+ */
+static int nbcon_context_try_acquire_requested(struct nbcon_context *ctxt,
+ struct nbcon_state *cur)
+{
+ unsigned int cpu = smp_processor_id();
+ struct console *con = ctxt->console;
+ struct nbcon_state new;
+
+ do {
+ /*
+ * Note: If the acquire is aborted due to a panic CPU,
+ * the caller must still remove the request!
+ */
+ if (other_cpu_in_panic())
+ return -EPERM;
+
+ /*
+ * If an unsafe hostile takeover has occurred, a handover
+ * is no longer possible.
+ */
+ if (cur->unsafe_takeover)
+ return -EPERM;
+
+ /* Is this context still the requester? */
+ if (!nbcon_waiter_matches(cur, ctxt->prio))
+ return -EPERM;
+
+ /* If still locked, caller should continue waiting. */
+ if (cur->prio != NBCON_PRIO_NONE)
+ return -EBUSY;
+
+ /*
+ * The previous owner should have never released ownership
+ * in an unsafe region.
+ */
+ WARN_ON_ONCE(cur->unsafe);
+
+ new.atom = cur->atom;
+ new.prio = ctxt->prio;
+ new.req_prio = NBCON_PRIO_NONE;
+ new.unsafe = cur->unsafe_takeover;
+ new.cpu = cpu;
+
+ } while (!nbcon_state_try_cmpxchg(con, cur, &new));
+
+ /* Handover success. This context now owns the console. */
+
+ 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: This context is on the same CPU as the current owner
+ * or the console is permanently in an unsafe state or
+ * this context is unwilling to wait or a panic is in
+ * progress and this is not the panic CPU. This is not
+ * the panic context, so no acquire method can be
+ * successful for this context.
+ *
+ * -EBUSY: The current owner or waiter is such that this context
+ * is not able to execute a handover. The caller can use
+ * an unsafe hostile takeover to acquire.
+ *
+ * -EAGAIN: @cur is not the current console state. @cur has been
+ * updated. The caller should retry with direct acquire.
+ *
+ * The general procedure is to set @req_prio and wait until unowned. Then
+ * set @prio (claiming ownership) and clearing @req_prio.
+ *
+ * Note that it is expected that the caller tried
+ * nbcon_context_try_acquire_direct() with @cur before calling this function.
+ */
+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;
+
+ /*
+ * Handovers are not possible on the same CPU or when the console
+ * is permanently in an unsafe state or if the caller is unwilling
+ * to wait.
+ */
+ if (cur->cpu == cpu ||
+ cur->unsafe_takeover ||
+ ctxt->spinwait_max_us == 0) {
+ goto fail_handover;
+ }
+
+ /* Setup a 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;
+
+ /* 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_requested(ctxt, cur);
+ if (!err)
+ return 0;
+
+ /*
+ * If the acquire should be aborted, it must be ensured
+ * that the request is removed before returning to caller.
+ */
+ if (err == -EPERM)
+ break;
+
+ udelay(1);
+
+ /* Re-read the state because some time has passed. */
+ nbcon_state_read(con, cur);
+ }
+
+ /* Timed out or should abort. Carefully remove handover request. */
+ do {
+ /* No need to remove request if there is a new waiter. */
+ if (!nbcon_waiter_matches(cur, ctxt->prio))
+ goto fail_handover;
+
+ /* Unset request for handover. */
+ new.atom = cur->atom;
+ new.req_prio = NBCON_PRIO_NONE;
+ if (nbcon_state_try_cmpxchg(con, cur, &new)) {
+ /*
+ * Request successfully unset. Report failure of
+ * acquiring via handover.
+ */
+ cur->atom = new.atom;
+ goto fail_handover;
+ }
+
+ /*
+ * Unable to remove request. Try to acquire in case
+ * the owner has released the lock.
+ */
+ } while (nbcon_context_try_acquire_requested(ctxt, cur));
+
+ /* Acquire at timeout succeeded! */
+ return 0;
+
+fail_handover:
+ /*
+ * If this is the panic context, the caller can try to acquire using
+ * the unsafe hostile takeover method.
+ */
+ if (ctxt->prio == NBCON_PRIO_PANIC &&
+ ctxt->prio > cur->prio &&
+ ctxt->prio > cur->req_prio &&
+ !other_cpu_in_panic()) {
+ return -EBUSY;
+ }
+ return -EPERM;
+}
+
+/**
+ * nbcon_context_acquire_hostile - Acquire via unsafe hostile takeover
+ * @ctxt: The context of the caller
+ * @cur: The current console state
+ *
+ * @cur is updated to the new console state.
+ *
+ * The general procedure is to set @prio (forcing ownership). This method
+ * must only be used as a final attempt during panic.
+ */
+static void nbcon_context_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;
+
+ do {
+ new.atom = cur->atom;
+ new.cpu = cpu;
+ new.prio = ctxt->prio;
+ new.unsafe |= cur->unsafe_takeover;
+ new.unsafe_takeover |= cur->unsafe;
+
+ } while (!nbcon_state_try_cmpxchg(con, cur, &new));
+
+ cur->atom = new.atom;
+}
+
+/**
+ * nbcon_context_try_acquire - Try to acquire nbcon console
+ * @ctxt: The context of the caller
+ *
+ * Return: True if the console was acquired. False otherwise.
+ *
+ * If the caller allowed an unsafe hostile takeover, on success the
+ * caller should check the current console state to see if it is
+ * in an unsafe state. Otherwise, on success the caller may assume
+ * the console is not in an unsafe state.
+ */
+__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;
+ int err;
+
+ /* unsafe hostile takeovers are only allowed for panic */
+ WARN_ON_ONCE(ctxt->allow_unsafe_takeover && (ctxt->prio != NBCON_PRIO_PANIC));
+
+ nbcon_state_read(con, &cur);
+
+ do {
+ err = nbcon_context_try_acquire_direct(ctxt, &cur);
+ if (!err)
+ goto success;
+ else if (err == -EPERM)
+ return false;
+
+ err = nbcon_context_try_acquire_handover(ctxt, &cur);
+ if (!err)
+ goto success;
+ else if (err == -EPERM)
+ return false;
+
+ } while (err == -EAGAIN);
+
+ /* Only attempt unsafe hostile takeover if explicitly requested. */
+ if (!ctxt->allow_unsafe_takeover)
+ return false;
+
+ nbcon_context_acquire_hostile(ctxt, &cur);
+success:
+ 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;
+
+ /*
+ * If @unsafe_takeover is set, it is kept set so that
+ * the state remains permanently unsafe.
+ */
+ new.unsafe |= cur.unsafe_takeover;
+
+ } 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] 18+ messages in thread* Re: [PATCH printk v4 2/8] printk: nbcon: Add acquire/release logic
2023-09-08 18:50 ` [PATCH printk v4 2/8] printk: nbcon: Add acquire/release logic John Ogness
@ 2023-09-14 13:16 ` Petr Mladek
2023-09-14 13:23 ` [PATCH v4 2+/8] " Petr Mladek
0 siblings, 1 reply; 18+ messages in thread
From: Petr Mladek @ 2023-09-14 13:16 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
Hi,
I have spent on this patch much more time than expected. There were
basically two triggers:
+ I decided to check the commit message and all the comments more
carefully and some sentences looked outdated, harder to follow,
or confused me a bit.
+ It looked strange that try_acquire_direct() could take over
lock from a context with lower priority when the console was
in the safe state. But try_acquire_handover_requested()
did not check the 'unsafe' flag and just waited until
the current owner releases the lock.
I started modifying the code to add the check of the 'unsafe'.
It triggered some code shuffling and updates of comments.
Then I realized that it actually worked even before because
try_acquire_handover() was called when the console was
in unstable state and the current owner released the console
immediately on exit from unsafe_exit(). So that handover
never had to wait for the next unsafe_enter(). (I am not sure
if I am dumb or if it was really that tricky to realize.)
Also I realized that the do/while cycle should not be
needed in try_acquire_handover_requested() anymore.
Also I updated the comments and did few code clean ups.
Below, I just describe motivation for the changes. I'll
attach a diff with the proposed changes so that you could
just use them if you agree.
On Fri 2023-09-08 20:56:02, 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:
>
> - The 'prio' field contains the priority 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
It is not longer only handover. It is also takeover in the safe state.
> 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.
If we explained what 'prio' is for. It might make sense to explain
what 'cpu' is for.
> The acquire mechanism comes with several flavours:
>
> - Direct acquire when the console is not owned or is owned by a
> lower priority context and the console is in a safe state.
>
> - Friendly handover mechanism based on a request/grant handshake.
> The requesting context must have a higher priority than the
> current owner.
I would like mention that it is used when the console is in an unsafe
state. And that the current owner releases the lock on exit from
the unsafe state.
IMHO, this is very important piece of the puzzle. It took me long
time to realize it, see above.
> 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.
>
> - Unsafe hostile takeover
>
> The new owner takes the console over without 'req_prio'
> handshake. The new owner must have a higher priority than
> the previous owner.
>
> 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.
This is too generic. There is only one user in the end.
> The release is the counterpart that either releases the console
> directly or yields it gracefully over to a requester.
The word 'yield' has always been a magic to me. Does it still
fit here? The release method does not longer assign the lock
to the requestor. It just makes the lock free and keeps the request.
> 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 spin-wait if there is already an owner and the
> console is in an unsafe state.
> - Whether to attempt an unsafe hostile takeover.
>
> 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.
IMHO, the locking around each line will be used in panic as well.
So I would not mention this.
> diff --git a/include/linux/console.h b/include/linux/console.h
> index a2d37a7a98a8..eeebb82d6d07 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -194,6 +210,49 @@ 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.
Some things are not longer valid in the last versions. I would write
something li
* A higher priority context can takeover the console when it is
* in the safe state. The final attempt to flush consoles in panic()
* can be allowed to do so even in an unsafe state (Hope and pray).
> + */
> +enum nbcon_prio {
> + NBCON_PRIO_NONE = 0,
> + NBCON_PRIO_NORMAL,
> + NBCON_PRIO_EMERGENCY,
> + NBCON_PRIO_PANIC,
> + NBCON_PRIO_MAX,
> +};
> +
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 10266d3e7883..57ddfb7f0994 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -4,10 +4,43 @@
>
> #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.
I have realized that the comments are a bit outdated, see below.
I would personally use the (updated) text from the commit message
here. It looks easier to follow.
> + *
> + * 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.
OK
> + *
> + * 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.
It is not only about nested contexts. It is primary about contexts with a
higher priority. I think that the rules were different in the original
implementation.
> + *
> + * In case of panic the nesting context can take over the console forcefully.
This describes panic.
> + * A concurrent writer on a different CPU with a higher priority can directly
> + * take over if the console is not in an unsafe state by carefully writing
> + * its priority into @nbcon_state::prio.
This acutally describes the contex with a higher priority.
> + *
> + * If the console is in an unsafe state, the concurrent writer 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 the owner does not react on the request and does not make
> + * observable progress, the waiter will timeout and can then decide to do
> + * an unsafe hostile takeover. Upon unsafe hostile takeover, the console
> + * is kept permanently in the unsafe state.
The comment does not take about direct acquire. I think that all the
variants were better described in the commit message.
> */
> /**
> @@ -47,6 +80,405 @@ 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.
There is not need to update @cur on success. It won't be used any
longer. On the other hand, it might be useful to say that it is
updated when any modification failed.
> + *
> + * Errors:
> + *
> + * -EPERM: A panic is in progress and this is not the panic CPU
> + * or this context does not have a priority above the
> + * current owner or waiter. No acquire method can be
> + * successful for this context.
> + *
> + * -EBUSY: The console is in an unsafe state. The caller should
> + * try using the handover acquire method.
> + *
> + * The general procedure is to change @nbcon_state::prio from unowned to
> + * owned. Or, if the console is not in the unsafe state, to change
> + * @nbcon_state::prio to a higher priority owner.
This is common for all try_acquire() variant. A more important
information is when this variant is supposed to succeed.
> + */
> +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;
> +
> + do {
> + if (other_cpu_in_panic())
> + return -EPERM;
> +
> + if (ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio)
> + return -EPERM;
> +
> + if (cur->unsafe)
> + return -EBUSY;
> +
> + /*
> + * The console should never be safe for a direct acquire
> + * if an unsafe hostile takeover has ever happened.
> + */
> + WARN_ON_ONCE(cur->unsafe_takeover);
> +
> + new.atom = cur->atom;
> + new.prio = ctxt->prio;
> + new.req_prio = NBCON_PRIO_NONE;
> + new.unsafe = cur->unsafe_takeover;
> + new.cpu = cpu;
> +
> + } while (!nbcon_state_try_cmpxchg(con, cur, &new));
> +
> + cur->atom = new.atom;
There is no need to update @cur. The caller will just return 0 as well.
> + return 0;
> +}
> +
> +/**
> + * nbcon_context_try_acquire_requested - Try to acquire after having
> + * requested a 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.
> + *
Same as above. There is not need to update @cur on success. It won't be used any
longer. On the other hand, it might be useful to say that it is
updated when any modification failed.
> + * Errors:
> + *
> + * -EPERM: A panic is in progress and this is not the panic CPU
> + * or this context is no longer the waiter. For the
> + * former case, the caller must carefully remove the
> + * request before aborting the acquire.
> + *
> + * -EBUSY: The console is still locked. The caller should
> + * continue waiting.
> + *
> + * This is a helper function for nbcon_context_try_acquire_handover().
> + */
> +static int nbcon_context_try_acquire_requested(struct nbcon_context *ctxt,
> + struct nbcon_state *cur)
> +{
> + unsigned int cpu = smp_processor_id();
> + struct console *con = ctxt->console;
> + struct nbcon_state new;
> +
> + do {
> + /*
> + * Note: If the acquire is aborted due to a panic CPU,
> + * the caller must still remove the request!
> + */
> + if (other_cpu_in_panic())
> + return -EPERM;
> +
> + /*
> + * If an unsafe hostile takeover has occurred, a handover
> + * is no longer possible.
> + */
> + if (cur->unsafe_takeover)
> + return -EPERM;
This should never happen without clearing the waiter. The check
of nbcon_waiter_matches() and WARN_ON_ONCE(cur->unsafe) should
be enough.
> +
> + /* Is this context still the requester? */
> + if (!nbcon_waiter_matches(cur, ctxt->prio))
> + return -EPERM;
> +
> + /* If still locked, caller should continue waiting. */
> + if (cur->prio != NBCON_PRIO_NONE)
> + return -EBUSY;
> +
> + /*
> + * The previous owner should have never released ownership
> + * in an unsafe region.
> + */
> + WARN_ON_ONCE(cur->unsafe);
> +
> + new.atom = cur->atom;
> + new.prio = ctxt->prio;
> + new.req_prio = NBCON_PRIO_NONE;
> + new.unsafe = cur->unsafe_takeover;
> + new.cpu = cpu;
We are here because the console has been released because of our
request. The following cmpxchg() could fail only when the console
has been acquired by a higher priority context. In which
case we are not longer owner of the request and should
return -EPERM.
> + } while (!nbcon_state_try_cmpxchg(con, cur, &new));
I would replace the while cycle with:
if (!nbcon_state_try_cmpxchg(con, cur, &new)) {
/*
* The console has been released because our request.
* The acquire could fail only when it has been taken
* over by a higher priority context.
*/
WARN_ON_ONCE(nbcon_waiter_matches(cur, ctxt->prio));
return -EPERM;
}
It would help to find when it does not work as designed.
> +
> + /* Handover success. This context now owns the console. */
> +
> + cur->atom = new.atom;
There is no need to update @cur. The caller will just return 0 as well.
> +
> + 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: This context is on the same CPU as the current owner
This is not a reason to return -EPERM. The console still could get
acquired using the hostile takeover.
> + * or the console is permanently in an unsafe state or
> + * this context is unwilling to wait or a panic is in
What does exactly mean that this context is unwilling to wait, please?
Should we return -EPERM in this case?
> + * progress and this is not the panic CPU. This is not
> + * the panic context, so no acquire method can be
> + * successful for this context.
We should add that -EPERM is returned also when a higher priority context has
taken the console in the meantime.
> + *
> + * -EBUSY: The current owner or waiter is such that this context
> + * is not able to execute a handover.
This is a bit vague.
> + * an unsafe hostile takeover to acquire.
> + *
> + * -EAGAIN: @cur is not the current console state. @cur has been
> + * updated. The caller should retry with direct acquire.
> + *
> + * The general procedure is to set @req_prio and wait until unowned. Then
> + * set @prio (claiming ownership) and clearing @req_prio.
> + *
> + * Note that it is expected that the caller tried
> + * nbcon_context_try_acquire_direct() with @cur before calling this function.
> + */
> +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;
> +
> + /*
> + * Handovers are not possible on the same CPU or when the console
> + * is permanently in an unsafe state or if the caller is unwilling
> + * to wait.
> + */
> + if (cur->cpu == cpu ||
> + cur->unsafe_takeover ||
> + ctxt->spinwait_max_us == 0) {
> + goto fail_handover;
> + }
> +
> + /* Setup a 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;
> +
> + /* 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_requested(ctxt, cur);
> + if (!err)
> + return 0;
> +
> + /*
> + * If the acquire should be aborted, it must be ensured
> + * that the request is removed before returning to caller.
> + */
> + if (err == -EPERM)
> + break;
> +
> + udelay(1);
> +
> + /* Re-read the state because some time has passed. */
> + nbcon_state_read(con, cur);
> + }
> +
> + /* Timed out or should abort. Carefully remove handover request. */
> + do {
> + /* No need to remove request if there is a new waiter. */
> + if (!nbcon_waiter_matches(cur, ctxt->prio))
> + goto fail_handover;
> +
> + /* Unset request for handover. */
> + new.atom = cur->atom;
> + new.req_prio = NBCON_PRIO_NONE;
> + if (nbcon_state_try_cmpxchg(con, cur, &new)) {
> + /*
> + * Request successfully unset. Report failure of
> + * acquiring via handover.
> + */
> + cur->atom = new.atom;
> + goto fail_handover;
> + }
> +
> + /*
> + * Unable to remove request. Try to acquire in case
> + * the owner has released the lock.
> + */
> + } while (nbcon_context_try_acquire_requested(ctxt, cur));
> +
> + /* Acquire at timeout succeeded! */
> + return 0;
> +
> +fail_handover:
> + /*
> + * If this is the panic context, the caller can try to acquire using
> + * the unsafe hostile takeover method.
> + */
> + if (ctxt->prio == NBCON_PRIO_PANIC &&
> + ctxt->prio > cur->prio &&
> + ctxt->prio > cur->req_prio &&
> + !other_cpu_in_panic()) {
> + return -EBUSY;
I did not like it even in v3. I am sorry that I did not tell you.
Any location calling "goto fail_handover" knows whether to return
-EBUSY or -EPERM. I mean that it know whether the error is fatal
or the harsh takeover is allowed. The error codes are even used
inside this function. It is strange and hard to follow when
we re-compute the error code by a "magic" check.
They just do not know whether "ctxt->prio == NBCON_PRIO_PANIC"
but it would be more appropriate to check it in
nbcon_context_acquire_hostile() anyway.
The priority and panic context should be checked using
some consistency checkes in try_acquire_hostile().
> + }
> + return -EPERM;
> +}
> +
> +/**
> + * nbcon_context_acquire_hostile - Acquire via unsafe hostile takeover
> + * @ctxt: The context of the caller
> + * @cur: The current console state
> + *
> + * @cur is updated to the new console state.
> + *
> + * The general procedure is to set @prio (forcing ownership). This method
> + * must only be used as a final attempt during panic.
> + */
> +static void nbcon_context_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;
> +
It would be more appropriate to check that "ctxt->prio == NBCON_PRIO_PANIC" here.
> + do {
> + new.atom = cur->atom;
> + new.cpu = cpu;
> + new.prio = ctxt->prio;
> + new.unsafe |= cur->unsafe_takeover;
> + new.unsafe_takeover |= cur->unsafe;
> +
> + } while (!nbcon_state_try_cmpxchg(con, cur, &new));
> +
> + cur->atom = new.atom;
There is not need to set @cur on success. I won't be used.
> +}
> +
> +/**
> + * nbcon_context_try_acquire - Try to acquire nbcon console
> + * @ctxt: The context of the caller
> + *
> + * Return: True if the console was acquired. False otherwise.
> + *
> + * If the caller allowed an unsafe hostile takeover, on success the
> + * caller should check the current console state to see if it is
> + * in an unsafe state. Otherwise, on success the caller may assume
> + * the console is not in an unsafe state.
> + */
> +__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;
> + int err;
> +
> + /* unsafe hostile takeovers are only allowed for panic */
> + WARN_ON_ONCE(ctxt->allow_unsafe_takeover && (ctxt->prio != NBCON_PRIO_PANIC));
This would better fit into nbcon_context_acquire_hostile(). I know
that it would be checked there only when direct and handover failed.
But there is a good chance to hit it when used in non-panic context.
> + nbcon_state_read(con, &cur);
> +
> + do {
> + err = nbcon_context_try_acquire_direct(ctxt, &cur);
> + if (!err)
> + goto success;
> + else if (err == -EPERM)
> + return false;
> +
> + err = nbcon_context_try_acquire_handover(ctxt, &cur);
> + if (!err)
> + goto success;
> + else if (err == -EPERM)
> + return false;
> +
> + } while (err == -EAGAIN);
> +
> + /* Only attempt unsafe hostile takeover if explicitly requested. */
> + if (!ctxt->allow_unsafe_takeover)
> + return false;
> +
> + nbcon_context_acquire_hostile(ctxt, &cur);
> +success:
> + return true;
> +}
Best Regards,
Petr
^ permalink raw reply [flat|nested] 18+ messages in thread* [PATCH v4 2+/8] printk: nbcon: Add acquire/release logic
2023-09-14 13:16 ` Petr Mladek
@ 2023-09-14 13:23 ` Petr Mladek
2023-09-15 17:01 ` John Ogness
0 siblings, 1 reply; 18+ messages in thread
From: Petr Mladek @ 2023-09-14 13:23 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
// This patch includes my proposed changes on top of
// https://lore.kernel.org/all/20230908185008.468566-3-john.ogness@linutronix.de/
Add per console acquire/release functionality.
The state of the console is maintained in the "nbcon_state" atomic
variable.
The console is locked when:
- The 'prio' field contains the priority of the context that owns
the console. Only a higher priority contexts are allowed
to take over the lock. A value of 0 (NBCON_PRIO_NONE) means
the console is not locked.
- The 'cpu' field denotes on which CPU the console is locked.
It is used to prevent busy waiting on the same CPU. Also it informs
the lock owner that it has lost the lock in a more complex scenario
when the lock was taken over by a higher priority context, released,
and taken on another CPU with the same priority as the interrupted
owner.
The acquire mechanism uses few more fields:
- The 'req_prio' field is used by handover approach to make the current
user aware that there a context with a higher priority waiting
for the friendly handover.
- The 'unsafe' field allows to take over the console a safe way in
the middle of emitting a message. The filed is set only when accessing
some shared resources or when the console device is manipulated.
It can be cleared, for example, after emitting one character when
the console device is in a consistent state.
- The 'unsafe_takeover' field is set when a hostile takeover
took the console in an unsafe state. The console will stay
in the unsafe state until reinitialized.
The acquire mechanism uses three approaches:
1) Direct acquire when the console is not owned or is owned by a
lower priority context and is in a safe state.
2) Friendly handover mechanism uses a request/grant handshake. It is used
when the current owner has lower priority and the console is in
an unsafe state.
The requesting context:
a) Sets its priority into the 'req_prio' field.
b) Waits (with a timeout) for the owning context to unlock the
console.
c) Takes the lock and clears the 'req_prio' field.
The owning context:
a) Observes the 'req_prio' field set on exit from the unsafe
console state.
b) Gives up console ownership by clearing the 'prio' and 'cpu'
fields.
3) Unsafe hostile takeover allows to take over the lock even when
the console is an unsafe state. It is used only in panic() by
the final attempt to flush consoles in a try and hope mode.
The release function simply clears the 'prio' and 'cpu' fields.
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 spin-wait if there is already an owner and the
console is in an unsafe state.
- Whether to attempt an unsafe hostile takeover.
The design allows to implement the well known:
acquire()
output_one_printk_record()
release()
The output of one printk record might be interrupted with a higher
priority context. The new owner is supposed to reprint the entire
record from scratch.
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 | 9 +-
kernel/printk/nbcon.c | 390 +++++++++++++++++++++++-----------------
2 files changed, 233 insertions(+), 166 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index eeebb82d6d07..98210fd01f18 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -218,12 +218,9 @@ static_assert(sizeof(struct nbcon_state) <= sizeof(int));
* @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.
+ * A higher priority context can takeover the console when it is
+ * in the safe state. The final attempt to flush consoles in panic()
+ * can be allowed to do so even in an unsafe state (Hope and pray).
*/
enum nbcon_prio {
NBCON_PRIO_NONE = 0,
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 57ddfb7f0994..10d2f8b4f86a 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -7,40 +7,98 @@
#include <linux/delay.h>
#include "internal.h"
/*
- * Printk console printing implementation for consoles that do not depend on
+ * Printk console printing implementation for consoles which does 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.
+ * The state of the console is maintained in the "nbcon_state" atomic
+ * variable.
*
- * 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.
+ * The console is locked when:
*
- * In case of panic the nesting context can take over the console forcefully.
+ * - The 'prio' field contains the priority of the context that owns
+ * the console. Only a higher priority contexts are allowed
+ * to take over the lock. A value of 0 (NBCON_PRIO_NONE) means
+ * the console is not locked.
*
- * A concurrent writer on a different CPU with a higher priority can directly
- * take over if the console is not in an unsafe state by carefully writing
- * its priority into @nbcon_state::prio.
+ * - The 'cpu' field denotes on which CPU the console is locked.
+ * It is used to prevent busy waiting on the same CPU. Also it informs
+ * the lock owner that it has lost the lock in a more complex scenario
+ * when the lock was taken over by a higher priority context, released,
+ * and taken on another CPU with the same priority as the interrupted
+ * owner.
*
- * If the console is in an unsafe state, the concurrent writer with a higher
- * priority can request to take over the console by:
+ * The acquire mechanism uses few more fields:
*
- * 1) Carefully writing the desired priority into @nbcon_state::req_prio
- * if there is no higher priority request pending.
+ * - The 'req_prio' field is used by handover approach to make the current
+ * user aware that there a context with a higher priority waiting
+ * for the friendly handover.
*
- * 2) Carefully spin on @nbcon_state::prio until it is no longer locked.
+ * - The 'unsafe' field allows to take over the console a safe way in
+ * the middle of emitting a message. The filed is set only when accessing
+ * some shared resources or when the console device is manipulated.
+ * It can be cleared, for example, after emitting one character when
+ * the console device is in a consistent state.
*
- * 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.
+ * - The 'unsafe_takeover' field is set when a hostile takeover
+ * took the console in an unsafe state. The console will stay
+ * in the unsafe state until reinitialized.
*
- * In case the owner does not react on the request and does not make
- * observable progress, the waiter will timeout and can then decide to do
- * an unsafe hostile takeover. Upon unsafe hostile takeover, the console
- * is kept permanently in the unsafe state.
+ * The acquire mechanism uses three approaches:
+ *
+ * 1) Direct acquire when the console is not owned or is owned by a
+ * lower priority context and is in a safe state.
+ *
+ * 2) Friendly handover mechanism uses a request/grant handshake. It is used
+ * when the current owner has lower priority and the console is in
+ * an unsafe state.
+ *
+ * The requesting context:
+ *
+ * a) Sets its priority into the 'req_prio' field.
+ *
+ * b) Waits (with a timeout) for the owning context to unlock the
+ * console.
+ *
+ * c) Takes the lock and clears the 'req_prio' field.
+ *
+ * The owning context:
+ *
+ * a) Observes the 'req_prio' field set on exit from the unsafe
+ * console state.
+ *
+ * b) Gives up console ownership by clearing the 'prio' and 'cpu'
+ * fields.
+ *
+ * 3) Unsafe hostile takeover allows to take over the lock even when
+ * the console is an unsafe state. It is used only in panic() by
+ * the final attempt to flush consoles in a try and hope mode.
+ *
+ * The release function simply clears the 'prio' and 'cpu' fields.
+ *
+ * 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 spin-wait if there is already an owner and the
+ * console is in an unsafe state.
+ * - Whether to attempt an unsafe hostile takeover.
+ *
+ * The design allows to implement the well known:
+ *
+ * acquire()
+ * output_one_printk_record()
+ * release()
+ *
+ * The output of one printk record might be interrupted with a higher
+ * priority context. The new owner is supposed to reprint the entire
+ * record from scratch.
*/
/**
@@ -85,22 +143,22 @@ static inline bool nbcon_state_try_cmpxchg(struct console *con, struct nbcon_sta
* @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.
+ * Acquire the console when it is released. Also acquire the console when
+ * the current owner has a lower priority and the console is in a safe state.
+ *
+ * Return: 0 on success. Otherwise, an error code on failure. Also @cur
+ * is updated to the latest state when failed to modify it.
*
* Errors:
*
- * -EPERM: A panic is in progress and this is not the panic CPU
- * or this context does not have a priority above the
- * current owner or waiter. No acquire method can be
- * successful for this context.
+ * -EPERM: A panic is in progress and this is not the panic CPU.
+ * Or the current owner or waiter has the same or higher
+ * priority. No acquire method can be successful in
+ * this case.
*
- * -EBUSY: The console is in an unsafe state. The caller should
- * try using the handover acquire method.
- *
- * The general procedure is to change @nbcon_state::prio from unowned to
- * owned. Or, if the console is not in the unsafe state, to change
- * @nbcon_state::prio to a higher priority owner.
+ * -EBUSY: The current owner has a lower priority but the console
+ * in an unsafe state. The caller should try using
+ * the handover acquire method.
*/
static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
struct nbcon_state *cur)
@@ -133,8 +191,6 @@ static int nbcon_context_try_acquire_direct(struct nbcon_context *ctxt,
} while (!nbcon_state_try_cmpxchg(con, cur, &new));
- cur->atom = new.atom;
-
return 0;
}
@@ -166,20 +222,23 @@ static bool nbcon_waiter_matches(struct nbcon_state *cur, int expected_prio)
* @ctxt: The context of the caller
* @cur: The current console state
*
+ * This is a helper function for nbcon_context_try_acquire_handover().
+ * It is called when the console is in an unsafe state. The current
+ * owner will release the console on exit from the unsafe region.
+ *
* 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 and this is not the panic CPU
- * or this context is no longer the waiter. For the
- * former case, the caller must carefully remove the
- * request before aborting the acquire.
+ * or this context is no longer the waiter.
*
* -EBUSY: The console is still locked. The caller should
* continue waiting.
*
- * This is a helper function for nbcon_context_try_acquire_handover().
+ * Note: The caller must still remove the request when an error has occurred
+ * except when this context is no longer the waiter.
*/
static int nbcon_context_try_acquire_requested(struct nbcon_context *ctxt,
struct nbcon_state *cur)
@@ -188,47 +247,40 @@ static int nbcon_context_try_acquire_requested(struct nbcon_context *ctxt,
struct console *con = ctxt->console;
struct nbcon_state new;
- do {
+ /* Note that the caller must still remove the request! */
+ if (other_cpu_in_panic())
+ return -EPERM;
+
+ if (!nbcon_waiter_matches(cur, ctxt->prio))
+ return -EPERM;
+
+ /* If still locked, caller should continue waiting. */
+ if (cur->prio != NBCON_PRIO_NONE)
+ return -EBUSY;
+
+ /*
+ * The previous owner should have never released ownership
+ * in an unsafe region. And this function should not be
+ * called when the 'unsafe_takeover' is set.
+ */
+ WARN_ON_ONCE(cur->unsafe);
+
+ new.atom = cur->atom;
+ new.prio = ctxt->prio;
+ new.req_prio = NBCON_PRIO_NONE;
+ new.unsafe = cur->unsafe_takeover;
+ new.cpu = cpu;
+
+ if (!nbcon_state_try_cmpxchg(con, cur, &new)) {
/*
- * Note: If the acquire is aborted due to a panic CPU,
- * the caller must still remove the request!
+ * The acquire could fail only when it has been taken
+ * over by a higher priority context.
*/
- if (other_cpu_in_panic())
- return -EPERM;
-
- /*
- * If an unsafe hostile takeover has occurred, a handover
- * is no longer possible.
- */
- if (cur->unsafe_takeover)
- return -EPERM;
-
- /* Is this context still the requester? */
- if (!nbcon_waiter_matches(cur, ctxt->prio))
- return -EPERM;
-
- /* If still locked, caller should continue waiting. */
- if (cur->prio != NBCON_PRIO_NONE)
- return -EBUSY;
-
- /*
- * The previous owner should have never released ownership
- * in an unsafe region.
- */
- WARN_ON_ONCE(cur->unsafe);
-
- new.atom = cur->atom;
- new.prio = ctxt->prio;
- new.req_prio = NBCON_PRIO_NONE;
- new.unsafe = cur->unsafe_takeover;
- new.cpu = cpu;
-
- } while (!nbcon_state_try_cmpxchg(con, cur, &new));
+ WARN_ON_ONCE(nbcon_waiter_matches(cur, ctxt->prio));
+ return -EPERM;
+ }
/* Handover success. This context now owns the console. */
-
- cur->atom = new.atom;
-
return 0;
}
@@ -237,30 +289,37 @@ static int nbcon_context_try_acquire_requested(struct nbcon_context *ctxt,
* @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.
+ * The function must be called only when the context has higher priority
+ * than the current owner and the console is in an unsafe state.
+ * It is the case when nbcon_context_try_acquire_direct() returns -EBUSY.
+ *
+ * The function sets "req_prio" field to make the current owner aware of
+ * the request. Then it waits until the current context until the current
+ * owner releases the console, or even higher context takes over the request,
+ * or timeout expires.
+ *
+ * The current owner checks the "req_prio" field on exit from the unsafe
+ * region and releases the console. It does not touch "req_prio" filed
+ * so that the console stays reserved for the waiter.
+ *
+ * Return: 0 on success. Otherwise, an error code on failure. Also @cur
+ * is updated to the latest state when failed to modify it.
*
* Errors:
*
- * -EPERM: This context is on the same CPU as the current owner
- * or the console is permanently in an unsafe state or
- * this context is unwilling to wait or a panic is in
- * progress and this is not the panic CPU. This is not
- * the panic context, so no acquire method can be
- * successful for this context.
+ * -EPERM: A panic is in progress and this is not the panic CPU.
+ * A higher priority context has taken over the console
+ * or the handover request.
*
- * -EBUSY: The current owner or waiter is such that this context
- * is not able to execute a handover. The caller can use
- * an unsafe hostile takeover to acquire.
+ * -EBUSY: The current owner is on the same CPU so that the hand
+ * shake could not work. The current owner is not willing
+ * to wait (zero timeout). The console does not enter
+ * the safe state before timeout passed. The caller
+ * might still use the unsafe hostile takeover when
+ * allowed.
*
- * -EAGAIN: @cur is not the current console state. @cur has been
- * updated. The caller should retry with direct acquire.
- *
- * The general procedure is to set @req_prio and wait until unowned. Then
- * set @prio (claiming ownership) and clearing @req_prio.
- *
- * Note that it is expected that the caller tried
- * nbcon_context_try_acquire_direct() with @cur before calling this function.
+ * -EAGAIN: @cur has changed when creating the handover request.
+ * The caller should retry with direct acquire.
*/
static int nbcon_context_try_acquire_handover(struct nbcon_context *ctxt,
struct nbcon_state *cur)
@@ -269,20 +328,35 @@ static int nbcon_context_try_acquire_handover(struct nbcon_context *ctxt,
struct console *con = ctxt->console;
struct nbcon_state new;
int timeout;
- int err = 0;
+ int request_err = -EBUSY;
/*
- * Handovers are not possible on the same CPU or when the console
- * is permanently in an unsafe state or if the caller is unwilling
- * to wait.
+ * Check that the handover is called when the direct acquire failed
+ * with -EBUSY.
*/
- if (cur->cpu == cpu ||
- cur->unsafe_takeover ||
- ctxt->spinwait_max_us == 0) {
- goto fail_handover;
- }
+ WARN_ON_ONCE(ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio);
+ WARN_ON_ONCE(!cur->unsafe);
- /* Setup a request for handover. */
+ /* Handover is not possible on the same CPU. */
+ if (cur->cpu == cpu)
+ return -EBUSY;
+
+ /*
+ * Console stays unsafe after an unsafe takeover until re-initialized.
+ * Waiting is not going to help in this case.
+ */
+ if (cur->unsafe_takeover)
+ return -EBUSY;
+
+ /* Is the caller willing to wait? */
+ if (ctxt->spinwait_max_us == 0)
+ return -EBUSY;
+
+
+ /*
+ * Setup a request for the handover. The caller should try to acquire
+ * the conosle directly when the current state has been modified.
+ */
new.atom = cur->atom;
new.req_prio = ctxt->prio;
if (!nbcon_state_try_cmpxchg(con, cur, &new))
@@ -290,18 +364,18 @@ static int nbcon_context_try_acquire_handover(struct nbcon_context *ctxt,
cur->atom = new.atom;
- /* Wait until there is no owner and then acquire directly. */
+ /* Wait until there is no owner and then acquire the console. */
for (timeout = ctxt->spinwait_max_us; timeout >= 0; timeout--) {
/* On successful acquire, this request is cleared. */
- err = nbcon_context_try_acquire_requested(ctxt, cur);
- if (!err)
+ request_err = nbcon_context_try_acquire_requested(ctxt, cur);
+ if (!request_err)
return 0;
/*
* If the acquire should be aborted, it must be ensured
* that the request is removed before returning to caller.
*/
- if (err == -EPERM)
+ if (request_err == -EPERM)
break;
udelay(1);
@@ -310,11 +384,11 @@ static int nbcon_context_try_acquire_handover(struct nbcon_context *ctxt,
nbcon_state_read(con, cur);
}
- /* Timed out or should abort. Carefully remove handover request. */
+ /* Timed out or aborted. Carefully remove handover request. */
do {
/* No need to remove request if there is a new waiter. */
if (!nbcon_waiter_matches(cur, ctxt->prio))
- goto fail_handover;
+ return -EPERM;
/* Unset request for handover. */
new.atom = cur->atom;
@@ -325,7 +399,7 @@ static int nbcon_context_try_acquire_handover(struct nbcon_context *ctxt,
* acquiring via handover.
*/
cur->atom = new.atom;
- goto fail_handover;
+ return request_err;
}
/*
@@ -334,40 +408,48 @@ static int nbcon_context_try_acquire_handover(struct nbcon_context *ctxt,
*/
} while (nbcon_context_try_acquire_requested(ctxt, cur));
- /* Acquire at timeout succeeded! */
+ /* Lucky timing. The acquire succeeded while removing the request. */
return 0;
-
-fail_handover:
- /*
- * If this is the panic context, the caller can try to acquire using
- * the unsafe hostile takeover method.
- */
- if (ctxt->prio == NBCON_PRIO_PANIC &&
- ctxt->prio > cur->prio &&
- ctxt->prio > cur->req_prio &&
- !other_cpu_in_panic()) {
- return -EBUSY;
- }
- return -EPERM;
}
/**
- * nbcon_context_acquire_hostile - Acquire via unsafe hostile takeover
+ * nbcon_context_try_acquire_hostile - Acquire via unsafe hostile takeover
* @ctxt: The context of the caller
* @cur: The current console state
*
- * @cur is updated to the new console state.
+ * Acquire the console even in the unsafe state.
*
- * The general procedure is to set @prio (forcing ownership). This method
- * must only be used as a final attempt during panic.
+ * It can be permitted by setting the 'allow_unsafe_takeover' field only
+ * by the final attempt to flush messages in panic().
+ *
+ * Return: 0 on success. -EPERM when not allowed by the context.
*/
-static void nbcon_context_acquire_hostile(struct nbcon_context *ctxt,
- struct nbcon_state *cur)
+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;
+ if (!ctxt->allow_unsafe_takeover)
+ return -EPERM;
+
+ /*
+ * Check that the system is going to die and lower priorities will
+ * always be ignored. Using an unsafe consoles could break the running
+ * system. Also switching back to lower priorities would create a race
+ * in nbcon_waiter_matches().
+ */
+ if (WARN_ON_ONCE(ctxt->prio != NBCON_PRIO_PANIC))
+ return -EPERM;
+
+ /*
+ * Check that try_acquire_direct() and try_acquire_handover() returned
+ * -EBUSY in the right situation.
+ */
+ WARN_ON_ONCE(ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio);
+ WARN_ON_ONCE(cur->unsafe != true);
+
do {
new.atom = cur->atom;
new.cpu = cpu;
@@ -377,7 +459,7 @@ static void nbcon_context_acquire_hostile(struct nbcon_context *ctxt,
} while (!nbcon_state_try_cmpxchg(con, cur, &new));
- cur->atom = new.atom;
+ return 0;
}
/**
@@ -400,33 +482,21 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
struct nbcon_state cur;
int err;
- /* unsafe hostile takeovers are only allowed for panic */
- WARN_ON_ONCE(ctxt->allow_unsafe_takeover && (ctxt->prio != NBCON_PRIO_PANIC));
-
nbcon_state_read(con, &cur);
+try_again:
+ err = nbcon_context_try_acquire_direct(ctxt, &cur);
+ if (err != -EBUSY)
+ goto out;
- do {
- err = nbcon_context_try_acquire_direct(ctxt, &cur);
- if (!err)
- goto success;
- else if (err == -EPERM)
- return false;
+ err = nbcon_context_try_acquire_handover(ctxt, &cur);
+ if (err == -EAGAIN)
+ goto try_again;
+ if (err != -EBUSY)
+ goto out;
- err = nbcon_context_try_acquire_handover(ctxt, &cur);
- if (!err)
- goto success;
- else if (err == -EPERM)
- return false;
-
- } while (err == -EAGAIN);
-
- /* Only attempt unsafe hostile takeover if explicitly requested. */
- if (!ctxt->allow_unsafe_takeover)
- return false;
-
- nbcon_context_acquire_hostile(ctxt, &cur);
-success:
- return true;
+ err = nbcon_context_try_acquire_hostile(ctxt, &cur);
+out:
+ return !err;
}
static bool nbcon_owner_matches(struct nbcon_state *cur, int expected_cpu,
--
2.35.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH v4 2+/8] printk: nbcon: Add acquire/release logic
2023-09-14 13:23 ` [PATCH v4 2+/8] " Petr Mladek
@ 2023-09-15 17:01 ` John Ogness
0 siblings, 0 replies; 18+ messages in thread
From: John Ogness @ 2023-09-15 17:01 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On 2023-09-14, Petr Mladek <pmladek@suse.com> wrote:
> // This patch includes my proposed changes on top of
> // https://lore.kernel.org/all/20230908185008.468566-3-john.ogness@linutronix.de/
The changes in the code are fine. I agree with them and they pass all my
runtime tests. However, there are a couple places where I disagree with
your comments (or lack thereof) and would like to add some
clarification.
Following is your patched version, with my comments inline:
> static int nbcon_context_try_acquire_requested(struct nbcon_context *ctxt,
> struct nbcon_state *cur)
> {
> unsigned int cpu = smp_processor_id();
> struct console *con = ctxt->console;
> struct nbcon_state new;
>
> /* Note that the caller must still remove the request! */
> if (other_cpu_in_panic())
> return -EPERM;
You have no comments here. The following waiter check is an obvious
check to make. But what I think is not obvious, is that it will also
catch the situation when an unsafe hostile takeover has occurred. (My v4
had an explicit test for that case, but actually it is enough to check
the waiter.) I am adding comments here to mention that.
/*
* Note that the waiter will also change if there was an unsafe
* hostile takeover.
*/
> if (!nbcon_waiter_matches(cur, ctxt->prio))
> return -EPERM;
>
> /* If still locked, caller should continue waiting. */
> if (cur->prio != NBCON_PRIO_NONE)
> return -EBUSY;
>
> /*
> * The previous owner should have never released ownership
> * in an unsafe region. And this function should not be
> * called when the 'unsafe_takeover' is set.
> */
The above comment says this function should not be called when
@unsafe_takeover is set. But that can legally occur. While spinning
there is a udelay(1) and then the state is re-read. During the udelay()
an unsafe hostile takeover is allowed to occur. This is another reason
why I wanted to explicitly mention that in the comment above. And I am
removing the 2nd sentence of this comment.
> WARN_ON_ONCE(cur->unsafe);
[...]
> 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 request_err = -EBUSY;
>
> /*
> * Check that the handover is called when the direct acquire failed
> * with -EBUSY.
> */
> WARN_ON_ONCE(ctxt->prio <= cur->prio || ctxt->prio <= cur->req_prio);
> WARN_ON_ONCE(!cur->unsafe);
>
> /* Handover is not possible on the same CPU. */
> if (cur->cpu == cpu)
> return -EBUSY;
>
> /*
> * Console stays unsafe after an unsafe takeover until re-initialized.
> * Waiting is not going to help in this case.
> */
> if (cur->unsafe_takeover)
> return -EBUSY;
>
> /* Is the caller willing to wait? */
> if (ctxt->spinwait_max_us == 0)
> return -EBUSY;
>
> /*
> * Setup a request for the handover. The caller should try to acquire
> * the conosle directly when the current state has been modified.
> */
> new.atom = cur->atom;
> new.req_prio = ctxt->prio;
> if (!nbcon_state_try_cmpxchg(con, cur, &new))
> return -EAGAIN;
>
> cur->atom = new.atom;
>
> /* Wait until there is no owner and then acquire the console. */
> for (timeout = ctxt->spinwait_max_us; timeout >= 0; timeout--) {
> /* On successful acquire, this request is cleared. */
> request_err = nbcon_context_try_acquire_requested(ctxt, cur);
> if (!request_err)
> return 0;
>
> /*
> * If the acquire should be aborted, it must be ensured
> * that the request is removed before returning to caller.
> */
> if (request_err == -EPERM)
> break;
>
> udelay(1);
>
> /* Re-read the state because some time has passed. */
> nbcon_state_read(con, cur);
> }
>
> /* Timed out or aborted. Carefully remove handover request. */
> do {
> /* No need to remove request if there is a new waiter. */
For me it was not obvious why -EPERM should be returned here. I am
extending this comment to provide some more details that may not be
immediately obvious.
/*
* No need to remove request if there is a new waiter. This
* can only happen if a higher priority context has taken over
* the console or the handover request.
*/
> if (!nbcon_waiter_matches(cur, ctxt->prio))
> return -EPERM;
[...]
> 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;
>
> if (!ctxt->allow_unsafe_takeover)
> return -EPERM;
>
> /*
> * Check that the system is going to die and lower priorities will
> * always be ignored. Using an unsafe consoles could break the running
> * system. Also switching back to lower priorities would create a race
> * in nbcon_waiter_matches().
> */
Three things about this comment. First, it is not just a "check" but
actually an "enforcement". The if-statement prevents the hostile
takeover from happening.
Second, it mentions that "lower priorities will always be ignored". I
was confused by this because lower priorities are _always_ ignored. Only
higher priorities can ever take over ownership.
Third, it mentions switching back to lower priorities would create races
in nbcon_waiter_matches(). This seems like a strange place to talk about
that race. The comments within nbcon_waiter_matches() talk about why
that race does not exist. The unsafe hostile takeover is not a part of
that explanation.
It should also be clear that after all pending records have been flushed
in panic, the panic CPU _will_ release the console. What will prevent
other CPUs from taking ownership after that is _not_ a PANIC-prio owner
of the console, but rather because it is a panic situation and other
CPUs are not the panic CPU.
I am just simplifying the comment to:
/* Ensure caller is allowed to perform unsafe hostile takeovers. */
> if (WARN_ON_ONCE(ctxt->prio != NBCON_PRIO_PANIC))
> return -EPERM;
John Ogness
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH printk v4 3/8] printk: Make static printk buffers available to nbcon
2023-09-08 18:50 [PATCH printk v4 0/8] provide nbcon base John Ogness
2023-09-08 18:50 ` [PATCH printk v4 1/8] printk: Add non-BKL (nbcon) console basic infrastructure John Ogness
2023-09-08 18:50 ` [PATCH printk v4 2/8] printk: nbcon: Add acquire/release logic John Ogness
@ 2023-09-08 18:50 ` John Ogness
2023-09-14 13:29 ` Petr Mladek
2023-09-08 18:50 ` [PATCH printk v4 4/8] printk: nbcon: Add buffer management John Ogness
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: John Ogness @ 2023-09-08 18:50 UTC (permalink / raw)
To: Petr Mladek
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
The nbcon boot consoles also need printk buffers that are available
very early. Since the nbcon boot consoles will also be serialized
by the console_lock, they can use the same static printk buffers
that the legacy consoles are using.
Make the legacy static printk buffers available outside of printk.c
so they can be used by nbcon.c.
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
kernel/printk/internal.h | 2 ++
kernel/printk/printk.c | 13 +++++++++----
2 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 2ca0ab78802c..7199d60bfc25 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -86,6 +86,8 @@ static inline void nbcon_cleanup(struct console *con) { }
#endif /* CONFIG_PRINTK */
+extern struct printk_buffers printk_shared_pbufs;
+
/**
* struct printk_buffers - Buffers to read/format/output printk messages.
* @outbuf: After formatting, contains text to output.
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c0246093ea41..9a2ddab16abe 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2844,6 +2844,13 @@ static bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
return true;
}
+/*
+ * Used as the printk buffers for non-panic, serialized console printing.
+ * This is for legacy (!CON_NBCON) as well as all boot (CON_BOOT) consoles.
+ * Its usage requires the console_lock held.
+ */
+struct printk_buffers printk_shared_pbufs;
+
/*
* Print one record for the given console. The record printed is whatever
* record is the next available record for the given console.
@@ -2861,12 +2868,10 @@ static bool printk_get_next_message(struct printk_message *pmsg, u64 seq,
*/
static bool console_emit_next_record(struct console *con, bool *handover, int cookie)
{
- static struct printk_buffers pbufs;
-
bool is_extended = console_srcu_read_flags(con) & CON_EXTENDED;
- char *outbuf = &pbufs.outbuf[0];
+ char *outbuf = &printk_shared_pbufs.outbuf[0];
struct printk_message pmsg = {
- .pbufs = &pbufs,
+ .pbufs = &printk_shared_pbufs,
};
unsigned long flags;
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH printk v4 3/8] printk: Make static printk buffers available to nbcon
2023-09-08 18:50 ` [PATCH printk v4 3/8] printk: Make static printk buffers available to nbcon John Ogness
@ 2023-09-14 13:29 ` Petr Mladek
0 siblings, 0 replies; 18+ messages in thread
From: Petr Mladek @ 2023-09-14 13:29 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Fri 2023-09-08 20:56:03, John Ogness wrote:
> The nbcon boot consoles also need printk buffers that are available
> very early. Since the nbcon boot consoles will also be serialized
> by the console_lock, they can use the same static printk buffers
> that the legacy consoles are using.
>
> Make the legacy static printk buffers available outside of printk.c
> so they can be used by nbcon.c.
>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH printk v4 4/8] printk: nbcon: Add buffer management
2023-09-08 18:50 [PATCH printk v4 0/8] provide nbcon base John Ogness
` (2 preceding siblings ...)
2023-09-08 18:50 ` [PATCH printk v4 3/8] printk: Make static printk buffers available to nbcon John Ogness
@ 2023-09-08 18:50 ` John Ogness
2023-09-14 14:55 ` Petr Mladek
2023-09-08 18:50 ` [PATCH printk v4 5/8] printk: nbcon: Add ownership state functions John Ogness
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: John Ogness @ 2023-09-08 18:50 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 non-boot console upon console
registration. This buffer is used by the console owner when not
in panic context. (For boot consoles, the existing shared global
legacy output buffer is used instead. Boot console printing will
be synchronized with legacy console printing.)
- 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 | 12 ++++++--
kernel/printk/nbcon.c | 65 ++++++++++++++++++++++++++++++++++++----
kernel/printk/printk.c | 17 ++++++-----
4 files changed, 87 insertions(+), 14 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index eeebb82d6d07..b7279ebafe0f 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -234,6 +234,7 @@ enum nbcon_prio {
};
struct console;
+struct printk_buffers;
/**
* struct nbcon_context - Context for console acquire/release
@@ -244,6 +245,7 @@ struct console;
* be used only with NBCON_PRIO_PANIC @prio. It
* might cause a system freeze when the console
* is used later.
+ * @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 spinwait_max_us;
enum nbcon_prio prio;
unsigned int allow_unsafe_takeover : 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 7199d60bfc25..f6161cd75d7d 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
@@ -63,8 +69,9 @@ void defer_console_output(void);
u16 printk_parse_prefix(const char *text, int *level,
enum printk_info_flags *flags);
+bool nbcon_alloc(struct console *con);
void nbcon_init(struct console *con);
-void nbcon_cleanup(struct console *con);
+void nbcon_free(struct console *con);
#else
@@ -81,8 +88,9 @@ 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 bool nbcon_alloc(struct console *con) { return false; }
static inline void nbcon_init(struct console *con) { }
-static inline void nbcon_cleanup(struct console *con) { }
+static inline void nbcon_free(struct console *con) { }
#endif /* CONFIG_PRINTK */
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 57ddfb7f0994..cad1cf663078 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/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 directly
* take over if the console is not in an unsafe state by carefully writing
@@ -380,6 +384,8 @@ static void nbcon_context_acquire_hostile(struct nbcon_context *ctxt,
cur->atom = new.atom;
}
+static struct printk_buffers panic_nbcon_pbufs;
+
/**
* nbcon_context_try_acquire - Try to acquire nbcon console
* @ctxt: The context of the caller
@@ -394,7 +400,6 @@ static void nbcon_context_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;
@@ -426,6 +431,12 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
nbcon_context_acquire_hostile(ctxt, &cur);
success:
+ /* 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;
}
@@ -465,7 +476,7 @@ static void nbcon_context_release(struct nbcon_context *ctxt)
do {
if (!nbcon_owner_matches(&cur, cpu, ctxt->prio))
- return;
+ break;
new.atom = cur.atom;
new.prio = NBCON_PRIO_NONE;
@@ -477,26 +488,70 @@ static void nbcon_context_release(struct nbcon_context *ctxt)
new.unsafe |= cur.unsafe_takeover;
} while (!nbcon_state_try_cmpxchg(con, &cur, &new));
+
+ ctxt->pbufs = NULL;
+}
+
+/**
+ * nbcon_alloc - Allocate buffers needed by the nbcon console
+ * @con: Console to allocate buffers for
+ *
+ * Return: True on success. False otherwise and the console cannot
+ * be used.
+ *
+ * This is not part of nbcon_init() because buffer allocation must
+ * be performed earlier in the console registration process.
+ */
+bool nbcon_alloc(struct console *con)
+{
+ if (con->flags & CON_BOOT) {
+ /*
+ * Boot console printing is synchronized with legacy console
+ * printing, so boot consoles can share the same global printk
+ * buffers.
+ */
+ con->pbufs = &printk_shared_pbufs;
+ } else {
+ con->pbufs = kmalloc(sizeof(*con->pbufs), GFP_KERNEL);
+ if (!con->pbufs) {
+ con_printk(KERN_ERR, con, "failed to allocate printing buffer\n");
+ return false;
+ }
+ }
+
+ return true;
}
/**
* nbcon_init - Initialize the nbcon console specific data
* @con: Console to initialize
+ *
+ * nbcon_alloc() *must* be called and succeed before this function
+ * is called.
*/
void nbcon_init(struct console *con)
{
struct nbcon_state state = { };
+ /* nbcon_alloc() must have been called and successful! */
+ BUG_ON(!con->pbufs);
+
nbcon_state_set(con, &state);
}
/**
- * nbcon_cleanup - Cleanup the nbcon console specific data
- * @con: Console to cleanup
+ * nbcon_free - Free and cleanup the nbcon console specific data
+ * @con: Console to free/cleanup nbcon data
*/
-void nbcon_cleanup(struct console *con)
+void nbcon_free(struct console *con)
{
struct nbcon_state state = { };
nbcon_state_set(con, &state);
+
+ /* Boot consoles share global printk buffers. */
+ if (!(con->flags & CON_BOOT))
+ kfree(con->pbufs);
+
+ con->pbufs = NULL;
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9a2ddab16abe..d1c43fa1ffd3 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3329,12 +3329,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;
@@ -3448,6 +3442,15 @@ void register_console(struct console *newcon)
goto unlock;
}
+ if (newcon->flags & CON_NBCON) {
+ /*
+ * Ensure the nbcon console buffers can be allocated
+ * before modifying any global data.
+ */
+ if (!nbcon_alloc(newcon))
+ goto unlock;
+ }
+
/*
* See if we want to enable this console driver by default.
*
@@ -3587,7 +3590,7 @@ static int unregister_console_locked(struct console *console)
synchronize_srcu(&console_srcu);
if (console->flags & CON_NBCON)
- nbcon_cleanup(console);
+ nbcon_free(console);
console_sysfs_notify();
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH printk v4 4/8] printk: nbcon: Add buffer management
2023-09-08 18:50 ` [PATCH printk v4 4/8] printk: nbcon: Add buffer management John Ogness
@ 2023-09-14 14:55 ` Petr Mladek
0 siblings, 0 replies; 18+ messages in thread
From: Petr Mladek @ 2023-09-14 14:55 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-09-08 20:56:04, 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 non-boot console upon console
> registration. This buffer is used by the console owner when not
> in panic context. (For boot consoles, the existing shared global
> legacy output buffer is used instead. Boot console printing will
> be synchronized with legacy console printing.)
>
> - Choosing the appropriate buffer is handled in the acquire/release
> functions.
>
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -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 directly
> * take over if the console is not in an unsafe state by carefully writing
The above chunk had a merge conflict with my proposed changes for
the 2nd patch. It uses a completely different text from the commit
message. I added the info into the new text the following way:
@@ -73,6 +74,10 @@
* the console is an unsafe state. It is used only in panic() by
* the final attempt to flush consoles in a try and hope mode.
*
+ * Note that separate record buffers are used in panic(). As a result,
+ * the messages can be read and formatted without any risk even after
+ * using the hostile takeover in unsafe state.
+ *
* The release function simply clears the 'prio' and 'cpu' fields.
*
* All operations on @console::nbcon_state are atomic cmpxchg based
> @@ -426,6 +431,12 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
>
> nbcon_context_acquire_hostile(ctxt, &cur);
> success:
> + /* 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;
> }
Also the above chunk had conflict with the proposed changes. I
resolbed it the following way:
@@ -496,7 +502,18 @@ static bool nbcon_context_try_acquire(struct nbcon_context *ctxt)
err = nbcon_context_try_acquire_hostile(ctxt, &cur);
out:
- return !err;
+ if (err)
+ return false;
+
+ /* Acquire succeded */
+
+ /* 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;
}
static bool nbcon_owner_matches(struct nbcon_state *cur, int expected_cpu,
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3448,6 +3442,15 @@ void register_console(struct console *newcon)
> goto unlock;
> }
>
> + if (newcon->flags & CON_NBCON) {
> + /*
> + * Ensure the nbcon console buffers can be allocated
> + * before modifying any global data.
> + */
> + if (!nbcon_alloc(newcon))
> + goto unlock;
> + }
> +
> /*
> * See if we want to enable this console driver by default.
> *
We have to call nbcon_free() when try_enable_*_console() failed.
Something like:
@@ -3484,8 +3484,10 @@ void register_console(struct console *newcon)
err = try_enable_preferred_console(newcon, false);
/* printk() messages are not printed to the Braille console. */
- if (err || newcon->flags & CON_BRL)
+ if (err || newcon->flags & CON_BRL) {
+ nbcon_free(newcon);
goto unlock;
+ }
/*
* If we have a bootconsole, and are switching to a real console,
With the proposed changes:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH printk v4 5/8] printk: nbcon: Add ownership state functions
2023-09-08 18:50 [PATCH printk v4 0/8] provide nbcon base John Ogness
` (3 preceding siblings ...)
2023-09-08 18:50 ` [PATCH printk v4 4/8] printk: nbcon: Add buffer management John Ogness
@ 2023-09-08 18:50 ` John Ogness
2023-09-14 15:38 ` Petr Mladek
2023-09-08 18:50 ` [PATCH printk v4 6/8] printk: nbcon: Add sequence handling John Ogness
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: John Ogness @ 2023-09-08 18:50 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 has been taken over by another
context. If a handover request is pending, this function will
also perform the handover, thus cancelling its own ownership.
- nbcon_context_enter_unsafe()/nbcon_context_exit_unsafe()
Invoked by a console owner to denote that the driver is about
to enter or leave a critical region where a take over is unsafe.
These functions are also cancellation points 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 and possibly 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>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
kernel/printk/nbcon.c | 110 +++++++++++++++++++++++++++++++++++++++++-
1 file changed, 109 insertions(+), 1 deletion(-)
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index cad1cf663078..644c4b9a4540 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -464,7 +464,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();
@@ -492,6 +491,115 @@ 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 this context still owns the console. 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 no longer owns
+ * the console 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 still owns the console. */
+ if (!nbcon_owner_matches(cur, cpu, ctxt->prio))
+ 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)
+ return true;
+
+ /* Waiters always have higher priorities than owners. */
+ WARN_ON_ONCE(cur->req_prio <= cur->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;
+}
+
+#define nbcon_context_enter_unsafe(c) __nbcon_context_update_unsafe(c, true)
+#define nbcon_context_exit_unsafe(c) __nbcon_context_update_unsafe(c, 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 unsafe state was updated and this context still
+ * owns the console. Otherwise false if ownership was handed
+ * over or taken.
+ *
+ * This function allows console owners to modify the unsafe status of the
+ * console.
+ *
+ * When this function returns false then the calling context no longer owns
+ * the console 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);
+
+ do {
+ /*
+ * The unsafe bit must not be cleared if an
+ * unsafe hostile takeover has occurred.
+ */
+ if (!unsafe && cur.unsafe_takeover)
+ goto out;
+
+ if (!nbcon_context_can_proceed(ctxt, &cur))
+ return false;
+
+ new.atom = cur.atom;
+ new.unsafe = unsafe;
+ } while (!nbcon_state_try_cmpxchg(con, &cur, &new));
+
+ cur.atom = new.atom;
+out:
+ return nbcon_context_can_proceed(ctxt, &cur);
+}
+
/**
* nbcon_alloc - Allocate buffers needed by the nbcon console
* @con: Console to allocate buffers for
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH printk v4 5/8] printk: nbcon: Add ownership state functions
2023-09-08 18:50 ` [PATCH printk v4 5/8] printk: nbcon: Add ownership state functions John Ogness
@ 2023-09-14 15:38 ` Petr Mladek
0 siblings, 0 replies; 18+ messages in thread
From: Petr Mladek @ 2023-09-14 15:38 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel
On Fri 2023-09-08 20:56:05, 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:
>
> - nbcon_context_can_proceed()
>
> Invoked by a console owner to check whether a handover request
> is pending or whether the console has been taken over by another
> context. If a handover request is pending, this function will
> also perform the handover, thus cancelling its own ownership.
>
> - nbcon_context_enter_unsafe()/nbcon_context_exit_unsafe()
>
> Invoked by a console owner to denote that the driver is about
> to enter or leave a critical region where a take over is unsafe.
> These functions are also cancellation points where loss of
> ownership can occur.
The last sentence is not completely clear to me. I would write:
The exit variant is the point where the current owner releases
the lock for a higher priority context which asked for
the friendly handover.
> 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 and possibly take a special emergency path.
>
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -492,6 +491,115 @@ 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 this context still owns the console. 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.
I think that is not longer called explicitly in nbcon_emit_next_record().
I would write something like:
* Must be invoked when entering the unsafe state to make sure that it still
* owns the lock. Also must be invoked when exiting the unsafe context
* to eventually free the lock for a higher priority context which asked
* for the friendly handover.
*
* It can be called inside an unsafe section when the console is just
* temporary in safe state instead of exiting and entering the unsafe
* state.
*
* Also it can be called in the safe context before doing an expensive
* safe operation. It does not make sense to do the operation when
* a higher priority context took the lock.
> + *
> + * When this function returns false then the calling context no longer owns
> + * the console 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();
With the proposed changes:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH printk v4 6/8] printk: nbcon: Add sequence handling
2023-09-08 18:50 [PATCH printk v4 0/8] provide nbcon base John Ogness
` (4 preceding siblings ...)
2023-09-08 18:50 ` [PATCH printk v4 5/8] printk: nbcon: Add ownership state functions John Ogness
@ 2023-09-08 18:50 ` John Ogness
2023-09-14 16:02 ` Petr Mladek
2023-09-08 18:50 ` [PATCH printk v4 7/8] printk: nbcon: Add emit function and callback function for atomic printing John Ogness
2023-09-08 18:50 ` [PATCH printk v4 8/8] printk: nbcon: Allow drivers to mark unsafe regions and check state John Ogness
7 siblings, 1 reply; 18+ messages in thread
From: John Ogness @ 2023-09-08 18:50 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>
Reviewed-by: Petr Mladek <pmladek@suse.com>
---
include/linux/console.h | 4 ++
kernel/printk/internal.h | 7 +++
kernel/printk/nbcon.c | 125 +++++++++++++++++++++++++++++++++++++--
kernel/printk/printk.c | 31 +++++++---
4 files changed, 155 insertions(+), 12 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index b7279ebafe0f..9d2580a33308 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -246,6 +246,7 @@ struct printk_buffers;
* might cause a system freeze when the console
* is used later.
* @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 f6161cd75d7d..6473f5ae4a18 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,8 @@ enum printk_info_flags {
LOG_CONT = 8, /* text is a fragment of a continuation line */
};
+extern struct printk_ringbuffer *prb;
+
__printf(4, 0)
int vprintk_store(int facility, int level,
const struct dev_printk_info *dev_info,
@@ -69,6 +72,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_alloc(struct console *con);
void nbcon_init(struct console *con);
void nbcon_free(struct console *con);
@@ -88,6 +93,8 @@ void nbcon_free(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_alloc(struct console *con) { return false; }
static inline void nbcon_init(struct console *con) { }
static inline void nbcon_free(struct console *con) { }
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index 644c4b9a4540..d23aa132fdcb 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -84,6 +84,112 @@ 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);
}
+#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 */
+
+/**
+ * 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 especially important on 32bit systems because 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), __seq_to_nbcon_seq(seq));
+
+ /* Clear con->seq since nbcon consoles use con->nbcon_seq instead. */
+ con->seq = 0;
+}
+
+/**
+ * nbcon_seq_read - 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 nbcon_seq = atomic_long_read(&ACCESS_PRIVATE(con, nbcon_seq));
+
+ return __nbcon_seq_to_seq(nbcon_seq);
+}
+
+/**
+ * nbcon_seq_force - Force console sequence to a specific value
+ * @con: Console to work on
+ * @seq: Sequence number value to set
+ *
+ * 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), __seq_to_nbcon_seq(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
+ * @new_seq: The new sequence number to set
+ *
+ * @ctxt->seq is updated to the new value of @con::nbcon_seq (expanded to
+ * the 64bit value). This could be a different value than @new_seq if
+ * nbcon_seq_force() was used or the current context no longer owns the
+ * console. In the later case, it will stop printing anyway.
+ */
+__maybe_unused
+static void nbcon_seq_try_update(struct nbcon_context *ctxt, u64 new_seq)
+{
+ unsigned long nbcon_seq = __seq_to_nbcon_seq(ctxt->seq);
+ struct console *con = ctxt->console;
+
+ if (atomic_long_try_cmpxchg(&ACCESS_PRIVATE(con, nbcon_seq), &nbcon_seq,
+ __seq_to_nbcon_seq(new_seq))) {
+ ctxt->seq = new_seq;
+ } else {
+ ctxt->seq = nbcon_seq_read(con);
+ }
+}
+
/**
* nbcon_context_try_acquire_direct - Try to acquire directly
* @ctxt: The context of the caller
@@ -437,6 +543,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;
}
@@ -540,11 +649,14 @@ static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_s
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.
+ * It is not clear whether the waiter really took over ownership. The
+ * outermost callsite must make the final decision whether console
+ * ownership is needed for it to proceed. If yes, it must reacquire
+ * ownership (possibly hostile) before carefully proceeding.
+ *
+ * The calling context no longer owns the console so go back all the
+ * way instead of trying to implement reacquire heuristics in tons of
+ * places.
*/
return false;
}
@@ -636,6 +748,8 @@ bool nbcon_alloc(struct console *con)
*
* nbcon_alloc() *must* be called and succeed before this function
* is called.
+ *
+ * This function expects that the legacy @con->seq has been set.
*/
void nbcon_init(struct console *con)
{
@@ -644,6 +758,7 @@ void nbcon_init(struct console *con)
/* nbcon_alloc() must have been called and successful! */
BUG_ON(!con->pbufs);
+ nbcon_seq_init(con);
nbcon_state_set(con, &state);
}
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d1c43fa1ffd3..325ba9a1f6e7 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -492,7 +492,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
@@ -3166,6 +3166,7 @@ void console_flush_on_panic(enum con_flush_mode mode)
if (mode == CONSOLE_REPLAY_ALL) {
struct console *c;
+ short flags;
int cookie;
u64 seq;
@@ -3173,11 +3174,17 @@ void console_flush_on_panic(enum con_flush_mode mode)
cookie = console_srcu_read_lock();
for_each_console_srcu(c) {
- /*
- * This is an unsynchronized assignment, but the
- * kernel is in "hope and pray" mode anyway.
- */
- c->seq = seq;
+ flags = console_srcu_read_flags(c);
+
+ if (flags & CON_NBCON) {
+ nbcon_seq_force(c, seq);
+ } else {
+ /*
+ * This is an unsynchronized assignment. On
+ * panic legacy consoles are only best effort.
+ */
+ c->seq = seq;
+ }
}
console_srcu_read_unlock(cookie);
}
@@ -3745,6 +3752,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;
int cookie;
u64 diff;
u64 seq;
@@ -3766,6 +3774,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
@@ -3773,7 +3784,13 @@ 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 (flags & CON_NBCON) {
+ printk_seq = nbcon_seq_read(c);
+ } else {
+ printk_seq = c->seq;
+ }
+
if (printk_seq < seq)
diff += seq - printk_seq;
}
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH printk v4 6/8] printk: nbcon: Add sequence handling
2023-09-08 18:50 ` [PATCH printk v4 6/8] printk: nbcon: Add sequence handling John Ogness
@ 2023-09-14 16:02 ` Petr Mladek
0 siblings, 0 replies; 18+ messages in thread
From: Petr Mladek @ 2023-09-14 16:02 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-09-08 20:56:06, 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.
>
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index 644c4b9a4540..d23aa132fdcb 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> +/**
> + * 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 especially important on 32bit systems because 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), __seq_to_nbcon_seq(seq));
> +
> + /* Clear con->seq since nbcon consoles use con->nbcon_seq instead. */
> + con->seq = 0;
> +}
> +
> +/**
> + * nbcon_seq_read - 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 nbcon_seq = atomic_long_read(&ACCESS_PRIVATE(con, nbcon_seq));
> +
> + return __nbcon_seq_to_seq(nbcon_seq);
> +}
> +
> +/**
> + * nbcon_seq_force - Force console sequence to a specific value
> + * @con: Console to work on
> + * @seq: Sequence number value to set
> + *
> + * 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), __seq_to_nbcon_seq(seq));
We should actually do the same trick as in nbcon_seq_init() to make
sure that the 32-bit seq is shrinked against the prb_first_valid_seq().
I mean to do:
/* If the starting record no longer exists, the oldest available record
* is chosen. This is especially important on 32bit systems because 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.
*/
u64 valid_seq = max_t(u64, seq, prb_first_valid_seq(prb));
atomic_long_set(&ACCESS_PRIVATE(con, nbcon_seq), __seq_to_nbcon_seq(valid));
> +}
And we might implement nbcon_seq_init() using nbcon_seq_force(). I mean:
static void nbcon_seq_init(struct console *con)
{
nbcon_seq_force(con->seq);
/* Clear con->seq since nbcon consoles use con->nbcon_seq instead. */
con->seq = 0;
}
> @@ -540,11 +649,14 @@ static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_s
> 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.
> + * It is not clear whether the waiter really took over ownership. The
> + * outermost callsite must make the final decision whether console
> + * ownership is needed for it to proceed. If yes, it must reacquire
> + * ownership (possibly hostile) before carefully proceeding.
> + *
> + * The calling context no longer owns the console so go back all the
> + * way instead of trying to implement reacquire heuristics in tons of
> + * places.
> */
> return false;
> }
This change probably should have been done in the patch introducing
nbcon_context_can_proceed().
> @@ -636,6 +748,8 @@ bool nbcon_alloc(struct console *con)
> *
> * nbcon_alloc() *must* be called and succeed before this function
> * is called.
> + *
> + * This function expects that the legacy @con->seq has been set.
> */
> void nbcon_init(struct console *con)
> {
Best Regards,
Petr
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH printk v4 7/8] printk: nbcon: Add emit function and callback function for atomic printing
2023-09-08 18:50 [PATCH printk v4 0/8] provide nbcon base John Ogness
` (5 preceding siblings ...)
2023-09-08 18:50 ` [PATCH printk v4 6/8] printk: nbcon: Add sequence handling John Ogness
@ 2023-09-08 18:50 ` John Ogness
2023-09-15 8:21 ` Petr Mladek
2023-09-08 18:50 ` [PATCH printk v4 8/8] printk: nbcon: Allow drivers to mark unsafe regions and check state John Ogness
7 siblings, 1 reply; 18+ messages in thread
From: John Ogness @ 2023-09-08 18:50 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 | 21 ++++++++
kernel/printk/internal.h | 6 +++
kernel/printk/nbcon.c | 106 ++++++++++++++++++++++++++++++++++++++-
kernel/printk/printk.c | 9 ++--
4 files changed, 134 insertions(+), 8 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 9d2580a33308..0ce7a2a856ab 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -245,6 +245,7 @@ struct printk_buffers;
* be used only with NBCON_PRIO_PANIC @prio. It
* might cause a system freeze when the console
* is used later.
+ * @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 {
enum nbcon_prio prio;
unsigned int allow_unsafe_takeover : 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
+ * @unsafe_takeover: If a hostile takeover in an unsafe state has occurred
+ */
+struct nbcon_write_context {
+ struct nbcon_context __private ctxt;
+ char *outbuf;
+ unsigned int len;
+ bool unsafe_takeover;
+};
+
/**
* 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;
diff --git a/kernel/printk/internal.h b/kernel/printk/internal.h
index 6473f5ae4a18..6c2afee5ef62 100644
--- a/kernel/printk/internal.h
+++ b/kernel/printk/internal.h
@@ -130,3 +130,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/nbcon.c b/kernel/printk/nbcon.c
index d23aa132fdcb..e2c274f4142e 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -176,7 +176,6 @@ void nbcon_seq_force(struct console *con, u64 seq)
* nbcon_seq_force() was used or the current context no longer owns the
* console. In the later case, it will stop printing anyway.
*/
-__maybe_unused
static void nbcon_seq_try_update(struct nbcon_context *ctxt, u64 new_seq)
{
unsigned long nbcon_seq = __seq_to_nbcon_seq(ctxt->seq);
@@ -683,7 +682,6 @@ static bool nbcon_context_can_proceed(struct nbcon_context *ctxt, struct nbcon_s
*
* 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;
@@ -712,6 +710,110 @@ static bool __nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsaf
return nbcon_context_can_proceed(ctxt, &cur);
}
+/**
+ * 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 this context still owns the console. False if
+ * ownership was handed over or taken.
+ *
+ * When this function returns false then the calling context no longer owns
+ * the console 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;
+
+ /*
+ * The printk buffers are filled within an unsafe section. This
+ * prevents NBCON_PRIO_NORMAL and NBCON_PRIO_EMERGENCY from
+ * clobbering each other.
+ */
+
+ if (!nbcon_context_enter_unsafe(ctxt))
+ return false;
+
+ ctxt->backlog = printk_get_next_message(&pmsg, ctxt->seq, is_extended, true);
+ if (!ctxt->backlog)
+ return nbcon_context_exit_unsafe(ctxt);
+
+ /*
+ * @con->dropped is not protected in case of an unsafe hostile
+ * takeover. 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);
+
+ if (!nbcon_context_exit_unsafe(ctxt))
+ return false;
+
+ /* For skipped records just update seq/dropped in @con. */
+ if (pmsg.outbuf_len == 0)
+ goto update_con;
+
+ /* Initialize the write context for driver callbacks. */
+ wctxt->outbuf = &pmsg.pbufs->outbuf[0];
+ wctxt->len = pmsg.outbuf_len;
+ nbcon_state_read(con, &cur);
+ wctxt->unsafe_takeover = cur.unsafe_takeover;
+
+ 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:
+ /*
+ * The dropped count and the sequence number are updated within an
+ * unsafe section. This limits update races to the panic context and
+ * allows the panic context to win.
+ */
+
+ if (!nbcon_context_enter_unsafe(ctxt))
+ return false;
+
+ if (dropped != con_dropped) {
+ /* Counterpart to the READ_ONCE() above. */
+ WRITE_ONCE(con->dropped, dropped);
+ }
+
+ nbcon_seq_try_update(ctxt, pmsg.seq + 1);
+
+ return nbcon_context_exit_unsafe(ctxt);
+}
+
/**
* nbcon_alloc - Allocate buffers needed by the nbcon console
* @con: Console to allocate buffers for
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 325ba9a1f6e7..683100002de7 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -696,9 +696,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;
@@ -2731,7 +2728,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);
@@ -2785,8 +2782,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;
--
2.39.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH printk v4 7/8] printk: nbcon: Add emit function and callback function for atomic printing
2023-09-08 18:50 ` [PATCH printk v4 7/8] printk: nbcon: Add emit function and callback function for atomic printing John Ogness
@ 2023-09-15 8:21 ` Petr Mladek
0 siblings, 0 replies; 18+ messages in thread
From: Petr Mladek @ 2023-09-15 8:21 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-09-08 20:56:07, 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.
>
> 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>
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH printk v4 8/8] printk: nbcon: Allow drivers to mark unsafe regions and check state
2023-09-08 18:50 [PATCH printk v4 0/8] provide nbcon base John Ogness
` (6 preceding siblings ...)
2023-09-08 18:50 ` [PATCH printk v4 7/8] printk: nbcon: Add emit function and callback function for atomic printing John Ogness
@ 2023-09-08 18:50 ` John Ogness
2023-09-15 8:38 ` Petr Mladek
7 siblings, 1 reply; 18+ messages in thread
From: John Ogness @ 2023-09-08 18:50 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.
Also provide a function for drivers to check if they are still the
owner of the console.
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 | 10 +++++++
kernel/printk/nbcon.c | 66 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+)
diff --git a/include/linux/console.h b/include/linux/console.h
index 0ce7a2a856ab..de8fd92a960b 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -454,6 +454,16 @@ 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);
+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;
extern struct console *early_console;
diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
index e2c274f4142e..04fac73c6e96 100644
--- a/kernel/printk/nbcon.c
+++ b/kernel/printk/nbcon.c
@@ -660,6 +660,32 @@ 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 this context still owns the console. False if
+ * ownership was handed over or taken.
+ *
+ * Must be invoked at appropriate safe places in the driver.
+ *
+ * When this function returns false then the calling context no longer owns
+ * the console 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);
+
#define nbcon_context_enter_unsafe(c) __nbcon_context_update_unsafe(c, true)
#define nbcon_context_exit_unsafe(c) __nbcon_context_update_unsafe(c, false)
@@ -710,6 +736,46 @@ static bool __nbcon_context_update_unsafe(struct nbcon_context *ctxt, bool unsaf
return nbcon_context_can_proceed(ctxt, &cur);
}
+/**
+ * nbcon_enter_unsafe - Enter an unsafe region in the driver
+ * @wctxt: The write context that was handed to the write function
+ *
+ * Return: True if this context still owns the console. False if
+ * ownership was handed over or taken.
+ *
+ * When this function returns false then the calling context no longer owns
+ * the console 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_enter_unsafe(ctxt);
+}
+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 this context still owns the console. False if
+ * ownership was handed over or taken.
+ *
+ * When this function returns false then the calling context no longer owns
+ * the console 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_exit_unsafe(ctxt);
+}
+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] 18+ messages in thread* Re: [PATCH printk v4 8/8] printk: nbcon: Allow drivers to mark unsafe regions and check state
2023-09-08 18:50 ` [PATCH printk v4 8/8] printk: nbcon: Allow drivers to mark unsafe regions and check state John Ogness
@ 2023-09-15 8:38 ` Petr Mladek
0 siblings, 0 replies; 18+ messages in thread
From: Petr Mladek @ 2023-09-15 8:38 UTC (permalink / raw)
To: John Ogness
Cc: Sergey Senozhatsky, Steven Rostedt, Thomas Gleixner, linux-kernel,
Greg Kroah-Hartman
On Fri 2023-09-08 20:56:08, 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.
>
> Also provide a function for drivers to check if they are still the
> owner of the console.
>
> diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> index e2c274f4142e..04fac73c6e96 100644
> --- a/kernel/printk/nbcon.c
> +++ b/kernel/printk/nbcon.c
> @@ -660,6 +660,32 @@ 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 this context still owns the console. False if
> + * ownership was handed over or taken.
> + *
> + * Must be invoked at appropriate safe places in the driver.
This is a bit vague. I guess that enter_unsafe()/exit_unsafe() will be
used most of the time. The guestion is if this need to be called
in another locations explicitely.
I would write something similar as I suggested for nbcon_context_can_proceed():
* It is used in nbcon_enter_unsafe() to make sure that it still owns the lock.
* Also it is used in nbcon_exit_unsafe() to eventually free the lock
* for a higher priority context which asked for the friendly handover.
*
* It can be called inside an unsafe section when the console is just
* temporary in safe state instead of exiting and entering the unsafe
* state.
*
* Also it can be called in the safe context before doing an expensive
* safe operation. It does not make sense to do the operation when
* a higher priority context took the lock.
> + *
> + * When this function returns false then the calling context no longer owns
> + * the console 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);
> +
With the updated comment:
Reviewed-by: Petr Mladek <pmladek@suse.com>
Best Regards,
Petr
^ permalink raw reply [flat|nested] 18+ messages in thread