Hi,
This series is essentially the first 18 patches of tglx's RFC series
[0] with only minor changes in comments and commit messages. It's
purpose is to lay the groundwork for the upcoming threaded/atomic
console printing posted as the RFC series and demonstrated at
LPC2022 [1].
This series is interesting for mainline because it cleans up various
code and documentation quirks discovered while working on the new
console printing implementation.
Aside from cleanups, the main features introduced here are:
- Converts the console's DIY linked list implementation to hlist.
- Introduces a console list lock (mutex) so that readers (such as
/proc/consoles) can safely iterate the consoles without blocking
console printing.
- Adds SRCU support to the console list to prepare for safe console
list iterating from any context.
- Refactors buffer handling to prepare for per-console, per-cpu,
per-context atomic printing.
The series has the following parts:
Patches 1 - 5: Cleanups
Patches 6 - 12: Locking and list conversion
Patches 13 - 18: Improved output buffer handling to prepare for
code sharing
John Ogness
[0] https://lore.kernel.org/lkml/[email protected]
[1] https://lore.kernel.org/lkml/[email protected]
Thomas Gleixner (18):
printk: Make pr_flush() static
printk: Declare log_wait properly
printk: Remove write only variable nr_ext_console_drivers
printk: Remove bogus comment vs. boot consoles
printk: Mark __printk percpu data ready __ro_after_init
printk: Protect [un]register_console() with a mutex
printk: Convert console list walks for readers to list lock
parisc: Put console abuse into one place
serial: kgdboc: Lock console list in probe function
kgbd: Pretend that console list walk is safe
printk: Convert console_drivers list to hlist
printk: Prepare for SCRU console list protection
printk: Move buffer size defines
printk: Document struct console
printk: Add struct cons_text_buf
printk: Use struct cons_text_buf
printk: Use an output descriptor struct for emit
printk: Handle dropped message smarter
arch/parisc/include/asm/pdc.h | 2 +-
arch/parisc/kernel/pdc_cons.c | 55 +++--
arch/parisc/kernel/traps.c | 17 +-
drivers/tty/serial/kgdboc.c | 9 +-
drivers/tty/tty_io.c | 6 +-
fs/proc/consoles.c | 11 +-
fs/proc/kmsg.c | 2 -
include/linux/console.h | 197 +++++++++++++---
include/linux/printk.h | 9 -
include/linux/syslog.h | 3 +
kernel/debug/kdb/kdb_io.c | 7 +-
kernel/printk/printk.c | 414 ++++++++++++++++++++++------------
12 files changed, 499 insertions(+), 233 deletions(-)
base-commit: dc453dd89daacdc0da6d66234aa27e417df7edcd
--
2.30.2
From: Thomas Gleixner <[email protected]>
Facilities which expose console information to sysfs or procfs can use the
new list protection to keep the list stable. No need to hold console lock.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
---
drivers/tty/tty_io.c | 6 +++---
fs/proc/consoles.c | 6 +++---
kernel/printk/printk.c | 8 ++++----
3 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 8fec1d8648f5..6fa142155b94 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3535,8 +3535,8 @@ static ssize_t show_cons_active(struct device *dev,
struct console *c;
ssize_t count = 0;
- console_lock();
- for_each_console(c) {
+ console_list_lock();
+ for_each_registered_console(c) {
if (!c->device)
continue;
if (!c->write)
@@ -3560,7 +3560,7 @@ static ssize_t show_cons_active(struct device *dev,
count += sprintf(buf + count, "%c", i ? ' ':'\n');
}
- console_unlock();
+ console_list_unlock();
return count;
}
diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index dfe6ce3505ce..6775056eecd5 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -63,8 +63,8 @@ static void *c_start(struct seq_file *m, loff_t *pos)
struct console *con;
loff_t off = 0;
- console_lock();
- for_each_console(con)
+ console_list_lock();
+ for_each_registered_console(con)
if (off++ == *pos)
break;
@@ -80,7 +80,7 @@ static void *c_next(struct seq_file *m, void *v, loff_t *pos)
static void c_stop(struct seq_file *m, void *v)
{
- console_unlock();
+ console_list_unlock();
}
static const struct seq_operations consoles_op = {
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 8c56f6071873..80a728ef9d96 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2990,18 +2990,18 @@ void console_flush_on_panic(enum con_flush_mode mode)
*/
struct tty_driver *console_device(int *index)
{
- struct console *c;
struct tty_driver *driver = NULL;
+ struct console *c;
- console_lock();
- for_each_console(c) {
+ console_list_lock();
+ for_each_registered_console(c) {
if (!c->device)
continue;
driver = c->device(c, index);
if (driver)
break;
}
- console_unlock();
+ console_list_unlock();
return driver;
}
--
2.30.2
From: Thomas Gleixner <[email protected]>
Unprotected list walks are a brilliant idea. Especially in the context of
hotpluggable consoles.
The new list lock provides not only synchronization for console list
manipulation, but also for manipulation of console->flags:
console_list_lock();
console_lock();
/* may now manipulate the console list and/or console->flags */
console_unlock();
console_list_unlock();
Therefore it is safe to iterate the console list and read console->flags
if holding either the console lock or the console list lock.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 30 +++++++++++++--
kernel/printk/printk.c | 84 ++++++++++++++++++++++++++++++++++-------
2 files changed, 98 insertions(+), 16 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 8c1686e2c233..24344f9b0bc1 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -157,10 +157,34 @@ struct console {
struct console *next;
};
-/*
- * for_each_console() allows you to iterate on each console
+#ifdef CONFIG_LOCKDEP
+extern void lockdep_assert_console_list_lock_held(void);
+#else
+static inline void lockdep_assert_console_list_lock_held(void) { }
+#endif
+
+extern void console_list_lock(void) __acquires(console_mutex);
+extern void console_list_unlock(void) __releases(console_mutex);
+
+/**
+ * for_each_registered_console() - Iterator over registered consoles
+ * @con: struct console pointer used as loop cursor
+ *
+ * Requires console_list_lock to be held. Can only be invoked from
+ * preemptible context.
+ */
+#define for_each_registered_console(con) \
+ lockdep_assert_console_list_lock_held(); \
+ for (con = console_drivers; con != NULL; con = con->next)
+
+/**
+ * for_each_console() - Iterator over registered consoles
+ * @con: struct console pointer used as loop cursor
+ *
+ * Requires console_lock to be held which guarantees that the
+ * list is immutable.
*/
-#define for_each_console(con) \
+#define for_each_console(con) \
for (con = console_drivers; con != NULL; con = con->next)
extern int console_set_on_cmdline;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e4f1e7478b52..8c56f6071873 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -79,10 +79,17 @@ int oops_in_progress;
EXPORT_SYMBOL(oops_in_progress);
/*
- * console_sem protects the console_drivers list, and also
- * provides serialisation for access to the entire console
- * driver system.
+ * console_sem protects the console_drivers list, and also provides
+ * serialization for access to the entire console driver system.
+ *
+ * console_mutex serializes register/unregister.
+ *
+ * console_sem must be taken inside a console_mutex locked section
+ * for any list manipulation in order to keep the console BKL
+ * machinery happy. This requirement also applies to manipulation
+ * of console->flags.
*/
+static DEFINE_MUTEX(console_mutex);
static DEFINE_SEMAPHORE(console_sem);
struct console *console_drivers;
EXPORT_SYMBOL_GPL(console_drivers);
@@ -103,6 +110,12 @@ static int __read_mostly suppress_panic_printk;
static struct lockdep_map console_lock_dep_map = {
.name = "console_lock"
};
+
+void lockdep_assert_console_list_lock_held(void)
+{
+ lockdep_assert_held(&console_mutex);
+}
+
#endif
enum devkmsg_log_bits {
@@ -220,6 +233,28 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
}
#endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
+/**
+ * console_list_lock - Lock the console list
+ *
+ * For non-console related list walks, e.g. procfs, sysfs...
+ */
+void console_list_lock(void)
+{
+ mutex_lock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_lock);
+
+/**
+ * console_list_unlock - Unlock the console list
+ *
+ * Counterpart to console_list_lock()
+ */
+void console_list_unlock(void)
+{
+ mutex_unlock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_unlock);
+
/*
* Helper macros to handle lockdep when locking/unlocking console_sem. We use
* macros instead of functions so that _RET_IP_ contains useful information.
@@ -2978,17 +3013,21 @@ struct tty_driver *console_device(int *index)
void console_stop(struct console *console)
{
__pr_flush(console, 1000, true);
+ console_list_lock();
console_lock();
console->flags &= ~CON_ENABLED;
console_unlock();
+ console_list_unlock();
}
EXPORT_SYMBOL(console_stop);
void console_start(struct console *console)
{
+ console_list_lock();
console_lock();
console->flags |= CON_ENABLED;
console_unlock();
+ console_list_unlock();
__pr_flush(console, 1000, true);
}
EXPORT_SYMBOL(console_start);
@@ -3081,6 +3120,8 @@ static void try_enable_default_console(struct console *newcon)
(con->flags & CON_BOOT) ? "boot" : "", \
con->name, con->index, ##__VA_ARGS__)
+static int console_unregister_locked(struct console *console);
+
/*
* The console driver calls this routine during kernel initialization
* to register the console printing procedure with printk() and to
@@ -3107,13 +3148,14 @@ void register_console(struct console *newcon)
bool realcon_enabled = false;
int err;
- for_each_console(con) {
+ console_list_lock();
+ for_each_registered_console(con) {
if (WARN(con == newcon, "console '%s%d' already registered\n",
con->name, con->index))
- return;
+ goto unlock;
}
- for_each_console(con) {
+ for_each_registered_console(con) {
if (con->flags & CON_BOOT)
bootcon_enabled = true;
else
@@ -3124,7 +3166,7 @@ void register_console(struct console *newcon)
if (newcon->flags & CON_BOOT && realcon_enabled) {
pr_info("Too late to register bootconsole %s%d\n",
newcon->name, newcon->index);
- return;
+ goto unlock;
}
/*
@@ -3155,7 +3197,7 @@ void register_console(struct console *newcon)
/* printk() messages are not printed to the Braille console. */
if (err || newcon->flags & CON_BRL)
- return;
+ goto unlock;
/*
* If we have a bootconsole, and are switching to a real console,
@@ -3209,14 +3251,17 @@ void register_console(struct console *newcon)
if (bootcon_enabled &&
((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
!keep_bootcon) {
- for_each_console(con)
+ for_each_console(con) {
if (con->flags & CON_BOOT)
- unregister_console(con);
+ console_unregister_locked(con);
+ }
}
+unlock:
+ console_list_unlock();
}
EXPORT_SYMBOL(register_console);
-int unregister_console(struct console *console)
+static int console_unregister_locked(struct console *console)
{
struct console *con;
int res;
@@ -3269,6 +3314,16 @@ int unregister_console(struct console *console)
return res;
}
+
+int unregister_console(struct console *console)
+{
+ int res;
+
+ console_list_lock();
+ res = console_unregister_locked(console);
+ console_list_unlock();
+ return res;
+}
EXPORT_SYMBOL(unregister_console);
/*
@@ -3320,7 +3375,8 @@ static int __init printk_late_init(void)
struct console *con;
int ret;
- for_each_console(con) {
+ console_list_lock();
+ for_each_registered_console(con) {
if (!(con->flags & CON_BOOT))
continue;
@@ -3337,9 +3393,11 @@ static int __init printk_late_init(void)
*/
pr_warn("bootconsole [%s%d] uses init memory and must be disabled even before the real one is ready\n",
con->name, con->index);
- unregister_console(con);
+ console_unregister_locked(con);
}
}
+ console_list_unlock();
+
ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
console_cpu_notify);
WARN_ON(ret < 0);
--
2.30.2
From: Thomas Gleixner <[email protected]>
Provide a SRCU protected variant to walk the console list.
Preperatory change for a new console infrastructure which operates
independent of console BKL.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 14 +++++++++++++-
kernel/printk/printk.c | 16 +++++++++++++---
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 1e3d0a50cef1..dc0df9d9e7d9 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -15,7 +15,7 @@
#define _LINUX_CONSOLE_H_ 1
#include <linux/atomic.h>
-#include <linux/list.h>
+#include <linux/rculist.h>
#include <linux/types.h>
struct vc_data;
@@ -161,6 +161,7 @@ struct console {
#ifdef CONFIG_LOCKDEP
extern void lockdep_assert_console_lock_held(void);
extern void lockdep_assert_console_list_lock_held(void);
+extern bool console_srcu_read_lock_is_held(void);
#else
static inline void lockdep_assert_console_lock_held(void) { }
static inline void lockdep_assert_console_list_lock_held(void) { }
@@ -171,6 +172,17 @@ extern struct hlist_head console_list;
extern void console_list_lock(void) __acquires(console_mutex);
extern void console_list_unlock(void) __releases(console_mutex);
+/**
+ * for_each_console_srcu() - Iterator over registered consoles
+ * @con: struct console pointer used as loop cursor
+ *
+ * Requires console_srcu_read_lock to be held. Can be invoked from
+ * any context.
+ */
+#define for_each_console_srcu(con) \
+ hlist_for_each_entry_srcu(con, &console_list, node, \
+ console_srcu_read_lock_is_held())
+
/**
* for_each_registered_console() - Iterator over registered consoles
* @con: struct console pointer used as loop cursor
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f1d31dcbd6ba..7e6d1cd34452 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -93,6 +93,7 @@ static DEFINE_MUTEX(console_mutex);
static DEFINE_SEMAPHORE(console_sem);
HLIST_HEAD(console_list);
EXPORT_SYMBOL_GPL(console_list);
+DEFINE_STATIC_SRCU(console_srcu);
/*
* System may need to suppress printk message under certain
@@ -121,6 +122,10 @@ void lockdep_assert_console_list_lock_held(void)
lockdep_assert_held(&console_mutex);
}
+bool console_srcu_read_lock_is_held(void)
+{
+ return srcu_read_lock_held(&console_srcu);
+}
#endif
enum devkmsg_log_bits {
@@ -3232,9 +3237,9 @@ void register_console(struct console *newcon)
*/
console_lock();
if (newcon->flags & CON_CONSDEV || hlist_empty(&console_list))
- hlist_add_head(&newcon->node, &console_list);
+ hlist_add_head_rcu(&newcon->node, &console_list);
else
- hlist_add_behind(&newcon->node, console_list.first);
+ hlist_add_behind_rcu(&newcon->node, console_list.first);
/* Ensure this flag is always set for the head of the list */
cons_first()->flags |= CON_CONSDEV;
@@ -3250,6 +3255,7 @@ void register_console(struct console *newcon)
newcon->seq = prb_next_seq(prb);
}
console_unlock();
+ /* No need to synchronize SRCU here! */
console_sysfs_notify();
/*
@@ -3295,7 +3301,7 @@ static int console_unregister_locked(struct console *console)
if (hlist_unhashed(&console->node))
goto out_unlock;
- hlist_del_init(&console->node);
+ hlist_del_init_rcu(&console->node);
/*
* <HISTORICAL>
@@ -3310,6 +3316,10 @@ static int console_unregister_locked(struct console *console)
cons_first()->flags |= CON_CONSDEV;
console_unlock();
+
+ /* Ensure that all SRCU list walks have completed */
+ synchronize_srcu(&console_srcu);
+
console_sysfs_notify();
if (console->exit)
--
2.30.2
From: Thomas Gleixner <[email protected]>
This variable cannot change post boot.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
---
kernel/printk/printk.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 770511b89504..e4f1e7478b52 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -430,7 +430,7 @@ static struct printk_ringbuffer *prb = &printk_rb_static;
* per_cpu_areas are initialised. This variable is set to true when
* it's safe to access per-CPU data.
*/
-static bool __printk_percpu_data_ready __read_mostly;
+static bool __printk_percpu_data_ready __ro_after_init;
bool printk_percpu_data_ready(void)
{
--
2.30.2
From: Thomas Gleixner <[email protected]>
Add docbook comments to struct console.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 95 +++++++++++++++++++++++++++++------------
1 file changed, 68 insertions(+), 27 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 3bb5bc62e154..8ec24fe097d3 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -15,6 +15,7 @@
#define _LINUX_CONSOLE_H_ 1
#include <linux/atomic.h>
+#include <linux/bits.h>
#include <linux/rculist.h>
#include <linux/types.h>
@@ -139,37 +140,77 @@ static inline int con_debug_leave(void)
/*
* The interface for a console, or any other device that wants to capture
* console messages (printer driver?)
- *
- * If a console driver is marked CON_BOOT then it will be auto-unregistered
- * when the first real console is registered. This is for early-printk drivers.
*/
-#define CON_PRINTBUFFER (1)
-#define CON_CONSDEV (2) /* Preferred console, /dev/console */
-#define CON_ENABLED (4)
-#define CON_BOOT (8)
-#define CON_ANYTIME (16) /* Safe to call when cpu is offline */
-#define CON_BRL (32) /* Used for a braille device */
-#define CON_EXTENDED (64) /* Use the extended output format a la /dev/kmsg */
+/**
+ * cons_flags - General console flags
+ * @CON_PRINTBUFFER: On register, start from the oldest dmesg record
+ * @CON_CONSDEV: Questionable historical leftover to denote which console
+ * driver is the preferred console which is defining what
+ * backs up /dev/console
+ * @CON_ENABLED: Indicates if a console is allowed to print records. If false,
+ * the console also will not advance to later records.
+ * @CON_BOOT: Marks the console driver as early console driver which
+ * is used during boot before the real driver becomes available.
+ * It will be automatically unregistered unless the early console
+ * command line parameter for this console has the 'keep' option set.
+ * @CON_ANYTIME: A misnomed historical flag which tells the core code that the
+ * legacy @console::write callback can be invoked on a CPU which
+ * is marked OFFLINE. That's misleading as it suggests that there
+ * is no contextual limit for invoking the callback.
+ * @CON_BRL: Indicates a braille device which is exempt from receiving the
+ * printk spam for obvious reasons
+ * @CON_EXTENDED: The console supports the extended output format of /dev/kmesg
+ * which requires a larger output record buffer
+ */
+enum cons_flags {
+ CON_PRINTBUFFER = BIT(0),
+ CON_CONSDEV = BIT(1),
+ CON_ENABLED = BIT(2),
+ CON_BOOT = BIT(3),
+ CON_ANYTIME = BIT(4),
+ CON_BRL = BIT(5),
+ CON_EXTENDED = BIT(6),
+};
+/**
+ * struct console - The console descriptor structure
+ * @name: The name of the console driver
+ * @write: Write callback to output messages (Optional)
+ * @read: Read callback for console input (Optional)
+ * @device: The underlying TTY device driver (Optional)
+ * @unblank: Callback to unblank the console (Optional)
+ * @setup: Callback for initializing the console (Optional)
+ * @exit: Callback for teardown of the console (Optional)
+ * @match: Callback for matching a console (Optional)
+ * @flags: Console flags. See enum cons_flags
+ * @index: Console index, e.g. port number
+ * @cflag: TTY control mode flags
+ * @ispeed: TTY input speed
+ * @ospeed: TTY output speed
+ * @seq: Sequence number of the next ringbuffer record to print
+ * @dropped: Number of dropped ringbuffer records
+ * @data: Driver private data
+ * @node: hlist node for the console list
+ */
struct console {
- char name[16];
- void (*write)(struct console *, const char *, unsigned);
- int (*read)(struct console *, char *, unsigned);
- struct tty_driver *(*device)(struct console *, int *);
- void (*unblank)(void);
- int (*setup)(struct console *, char *);
- int (*exit)(struct console *);
- int (*match)(struct console *, char *name, int idx, char *options);
- short flags;
- short index;
- int cflag;
- uint ispeed;
- uint ospeed;
- u64 seq;
- unsigned long dropped;
- void *data;
- struct hlist_node node;
+ char name[16];
+ void (*write)(struct console *, const char *, unsigned);
+ int (*read)(struct console *, char *, unsigned);
+ struct tty_driver *(*device)(struct console *, int *);
+ void (*unblank)(void);
+ int (*setup)(struct console *, char *);
+ int (*exit)(struct console *);
+ int (*match)(struct console *, char *name, int idx, char *options);
+ short flags;
+ short index;
+ int cflag;
+ uint ispeed;
+ uint ospeed;
+ u64 seq;
+ unsigned long dropped;
+ void *data;
+ struct hlist_node node;
};
#ifdef CONFIG_LOCKDEP
--
2.30.2
From: Thomas Gleixner <[email protected]>
Provide a special list iterator macro for KGDB to allow unprotected list
walks and add a few comments to explain the hope based approach.
Preperatory change for changing the console list to hlist and adding
lockdep asserts to regular list walks.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
---
drivers/tty/serial/kgdboc.c | 5 ++++-
include/linux/console.h | 10 ++++++++++
kernel/debug/kdb/kdb_io.c | 7 ++++++-
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index af2aa76bae15..57a5fd27dffe 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -462,10 +462,13 @@ static void kgdboc_earlycon_pre_exp_handler(void)
* we have no other choice so we keep using it. Since not all
* serial drivers might be OK with this, print a warning once per
* boot if we detect this case.
+ *
+ * Pretend that walking the console list is safe...
*/
- for_each_console(con)
+ for_each_console_kgdb(con) {
if (con == kgdboc_earlycon_io_ops.cons)
return;
+ }
already_warned = true;
pr_warn("kgdboc_earlycon is still using bootconsole\n");
diff --git a/include/linux/console.h b/include/linux/console.h
index 24344f9b0bc1..86a6125512b9 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -187,6 +187,16 @@ extern void console_list_unlock(void) __releases(console_mutex);
#define for_each_console(con) \
for (con = console_drivers; con != NULL; con = con->next)
+/**
+ * for_each_console_kgdb() - Iterator over registered consoles for KGDB
+ * @con: struct console pointer used as loop cursor
+ *
+ * Has no serialization requirements and KGDB pretends that this is safe.
+ * Don't use outside of the KGDB fairy tale land!
+ */
+#define for_each_console_kgdb(con) \
+ for (con = console_drivers; con != NULL; con = con->next)
+
extern int console_set_on_cmdline;
extern struct console *early_console;
diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
index 67d3c48a1522..fb3775e61a3b 100644
--- a/kernel/debug/kdb/kdb_io.c
+++ b/kernel/debug/kdb/kdb_io.c
@@ -558,7 +558,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
cp++;
}
- for_each_console(c) {
+ /*
+ * This is a completely unprotected list walk designed by the
+ * wishful thinking department. See the oops_in_progress comment
+ * below - especially the encourage section...
+ */
+ for_each_console_kgdb(c) {
if (!(c->flags & CON_ENABLED))
continue;
if (c == dbg_io_ops->cons)
--
2.30.2
From: Thomas Gleixner <[email protected]>
Move the buffer size defines to console.h in preparation of adding a buffer
structure.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 14 ++++++++++++++
include/linux/printk.h | 2 --
kernel/printk/printk.c | 8 --------
3 files changed, 14 insertions(+), 10 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index dc0df9d9e7d9..3bb5bc62e154 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -122,6 +122,20 @@ static inline int con_debug_leave(void)
#define CM_ERASE (2)
#define CM_MOVE (3)
+#ifdef CONFIG_PRINTK
+/* The maximum size of a formatted record (i.e. with prefix added per line) */
+#define CONSOLE_LOG_MAX 1024
+
+/* The maximum size for a dropped text message */
+#define DROPPED_TEXT_MAX 64
+#else
+#define CONSOLE_LOG_MAX 0
+#define DROPPED_TEXT_MAX 0
+#endif
+
+/* The maximum size of an formatted extended record */
+#define CONSOLE_EXT_LOG_MAX 8192
+
/*
* The interface for a console, or any other device that wants to capture
* console messages (printer driver?)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index 8c81806c2e99..8ef499ab3c1e 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -44,8 +44,6 @@ static inline const char *printk_skip_headers(const char *buffer)
return buffer;
}
-#define CONSOLE_EXT_LOG_MAX 8192
-
/* printk's without a loglevel use this.. */
#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_MESSAGE_LOGLEVEL_DEFAULT
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 7e6d1cd34452..65e9903d066f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -433,12 +433,6 @@ static struct latched_seq clear_seq = {
#define PREFIX_MAX 32
#endif
-/* the maximum size of a formatted record (i.e. with prefix added per line) */
-#define CONSOLE_LOG_MAX 1024
-
-/* the maximum size for a dropped text message */
-#define DROPPED_TEXT_MAX 64
-
/* the maximum size allowed to be reserved for a record */
#define LOG_LINE_MAX (CONSOLE_LOG_MAX - PREFIX_MAX)
@@ -2343,8 +2337,6 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
#else /* CONFIG_PRINTK */
-#define CONSOLE_LOG_MAX 0
-#define DROPPED_TEXT_MAX 0
#define printk_time false
#define prb_read_valid(rb, seq, r) false
--
2.30.2
From: Thomas Gleixner <[email protected]>
To prepare for a new console infrastructure that is independent of the
console BKL, wrap the output mode into a descriptor struct so the new
infrastrucure can share the emit code that dumps the ringbuffer record
into the output text buffers.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 15 +++++++
kernel/printk/printk.c | 88 ++++++++++++++++++++++++++++++-----------
2 files changed, 79 insertions(+), 24 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 05c7325e98f9..590ab62c01d9 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -187,6 +187,21 @@ struct cons_text_buf {
char text[CONSOLE_LOG_MAX];
};
+/**
+ * struct cons_outbuf_desc - console output buffer descriptor
+ * @txtbuf: Pointer to buffer for storing the text
+ * @outbuf: Pointer to the position in @buffer for
+ * writing it out to the device
+ * @len: Message length
+ * @extmsg: Select extended format printing
+ */
+struct cons_outbuf_desc {
+ struct cons_text_buf *txtbuf;
+ char *outbuf;
+ unsigned int len;
+ bool extmsg;
+};
+
/**
* struct console - The console descriptor structure
* @name: The name of the console driver
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 9cbd44e9fc45..c9207d81b90c 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2689,40 +2689,39 @@ static void __console_unlock(void)
up_console_sem();
}
-/*
- * Print one record for the given console. The record printed is whatever
- * record is the next available record for the given console.
- *
- * @text is a buffer of size CONSOLE_LOG_MAX.
- *
- * If extended messages should be printed, @ext_text is a buffer of size
- * CONSOLE_EXT_LOG_MAX. Otherwise @ext_text must be NULL.
+
+/**
+ * cons_fill_outbuf - Fill the output buffer with the next record
+ * @con: The console to print on
+ * @desc: Pointer to the output descriptor
*
- * If dropped messages should be printed, @dropped_text is a buffer of size
- * DROPPED_TEXT_MAX. Otherwise @dropped_text must be NULL.
+ * The output descriptor contains all information which is necessary
+ * to print (buffer pointer, extended format selector...).
*
- * @handover will be set to true if a printk waiter has taken over the
- * console_lock, in which case the caller is no longer holding the
- * console_lock. Otherwise it is set to false.
+ * Returns: False if there is no pending record in the ringbuffer
+ * True if there is a pending record in the ringbuffer.
*
- * Returns false if the given console has no next record to print, otherwise
- * true.
+ * When the return value is true, then the caller has to check
+ * @desc->outbuf. If not NULL it points to the first character to write to
+ * the device and @desc->len contains the length of the message.
*
- * Requires the console_lock.
+ * If it is NULL then records have been dropped or skipped and con->seq
+ * has been forwarded so the caller can try to print the next record.
*/
-static bool console_emit_next_record(struct console *con, struct cons_text_buf *txtbuf,
- bool *handover, bool extmsg)
+static bool cons_fill_outbuf(struct console *con, struct cons_outbuf_desc *desc)
{
static int panic_console_dropped;
+
+ struct cons_text_buf *txtbuf = desc->txtbuf;
struct printk_info info;
struct printk_record r;
- unsigned long flags;
char *write_text;
size_t len;
- prb_rec_init_rd(&r, &info, txtbuf->text, CONSOLE_LOG_MAX);
+ desc->outbuf = NULL;
+ desc->len = 0;
- *handover = false;
+ prb_rec_init_rd(&r, &info, txtbuf->text, CONSOLE_LOG_MAX);
if (!prb_read_valid(prb, con->seq, &r))
return false;
@@ -2739,10 +2738,10 @@ static bool console_emit_next_record(struct console *con, struct cons_text_buf *
/* Skip record that has level above the console loglevel. */
if (suppress_message_printing(r.info->level)) {
con->seq++;
- goto skip;
+ return true;
}
- if (extmsg) {
+ if (desc->extmsg) {
write_text = txtbuf->ext_text;
len = info_print_ext_header(write_text, CONSOLE_EXT_LOG_MAX, r.info);
len += msg_print_ext_body(write_text + len, CONSOLE_EXT_LOG_MAX - len,
@@ -2752,6 +2751,47 @@ static bool console_emit_next_record(struct console *con, struct cons_text_buf *
len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
}
+ desc->len = len;
+ desc->outbuf = write_text;
+ return true;
+}
+
+/**
+ * console_emit_next_record - Print one record for the given console
+ * @con: The console to print on
+ * @txtbuf: Pointer to the output buffer
+ * @handover: Pointer to Handover handshake storage
+ * @extmsg: Selects extended message format
+ *
+ * The record printed is whatever record is the next available record for
+ * the given console.
+ *
+ * @handover will be set to true if a printk waiter has taken over the
+ * console_lock, in which case the caller is no longer holding the
+ * console_lock. Otherwise it is set to false.
+ *
+ * Returns false if the given console has no next record to print, otherwise
+ * true.
+ *
+ * Requires the console_lock.
+ */
+static bool console_emit_next_record(struct console *con, struct cons_text_buf *txtbuf,
+ bool *handover, bool extmsg)
+{
+ struct cons_outbuf_desc desc = {
+ .txtbuf = txtbuf,
+ .extmsg = extmsg,
+ };
+ unsigned long flags;
+
+ *handover = false;
+
+ if (!cons_fill_outbuf(con, &desc))
+ return false;
+
+ if (!desc.outbuf)
+ goto skip;
+
/*
* While actively printing out messages, if another printk()
* were to occur on another CPU, it may wait for this one to
@@ -2766,7 +2806,7 @@ static bool console_emit_next_record(struct console *con, struct cons_text_buf *
console_lock_spinning_enable();
stop_critical_timings(); /* don't trace print latency */
- call_console_driver(con, write_text, len, extmsg ? NULL : txtbuf->dropped_text);
+ call_console_driver(con, desc.outbuf, desc.len, extmsg ? NULL : txtbuf->dropped_text);
start_critical_timings();
con->seq++;
--
2.30.2
From: Thomas Gleixner <[email protected]>
PARISC has a hope based mechanism to restore consoles in case of a HPMC
(machine check exception) which is scattered over several places.
Move it into one place to make further changes simpler and add comments.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
---
arch/parisc/include/asm/pdc.h | 2 +-
arch/parisc/kernel/pdc_cons.c | 38 ++++++++++++++++++++++++++++-------
arch/parisc/kernel/traps.c | 17 +++++-----------
3 files changed, 37 insertions(+), 20 deletions(-)
diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h
index b643092d4b98..12bffd8a5d05 100644
--- a/arch/parisc/include/asm/pdc.h
+++ b/arch/parisc/include/asm/pdc.h
@@ -20,7 +20,7 @@ extern unsigned long parisc_pat_pdc_cap; /* PDC capabilities (PAT) */
#define PDC_TYPE_SNAKE 2 /* Doesn't support SYSTEM_MAP */
void pdc_console_init(void); /* in pdc_console.c */
-void pdc_console_restart(void);
+void pdc_console_restart(bool hpmc);
void setup_pdc(void); /* in inventory.c */
diff --git a/arch/parisc/kernel/pdc_cons.c b/arch/parisc/kernel/pdc_cons.c
index 2661cdd256ae..9a0c0932d2f9 100644
--- a/arch/parisc/kernel/pdc_cons.c
+++ b/arch/parisc/kernel/pdc_cons.c
@@ -237,20 +237,44 @@ void __init pdc_console_init(void)
/*
- * Used for emergencies. Currently only used if an HPMC occurs. If an
- * HPMC occurs, it is possible that the current console may not be
- * properly initialised after the PDC IO reset. This routine unregisters
- * all of the current consoles, reinitializes the pdc console and
- * registers it.
+ * <Historical comments>
+ *
+ * Used for emergencies.
+ *
+ * - If an HPMC occurs, it is possible that the current console may not be
+ * properly initialised after the PDC IO reset. This routine unregisters
+ * all of the current consoles, reinitializes the pdc console and registers
+ * it.
+ *
+ * - Maybe the kernel hasn't booted very far yet and hasn't been able
+ * to initialize the serial or STI console. In that case we should
+ * re-enable the pdc console, so that the user will be able to
+ * identify the problem.
+ *
+ * </Historical comments>
+ *
+ * The above is all wishful thinking:
+ *
+ * - Invoking [un]register_console() from exception contexts is obviously
+ * unsafe.
+ *
+ * - If the HPMC left the machine in unpleasant state and the pdc console
+ * was already initialized, but later removed due to CON_BOOT then this
+ * will do nothing.
+ *
+ * Pretend that any of the below works in the same way as we pretend that
+ * any of PARISC works.
*/
-
-void pdc_console_restart(void)
+void pdc_console_restart(bool hpmc)
{
struct console *console;
if (pdc_console_initialized)
return;
+ if (!hpmc && console_drivers)
+ return;
+
/* If we've already seen the output, don't bother to print it again */
if (console_drivers != NULL)
pdc_cons.flags &= ~CON_PRINTBUFFER;
diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c
index b78f1b9d45c1..81e23f589d3d 100644
--- a/arch/parisc/kernel/traps.c
+++ b/arch/parisc/kernel/traps.c
@@ -235,17 +235,12 @@ void die_if_kernel(char *str, struct pt_regs *regs, long err)
" (__)\\ )\\/\\\n"
" U ||----w |\n"
" || ||\n");
-
+
/* unlock the pdc lock if necessary */
pdc_emergency_unlock();
- /* maybe the kernel hasn't booted very far yet and hasn't been able
- * to initialize the serial or STI console. In that case we should
- * re-enable the pdc console, so that the user will be able to
- * identify the problem. */
- if (!console_drivers)
- pdc_console_restart();
-
+ pdc_console_restart(false);
+
if (err)
printk(KERN_CRIT "%s (pid %d): %s (code %ld)\n",
current->comm, task_pid_nr(current), str, err);
@@ -429,9 +424,7 @@ void parisc_terminate(char *msg, struct pt_regs *regs, int code, unsigned long o
/* unlock the pdc lock if necessary */
pdc_emergency_unlock();
- /* restart pdc console if necessary */
- if (!console_drivers)
- pdc_console_restart();
+ pdc_console_restart(false);
/* Not all paths will gutter the processor... */
switch(code){
@@ -483,7 +476,7 @@ void notrace handle_interruption(int code, struct pt_regs *regs)
int si_code;
if (code == 1)
- pdc_console_restart(); /* switch back to pdc if HPMC */
+ pdc_console_restart(true); /* switch back to pdc if HPMC */
else if (!irqs_disabled_flags(regs->gr[0]))
local_irq_enable();
--
2.30.2
From: Thomas Gleixner <[email protected]>
Replace the separately allocated output buffers with a single instance of
struct cons_text_buf.
Note that the buffer size of devkmsg_user.text_buf, when replaced with
cons_text_buf.text, reduces from CONSOLE_EXT_LOG_MAX to CONSOLE_LOG_MAX.
However, the buffer is only used to read ringbuffer records, which have
a maximum size of LOG_LINE_MAX (CONSOLE_LOG_MAX - PREFIX_MAX).
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
kernel/printk/printk.c | 50 ++++++++++++++++++------------------------
1 file changed, 21 insertions(+), 29 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 65e9903d066f..9cbd44e9fc45 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -671,11 +671,9 @@ struct devkmsg_user {
atomic64_t seq;
struct ratelimit_state rs;
struct mutex lock;
- char buf[CONSOLE_EXT_LOG_MAX];
-
struct printk_info info;
- char text_buf[CONSOLE_EXT_LOG_MAX];
struct printk_record record;
+ struct cons_text_buf txtbuf;
};
static __printf(3, 4) __cold
@@ -758,6 +756,8 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
{
struct devkmsg_user *user = file->private_data;
struct printk_record *r = &user->record;
+ char *outbuf = user->txtbuf.ext_text;
+ const int maxlen = sizeof(user->txtbuf.ext_text);
size_t len;
ssize_t ret;
@@ -798,8 +798,8 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
goto out;
}
- len = info_print_ext_header(user->buf, sizeof(user->buf), r->info);
- len += msg_print_ext_body(user->buf + len, sizeof(user->buf) - len,
+ len = info_print_ext_header(outbuf, maxlen, r->info);
+ len += msg_print_ext_body(outbuf + len, maxlen - len,
&r->text_buf[0], r->info->text_len,
&r->info->dev_info);
@@ -810,7 +810,7 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
goto out;
}
- if (copy_to_user(buf, user->buf, len)) {
+ if (copy_to_user(buf, outbuf, len)) {
ret = -EFAULT;
goto out;
}
@@ -909,7 +909,8 @@ static int devkmsg_open(struct inode *inode, struct file *file)
mutex_init(&user->lock);
prb_rec_init_rd(&user->record, &user->info,
- &user->text_buf[0], sizeof(user->text_buf));
+ user->txtbuf.text,
+ sizeof(user->txtbuf.text));
atomic64_set(&user->seq, prb_first_valid_seq(prb));
@@ -2709,8 +2710,8 @@ static void __console_unlock(void)
*
* Requires the console_lock.
*/
-static bool console_emit_next_record(struct console *con, char *text, char *ext_text,
- char *dropped_text, bool *handover)
+static bool console_emit_next_record(struct console *con, struct cons_text_buf *txtbuf,
+ bool *handover, bool extmsg)
{
static int panic_console_dropped;
struct printk_info info;
@@ -2719,7 +2720,7 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
char *write_text;
size_t len;
- prb_rec_init_rd(&r, &info, text, CONSOLE_LOG_MAX);
+ prb_rec_init_rd(&r, &info, txtbuf->text, CONSOLE_LOG_MAX);
*handover = false;
@@ -2741,13 +2742,13 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
goto skip;
}
- if (ext_text) {
- write_text = ext_text;
- len = info_print_ext_header(ext_text, CONSOLE_EXT_LOG_MAX, r.info);
- len += msg_print_ext_body(ext_text + len, CONSOLE_EXT_LOG_MAX - len,
+ if (extmsg) {
+ write_text = txtbuf->ext_text;
+ len = info_print_ext_header(write_text, CONSOLE_EXT_LOG_MAX, r.info);
+ len += msg_print_ext_body(write_text + len, CONSOLE_EXT_LOG_MAX - len,
&r.text_buf[0], r.info->text_len, &r.info->dev_info);
} else {
- write_text = text;
+ write_text = txtbuf->text;
len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
}
@@ -2765,7 +2766,7 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
console_lock_spinning_enable();
stop_critical_timings(); /* don't trace print latency */
- call_console_driver(con, write_text, len, dropped_text);
+ call_console_driver(con, write_text, len, extmsg ? NULL : txtbuf->dropped_text);
start_critical_timings();
con->seq++;
@@ -2801,9 +2802,7 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
*/
static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handover)
{
- static char dropped_text[DROPPED_TEXT_MAX];
- static char ext_text[CONSOLE_EXT_LOG_MAX];
- static char text[CONSOLE_LOG_MAX];
+ static struct cons_text_buf txtbuf;
bool any_usable = false;
struct console *con;
bool any_progress;
@@ -2821,16 +2820,9 @@ static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
continue;
any_usable = true;
- if (con->flags & CON_EXTENDED) {
- /* Extended consoles do not print "dropped messages". */
- progress = console_emit_next_record(con, &text[0],
- &ext_text[0], NULL,
- handover);
- } else {
- progress = console_emit_next_record(con, &text[0],
- NULL, &dropped_text[0],
- handover);
- }
+ progress = console_emit_next_record(con, &txtbuf, handover,
+ con->flags & CON_EXTENDED);
+
if (*handover)
return false;
--
2.30.2
From: Thomas Gleixner <[email protected]>
No user outside the printk code and no reason to export this.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
---
include/linux/printk.h | 7 -------
kernel/printk/printk.c | 5 +++--
2 files changed, 3 insertions(+), 9 deletions(-)
diff --git a/include/linux/printk.h b/include/linux/printk.h
index cf7d666ab1f8..8c81806c2e99 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -169,8 +169,6 @@ extern void __printk_safe_exit(void);
#define printk_deferred_enter __printk_safe_enter
#define printk_deferred_exit __printk_safe_exit
-extern bool pr_flush(int timeout_ms, bool reset_on_progress);
-
/*
* Please don't use printk_ratelimit(), because it shares ratelimiting state
* with all other unrelated printk_ratelimit() callsites. Instead use
@@ -221,11 +219,6 @@ static inline void printk_deferred_exit(void)
{
}
-static inline bool pr_flush(int timeout_ms, bool reset_on_progress)
-{
- return true;
-}
-
static inline int printk_ratelimit(void)
{
return 0;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index a1a81fd9889b..14d7d39d118d 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -2296,6 +2296,7 @@ asmlinkage __visible int _printk(const char *fmt, ...)
}
EXPORT_SYMBOL(_printk);
+static bool pr_flush(int timeout_ms, bool reset_on_progress);
static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress);
#else /* CONFIG_PRINTK */
@@ -2330,6 +2331,7 @@ static void call_console_driver(struct console *con, const char *text, size_t le
{
}
static bool suppress_message_printing(int level) { return false; }
+static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
#endif /* CONFIG_PRINTK */
@@ -3438,11 +3440,10 @@ static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progre
* Context: Process context. May sleep while acquiring console lock.
* Return: true if all enabled printers are caught up.
*/
-bool pr_flush(int timeout_ms, bool reset_on_progress)
+static bool pr_flush(int timeout_ms, bool reset_on_progress)
{
return __pr_flush(NULL, timeout_ms, reset_on_progress);
}
-EXPORT_SYMBOL(pr_flush);
/*
* Delayed printk version, for scheduler-internal messages:
--
2.30.2
From: Thomas Gleixner <[email protected]>
Unprotected list walks are not necessarily safe.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
---
drivers/tty/serial/kgdboc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
index 79b7db8580e0..af2aa76bae15 100644
--- a/drivers/tty/serial/kgdboc.c
+++ b/drivers/tty/serial/kgdboc.c
@@ -193,7 +193,8 @@ static int configure_kgdboc(void)
if (!p)
goto noconfig;
- for_each_console(cons) {
+ console_list_lock();
+ for_each_registered_console(cons) {
int idx;
if (cons->device && cons->device(cons, &idx) == p &&
idx == tty_line) {
@@ -201,6 +202,7 @@ static int configure_kgdboc(void)
break;
}
}
+ console_list_unlock();
kgdb_tty_driver = p;
kgdb_tty_line = tty_line;
--
2.30.2
From: Thomas Gleixner <[email protected]>
Replace the open coded single linked list with a hlist so a conversion to
SRCU protected list walks can reuse the existing primitives.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
arch/parisc/kernel/pdc_cons.c | 19 +++----
fs/proc/consoles.c | 5 +-
include/linux/console.h | 15 ++++--
kernel/printk/printk.c | 99 +++++++++++++++++++----------------
4 files changed, 75 insertions(+), 63 deletions(-)
diff --git a/arch/parisc/kernel/pdc_cons.c b/arch/parisc/kernel/pdc_cons.c
index 9a0c0932d2f9..3f9abf0263ee 100644
--- a/arch/parisc/kernel/pdc_cons.c
+++ b/arch/parisc/kernel/pdc_cons.c
@@ -147,13 +147,8 @@ static int __init pdc_console_tty_driver_init(void)
struct console *tmp;
- console_lock();
- for_each_console(tmp)
- if (tmp == &pdc_cons)
- break;
- console_unlock();
-
- if (!tmp) {
+ /* Pretend that this works as much as it pretended to work historically */
+ if (hlist_unhashed_lockless(&pdc_cons.node)) {
printk(KERN_INFO "PDC console driver not registered anymore, not creating %s\n", pdc_cons.name);
return -ENODEV;
}
@@ -272,15 +267,17 @@ void pdc_console_restart(bool hpmc)
if (pdc_console_initialized)
return;
- if (!hpmc && console_drivers)
+ if (!hpmc && !hlist_empty(&console_list))
return;
/* If we've already seen the output, don't bother to print it again */
- if (console_drivers != NULL)
+ if (!hlist_empty(&console_list))
pdc_cons.flags &= ~CON_PRINTBUFFER;
- while ((console = console_drivers) != NULL)
- unregister_console(console_drivers);
+ while (!hlist_empty(&console_list)) {
+ unregister_console(READ_ONCE(hlist_entry(console_list.first,
+ struct console, node)));
+ }
/* force registering the pdc console */
pdc_console_init_force();
diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
index 6775056eecd5..70994d1e52f6 100644
--- a/fs/proc/consoles.c
+++ b/fs/proc/consoles.c
@@ -74,8 +74,11 @@ static void *c_start(struct seq_file *m, loff_t *pos)
static void *c_next(struct seq_file *m, void *v, loff_t *pos)
{
struct console *con = v;
+
++*pos;
- return con->next;
+ hlist_for_each_entry_continue(con, node)
+ break;
+ return con;
}
static void c_stop(struct seq_file *m, void *v)
diff --git a/include/linux/console.h b/include/linux/console.h
index 86a6125512b9..1e3d0a50cef1 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -15,6 +15,7 @@
#define _LINUX_CONSOLE_H_ 1
#include <linux/atomic.h>
+#include <linux/list.h>
#include <linux/types.h>
struct vc_data;
@@ -154,15 +155,19 @@ struct console {
u64 seq;
unsigned long dropped;
void *data;
- struct console *next;
+ struct hlist_node node;
};
#ifdef CONFIG_LOCKDEP
+extern void lockdep_assert_console_lock_held(void);
extern void lockdep_assert_console_list_lock_held(void);
#else
+static inline void lockdep_assert_console_lock_held(void) { }
static inline void lockdep_assert_console_list_lock_held(void) { }
#endif
+extern struct hlist_head console_list;
+
extern void console_list_lock(void) __acquires(console_mutex);
extern void console_list_unlock(void) __releases(console_mutex);
@@ -175,7 +180,7 @@ extern void console_list_unlock(void) __releases(console_mutex);
*/
#define for_each_registered_console(con) \
lockdep_assert_console_list_lock_held(); \
- for (con = console_drivers; con != NULL; con = con->next)
+ hlist_for_each_entry(con, &console_list, node)
/**
* for_each_console() - Iterator over registered consoles
@@ -185,7 +190,8 @@ extern void console_list_unlock(void) __releases(console_mutex);
* list is immutable.
*/
#define for_each_console(con) \
- for (con = console_drivers; con != NULL; con = con->next)
+ lockdep_assert_console_lock_held(); \
+ hlist_for_each_entry(con, &console_list, node)
/**
* for_each_console_kgdb() - Iterator over registered consoles for KGDB
@@ -195,7 +201,7 @@ extern void console_list_unlock(void) __releases(console_mutex);
* Don't use outside of the KGDB fairy tale land!
*/
#define for_each_console_kgdb(con) \
- for (con = console_drivers; con != NULL; con = con->next)
+ hlist_for_each_entry(con, &console_list, node)
extern int console_set_on_cmdline;
extern struct console *early_console;
@@ -208,7 +214,6 @@ enum con_flush_mode {
extern int add_preferred_console(char *name, int idx, char *options);
extern void register_console(struct console *);
extern int unregister_console(struct console *);
-extern struct console *console_drivers;
extern void console_lock(void);
extern int console_trylock(void);
extern void console_unlock(void);
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 80a728ef9d96..f1d31dcbd6ba 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -79,20 +79,20 @@ int oops_in_progress;
EXPORT_SYMBOL(oops_in_progress);
/*
- * console_sem protects the console_drivers list, and also provides
- * serialization for access to the entire console driver system.
+ * console_sem protects onsole_list, and also provides serialization for
+ * access to the entire console driver system.
*
* console_mutex serializes register/unregister.
*
* console_sem must be taken inside a console_mutex locked section
- * for any list manipulation in order to keep the console BKL
+ * for any console_list manipulation in order to keep the console BKL
* machinery happy. This requirement also applies to manipulation
* of console->flags.
*/
static DEFINE_MUTEX(console_mutex);
static DEFINE_SEMAPHORE(console_sem);
-struct console *console_drivers;
-EXPORT_SYMBOL_GPL(console_drivers);
+HLIST_HEAD(console_list);
+EXPORT_SYMBOL_GPL(console_list);
/*
* System may need to suppress printk message under certain
@@ -111,6 +111,11 @@ static struct lockdep_map console_lock_dep_map = {
.name = "console_lock"
};
+void lockdep_assert_console_lock_held(void)
+{
+ lockdep_assert(lock_is_held(&console_lock_dep_map));
+}
+
void lockdep_assert_console_list_lock_held(void)
{
lockdep_assert_held(&console_mutex);
@@ -2591,7 +2596,7 @@ static int console_cpu_notify(unsigned int cpu)
* console_lock - lock the console system for exclusive use.
*
* Acquires a lock which guarantees that the caller has
- * exclusive access to the console system and the console_drivers list.
+ * exclusive access to the console system and console_list.
*
* Can sleep, returns nothing.
*/
@@ -2611,7 +2616,7 @@ EXPORT_SYMBOL(console_lock);
* console_trylock - try to lock the console system for exclusive use.
*
* Try to acquire a lock which guarantees that the caller has exclusive
- * access to the console system and the console_drivers list.
+ * access to the console system and console_list.
*
* returns 1 on success, and 0 on failure to acquire the lock.
*/
@@ -2979,7 +2984,15 @@ void console_flush_on_panic(enum con_flush_mode mode)
u64 seq;
seq = prb_first_valid_seq(prb);
- for_each_console(c)
+ /*
+ * This cannot use for_each_console() because it's not established
+ * that the current context has console locked and neither there is
+ * a guarantee that there is no concurrency in that case.
+ *
+ * Open code it for documentation purposes and pretend that
+ * it works.
+ */
+ hlist_for_each_entry(c, &console_list, node)
c->seq = seq;
}
console_unlock();
@@ -3120,6 +3133,9 @@ static void try_enable_default_console(struct console *newcon)
(con->flags & CON_BOOT) ? "boot" : "", \
con->name, con->index, ##__VA_ARGS__)
+#define cons_first() \
+ hlist_entry(console_list.first, struct console, node)
+
static int console_unregister_locked(struct console *console);
/*
@@ -3182,8 +3198,8 @@ void register_console(struct console *newcon)
* flag set and will be first in the list.
*/
if (preferred_console < 0) {
- if (!console_drivers || !console_drivers->device ||
- console_drivers->flags & CON_BOOT) {
+ if (hlist_empty(&console_list) || !cons_first()->device ||
+ cons_first()->flags & CON_BOOT) {
try_enable_default_console(newcon);
}
}
@@ -3211,21 +3227,17 @@ void register_console(struct console *newcon)
}
/*
- * Put this console in the list - keep the
- * preferred driver at the head of the list.
+ * Put this console in the list and keep the referred driver at the
+ * head of the list.
*/
console_lock();
- if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) {
- newcon->next = console_drivers;
- console_drivers = newcon;
- if (newcon->next)
- newcon->next->flags &= ~CON_CONSDEV;
- /* Ensure this flag is always set for the head of the list */
- newcon->flags |= CON_CONSDEV;
- } else {
- newcon->next = console_drivers->next;
- console_drivers->next = newcon;
- }
+ if (newcon->flags & CON_CONSDEV || hlist_empty(&console_list))
+ hlist_add_head(&newcon->node, &console_list);
+ else
+ hlist_add_behind(&newcon->node, console_list.first);
+
+ /* Ensure this flag is always set for the head of the list */
+ cons_first()->flags |= CON_CONSDEV;
newcon->dropped = 0;
if (newcon->flags & CON_PRINTBUFFER) {
@@ -3251,7 +3263,9 @@ void register_console(struct console *newcon)
if (bootcon_enabled &&
((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
!keep_bootcon) {
- for_each_console(con) {
+ struct hlist_node *tmp;
+
+ hlist_for_each_entry_safe(con, tmp, &console_list, node) {
if (con->flags & CON_BOOT)
console_unregister_locked(con);
}
@@ -3263,7 +3277,6 @@ EXPORT_SYMBOL(register_console);
static int console_unregister_locked(struct console *console)
{
- struct console *con;
int res;
con_printk(KERN_INFO, console, "disabled\n");
@@ -3274,32 +3287,28 @@ static int console_unregister_locked(struct console *console)
if (res > 0)
return 0;
- res = -ENODEV;
console_lock();
- if (console_drivers == console) {
- console_drivers=console->next;
- res = 0;
- } else {
- for_each_console(con) {
- if (con->next == console) {
- con->next = console->next;
- res = 0;
- break;
- }
- }
- }
- if (res)
- goto out_disable_unlock;
+ /* Disable it unconditionally */
+ console->flags &= ~CON_ENABLED;
+
+ if (hlist_unhashed(&console->node))
+ goto out_unlock;
+
+ hlist_del_init(&console->node);
/*
+ * <HISTORICAL>
* If this isn't the last console and it has CON_CONSDEV set, we
* need to set it on the next preferred console.
+ * </HISTORICAL>
+ *
+ * The above makes no sense as there is no guarantee that the next
+ * console has any device attached. Oh well....
*/
- if (console_drivers != NULL && console->flags & CON_CONSDEV)
- console_drivers->flags |= CON_CONSDEV;
+ if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
+ cons_first()->flags |= CON_CONSDEV;
- console->flags &= ~CON_ENABLED;
console_unlock();
console_sysfs_notify();
@@ -3308,10 +3317,8 @@ static int console_unregister_locked(struct console *console)
return res;
-out_disable_unlock:
- console->flags &= ~CON_ENABLED;
+out_unlock:
console_unlock();
-
return res;
}
--
2.30.2
From: Thomas Gleixner <[email protected]>
The comment about unregistering boot consoles is just not matching the
reality. Remove it.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
---
kernel/printk/printk.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d6bba2ea14e8..770511b89504 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3209,9 +3209,6 @@ void register_console(struct console *newcon)
if (bootcon_enabled &&
((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
!keep_bootcon) {
- /* We need to iterate through all boot consoles, to make
- * sure we print everything out, before we unregister them.
- */
for_each_console(con)
if (con->flags & CON_BOOT)
unregister_console(con);
--
2.30.2
From: Thomas Gleixner <[email protected]>
Create a data structure to replace the open coded separate buffers for
regular and extended formatting.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/linux/console.h b/include/linux/console.h
index 8ec24fe097d3..05c7325e98f9 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -173,6 +173,20 @@ enum cons_flags {
CON_EXTENDED = BIT(6),
};
+/**
+ * struct cons_text_buf - console output text buffer
+ * @ext_text: Buffer for extended log format text
+ * @dropped_text: Buffer for dropped text
+ * @text: Buffer for ringbuffer text
+ */
+struct cons_text_buf {
+ union {
+ char ext_text[CONSOLE_EXT_LOG_MAX];
+ char dropped_text[DROPPED_TEXT_MAX];
+ };
+ char text[CONSOLE_LOG_MAX];
+};
+
/**
* struct console - The console descriptor structure
* @name: The name of the console driver
--
2.30.2
From: Thomas Gleixner <[email protected]>
No need for an extra buffer type. Regular formatting which needs the '$N
messages dropped' printout can sprintf() it into the unused extended text
buffer.
As a further simplification move the 'dropped' message right in front of
the actual record text and let the write function output it in one go.
Signed-off-by: Thomas Gleixner <[email protected]>
Signed-off-by: John Ogness <[email protected]>
---
include/linux/console.h | 12 +++---
kernel/printk/printk.c | 86 +++++++++++++++++++++++++++--------------
2 files changed, 61 insertions(+), 37 deletions(-)
diff --git a/include/linux/console.h b/include/linux/console.h
index 590ab62c01d9..83563694cc82 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -176,28 +176,26 @@ enum cons_flags {
/**
* struct cons_text_buf - console output text buffer
* @ext_text: Buffer for extended log format text
- * @dropped_text: Buffer for dropped text
* @text: Buffer for ringbuffer text
*/
struct cons_text_buf {
- union {
- char ext_text[CONSOLE_EXT_LOG_MAX];
- char dropped_text[DROPPED_TEXT_MAX];
- };
- char text[CONSOLE_LOG_MAX];
-};
+ char ext_text[CONSOLE_EXT_LOG_MAX];
+ char text[CONSOLE_LOG_MAX];
+} __no_randomize_layout;
/**
* struct cons_outbuf_desc - console output buffer descriptor
* @txtbuf: Pointer to buffer for storing the text
* @outbuf: Pointer to the position in @buffer for
* writing it out to the device
+ * @dropped: The dropped count
* @len: Message length
* @extmsg: Select extended format printing
*/
struct cons_outbuf_desc {
struct cons_text_buf *txtbuf;
char *outbuf;
+ unsigned long dropped;
unsigned int len;
bool extmsg;
};
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index c9207d81b90c..14ddd5c9363f 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -1961,27 +1961,6 @@ static int console_trylock_spinning(void)
return 1;
}
-/*
- * Call the specified console driver, asking it to write out the specified
- * text and length. If @dropped_text is non-NULL and any records have been
- * dropped, a dropped message will be written out first.
- */
-static void call_console_driver(struct console *con, const char *text, size_t len,
- char *dropped_text)
-{
- size_t dropped_len;
-
- if (con->dropped && dropped_text) {
- dropped_len = snprintf(dropped_text, DROPPED_TEXT_MAX,
- "** %lu printk messages dropped **\n",
- con->dropped);
- con->dropped = 0;
- con->write(con, dropped_text, dropped_len);
- }
-
- con->write(con, text, len);
-}
-
/*
* Recursion is tracked separately on each CPU. If NMIs are supported, an
* additional NMI context per CPU is also separately tracked. Until per-CPU
@@ -2361,10 +2340,6 @@ static ssize_t msg_print_ext_body(char *buf, size_t size,
struct dev_printk_info *dev_info) { return 0; }
static void console_lock_spinning_enable(void) { }
static int console_lock_spinning_disable_and_check(void) { return 0; }
-static void call_console_driver(struct console *con, const char *text, size_t len,
- char *dropped_text)
-{
-}
static bool suppress_message_printing(int level) { return false; }
static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
@@ -2689,6 +2664,46 @@ static void __console_unlock(void)
up_console_sem();
}
+/**
+ * cons_print_dropped - Print 'dropped' message if required
+ * @desc: Pointer to the output descriptor
+ *
+ * Prints the 'dropped' message info the output buffer if @desc->dropped is
+ * not 0 and the regular format is requested. Extended format does not
+ * need this message because it prints the sequence numbers.
+ *
+ * In regular format the extended message buffer is not in use.
+ * So print into it at the beginning and move the resulting string
+ * just in front of the regular text buffer so that the message can
+ * be printed in one go.
+ *
+ * In case of a message this returns with @desc->outbuf and @desc->len
+ * updated. If no message is required then @desc is not modified.
+ */
+static void cons_print_dropped(struct cons_outbuf_desc *desc)
+{
+ struct cons_text_buf *txtbuf = desc->txtbuf;
+ size_t len;
+
+ if (!desc->dropped || desc->extmsg)
+ return;
+
+ if (WARN_ON_ONCE(desc->outbuf != txtbuf->text))
+ return;
+
+ /* Print it into ext_text which is unused */
+ len = snprintf(txtbuf->ext_text, DROPPED_TEXT_MAX,
+ "** %lu printk messages dropped **\n", desc->dropped);
+ desc->dropped = 0;
+
+ /* Copy it just below text so it goes out with one write */
+ memcpy(txtbuf->text - len, txtbuf->ext_text, len);
+
+ /* Update the descriptor */
+ desc->len += len;
+ desc->outbuf -= len;
+}
+
/**
* cons_fill_outbuf - Fill the output buffer with the next record
@@ -2742,17 +2757,26 @@ static bool cons_fill_outbuf(struct console *con, struct cons_outbuf_desc *desc)
}
if (desc->extmsg) {
+ /*
+ * Extended messages do not require "dropped" message
+ * as that can be seen from the sequence number
+ */
write_text = txtbuf->ext_text;
len = info_print_ext_header(write_text, CONSOLE_EXT_LOG_MAX, r.info);
len += msg_print_ext_body(write_text + len, CONSOLE_EXT_LOG_MAX - len,
&r.text_buf[0], r.info->text_len, &r.info->dev_info);
+ desc->len = len;
+ desc->outbuf = write_text;
} else {
- write_text = txtbuf->text;
len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
+
+ desc->len = len;
+ desc->outbuf = txtbuf->text;
+ desc->dropped = con->dropped;
+ cons_print_dropped(desc);
+ con->dropped = desc->dropped;
}
- desc->len = len;
- desc->outbuf = write_text;
return true;
}
@@ -2805,8 +2829,10 @@ static bool console_emit_next_record(struct console *con, struct cons_text_buf *
printk_safe_enter_irqsave(flags);
console_lock_spinning_enable();
- stop_critical_timings(); /* don't trace print latency */
- call_console_driver(con, desc.outbuf, desc.len, extmsg ? NULL : txtbuf->dropped_text);
+ /* don't trace print latency */
+ stop_critical_timings();
+ /* Write everything out to the hardware */
+ con->write(con, desc.outbuf, desc.len);
start_critical_timings();
con->seq++;
--
2.30.2
On Sat, 24 Sep 2022 02:10:44 +0206
John Ogness <[email protected]> (on behalf of Thomas Gleixner) wrote:
> + * Pretend that any of the below works in the same way as we pretend that
> + * any of PARISC works.
Not approving of what PARISC did here?
;-)
-- Steve
On Sat, Sep 24, 2022 at 02:10:36AM +0206, John Ogness wrote:
> Hi,
>
> This series is essentially the first 18 patches of tglx's RFC series
> [0] with only minor changes in comments and commit messages. It's
> purpose is to lay the groundwork for the upcoming threaded/atomic
> console printing posted as the RFC series and demonstrated at
> LPC2022 [1].
>
> This series is interesting for mainline because it cleans up various
> code and documentation quirks discovered while working on the new
> console printing implementation.
>
> Aside from cleanups, the main features introduced here are:
>
> - Converts the console's DIY linked list implementation to hlist.
>
> - Introduces a console list lock (mutex) so that readers (such as
> /proc/consoles) can safely iterate the consoles without blocking
> console printing.
>
> - Adds SRCU support to the console list to prepare for safe console
> list iterating from any context.
>
> - Refactors buffer handling to prepare for per-console, per-cpu,
> per-context atomic printing.
>
> The series has the following parts:
>
> Patches 1 - 5: Cleanups
>
> Patches 6 - 12: Locking and list conversion
>
> Patches 13 - 18: Improved output buffer handling to prepare for
> code sharing
>
These all look great to me, thanks for resending them.
Do you want them to go through my serial/tty tree, or is there some
other tree to take them through (printk?)
If they are to go through someone else's tree, feel free to add:
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On (22/09/24 02:10), John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Unprotected list walks are a brilliant idea. Especially in the context of
> hotpluggable consoles.
>
> The new list lock provides not only synchronization for console list
> manipulation, but also for manipulation of console->flags:
>
> console_list_lock();
> console_lock();
>
> /* may now manipulate the console list and/or console->flags */
>
> console_unlock();
> console_list_unlock();
>
> Therefore it is safe to iterate the console list and read console->flags
> if holding either the console lock or the console list lock.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
On (22/09/24 02:10), John Ogness wrote:
>
> Patches 6 - 12: Locking and list conversion
>
A quick question: I wonder why xenfb_make_preferred_console() isn't
converted to list lock and for_each_registered_console()?
On (22/09/24 02:10), John Ogness wrote:
> Add docbook comments to struct console.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
On (22/09/24 02:10), John Ogness wrote:
[..]
> static int console_unregister_locked(struct console *console)
> {
> - struct console *con;
> int res;
>
> con_printk(KERN_INFO, console, "disabled\n");
> @@ -3274,32 +3287,28 @@ static int console_unregister_locked(struct console *console)
> if (res > 0)
> return 0;
>
> - res = -ENODEV;
> console_lock();
> - if (console_drivers == console) {
> - console_drivers=console->next;
> - res = 0;
> - } else {
> - for_each_console(con) {
> - if (con->next == console) {
> - con->next = console->next;
> - res = 0;
> - break;
> - }
> - }
> - }
>
> - if (res)
> - goto out_disable_unlock;
> + /* Disable it unconditionally */
> + console->flags &= ~CON_ENABLED;
> +
> + if (hlist_unhashed(&console->node))
> + goto out_unlock;
Shouldn't this set `ret` to -ENODEV before goto? Otherwise it will always
return 0 (which is set by _braille_unregister_console()).
> +
> + hlist_del_init(&console->node);
>
> /*
> + * <HISTORICAL>
> * If this isn't the last console and it has CON_CONSDEV set, we
> * need to set it on the next preferred console.
> + * </HISTORICAL>
> + *
> + * The above makes no sense as there is no guarantee that the next
> + * console has any device attached. Oh well....
> */
It's complicated...
On (22/09/24 02:10), John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Provide a SRCU protected variant to walk the console list.
>
> Preperatory change for a new console infrastructure which operates
> independent of console BKL.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
On (22/09/24 02:10), John Ogness wrote:
> Date: Sat, 24 Sep 2022 02:10:52 +0206
> From: John Ogness <[email protected]>
> To: Petr Mladek <[email protected]>
> Cc: Sergey Senozhatsky <[email protected]>, Steven Rostedt
> <[email protected]>, Thomas Gleixner <[email protected]>,
> [email protected]
> Subject: [PATCH printk 16/18] printk: Use struct cons_text_buf
> Message-Id: <[email protected]>
>
> From: Thomas Gleixner <[email protected]>
>
> Replace the separately allocated output buffers with a single instance of
> struct cons_text_buf.
>
> Note that the buffer size of devkmsg_user.text_buf, when replaced with
> cons_text_buf.text, reduces from CONSOLE_EXT_LOG_MAX to CONSOLE_LOG_MAX.
> However, the buffer is only used to read ringbuffer records, which have
> a maximum size of LOG_LINE_MAX (CONSOLE_LOG_MAX - PREFIX_MAX).
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
[..]
> @@ -2765,7 +2766,7 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
> console_lock_spinning_enable();
>
> stop_critical_timings(); /* don't trace print latency */
> - call_console_driver(con, write_text, len, dropped_text);
+ /* Extended consoles do not print "dropped messages". */ perhaps?
> + call_console_driver(con, write_text, len, extmsg ? NULL : txtbuf->dropped_text);
On (22/09/24 02:10), John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Create a data structure to replace the open coded separate buffers for
> regular and extended formatting.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
On (22/09/24 02:10), John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Move the buffer size defines to console.h in preparation of adding a buffer
> structure.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
Reviewed-by: Sergey Senozhatsky <[email protected]>
Hi John,
* John Ogness <[email protected]>:
> From: Thomas Gleixner <[email protected]>
>
> Replace the open coded single linked list with a hlist so a conversion to
> SRCU protected list walks can reuse the existing primitives.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> ---
> arch/parisc/kernel/pdc_cons.c | 19 +++----
> fs/proc/consoles.c | 5 +-
> include/linux/console.h | 15 ++++--
> kernel/printk/printk.c | 99 +++++++++++++++++++----------------
> 4 files changed, 75 insertions(+), 63 deletions(-)
>
> diff --git a/arch/parisc/kernel/pdc_cons.c b/arch/parisc/kernel/pdc_cons.c
> index 9a0c0932d2f9..3f9abf0263ee 100644
> --- a/arch/parisc/kernel/pdc_cons.c
> +++ b/arch/parisc/kernel/pdc_cons.c
> @@ -272,15 +267,17 @@ void pdc_console_restart(bool hpmc)
> if (pdc_console_initialized)
> return;
>
> - if (!hpmc && console_drivers)
> + if (!hpmc && !hlist_empty(&console_list))
> return;
>
> /* If we've already seen the output, don't bother to print it again */
> - if (console_drivers != NULL)
> + if (!hlist_empty(&console_list))
> pdc_cons.flags &= ~CON_PRINTBUFFER;
>
> - while ((console = console_drivers) != NULL)
> - unregister_console(console_drivers);
> + while (!hlist_empty(&console_list)) {
> + unregister_console(READ_ONCE(hlist_entry(console_list.first,
> + struct console, node)));
> + }
>
> /* force registering the pdc console */
> pdc_console_init_force();
Thanks for doing this!!
I had to add the hunks below on top of your patch to make it compile
and boot sucessfully on parisc.
Maybe you could fold those into your patch?
Thanks!
Helge
diff --git a/arch/parisc/kernel/pdc_cons.c b/arch/parisc/kernel/pdc_cons.c
index 3f9abf0263ee..f15998aa47a8 100644
--- a/arch/parisc/kernel/pdc_cons.c
+++ b/arch/parisc/kernel/pdc_cons.c
@@ -262,8 +262,6 @@ void __init pdc_console_init(void)
*/
void pdc_console_restart(bool hpmc)
{
- struct console *console;
-
if (pdc_console_initialized)
return;
@@ -275,8 +273,8 @@ void pdc_console_restart(bool hpmc)
pdc_cons.flags &= ~CON_PRINTBUFFER;
while (!hlist_empty(&console_list)) {
- unregister_console(READ_ONCE(hlist_entry(console_list.first,
- struct console, node)));
+ unregister_console(hlist_entry(console_list.first,
+ struct console, node));
}
/* force registering the pdc console */
On 9/24/22 02:04, John Ogness wrote:
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -79,20 +79,20 @@ int oops_in_progress;
> EXPORT_SYMBOL(oops_in_progress);
>
> /*
> - * console_sem protects the console_drivers list, and also provides
> - * serialization for access to the entire console driver system.
> + * console_sem protects onsole_list, and also provides serialization for
Typo: missing the "c"
onsole_list -> console_list
Helge
On (22/09/24 19:20), Helge Deller wrote:
> diff --git a/arch/parisc/kernel/pdc_cons.c b/arch/parisc/kernel/pdc_cons.c
> index 3f9abf0263ee..f15998aa47a8 100644
> --- a/arch/parisc/kernel/pdc_cons.c
> +++ b/arch/parisc/kernel/pdc_cons.c
> @@ -262,8 +262,6 @@ void __init pdc_console_init(void)
> */
> void pdc_console_restart(bool hpmc)
> {
> - struct console *console;
> -
> if (pdc_console_initialized)
> return;
>
> @@ -275,8 +273,8 @@ void pdc_console_restart(bool hpmc)
> pdc_cons.flags &= ~CON_PRINTBUFFER;
>
> while (!hlist_empty(&console_list)) {
> - unregister_console(READ_ONCE(hlist_entry(console_list.first,
> - struct console, node)));
> + unregister_console(hlist_entry(console_list.first,
> + struct console, node));
In this case we maybe can use cons_first() macro here (perhaps
give it a little more clear name (first_console()?) and move to
printk.h) and also do READ_ONCE there
On 2022-09-24, Greg Kroah-Hartman <[email protected]> wrote:
> These all look great to me, thanks for resending them.
>
> Do you want them to go through my serial/tty tree, or is there some
> other tree to take them through (printk?)
Thanks Greg. but I would prefer they go through the printk tree. In
particular, I want Petr to have the chance to review patches 15-18.
> If they are to go through someone else's tree, feel free to add:
>
> Reviewed-by: Greg Kroah-Hartman <[email protected]>
Thanks!
John
On (22/09/24 02:10), John Ogness wrote:
> +/**
> + * cons_print_dropped - Print 'dropped' message if required
> + * @desc: Pointer to the output descriptor
> + *
> + * Prints the 'dropped' message info the output buffer if @desc->dropped is
> + * not 0 and the regular format is requested. Extended format does not
> + * need this message because it prints the sequence numbers.
> + *
> + * In regular format the extended message buffer is not in use.
> + * So print into it at the beginning and move the resulting string
> + * just in front of the regular text buffer so that the message can
> + * be printed in one go.
> + *
> + * In case of a message this returns with @desc->outbuf and @desc->len
> + * updated. If no message is required then @desc is not modified.
> + */
> +static void cons_print_dropped(struct cons_outbuf_desc *desc)
> +{
> + struct cons_text_buf *txtbuf = desc->txtbuf;
> + size_t len;
> +
> + if (!desc->dropped || desc->extmsg)
> + return;
> +
> + if (WARN_ON_ONCE(desc->outbuf != txtbuf->text))
> + return;
> +
> + /* Print it into ext_text which is unused */
> + len = snprintf(txtbuf->ext_text, DROPPED_TEXT_MAX,
> + "** %lu printk messages dropped **\n", desc->dropped);
> + desc->dropped = 0;
> +
> + /* Copy it just below text so it goes out with one write */
> + memcpy(txtbuf->text - len, txtbuf->ext_text, len);
> +
> + /* Update the descriptor */
> + desc->len += len;
> + desc->outbuf -= len;
Oh, hmm. This does not look to me as a simplification. Quite
the opposite, moving cons_text_buf::text pointer to point to
cons_text_buf::text - strlen("... dropped messages...") looks
somewhat fragile.
Is printing 'dropped' and outbuf messages in one go such an
important feature?
On 2022-09-26, Sergey Senozhatsky <[email protected]> wrote:
> On (22/09/24 02:10), John Ogness wrote:
>> +/**
>> + * cons_print_dropped - Print 'dropped' message if required
>> + * @desc: Pointer to the output descriptor
>> + *
>> + * Prints the 'dropped' message info the output buffer if @desc->dropped is
>> + * not 0 and the regular format is requested. Extended format does not
>> + * need this message because it prints the sequence numbers.
>> + *
>> + * In regular format the extended message buffer is not in use.
>> + * So print into it at the beginning and move the resulting string
>> + * just in front of the regular text buffer so that the message can
>> + * be printed in one go.
>> + *
>> + * In case of a message this returns with @desc->outbuf and @desc->len
>> + * updated. If no message is required then @desc is not modified.
>> + */
>> +static void cons_print_dropped(struct cons_outbuf_desc *desc)
>> +{
>> + struct cons_text_buf *txtbuf = desc->txtbuf;
>> + size_t len;
>> +
>> + if (!desc->dropped || desc->extmsg)
>> + return;
>> +
>> + if (WARN_ON_ONCE(desc->outbuf != txtbuf->text))
>> + return;
>> +
>> + /* Print it into ext_text which is unused */
>> + len = snprintf(txtbuf->ext_text, DROPPED_TEXT_MAX,
>> + "** %lu printk messages dropped **\n", desc->dropped);
>> + desc->dropped = 0;
>> +
>> + /* Copy it just below text so it goes out with one write */
>> + memcpy(txtbuf->text - len, txtbuf->ext_text, len);
>> +
>> + /* Update the descriptor */
>> + desc->len += len;
>> + desc->outbuf -= len;
>
> Oh, hmm. This does not look to me as a simplification. Quite
> the opposite, moving cons_text_buf::text pointer to point to
> cons_text_buf::text - strlen("... dropped messages...") looks
> somewhat fragile.
It relies on @ext_text and @text being packed together, which yes, may
be fragile. As an alternative we could memcpy the message text (@text)
to the end of the dropped message text. There would be enough room.
Generally speaking, the dropped text will be less text to copy. But
since dropped messages are rare anyway, it might be worth copying more
data so that the code is not fragile. It would also allow us to remove
the __no_randomize_layout in "struct cons_text_buf".
If the end of cons_print_dropped was changed to:
memcpy(txtbuf->ext_text + len, txtbuf->text, desc->len);
desc->len += len;
desc->outbuf = txtbuf->ext_text;
Would that be OK for you?
> Is printing 'dropped' and outbuf messages in one go such an
> important feature?
I think it is a nice simplification. With the cons_text_buf, it makes it
quite easy to implement.
John
On (22/09/24 02:10), John Ogness wrote:
[..]
> +/**
> + * cons_print_dropped - Print 'dropped' message if required
> + * @desc: Pointer to the output descriptor
> + *
> + * Prints the 'dropped' message info the output buffer if @desc->dropped is
> + * not 0 and the regular format is requested. Extended format does not
> + * need this message because it prints the sequence numbers.
> + *
> + * In regular format the extended message buffer is not in use.
> + * So print into it at the beginning and move the resulting string
> + * just in front of the regular text buffer so that the message can
> + * be printed in one go.
> + *
> + * In case of a message this returns with @desc->outbuf and @desc->len
> + * updated. If no message is required then @desc is not modified.
> + */
> +static void cons_print_dropped(struct cons_outbuf_desc *desc)
A silly nit: as far as I can tell printk API uses console_foo for
naming, so my personal preference would be to spell console_ instead
of cons_ (in this and in previous patches).
> +{
> + struct cons_text_buf *txtbuf = desc->txtbuf;
> + size_t len;
> +
> + if (!desc->dropped || desc->extmsg)
> + return;
> +
> + if (WARN_ON_ONCE(desc->outbuf != txtbuf->text))
> + return;
> +
> + /* Print it into ext_text which is unused */
> + len = snprintf(txtbuf->ext_text, DROPPED_TEXT_MAX,
> + "** %lu printk messages dropped **\n", desc->dropped);
> + desc->dropped = 0;
> +
> + /* Copy it just below text so it goes out with one write */
> + memcpy(txtbuf->text - len, txtbuf->ext_text, len);
> +
> + /* Update the descriptor */
> + desc->len += len;
> + desc->outbuf -= len;
> +}
> +
>
An even sillier nit: extra blank line /* can't help noticing it every time
I read this function :) */
On Sat 2022-09-24 02:10 +0206, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Provide a special list iterator macro for KGDB to allow unprotected list
> walks and add a few comments to explain the hope based approach.
>
> Preperatory change for changing the console list to hlist and adding
> lockdep asserts to regular list walks.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
> ---
> drivers/tty/serial/kgdboc.c | 5 ++++-
> include/linux/console.h | 10 ++++++++++
> kernel/debug/kdb/kdb_io.c | 7 ++++++-
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index af2aa76bae15..57a5fd27dffe 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -462,10 +462,13 @@ static void kgdboc_earlycon_pre_exp_handler(void)
> * we have no other choice so we keep using it. Since not all
> * serial drivers might be OK with this, print a warning once per
> * boot if we detect this case.
> + *
> + * Pretend that walking the console list is safe...
> */
> - for_each_console(con)
> + for_each_console_kgdb(con) {
> if (con == kgdboc_earlycon_io_ops.cons)
> return;
> + }
>
> already_warned = true;
> pr_warn("kgdboc_earlycon is still using bootconsole\n");
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 24344f9b0bc1..86a6125512b9 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -187,6 +187,16 @@ extern void console_list_unlock(void) __releases(console_mutex);
> #define for_each_console(con) \
> for (con = console_drivers; con != NULL; con = con->next)
>
> +/**
> + * for_each_console_kgdb() - Iterator over registered consoles for KGDB
> + * @con: struct console pointer used as loop cursor
> + *
> + * Has no serialization requirements and KGDB pretends that this is safe.
> + * Don't use outside of the KGDB fairy tale land!
> + */
> +#define for_each_console_kgdb(con) \
> + for (con = console_drivers; con != NULL; con = con->next)
> +
> extern int console_set_on_cmdline;
> extern struct console *early_console;
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 67d3c48a1522..fb3775e61a3b 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -558,7 +558,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
> cp++;
> }
>
> - for_each_console(c) {
> + /*
> + * This is a completely unprotected list walk designed by the
> + * wishful thinking department. See the oops_in_progress comment
> + * below - especially the encourage section...
> + */
> + for_each_console_kgdb(c) {
> if (!(c->flags & CON_ENABLED))
> continue;
> if (c == dbg_io_ops->cons)
> --
> 2.30.2
>
Reviewed-by: Aaron Tomlin <[email protected]>
--
Aaron Tomlin
On (22/09/26 10:00), John Ogness wrote:
> > Oh, hmm. This does not look to me as a simplification. Quite
> > the opposite, moving cons_text_buf::text pointer to point to
> > cons_text_buf::text - strlen("... dropped messages...") looks
> > somewhat fragile.
>
> It relies on @ext_text and @text being packed together, which yes, may
> be fragile.
Right, and this assumption can be broken by both internal and external
sources: new gcc/clang plugins, config options, etc.
> As an alternative we could memcpy the message text (@text)
> to the end of the dropped message text. There would be enough room.
>
> Generally speaking, the dropped text will be less text to copy. But
> since dropped messages are rare anyway, it might be worth copying more
> data so that the code is not fragile. It would also allow us to remove
> the __no_randomize_layout in "struct cons_text_buf".
Agreed.
> If the end of cons_print_dropped was changed to:
>
> memcpy(txtbuf->ext_text + len, txtbuf->text, desc->len);
> desc->len += len;
> desc->outbuf = txtbuf->ext_text;
>
> Would that be OK for you?
Yes, this looks solid (nothing should be able to break it and cause
mem corruptions).
On Sat 2022-09-24 02:10:37, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> No user outside the printk code and no reason to export this.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Sat 2022-09-24 02:10:41, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> This variable cannot change post boot.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Sat 2022-09-24 02:10:40, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> The comment about unregistering boot consoles is just not matching the
> reality. Remove it.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Sat 2022-09-24 02:10:43, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Facilities which expose console information to sysfs or procfs can use the
> new list protection to keep the list stable. No need to hold console lock.
>
> drivers/tty/tty_io.c | 6 +++---
> fs/proc/consoles.c | 6 +++---
> kernel/printk/printk.c | 8 ++++----
As described in the review of the 6th patch, the semantic of
the list_lock (module_mutex) is not well defined from my POV.
I would prefer to keep only one global console lock.
That said, the procf and sysfs interface is read-only. It seems
to be safe to show the info under the new console_srcu read lock.
On the other hand, console_device() should see the console
list in a consistent state. The first console with tty console->driver
should have the CON_CONSDEV flag set. Alternatively, we could
manipulate the list and the flag a safe way from the SRCU POV
but it is not worth it. So, I would keep console_lock()
in console_device() for now.
Best Regards,
Petr
Resending my review on this patch also in this patchset.
I sent the review also to the RFC patchset by mistake, see
https://lore.kernel.org/r/YzLIy4emYX6JpzuN@alley
Please, continue the discussion here where I did review of the other patches.
On Sat 2022-09-24 02:10:42, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Unprotected list walks are a brilliant idea. Especially in the context of
> hotpluggable consoles.
Yeah, it is crazy. And it is there probably since the beginning.
> The new list lock provides not only synchronization for console list
> manipulation, but also for manipulation of console->flags:
>
> console_list_lock();
> console_lock();
>
> /* may now manipulate the console list and/or console->flags */
>
> console_unlock();
> console_list_unlock();
>
> Therefore it is safe to iterate the console list and read console->flags
> if holding either the console lock or the console list lock.
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -79,10 +79,17 @@ int oops_in_progress;
> EXPORT_SYMBOL(oops_in_progress);
>
> /*
> - * console_sem protects the console_drivers list, and also
> - * provides serialisation for access to the entire console
> - * driver system.
> + * console_sem protects the console_drivers list, and also provides
> + * serialization for access to the entire console driver system.
> + *
> + * console_mutex serializes register/unregister.
> + *
> + * console_sem must be taken inside a console_mutex locked section
> + * for any list manipulation in order to keep the console BKL
> + * machinery happy. This requirement also applies to manipulation
> + * of console->flags.
> */
> +static DEFINE_MUTEX(console_mutex);
> static DEFINE_SEMAPHORE(console_sem);
> struct console *console_drivers;
> EXPORT_SYMBOL_GPL(console_drivers);
> @@ -220,6 +233,28 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
> }
> #endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
>
> +/**
> + * console_list_lock - Lock the console list
> + *
> + * For non-console related list walks, e.g. procfs, sysfs...
> + */
> +void console_list_lock(void)
> +{
> + mutex_lock(&console_mutex);
> +}
> +EXPORT_SYMBOL(console_list_lock);
> +
> +/**
> + * console_list_unlock - Unlock the console list
> + *
> + * Counterpart to console_list_lock()
> + */
> +void console_list_unlock(void)
> +{
> + mutex_unlock(&console_mutex);
> +}
> +EXPORT_SYMBOL(console_list_unlock);
> +
> /*
> * Helper macros to handle lockdep when locking/unlocking console_sem. We use
> * macros instead of functions so that _RET_IP_ contains useful information.
> @@ -3081,6 +3120,8 @@ static void try_enable_default_console(struct console *newcon)
> (con->flags & CON_BOOT) ? "boot" : "", \
> con->name, con->index, ##__VA_ARGS__)
>
> +static int console_unregister_locked(struct console *console);
> +
> /*
> * The console driver calls this routine during kernel initialization
> * to register the console printing procedure with printk() and to
> @@ -3107,13 +3148,14 @@ void register_console(struct console *newcon)
> bool realcon_enabled = false;
> int err;
>
> - for_each_console(con) {
> + console_list_lock();
Hmm, the new mutex is really nasty. It has very strange semantic.
It makes the locking even more complicated.
The ideal solution would be take console_lock() here. We (me and
Sergey) never did it because con->match() and con->setup()
callbacks were called in try_enable_*console(). We were afraid
that some might want to take console_lock() and it could create
a deadlock. There were too many drivers and we did not found time
to check them all. And it had low priority because nobody reported
problems.
A good enough solution might be call this under the later
added srcu_read_lock(&console_srcu) and use for_each_console_srcu().
The srcu walk would prevent seeing broken list. Obviously,
the code might see outdated list and do bad decisions:
+ try to enable the same console twice
+ enable more consoles by default in try_enable_default_console()
+ associate more consoles with /dev/console, see CON_CONSDEV in
try_enable_preferred_console() and try_enable_default_console()
If we race then we could end up with more consoles enabled by default
and with more consoles with CON_CONSDEV flag.
IMHO, the rcu walk is an acceptable and conservative solution.
Registering the same driver twice is hard to imagine at all.
And I have never seen reports about too many default consoles
or CON_CONSDEV flags.
Anyway, I would like to avoid adding console_mutex. From my POV,
it is a hack that complicates the code. Taking console_lock()
should be enough. Using rcu walk would be good enough.
Do I miss something, please?
Or is this part of some strategy to remove console_sem later, please?
> + for_each_registered_console(con) {
> if (WARN(con == newcon, "console '%s%d' already registered\n",
> con->name, con->index))
> - return;
> + goto unlock;
> }
>
> - for_each_console(con) {
> + for_each_registered_console(con) {
> if (con->flags & CON_BOOT)
> bootcon_enabled = true;
> else
Best Regards,
Petr
On (22/09/27 17:16), Petr Mladek wrote:
[..]
> > +static int console_unregister_locked(struct console *console);
> > +
> > /*
> > * The console driver calls this routine during kernel initialization
> > * to register the console printing procedure with printk() and to
> > @@ -3107,13 +3148,14 @@ void register_console(struct console *newcon)
> > bool realcon_enabled = false;
> > int err;
> >
> > - for_each_console(con) {
> > + console_list_lock();
>
> Hmm, the new mutex is really nasty. It has very strange semantic.
> It makes the locking even more complicated.
[..]
I fully agree with everything you said. This lock nesting made me
scratch my head wondering was it previous CPU hotplug code that had
multiple nested locks or was it something else?
> Anyway, I would like to avoid adding console_mutex. From my POV,
> it is a hack that complicates the code. Taking console_lock()
> should be enough. Using rcu walk would be good enough.
>
> Do I miss something, please?
>
> Or is this part of some strategy to remove console_sem later, please?
So I can only explain what potential I saw in list lock: the idea
that third party that iterates over consoles lists does not stop
entire console output machinery, and, moreover, that third party
does not flush pending messages once it's done with the business
it had to do under console_sem. E.g. it can be a systemd or any
other user-space process doing something with /dev/tty, which can
suddenly stop all consoles output (console_lock()) and then also
has to flush pending kernel messages (console_unlock()). Was this
goal, however, fully achieved - no, a third party that wants to
->flags &= ~CON_ENABLED a particular console still stops the entire
console output (and flushes pending messages, unless handover-ed).
I like what you suggested with srcu.
Hi,
On Fri, Sep 23, 2022 at 5:05 PM John Ogness <[email protected]> wrote:
>
> From: Thomas Gleixner <[email protected]>
>
> Provide a special list iterator macro for KGDB to allow unprotected list
> walks and add a few comments to explain the hope based approach.
>
> Preperatory change for changing the console list to hlist and adding
s/Preperatory/Preparatory
> lockdep asserts to regular list walks.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
> ---
> drivers/tty/serial/kgdboc.c | 5 ++++-
> include/linux/console.h | 10 ++++++++++
> kernel/debug/kdb/kdb_io.c | 7 ++++++-
> 3 files changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index af2aa76bae15..57a5fd27dffe 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -462,10 +462,13 @@ static void kgdboc_earlycon_pre_exp_handler(void)
> * we have no other choice so we keep using it. Since not all
> * serial drivers might be OK with this, print a warning once per
> * boot if we detect this case.
> + *
> + * Pretend that walking the console list is safe...
To be fair, this is not quite as unsafe as your comment makes it
sound. kgdb is a "stop the world" debugger and when this function is
executing then all of the other CPUs in the system should have been
rounded up and idle (or, perhaps, busy looping). Essentially as long
as console list manipulation is always made in a way that each
instruction keeps the list in a reasonable state then what kgdb is
doing is actually "safe". Said another way: we could drop into the
debugger at any point when a task is manipulating the console list,
but once we're in the debugger and are executing the "pre_exp_handler"
then all the other CPUs have been frozen in time.
> */
> - for_each_console(con)
> + for_each_console_kgdb(con) {
> if (con == kgdboc_earlycon_io_ops.cons)
> return;
> + }
>
> already_warned = true;
> pr_warn("kgdboc_earlycon is still using bootconsole\n");
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 24344f9b0bc1..86a6125512b9 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -187,6 +187,16 @@ extern void console_list_unlock(void) __releases(console_mutex);
> #define for_each_console(con) \
> for (con = console_drivers; con != NULL; con = con->next)
>
> +/**
> + * for_each_console_kgdb() - Iterator over registered consoles for KGDB
> + * @con: struct console pointer used as loop cursor
> + *
> + * Has no serialization requirements and KGDB pretends that this is safe.
> + * Don't use outside of the KGDB fairy tale land!
> + */
> +#define for_each_console_kgdb(con) \
> + for (con = console_drivers; con != NULL; con = con->next)
> +
> extern int console_set_on_cmdline;
> extern struct console *early_console;
>
> diff --git a/kernel/debug/kdb/kdb_io.c b/kernel/debug/kdb/kdb_io.c
> index 67d3c48a1522..fb3775e61a3b 100644
> --- a/kernel/debug/kdb/kdb_io.c
> +++ b/kernel/debug/kdb/kdb_io.c
> @@ -558,7 +558,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
> cp++;
> }
>
> - for_each_console(c) {
> + /*
> + * This is a completely unprotected list walk designed by the
> + * wishful thinking department. See the oops_in_progress comment
> + * below - especially the encourage section...
The reality is also a little less dire here than the comment suggests.
IMO this is actually not the same as the "oops_in_progress" case that
the comment refers to.
Specifically, the "oops_in_progress" is referring to the fact that
it's not uncommon to drop into the debugger when a serial driver (the
same one you're using for kgdb) is holding its lock. Possibly it's
printing something to the tty running on the UART dumping stuff out
from the kernel's console. That's not great and I won't pretend that
the kgdb design is amazing here, but...
Just like above, I don't feel like iterating through the console list
here without holding the lock is necessarily unsafe. Just like above,
all the rest of the CPUs in the system are in a holding pattern and
aren't actively executing any code. While we may have interrupted them
at any given instruction, they won't execute any more instruction
until we leave kgdb and resume running.
Hi,
On Fri, Sep 23, 2022 at 5:05 PM John Ogness <[email protected]> wrote:
>
> From: Thomas Gleixner <[email protected]>
>
> Unprotected list walks are not necessarily safe.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
> ---
> drivers/tty/serial/kgdboc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> index 79b7db8580e0..af2aa76bae15 100644
> --- a/drivers/tty/serial/kgdboc.c
> +++ b/drivers/tty/serial/kgdboc.c
> @@ -193,7 +193,8 @@ static int configure_kgdboc(void)
> if (!p)
> goto noconfig;
>
> - for_each_console(cons) {
> + console_list_lock();
> + for_each_registered_console(cons) {
> int idx;
> if (cons->device && cons->device(cons, &idx) == p &&
> idx == tty_line) {
> @@ -201,6 +202,7 @@ static int configure_kgdboc(void)
> break;
> }
> }
> + console_list_unlock();
Seems right to me, thanks!
Reviewed-by: Douglas Anderson <[email protected]>
On 2022-09-27, Petr Mladek <[email protected]> wrote:
> Hmm, the new mutex is really nasty. It has very strange semantic.
> It makes the locking even more complicated.
We are working to replace the BKL-console_lock with new separate clearly
defined mechanisms.
The new mutex provides full synchronization for list changes as well as
changes to items of that list. (Really console->flags is the only change
to items of the list.)
For some places in the code it is very clear that the console_lock can
be completely replaced (either with srcu or the new mutex). For other
places, it is not yet clear why the console_lock is being used and so
both console_lock and mutex are used.
> The ideal solution would be take console_lock() here.
We should be looking where we can remove console_lock, not identifying
new locations to add it.
> A good enough solution might be call this under the later added
> srcu_read_lock(&console_srcu) and use for_each_console_srcu().
@console_srcu does not allow safe reading of console->flags. It only
provides safe list iteration and reading of immutable fields. The new
mutex must be used for reading console->flags.
Note that for the NOBKL consoles (not part of this series), a new atomic
state variable is used so that console->flags is not needed. That means
for NOBKL consoles the new mutex is further reduced in scope to provide
only list synchronization.
> Or is this part of some strategy to remove console_sem later, please?
Yes! One of the main points of this final phase of the rework is to
remove console_sem usage (for NOBKL consoles). If a system is running
with only NOBKL consoles registered, ideally that system should never
call console_lock()/console_trylock(). Once all drivers have converted
over to the NOBKL interface, console_sem will serve no purpose for the
printk and console frameworks, so it can be removed.
John
On Thu 2022-09-29 01:48:29, John Ogness wrote:
> On 2022-09-27, Petr Mladek <[email protected]> wrote:
> > Hmm, the new mutex is really nasty. It has very strange semantic.
> > It makes the locking even more complicated.
>
> We are working to replace the BKL-console_lock with new separate clearly
> defined mechanisms.
>
> The new mutex provides full synchronization for list changes as well as
> changes to items of that list. (Really console->flags is the only change
> to items of the list.)
OK.
> For some places in the code it is very clear that the console_lock can
> be completely replaced (either with srcu or the new mutex). For other
> places, it is not yet clear why the console_lock is being used and so
> both console_lock and mutex are used.
One important and tricky location is console_trylock() in
vprintk_emit(). And the related for_each_console() called from
console_unlock()->console_flush_all().
It is the legacy mode that tries to print to the consoles immediately.
I am not sure if we could _ever_ remove this mode.
And it is most likely the main reason why semaphore is used instead
of a mutex:
+ printk() can be called in atomic context
+ also there is the console_trylock_spinning() trick that allows
to transfer the semaphore to another owner without locking.
Do you see any RT-friendly solution for the legacy mode, please?
Maybe, an atomic variable (cmpxchg) can be used together with
the SRCU list. But I am not sure if srcu_read_lock can be
transferred to another context. Also this would not solve priority
inversion. Not to say that it might kill SRCU performance on
the entire system.
> > The ideal solution would be take console_lock() here.
>
> We should be looking where we can remove console_lock, not identifying
> new locations to add it.
Yes, we do not want this big kernel lock. Honestly, I am not
completely sure what is the exact purpose. My guess is that
console_lock() is used to prevent calling con->write() when
some internal console driver state is manipulated.
If the above is true then it might be solvable by some
driver-specific lock. The question is where the lock should
be. It is possible that it might require adding
the lock into struct console.
Anyway, some lock will still be needed to synchronize the list.
But could it be mutex? What about the legacy mode of printk_emit()?
> > A good enough solution might be call this under the later added
> > srcu_read_lock(&console_srcu) and use for_each_console_srcu().
>
> @console_srcu does not allow safe reading of console->flags. It only
> provides safe list iteration and reading of immutable fields. The new
> mutex must be used for reading console->flags.
>
> Note that for the NOBKL consoles (not part of this series), a new atomic
> state variable is used so that console->flags is not needed. That means
> for NOBKL consoles the new mutex is further reduced in scope to provide
> only list synchronization.
Good to know.
> > Or is this part of some strategy to remove console_sem later, please?
>
> Yes! One of the main points of this final phase of the rework is to
> remove console_sem usage (for NOBKL consoles). If a system is running
> with only NOBKL consoles registered, ideally that system should never
> call console_lock()/console_trylock(). Once all drivers have converted
> over to the NOBKL interface, console_sem will serve no purpose for the
> printk and console frameworks, so it can be removed.
Is this realistic?
And even if we convert all console drivers then people still might
want the legacy mode.
My understanding is that some atomic consoles would be real hacks.
They might be good enough for panic(). But what about running system.
It seems that people might want the legacy more even on running
system. Will it be doable with mutex?
I am sorry. I was about to answer this mail with "fair enough". But
then I thought more about it...
I would really like to avoid state where we have two locks (semaphore
and mutex) serializing the same thing (console list).
Best Regards,
Petr
On Sat 2022-09-24 02:10:36, John Ogness wrote:
> Hi,
>
> This series is essentially the first 18 patches of tglx's RFC series
> [0] with only minor changes in comments and commit messages. It's
> purpose is to lay the groundwork for the upcoming threaded/atomic
> console printing posted as the RFC series and demonstrated at
> LPC2022 [1].
>
> This series is interesting for mainline because it cleans up various
> code and documentation quirks discovered while working on the new
> console printing implementation.
>
> Aside from cleanups, the main features introduced here are:
>
> - Converts the console's DIY linked list implementation to hlist.
>
> - Introduces a console list lock (mutex) so that readers (such as
> /proc/consoles) can safely iterate the consoles without blocking
> console printing.
>
> - Adds SRCU support to the console list to prepare for safe console
> list iterating from any context.
>
> - Refactors buffer handling to prepare for per-console, per-cpu,
> per-context atomic printing.
>
> The series has the following parts:
>
> Patches 1 - 5: Cleanups
>
> Patches 6 - 12: Locking and list conversion
>
> Patches 13 - 18: Improved output buffer handling to prepare for
> code sharing
>
> Thomas Gleixner (18):
> printk: Make pr_flush() static
> printk: Declare log_wait properly
> printk: Remove write only variable nr_ext_console_drivers
> printk: Remove bogus comment vs. boot consoles
> printk: Mark __printk percpu data ready __ro_after_init
JFYI, I have pushed the first 5 cleanup patches into printk/linux.git,
branch rework/kthreads. The aim is to get them into 6.1.
The rest of the patchset is still being discussed.
Best Regards,
Petr
On Sat 2022-09-24 02:10:45, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Unprotected list walks are not necessarily safe.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
It looks correct in principle. There is still a discussion [1] whether
to introduce console_list_lock() or use the existing console_lock(),
see https://lore.kernel.org/r/[email protected]
Depending on the result of the discussion, with either
console_list_lock() or console_lock():
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Sat 2022-09-24 02:10:44, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> PARISC has a hope based mechanism to restore consoles in case of a HPMC
> (machine check exception) which is scattered over several places.
>
> Move it into one place to make further changes simpler and add comments.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> Reviewed-by: Sergey Senozhatsky <[email protected]>
I agree that the mechanism is weird. A bit more safe approach would be
to just disable all consoles instead of unregistering them...
Anyway, the patch does not change the existing behavior:
Reviewed-by: Petr Mladek <[email protected]>
Best Regards,
Petr
On Wed 2022-09-28 16:32:15, Doug Anderson wrote:
> Hi,
>
> On Fri, Sep 23, 2022 at 5:05 PM John Ogness <[email protected]> wrote:
> >
> > From: Thomas Gleixner <[email protected]>
> >
> > Provide a special list iterator macro for KGDB to allow unprotected list
> > walks and add a few comments to explain the hope based approach.
> >
> > Preperatory change for changing the console list to hlist and adding
>
> s/Preperatory/Preparatory
>
> > lockdep asserts to regular list walks.
> >
> > diff --git a/drivers/tty/serial/kgdboc.c b/drivers/tty/serial/kgdboc.c
> > index af2aa76bae15..57a5fd27dffe 100644
> > --- a/drivers/tty/serial/kgdboc.c
> > +++ b/drivers/tty/serial/kgdboc.c
> > @@ -462,10 +462,13 @@ static void kgdboc_earlycon_pre_exp_handler(void)
> > * we have no other choice so we keep using it. Since not all
> > * serial drivers might be OK with this, print a warning once per
> > * boot if we detect this case.
> > + *
> > + * Pretend that walking the console list is safe...
>
> To be fair, this is not quite as unsafe as your comment makes it
> sound. kgdb is a "stop the world" debugger and when this function is
> executing then all of the other CPUs in the system should have been
> rounded up and idle (or, perhaps, busy looping). Essentially as long
> as console list manipulation is always made in a way that each
> instruction keeps the list in a reasonable state then what kgdb is
> doing is actually "safe". Said another way: we could drop into the
> debugger at any point when a task is manipulating the console list,
> but once we're in the debugger and are executing the "pre_exp_handler"
> then all the other CPUs have been frozen in time.
The code in register_console()/unregister_console() seems to
manipulate the list in the right order. But the correctness
is not guaranteed because there are neither compiler nor
memory barriers.
That said, later patches add for_each_console_srcu(). IMHO,
the SRCU walk should be safe here.
>
> > */
> > - for_each_console(con)
> > + for_each_console_kgdb(con) {
> > if (con == kgdboc_earlycon_io_ops.cons)
> > return;
> > + }
> >
> > already_warned = true;
> > pr_warn("kgdboc_earlycon is still using bootconsole\n");
> > --- a/kernel/debug/kdb/kdb_io.c
> > +++ b/kernel/debug/kdb/kdb_io.c
> > @@ -558,7 +558,12 @@ static void kdb_msg_write(const char *msg, int msg_len)
> > cp++;
> > }
> >
> > - for_each_console(c) {
> > + /*
> > + * This is a completely unprotected list walk designed by the
> > + * wishful thinking department. See the oops_in_progress comment
> > + * below - especially the encourage section...
>
> The reality is also a little less dire here than the comment suggests.
> IMO this is actually not the same as the "oops_in_progress" case that
> the comment refers to.
>
> Specifically, the "oops_in_progress" is referring to the fact that
> it's not uncommon to drop into the debugger when a serial driver (the
> same one you're using for kgdb) is holding its lock. Possibly it's
> printing something to the tty running on the UART dumping stuff out
> from the kernel's console. That's not great and I won't pretend that
> the kgdb design is amazing here, but...
>
> Just like above, I don't feel like iterating through the console list
> here without holding the lock is necessarily unsafe. Just like above,
> all the rest of the CPUs in the system are in a holding pattern and
> aren't actively executing any code. While we may have interrupted them
> at any given instruction, they won't execute any more instruction
> until we leave kgdb and resume running.
The atomic consoles might improve the situation. Well, the hand shake
will not really work because the current owner might be stopped.
But we will at least know that the port is not in a safe state.
Anyway, what about using the later added SRCU walk here?
After all, this is exactly what RCU is for, isn't it?
Best Regards,
Petr
On Thu 2022-09-29 17:43:17, Petr Mladek wrote:
> On Thu 2022-09-29 01:48:29, John Ogness wrote:
> > On 2022-09-27, Petr Mladek <[email protected]> wrote:
> > > Hmm, the new mutex is really nasty. It has very strange semantic.
> > > It makes the locking even more complicated.
> >
> > We are working to replace the BKL-console_lock with new separate clearly
> > defined mechanisms.
> >
> > The new mutex provides full synchronization for list changes as well as
> > changes to items of that list. (Really console->flags is the only change
> > to items of the list.)
We should actually make the the reading of console->flags safe under
srcu_read_lock(). It would allow to use the SRCU walk by all the
readers.
> > For some places in the code it is very clear that the console_lock can
> > be completely replaced (either with srcu or the new mutex). For other
> > places, it is not yet clear why the console_lock is being used and so
> > both console_lock and mutex are used.
>
> One important and tricky location is console_trylock() in
> vprintk_emit(). And the related for_each_console() called from
> console_unlock()->console_flush_all().
>
> It is the legacy mode that tries to print to the consoles immediately.
> I am not sure if we could _ever_ remove this mode.
>
> I would really like to avoid state where we have two locks (semaphore
> and mutex) serializing the same thing (console list).
That said, I could imagine implementing console_lock() so that it
would be implemented by mutex when the legacy mode is disabled and
semaphore when it is allowed.
You were talking about command-line option that would allow to
disable the legacy mode on production RT systems. And I guess
that you added mutex because it behaves better on RT.
Also I could imagine using console_list_lock() as a wrapper
to console_lock(). It might help to distinguish locations where
the list is traversed and where the console_lock() is used for
another reason. I mean to remove the big-kernel-lock character
of the console_lock().
You know, the more locks we have, the bigger is the risk of
deadlocks, and the more hacks would be needed in
console_flush_on_panic(). And I am afraid
that console_lock() will be with us for many years and
maybe forever.
Does it make any sense, please?
Best Regards,
Petr
On 2022-09-29, Petr Mladek <[email protected]> wrote:
> It is the legacy mode that tries to print to the consoles immediately.
> I am not sure if we could _ever_ remove this mode.
We are not trying to remove this mode. We are trying to introduce a new
mode. Once all the console drivers have moved over to the new mode, the
old mode can disappear. If some console drivers never move over, the old
mode can hang around.
It is important to understand that we are not trying to change the old
mode. This was our big mistake leading to the 5.19-revert.
> And it is most likely the main reason why semaphore is used instead
> of a mutex:
>
> + printk() can be called in atomic context
>
> + also there is the console_trylock_spinning() trick that allows
> to transfer the semaphore to another owner without locking.
>
> Do you see any RT-friendly solution for the legacy mode, please?
No. Legacy mode will never work for RT because the console drivers are
using spinlocks, which for RT requires that preemption is enabled.
> Anyway, some lock will still be needed to synchronize the list.
> But could it be mutex? What about the legacy mode of printk_emit()?
For list updates a mutex is fine. All list updates already require
may_sleep contexts. For just iterating the list, SRCU is fine.
But we really need an atomic variable (or separate data-race bools) for
the properties that are not immutable. AFAIK this is only CON_ENABLED
and CON_CONSDEV (and I seriously question the usefulness/correctness of
CON_CONSDEV). If console_is_enabled() could be safely called without a
lock, neither console_lock nor console_list_lock would be needed to
safely iterate and act on the console list.
The NOBKL consoles (not included in this series) use a separate atomic
state variable to handle this. Perhaps the legacy consoles could
(mis)use that variable so that CON_ENABLED is atomic for them as well.
>> Yes! One of the main points of this final phase of the rework is to
>> remove console_sem usage (for NOBKL consoles). If a system is running
>> with only NOBKL consoles registered, ideally that system should never
>> call console_lock()/console_trylock(). Once all drivers have
>> converted over to the NOBKL interface, console_sem will serve no
>> purpose for the printk and console frameworks, so it can be removed.
>
> And even if we convert all console drivers then people still might
> want the legacy mode.
For converted drivers there is no use for the pseudo-synchronous legacy
mode. Converted drivers can run in true synchronous mode if the user
wants.
> My understanding is that some atomic consoles would be real hacks.
Well, it is up to the maintainers to make sure they are not real
hacks. We are not mandating that all drivers are converted. But I think
when devs start seeing the benefits of the converted drivers (and will
have many working examples to be inspired by) there will be honest
efforts to correctly convert the driver.
> They might be good enough for panic(). But what about running system.
> It seems that people might want the legacy more even on running
> system. Will it be doable with mutex?
I'm not sure what you mean here, but I think you are referencing
situations that are not valid. Either drivers are legacy (and continue
using the BKL) or they are correctly converted to the new atomic/thread
model (and have nothing to do with the BKL).
There will be some exceptions (such as fbdev), which is why we are also
considering special alternatives for this class of drivers (such as BSoD
splash on panic, rather than an atomic console).
> I would really like to avoid state where we have two locks (semaphore
> and mutex) serializing the same thing (console list).
I understand. I will look into this more closely. But it may just mean
adding comments above each console_lock() to say:
1. that it is being using to stop all console printing
2. why all console printing needs to stop
Notice that the above list does not include "provide synchronization for
the console list".
John
On 2022-09-30, Petr Mladek <[email protected]> wrote:
> Anyway, what about using the later added SRCU walk here?
> After all, this is exactly what RCU is for, isn't it?
So I think a lot of the problems with this series is that SRCU is
introduced too late. We are debating things in patch 6 that are
irrelevant by patch 12.
I will rework the series so that the changes come in the following
order:
1. provide an atomic console_is_enabled()
2. convert the list to SRCU
3. move all iterators from console_lock()/console_trylock() to SRCU
Step 3 may result in console_lock()/console_trylock() calls disappearing
or relocating to where they are needed for non-list-synchronization
purposes.
John
On 2022-09-30, Petr Mladek <[email protected]> wrote:
> We should actually make the the reading of console->flags safe under
> srcu_read_lock(). It would allow to use the SRCU walk by all the
> readers.
Agreed. I will do this for the next version.
> That said, I could imagine implementing console_lock() so that it
> would be implemented by mutex when the legacy mode is disabled and
> semaphore when it is allowed.
No, let's not imagine this. It is déjà vu for the code that was
reverted.
> You were talking about command-line option that would allow to
> disable the legacy mode on production RT systems. And I guess
> that you added mutex because it behaves better on RT.
We added mutex because list updates are always in may_sleep context and
we were moving to SRCU for list iteration. I think with v2, where SRCU
will be introduced earlier, things will be much clearer.
> Also I could imagine using console_list_lock() as a wrapper
> to console_lock(). It might help to distinguish locations where
> the list is traversed and where the console_lock() is used for
> another reason. I mean to remove the big-kernel-lock character
> of the console_lock().
No, locking the list should have nothing to do with console_lock(). We
want to remove the list synchronization responsibilities from
console_lock(). In this series, I did not make that clear in the commit
messages. (Perhaps it was not entirely clear to me then.) For v2 I will
make this point very clear.
> You know, the more locks we have, the bigger is the risk of
> deadlocks, and the more hacks would be needed in
> console_flush_on_panic(). And I am afraid
> that console_lock() will be with us for many years and
> maybe forever.
Sure. Removing console_lock() will be a long battle involving many
drivers. I am not trying to fight that battle right now. I just want
console_lock() out of the way of NOBKL consoles.
John
On Sat 2022-09-24 02:10:47, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Replace the open coded single linked list with a hlist so a conversion to
> SRCU protected list walks can reuse the existing primitives.
>
> --- a/arch/parisc/kernel/pdc_cons.c
> +++ b/arch/parisc/kernel/pdc_cons.c
> @@ -272,15 +267,17 @@ void pdc_console_restart(bool hpmc)
> if (pdc_console_initialized)
> return;
>
> - if (!hpmc && console_drivers)
> + if (!hpmc && !hlist_empty(&console_list))
> return;
>
> /* If we've already seen the output, don't bother to print it again */
> - if (console_drivers != NULL)
> + if (!hlist_empty(&console_list))
> pdc_cons.flags &= ~CON_PRINTBUFFER;
>
> - while ((console = console_drivers) != NULL)
> - unregister_console(console_drivers);
> + while (!hlist_empty(&console_list)) {
> + unregister_console(READ_ONCE(hlist_entry(console_list.first,
> + struct console, node)));
The READ_ONCE() is in a wrong place. This is why it did not compile.
It should be:
unregister_console(hlist_entry(READ_ONCE(console_list.first),
struct console,
node));
I know that it is all hope for good. But there is also a race between
the hlist_empty() and hlist_entry().
We might make it sligtly more safe by using hlist_entry_safe()
struct console *con;
while (con = hlist_entry_safe(READ_ONCE(console_list.first),
struct console, node)) {
unregister_console(con);
}
or
while (tmp = READ_ONCE(console_list.first) {
unregister_console(hlist_entry_safe(tmp, struct console, node));
}
> + }
>
> /* force registering the pdc console */
> pdc_console_init_force();
> diff --git a/fs/proc/consoles.c b/fs/proc/consoles.c
> index 6775056eecd5..70994d1e52f6 100644
> --- a/fs/proc/consoles.c
> +++ b/fs/proc/consoles.c
> @@ -74,8 +74,11 @@ static void *c_start(struct seq_file *m, loff_t *pos)
> static void *c_next(struct seq_file *m, void *v, loff_t *pos)
> {
> struct console *con = v;
> +
> ++*pos;
> - return con->next;
> + hlist_for_each_entry_continue(con, node)
> + break;
Nit: It looks weird and hacky. It does not look like a common patter.
I see that another code reads the next entry instead.
I would rather do:
return hlist_entry_safe(con->node.next, struct *console, node);
and we should later make it rcu safe, something like:
return hlist_entry_safe(rcu_dereference_raw(hlist_next_rcu(con, struct *console, node));
But I do not have strong opinion.
> + return con;
> }
>
> static void c_stop(struct seq_file *m, void *v)
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2979,7 +2984,15 @@ void console_flush_on_panic(enum con_flush_mode mode)
> u64 seq;
>
> seq = prb_first_valid_seq(prb);
> - for_each_console(c)
> + /*
> + * This cannot use for_each_console() because it's not established
> + * that the current context has console locked and neither there is
> + * a guarantee that there is no concurrency in that case.
> + *
> + * Open code it for documentation purposes and pretend that
> + * it works.
> + */
> + hlist_for_each_entry(c, &console_list, node)
> c->seq = seq;
It is not a big deal. But I would use the _safe() variant to make
it slightly more robust.
> }
> console_unlock();
> @@ -3211,21 +3227,17 @@ void register_console(struct console *newcon)
> }
>
> /*
> - * Put this console in the list - keep the
> - * preferred driver at the head of the list.
> + * Put this console in the list and keep the referred driver at the
> + * head of the list.
> */
> console_lock();
> - if ((newcon->flags & CON_CONSDEV) || console_drivers == NULL) {
> - newcon->next = console_drivers;
> - console_drivers = newcon;
> - if (newcon->next)
> - newcon->next->flags &= ~CON_CONSDEV;
> - /* Ensure this flag is always set for the head of the list */
> - newcon->flags |= CON_CONSDEV;
> - } else {
> - newcon->next = console_drivers->next;
> - console_drivers->next = newcon;
> - }
> + if (newcon->flags & CON_CONSDEV || hlist_empty(&console_list))
> + hlist_add_head(&newcon->node, &console_list);
> + else
> + hlist_add_behind(&newcon->node, console_list.first);
> +
> + /* Ensure this flag is always set for the head of the list */
> + cons_first()->flags |= CON_CONSDEV;
The patch removed:
if (newcon->next)
newcon->next->flags &= ~CON_CONSDEV;
As a result, all consoles will have CON_CONSDEV flag set.
We need to remove it in the 2nd console when exists.
See below for more details.
> newcon->dropped = 0;
> if (newcon->flags & CON_PRINTBUFFER) {
> @@ -3263,7 +3277,6 @@ EXPORT_SYMBOL(register_console);
>
> static int console_unregister_locked(struct console *console)
> {
> - struct console *con;
> int res;
>
> con_printk(KERN_INFO, console, "disabled\n");
> @@ -3274,32 +3287,28 @@ static int console_unregister_locked(struct console *console)
> if (res > 0)
> return 0;
>
> - res = -ENODEV;
> console_lock();
> - if (console_drivers == console) {
> - console_drivers=console->next;
> - res = 0;
> - } else {
> - for_each_console(con) {
> - if (con->next == console) {
> - con->next = console->next;
> - res = 0;
> - break;
> - }
> - }
> - }
>
> - if (res)
> - goto out_disable_unlock;
> + /* Disable it unconditionally */
> + console->flags &= ~CON_ENABLED;
> +
> + if (hlist_unhashed(&console->node))
> + goto out_unlock;
We should return -ENODEV here. I think that Sergey found this as well.
> + hlist_del_init(&console->node);
>
> /*
> + * <HISTORICAL>
> * If this isn't the last console and it has CON_CONSDEV set, we
> * need to set it on the next preferred console.
> + * </HISTORICAL>
> + *
> + * The above makes no sense as there is no guarantee that the next
> + * console has any device attached. Oh well....
This is a sad story. CON_CONSDEV used to be an implementation detail.
It was used to associate the preferred console (last on the
command line) with /dev/console. It was achieved by putting
it at the beginning of the list. All consoled had tty binding at
that time.
The problem started when the flags became readable by user space
via /proc/consoles. There is even a tool (showconsole) that is
reading it. As a result people wanted to show correct value.
The problem is that con->device never exist during boot. The consoles
are registered before the tty subsystem is initialized.
I have a patch that sets the flag correctly in console_device()
that is called from tty_lookup_driver(). But it is part of a bigger
clean up patchset that is sitting in my drawer :-/
On the other hand, the current code kind of works. Most console
drivers have the tty binding. I can't recall what is the exception.
Maybe boot consoles?
> */
> - if (console_drivers != NULL && console->flags & CON_CONSDEV)
> - console_drivers->flags |= CON_CONSDEV;
> + if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
> + cons_first()->flags |= CON_CONSDEV;
>
>
> - console->flags &= ~CON_ENABLED;
> console_unlock();
> console_sysfs_notify();
Best Regards,
Petr
On 9/30/22 16:20, Petr Mladek wrote:
> On Sat 2022-09-24 02:10:47, John Ogness wrote:
>> From: Thomas Gleixner <[email protected]>
>>
>> Replace the open coded single linked list with a hlist so a conversion to
>> SRCU protected list walks can reuse the existing primitives.
>>
>> --- a/arch/parisc/kernel/pdc_cons.c
>> +++ b/arch/parisc/kernel/pdc_cons.c
>> @@ -272,15 +267,17 @@ void pdc_console_restart(bool hpmc)
>> if (pdc_console_initialized)
>> return;
>>
>> - if (!hpmc && console_drivers)
>> + if (!hpmc && !hlist_empty(&console_list))
>> return;
>>
>> /* If we've already seen the output, don't bother to print it again */
>> - if (console_drivers != NULL)
>> + if (!hlist_empty(&console_list))
>> pdc_cons.flags &= ~CON_PRINTBUFFER;
>>
>> - while ((console = console_drivers) != NULL)
>> - unregister_console(console_drivers);
>> + while (!hlist_empty(&console_list)) {
>> + unregister_console(READ_ONCE(hlist_entry(console_list.first,
>> + struct console, node)));
>
> The READ_ONCE() is in a wrong place. This is why it did not compile.
> It should be:
>
> unregister_console(hlist_entry(READ_ONCE(console_list.first),
> struct console,
> node));
>
> I know that it is all hope for good. But there is also a race between
> the hlist_empty() and hlist_entry().
I wonder if pdc_console() is still needed as it is today.
When this was written, early_console and such didn't worked for parisc
as it should. That's proably why we have this register/unregister in here.
Would it make sense, and would we gain something for this printk-series,
if I'd try to convert pdc_console to a standard earlycon or earlyprintk device?
Helge
On Fri 2022-09-30 15:50:56, John Ogness wrote:
> On 2022-09-30, Petr Mladek <[email protected]> wrote:
> > Anyway, what about using the later added SRCU walk here?
> > After all, this is exactly what RCU is for, isn't it?
>
> So I think a lot of the problems with this series is that SRCU is
> introduced too late. We are debating things in patch 6 that are
> irrelevant by patch 12.
> I will rework the series so that the changes come in the following
> order:
>
> 1. provide an atomic console_is_enabled()
>
> 2. convert the list to SRCU
>
> 3. move all iterators from console_lock()/console_trylock() to SRCU
>
> Step 3 may result in console_lock()/console_trylock() calls disappearing
> or relocating to where they are needed for non-list-synchronization
> purposes.
I agree that introding SRCU as early as possible would
help. The current patchset converts the same code several times...
Best Regards,
Petr
On Fri 2022-09-30 16:22:30, John Ogness wrote:
> On 2022-09-30, Petr Mladek <[email protected]> wrote:
> > You know, the more locks we have, the bigger is the risk of
> > deadlocks, and the more hacks would be needed in
> > console_flush_on_panic(). And I am afraid
> > that console_lock() will be with us for many years and
> > maybe forever.
>
> Sure. Removing console_lock() will be a long battle involving many
> drivers. I am not trying to fight that battle right now. I just want
> console_lock() out of the way of NOBKL consoles.
There is some misunderstanding. I am going to think more about your
arguments over the weekend.
But maybe, the above is important. You want to get cosole_lock() out
of the way of NOBLK consoles. What does it exactly mean, please?
What code paths are important to achieve this?
From my POV, the most important code path is the kthread. But it
should use SRCU. I mean that the kthread will take neither
cosnole_lock() nor console_list_lock().
Is there any other code path where console_list_lock() will help
you to get console_lock() out of the way?
From my POV, the proposed code does:
register_console()
{
console_list_lock();
console_lock();
/* manipulate struct console and the console_list */
console_unlock();
console_list_unlock();
}
register_console()
{
console_list_lock();
console_lock();
/* manipulate struct console and the console_list */
console_unlock();
console_list_unlock();
}
printk_kthread()
{
while() {
srcu_read_lock();
if (read_flags_srcu())
/* print line */
srcu_read_unlock();
}
}
vprintk_emit()
{
/* store message */
if (do_not_allow_sync_mode)
return;
if (console_trylock()) {
console_flush_all();
__console_unlock();
}
}
some_other_func()
{
console_list_lock();
/* do something with all registered consoles */
console_list_unlock();
}
console_flush_all()
{
do_something_with_all_consoles();
do_something_else_with_all_consoles();
}
What if?
do_something_with_all_consoles()
{
console_list_lock();
/* do something */
console_list_unlock();
}
Wait, there is a possible ABBA deadlock because
do_something_with_all_consoles() takes console_list_lock()
under console_lock(). And register_console() does it
the other way around.
But it is less obvious because these are different locks.
From my POV, both locks serialize the same things
(console_list manipulation). SRCU walk should be
enough for most iterations over the list.
And I do not see which code path would really benefit from
having the new console_list_lock() instead of console_lock().
Best Regards,
Petr
On 2022-09-30, Helge Deller <[email protected]> wrote:
>> I know that it is all hope for good. But there is also a race between
>> the hlist_empty() and hlist_entry().
>
> I wonder if pdc_console() is still needed as it is today. When this
> was written, early_console and such didn't worked for parisc as it
> should. That's proably why we have this register/unregister in here.
>
> Would it make sense, and would we gain something for this
> printk-series, if I'd try to convert pdc_console to a standard
> earlycon or earlyprintk device?
Having an earlycon or earlyprintk device will not really help you here
since those drivers will have already unregistered.
However, once we get the new atomic/kthread interface available, it
certainly would be useful to implement the pdc_console as an atomic
console.
John Ogness
On 2022-09-30, Petr Mladek <[email protected]> wrote:
> You want to get cosole_lock() out of the way of NOBKL consoles. What
> does it exactly mean, please?
It means that a system with only NOBKL consoles will never take the
console_lock.
> What code paths are important to achieve this?
Anything that iterates or queries the consoles is taking the
console_lock right now. We want that code to use something else for
those tasks (console_srcu, console_mutex, atomic_state).
> From my POV, the most important code path is the kthread. But
> it should use SRCU. I mean that the kthread will take neither
> cosnole_lock() nor console_list_lock().
All iterators and querying are important because they need a safe
interface that does not rely on console_lock.
> Is there any other code path where console_list_lock() will help
> you to get console_lock() out of the way?
The difficulty arises because we are trying to share as much code as
possible. So, for example, NOBKL consoles are sitting on the same list
as legacy consoles. And since currently the list is protected by the
console_lock, we need to change how that list is protected.
> From my POV, both locks serialize the same things
> (console_list manipulation).
My v2 will hopefully change your POV. I will make it clear (in comments
and implementation) that the console_lock does _not_ protect the console
list. All iteration and querying will have no choice but to use the new
mechanisms for list iteration and checking/setting CON_ENABLED.
Then the console_lock's only function is to block legacy consoles from
printing and making sure that multiple legacy consoles are not printing
in parallel. And, of course, it will still function as a general BKL
lock for fbdev, which may be relying on its locking function to
synchronize some fbdev data.
Note that the end result will be no change in behavior for legacy
consoles. But it allows legacy and NOBKL consoles to run simultaneously
while sharing significant amounts of code, and provides a clear path for
console drivers to begin converting. As a side-effect, the first step of
reducing the scope of the console_lock will have been taken.
John
* John Ogness <[email protected]>:
> On 2022-09-30, Helge Deller <[email protected]> wrote:
> >> I know that it is all hope for good. But there is also a race between
> >> the hlist_empty() and hlist_entry().
> >
> > I wonder if pdc_console() is still needed as it is today. When this
> > was written, early_console and such didn't worked for parisc as it
> > should. That's proably why we have this register/unregister in here.
> >
> > Would it make sense, and would we gain something for this
> > printk-series, if I'd try to convert pdc_console to a standard
> > earlycon or earlyprintk device?
>
> Having an earlycon or earlyprintk device will not really help you here
> since those drivers will have already unregistered.
>
> However, once we get the new atomic/kthread interface available, it
> certainly would be useful to implement the pdc_console as an atomic
> console.
My idea was to drop most of the pdc console, so that patch #8 and parts
of patch #11 of the printk patch series could be dropped and you won't
need to take care of those parts when introducing the printk
threaded/atomic printing changes.
See patch below. Basically it drops all of the offending code.
I haven't yet checked it into my parisc for-next tree to not break
something.
Helge
From 5b697874e10729136ce7dd7b362b276f35fae56d Mon Sep 17 00:00:00 2001
From: Helge Deller <[email protected]>
Date: Sat, 1 Oct 2022 00:32:07 +0200
Subject: [PATCH] parisc: Drop PDC console and convert it to an early console
Rewrite the PDC console to become an early console, which can be used
for kgdb as well.
Signed-off-by: Helge Deller <[email protected]>
diff --git a/arch/parisc/include/asm/pdc.h b/arch/parisc/include/asm/pdc.h
index b643092d4b98..fcbcf9a96c11 100644
--- a/arch/parisc/include/asm/pdc.h
+++ b/arch/parisc/include/asm/pdc.h
@@ -19,9 +19,6 @@ extern unsigned long parisc_pat_pdc_cap; /* PDC capabilities (PAT) */
#define PDC_TYPE_SYSTEM_MAP 1 /* 32-bit, but supports PDC_SYSTEM_MAP */
#define PDC_TYPE_SNAKE 2 /* Doesn't support SYSTEM_MAP */
-void pdc_console_init(void); /* in pdc_console.c */
-void pdc_console_restart(void);
-
void setup_pdc(void); /* in inventory.c */
/* wrapper-functions from pdc.c */
diff --git a/arch/parisc/kernel/pdc_cons.c b/arch/parisc/kernel/pdc_cons.c
index 2661cdd256ae..45a4d2994857 100644
--- a/arch/parisc/kernel/pdc_cons.c
+++ b/arch/parisc/kernel/pdc_cons.c
@@ -1,46 +1,24 @@
// SPDX-License-Identifier: GPL-2.0-or-later
/*
- * PDC Console support - ie use firmware to dump text via boot console
+ * PDC early console support - ie use firmware to dump text via boot console
*
- * Copyright (C) 1999-2003 Matthew Wilcox <willy at parisc-linux.org>
- * Copyright (C) 2000 Martin K Petersen <mkp at mkp.net>
- * Copyright (C) 2000 John Marvin <jsm at parisc-linux.org>
- * Copyright (C) 2000-2003 Paul Bame <bame at parisc-linux.org>
- * Copyright (C) 2000 Philipp Rumpf <prumpf with tux.org>
- * Copyright (C) 2000 Michael Ang <mang with subcarrier.org>
- * Copyright (C) 2000 Grant Grundler <grundler with parisc-linux.org>
- * Copyright (C) 2001-2002 Ryan Bradetich <rbrad at parisc-linux.org>
- * Copyright (C) 2001 Helge Deller <deller at parisc-linux.org>
- * Copyright (C) 2001 Thomas Bogendoerfer <tsbogend at parisc-linux.org>
- * Copyright (C) 2002 Randolph Chung <tausq with parisc-linux.org>
- * Copyright (C) 2010 Guy Martin <gmsoft at tuxicoman.be>
+ * Copyright (C) 2001-2022 Helge Deller <[email protected]>
*/
/*
- * The PDC console is a simple console, which can be used for debugging
- * boot related problems on HP PA-RISC machines. It is also useful when no
- * other console works.
- *
* This code uses the ROM (=PDC) based functions to read and write characters
* from and to PDC's boot path.
*/
-/* Define EARLY_BOOTUP_DEBUG to debug kernel related boot problems.
- * On production kernels EARLY_BOOTUP_DEBUG should be undefined. */
-#define EARLY_BOOTUP_DEBUG
-
-
#include <linux/kernel.h>
#include <linux/console.h>
-#include <linux/string.h>
#include <linux/init.h>
-#include <linux/major.h>
-#include <linux/tty.h>
+#include <linux/serial_core.h>
+#include <linux/kgdb.h>
#include <asm/page.h> /* for PAGE0 */
#include <asm/pdc.h> /* for iodc_call() proto and friends */
static DEFINE_SPINLOCK(pdc_console_lock);
-static struct console pdc_cons;
static void pdc_console_write(struct console *co, const char *s, unsigned count)
{
@@ -54,210 +32,47 @@ static void pdc_console_write(struct console *co, const char *s, unsigned count)
spin_unlock_irqrestore(&pdc_console_lock, flags);
}
-int pdc_console_poll_key(struct console *co)
+static int kgdb_pdc_read_char(void)
{
- int c;
unsigned long flags;
+ int c;
spin_lock_irqsave(&pdc_console_lock, flags);
c = pdc_iodc_getc();
spin_unlock_irqrestore(&pdc_console_lock, flags);
- return c;
-}
-
-static int pdc_console_setup(struct console *co, char *options)
-{
- return 0;
-}
-
-#if defined(CONFIG_PDC_CONSOLE)
-#include <linux/vt_kern.h>
-#include <linux/tty_flip.h>
-
-#define PDC_CONS_POLL_DELAY (30 * HZ / 1000)
-
-static void pdc_console_poll(struct timer_list *unused);
-static DEFINE_TIMER(pdc_console_timer, pdc_console_poll);
-static struct tty_port tty_port;
-
-static int pdc_console_tty_open(struct tty_struct *tty, struct file *filp)
-{
- tty_port_tty_set(&tty_port, tty);
- mod_timer(&pdc_console_timer, jiffies + PDC_CONS_POLL_DELAY);
-
- return 0;
-}
-
-static void pdc_console_tty_close(struct tty_struct *tty, struct file *filp)
-{
- if (tty->count == 1) {
- del_timer_sync(&pdc_console_timer);
- tty_port_tty_set(&tty_port, NULL);
- }
+ return (c <= 0) ? NO_POLL_CHAR : c;
}
-static int pdc_console_tty_write(struct tty_struct *tty, const unsigned char *buf, int count)
+static void kgdb_pdc_write_char(u8 chr)
{
- pdc_console_write(NULL, buf, count);
- return count;
+ if (PAGE0->mem_cons.cl_class != CL_DUPLEX)
+ pdc_console_write(NULL, &chr, 1);
}
-static unsigned int pdc_console_tty_write_room(struct tty_struct *tty)
-{
- return 32768; /* no limit, no buffer used */
-}
-
-static const struct tty_operations pdc_console_tty_ops = {
- .open = pdc_console_tty_open,
- .close = pdc_console_tty_close,
- .write = pdc_console_tty_write,
- .write_room = pdc_console_tty_write_room,
+static struct kgdb_io kgdb_pdc_io_ops = {
+ .name = "kgdb_pdc",
+ .read_char = kgdb_pdc_read_char,
+ .write_char = kgdb_pdc_write_char,
};
-static void pdc_console_poll(struct timer_list *unused)
-{
- int data, count = 0;
-
- while (1) {
- data = pdc_console_poll_key(NULL);
- if (data == -1)
- break;
- tty_insert_flip_char(&tty_port, data & 0xFF, TTY_NORMAL);
- count ++;
- }
-
- if (count)
- tty_flip_buffer_push(&tty_port);
-
- if (pdc_cons.flags & CON_ENABLED)
- mod_timer(&pdc_console_timer, jiffies + PDC_CONS_POLL_DELAY);
-}
-
-static struct tty_driver *pdc_console_tty_driver;
-
-static int __init pdc_console_tty_driver_init(void)
-{
- struct tty_driver *driver;
- int err;
-
- /* Check if the console driver is still registered.
- * It is unregistered if the pdc console was not selected as the
- * primary console. */
-
- struct console *tmp;
-
- console_lock();
- for_each_console(tmp)
- if (tmp == &pdc_cons)
- break;
- console_unlock();
-
- if (!tmp) {
- printk(KERN_INFO "PDC console driver not registered anymore, not creating %s\n", pdc_cons.name);
- return -ENODEV;
- }
-
- printk(KERN_INFO "The PDC console driver is still registered, removing CON_BOOT flag\n");
- pdc_cons.flags &= ~CON_BOOT;
-
- driver = tty_alloc_driver(1, TTY_DRIVER_REAL_RAW |
- TTY_DRIVER_RESET_TERMIOS);
- if (IS_ERR(driver))
- return PTR_ERR(driver);
-
- tty_port_init(&tty_port);
-
- driver->driver_name = "pdc_cons";
- driver->name = "ttyB";
- driver->major = MUX_MAJOR;
- driver->minor_start = 0;
- driver->type = TTY_DRIVER_TYPE_SYSTEM;
- driver->init_termios = tty_std_termios;
- tty_set_operations(driver, &pdc_console_tty_ops);
- tty_port_link_device(&tty_port, driver, 0);
-
- err = tty_register_driver(driver);
- if (err) {
- printk(KERN_ERR "Unable to register the PDC console TTY driver\n");
- tty_port_destroy(&tty_port);
- tty_driver_kref_put(driver);
- return err;
- }
-
- pdc_console_tty_driver = driver;
-
- return 0;
-}
-device_initcall(pdc_console_tty_driver_init);
-
-static struct tty_driver * pdc_console_device (struct console *c, int *index)
+static int __init pdc_earlycon_setup(struct earlycon_device *device,
+ const char *opt)
{
- *index = c->index;
- return pdc_console_tty_driver;
-}
-#else
-#define pdc_console_device NULL
-#endif
-
-static struct console pdc_cons = {
- .name = "ttyB",
- .write = pdc_console_write,
- .device = pdc_console_device,
- .setup = pdc_console_setup,
- .flags = CON_BOOT | CON_PRINTBUFFER,
- .index = -1,
-};
+ struct console *earlycon_console;
-static int pdc_console_initialized;
-
-static void pdc_console_init_force(void)
-{
- if (pdc_console_initialized)
- return;
- ++pdc_console_initialized;
-
/* If the console is duplex then copy the COUT parameters to CIN. */
if (PAGE0->mem_cons.cl_class == CL_DUPLEX)
memcpy(&PAGE0->mem_kbd, &PAGE0->mem_cons, sizeof(PAGE0->mem_cons));
- /* register the pdc console */
- register_console(&pdc_cons);
-}
-
-void __init pdc_console_init(void)
-{
-#if defined(EARLY_BOOTUP_DEBUG) || defined(CONFIG_PDC_CONSOLE)
- pdc_console_init_force();
-#endif
-#ifdef EARLY_BOOTUP_DEBUG
- printk(KERN_INFO "Initialized PDC Console for debugging.\n");
-#endif
-}
-
-
-/*
- * Used for emergencies. Currently only used if an HPMC occurs. If an
- * HPMC occurs, it is possible that the current console may not be
- * properly initialised after the PDC IO reset. This routine unregisters
- * all of the current consoles, reinitializes the pdc console and
- * registers it.
- */
-
-void pdc_console_restart(void)
-{
- struct console *console;
+ earlycon_console = device->con;
+ earlycon_console->write = pdc_console_write;
+ device->port.iotype = UPIO_MEM32BE;
- if (pdc_console_initialized)
- return;
+ if (IS_ENABLED(CONFIG_KGDB))
+ kgdb_register_io_module(&kgdb_pdc_io_ops);
- /* If we've already seen the output, don't bother to print it again */
- if (console_drivers != NULL)
- pdc_cons.flags &= ~CON_PRINTBUFFER;
-
- while ((console = console_drivers) != NULL)
- unregister_console(console_drivers);
-
- /* force registering the pdc console */
- pdc_console_init_force();
+ return 0;
}
+
+EARLYCON_DECLARE(pdc, pdc_earlycon_setup);
diff --git a/arch/parisc/kernel/setup.c b/arch/parisc/kernel/setup.c
index f005ddedb50e..375f38d6e1a4 100644
--- a/arch/parisc/kernel/setup.c
+++ b/arch/parisc/kernel/setup.c
@@ -70,6 +70,10 @@ void __init setup_cmdline(char **cmdline_p)
strlcat(p, "tty0", COMMAND_LINE_SIZE);
}
+ /* default to use early console */
+ if (!strstr(p, "earlycon"))
+ strlcat(p, " earlycon=pdc", COMMAND_LINE_SIZE);
+
#ifdef CONFIG_BLK_DEV_INITRD
if (boot_args[2] != 0) /* did palo pass us a ramdisk? */
{
@@ -139,8 +143,6 @@ void __init setup_arch(char **cmdline_p)
if (__pa((unsigned long) &_end) >= KERNEL_INITIAL_SIZE)
panic("KERNEL_INITIAL_ORDER too small!");
- pdc_console_init();
-
#ifdef CONFIG_64BIT
if(parisc_narrow_firmware) {
printk(KERN_INFO "Kernel is using PDC in 32-bit mode.\n");
diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c
index b78f1b9d45c1..f9696fbf646c 100644
--- a/arch/parisc/kernel/traps.c
+++ b/arch/parisc/kernel/traps.c
@@ -239,13 +239,6 @@ void die_if_kernel(char *str, struct pt_regs *regs, long err)
/* unlock the pdc lock if necessary */
pdc_emergency_unlock();
- /* maybe the kernel hasn't booted very far yet and hasn't been able
- * to initialize the serial or STI console. In that case we should
- * re-enable the pdc console, so that the user will be able to
- * identify the problem. */
- if (!console_drivers)
- pdc_console_restart();
-
if (err)
printk(KERN_CRIT "%s (pid %d): %s (code %ld)\n",
current->comm, task_pid_nr(current), str, err);
@@ -429,10 +422,6 @@ void parisc_terminate(char *msg, struct pt_regs *regs, int code, unsigned long o
/* unlock the pdc lock if necessary */
pdc_emergency_unlock();
- /* restart pdc console if necessary */
- if (!console_drivers)
- pdc_console_restart();
-
/* Not all paths will gutter the processor... */
switch(code){
@@ -482,9 +471,7 @@ void notrace handle_interruption(int code, struct pt_regs *regs)
unsigned long fault_space = 0;
int si_code;
- if (code == 1)
- pdc_console_restart(); /* switch back to pdc if HPMC */
- else if (!irqs_disabled_flags(regs->gr[0]))
+ if (!irqs_disabled_flags(regs->gr[0]))
local_irq_enable();
/* Security check:
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 877173907c53..898728ab2c18 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -602,21 +602,6 @@ config SERIAL_MUX_CONSOLE
select SERIAL_CORE_CONSOLE
default y
-config PDC_CONSOLE
- bool "PDC software console support"
- depends on PARISC && !SERIAL_MUX && VT
- help
- Saying Y here will enable the software based PDC console to be
- used as the system console. This is useful for machines in
- which the hardware based console has not been written yet. The
- following steps must be completed to use the PDC console:
-
- 1. create the device entry (mknod /dev/ttyB0 c 11 0)
- 2. Edit the /etc/inittab to start a getty listening on /dev/ttyB0
- 3. Add device ttyB0 to /etc/securetty (if you want to log on as
- root on this console.)
- 4. Change the kernel command console parameter to: console=ttyB0
-
config SERIAL_SUNSAB
tristate "Sun Siemens SAB82532 serial support"
depends on SPARC && PCI
diff --git a/lib/Kconfig.kgdb b/lib/Kconfig.kgdb
index 05dae05b6cc9..3b9a44008433 100644
--- a/lib/Kconfig.kgdb
+++ b/lib/Kconfig.kgdb
@@ -121,7 +121,7 @@ config KDB_DEFAULT_ENABLE
config KDB_KEYBOARD
bool "KGDB_KDB: keyboard as input device"
- depends on VT && KGDB_KDB
+ depends on VT && KGDB_KDB && !PARISC
default n
help
KDB can use a PS/2 type keyboard for an input device
On Fri 2022-09-30 22:32:32, John Ogness wrote:
> On 2022-09-30, Petr Mladek <[email protected]> wrote:
> > You want to get cosole_lock() out of the way of NOBKL consoles. What
> > does it exactly mean, please?
>
> It means that a system with only NOBKL consoles will never take the
> console_lock.
What is exactly wrong with console_lock, please?
Is the main problem that it is a semaphore?
Or is it a problem that it is used in some console
drivers for other purposes?
My view:
If you use only NOBLK consoles then you should never take
console_lock via con->write(). Also the printk kthread
main-loop does not need to take console_lock.
If the above is true then console_lock should be needed
only by register_console() and unregister_console(). Anything
else should be doable via srcu_read_lock.
Is it a problem when console_lock is needed in register_console
and unregister_console on RT?
> > What code paths are important to achieve this?
>
> Anything that iterates or queries the consoles is taking the
> console_lock right now. We want that code to use something else for
> those tasks (console_srcu, console_mutex, atomic_state).
IMHO, console_srcu should be enough for any query.
And even when the write lock was needed from some
reasons why mutex is needed? Is semaphore completely
unacceptable on RT? Do you want to avoid semaphore on RT
at any cost?
> My v2 will hopefully change your POV. I will make it clear (in comments
> and implementation) that the console_lock does _not_ protect the console
> list. All iteration and querying will have no choice but to use the new
> mechanisms for list iteration and checking/setting CON_ENABLED.
Before you spend too much time on it then please try to solve
the problem below.
> Then the console_lock's only function is to block legacy consoles from
> printing and making sure that multiple legacy consoles are not printing
> in parallel. And, of course, it will still function as a general BKL
> lock for fbdev, which may be relying on its locking function to
> synchronize some fbdev data.
>
> Note that the end result will be no change in behavior for legacy
> consoles. But it allows legacy and NOBKL consoles to run simultaneously
> while sharing significant amounts of code, and provides a clear path for
> console drivers to begin converting. As a side-effect, the first step of
> reducing the scope of the console_lock will have been taken.
OK, there are people that want to disable kthreads by some command line
option. There is a non-trivial possibility that this "feature" will
be there forever.
How exactly do you want to support this legacy mode, please?
The above proposal suggests that it might be something like:
register_console()
{
console_list_lock();
if (!need_console())
goto out;
if (!try_enable_console())
goto out;
if (!(con->flags & CON_NOBLK))
console_lock()
add_console_into_the_list();
if (!(con->flags & CON_NOBLK))
console_unlock()
out:
console_list_unlock();
}
vprintk_emit()
{
vprintk_store();
wake_up_klogd();
if (only_noblk_consoles || in_sched)
return;
if (console_trylock()) {
console_flush_all();
__console_unlock();
}
console_flush_all()
{
/*
* !!! WARNING !!!
* Must take srcu_read_lock(&console_src) here.
* Must never take console_list_lock() here.
*/
srcu_read_lock(&console_srcu);
for_each_console() {
...
}
srcu_read_unlock(&console_srcu);
}
The srcu_read_lock() is needed because NOBKL consoles are
added into the list without console_lock().
There are actually two reasons why we could not take
console_list_lock() in console_flush_all():
+ it is a sleeping lock and vprintk_emit() might
be called in atomic context
+ it might cause ABBA deadlock with console_lock
IMHO, this is not obvious. The rules for using the three global
locks (console_lock(), console_list_lock(), console_srcu)
look quite complicated to me.
Anyway, are you able to implement vprintk_emit()/console_flush_all()
without console_lock()?
If we need to keep console_trylock() in vprintk_emit() forever
then we really need a good justification why console_list_lock()
is needed.
Please, show me a code path where console_mutex is needed
as the only acceptable solution for RT.
Best Regards,
Petr
On 2022-10-03, Petr Mladek <[email protected]> wrote:
> What is exactly wrong with console_lock, please?
It is ambiguously performing multiple tasks:
- protecting the console list
- protecting individual console fields
- serializing console printing
- stopping all console printing
That last item is actually quite complex because nobody really knows
_why_ all consoles need to be stopped. It is mostly because fbdev is
using the console_lock to protect itself from its own write()
callback. But (as has been mentioned in this thread) there are other
code sites where we are not sure which part of the above tasks it is
used for and why.
> Is the main problem that it is a semaphore?
A semaphore has been needed because we are performing global locking for
ambiguous reasons in all possible contexts. We should be using
fine-grained lock and synchronization mechanisms that are appropriate
for their used contexts to precisely lock/synchronize exactly what needs
to be locked/synchronized.
Your first question is literally, "what is wrong with a BKL".
And the answer to that is: A BKL is preventing us from optimizing the
kernel by decoupling unrelated activities.
> The above proposal suggests that it might be something like:
>
> register_console()
> {
> console_list_lock();
>
> if (!need_console())
> goto out;
>
> if (!try_enable_console())
> goto out;
>
> if (!(con->flags & CON_NOBLK))
> console_lock()
Why are you taking the console_lock here? The console_list_lock needs to
replace this responsibility. I realize the RFC and this v1 series does
not do this. For v2, it will be clear.
> add_console_into_the_list();
>
> if (!(con->flags & CON_NOBLK))
> console_unlock()
I would request that you continue reviewing the later patches in the
series. Particularly 13-18. My v2 will involve a significantly reworked
version of patches 6-12, not only changing the order of presentation,
but also explicitly removing console list update protection by the
console_lock. I think having actual code to discuss will greatly help us
continue this discussion.
Patches 13-18 will not change much for v2, unless I get some feedback
otherwise.
John
On (22/10/03 21:41), John Ogness wrote:
> A semaphore has been needed because we are performing global locking for
> ambiguous reasons in all possible contexts. We should be using
> fine-grained lock and synchronization mechanisms that are appropriate
> for their used contexts to precisely lock/synchronize exactly what needs
> to be locked/synchronized.
>
> Your first question is literally, "what is wrong with a BKL".
>
> And the answer to that is: A BKL is preventing us from optimizing the
> kernel by decoupling unrelated activities.
>
> > The above proposal suggests that it might be something like:
> >
> > register_console()
> > {
> > console_list_lock();
> >
> > if (!need_console())
> > goto out;
> >
> > if (!try_enable_console())
> > goto out;
> >
> > if (!(con->flags & CON_NOBLK))
> > console_lock()
>
> Why are you taking the console_lock here? The console_list_lock needs to
> replace this responsibility. I realize the RFC and this v1 series does
> not do this. For v2, it will be clear.
So tty/VT code also needs to take list_lock? list_lock does not look
precisely relevant to vt, which has it's own "list" of "struct vc" to
maintain.
On Mon 2022-10-03 21:41:22, John Ogness wrote:
> On 2022-10-03, Petr Mladek <[email protected]> wrote:
> > What is exactly wrong with console_lock, please?
>
> It is ambiguously performing multiple tasks:
>
> - protecting the console list
> - protecting individual console fields
> - serializing console printing
> - stopping all console printing
>
> And the answer to that is: A BKL is preventing us from optimizing the
> kernel by decoupling unrelated activities.
>
> > The above proposal suggests that it might be something like:
> >
> > register_console()
> > {
> > console_list_lock();
> >
> > if (!need_console())
> > goto out;
> >
> > if (!try_enable_console())
> > goto out;
> >
> > if (!(con->flags & CON_NOBLK))
> > console_lock()
>
> Why are you taking the console_lock here? The console_list_lock needs to
> replace this responsibility. I realize the RFC and this v1 series does
> not do this. For v2, it will be clear.
This is the important information that I missed. It is a great idea.
I agree that console_list_lock() would be a step forward if this worked.
As you say, in the RFC and this v1, console_lock() was still used
to synchronize the list and the metadata manipulation. It means that
console_lock() was as complex as before. In fact, it was even
more complex because console_list_lock() appeared in its lock
dependency chains. And it was not clear that v2 would be
any different in this regard.
Best Regards,
Petr
On Sat 2022-09-24 02:10:49, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Move the buffer size defines to console.h in preparation of adding a buffer
> structure.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> ---
> include/linux/console.h | 14 ++++++++++++++
> include/linux/printk.h | 2 --
> kernel/printk/printk.c | 8 --------
> 3 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/console.h b/include/linux/console.h
> index dc0df9d9e7d9..3bb5bc62e154 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -122,6 +122,20 @@ static inline int con_debug_leave(void)
> #define CM_ERASE (2)
> #define CM_MOVE (3)
>
> +#ifdef CONFIG_PRINTK
> +/* The maximum size of a formatted record (i.e. with prefix added per line) */
> +#define CONSOLE_LOG_MAX 1024
> +
> +/* The maximum size for a dropped text message */
> +#define DROPPED_TEXT_MAX 64
> +#else
> +#define CONSOLE_LOG_MAX 0
> +#define DROPPED_TEXT_MAX 0
> +#endif
> +
> +/* The maximum size of an formatted extended record */
> +#define CONSOLE_EXT_LOG_MAX 8192
It seems that all these defines are going to be used only under
kernel/printk/. I would prefer to move them into
kernel/printk/internal.h or another internal header file there.
Any public API can be misused. And these are rather implementation
details.
Best Regards,
Petr
On Sat 2022-09-24 02:10:50, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Add docbook comments to struct console.
Great!
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -15,6 +15,7 @@
> #define _LINUX_CONSOLE_H_ 1
>
> #include <linux/atomic.h>
> +#include <linux/bits.h>
This probably should have been in another patch?
> #include <linux/rculist.h>
> #include <linux/types.h>
>
> @@ -139,37 +140,77 @@ static inline int con_debug_leave(void)
> /*
> * The interface for a console, or any other device that wants to capture
> * console messages (printer driver?)
> - *
> - * If a console driver is marked CON_BOOT then it will be auto-unregistered
> - * when the first real console is registered. This is for early-printk drivers.
> */
>
> -#define CON_PRINTBUFFER (1)
> -#define CON_CONSDEV (2) /* Preferred console, /dev/console */
> -#define CON_ENABLED (4)
> -#define CON_BOOT (8)
> -#define CON_ANYTIME (16) /* Safe to call when cpu is offline */
> -#define CON_BRL (32) /* Used for a braille device */
> -#define CON_EXTENDED (64) /* Use the extended output format a la /dev/kmsg */
> +/**
> + * cons_flags - General console flags
> + * @CON_PRINTBUFFER: On register, start from the oldest dmesg record
This a bit misleading. It starts from the oldest record that has not
been read via syslog yet. dmesg has been using /dev/kmsg by default
since a long time and it does not touch syslog_seq.
IMHO, in practice, it starts from the oldest record available in the ring
buffer. I think that nobody is using syslog syscall any longer. Also
console drivers are registered quite early. I am not sure if userspace
has any chance to read the log.
Finally, the flag is cleared when a boot consoles are replaced by
the preferred console.
I would write something like:
+ * @CON_PRINTBUFFER: Used by newly registered consoles to avoid duplicate
* output of messages that were already shown by boot
* console or read by userspace via syslog() syscall.
> + * @CON_CONSDEV: Questionable historical leftover to denote which console
> + * driver is the preferred console which is defining what
> + * backs up /dev/console
It is true. But it sounds like it can be removed. But it can't be done
easily because it can be checked by userspace. For example, it is checked by
https://github.com/bitstreamout/showconsole/blob/master/showconsole.c
Another problem is that it is just the best effort. It might happen
that it is set for a wrong driver. But I would consider this a bug.
I would write something like:
* @CON_CONSDEV: Indicates that the console driver is backing
* /dev/console.
> + * @CON_ENABLED: Indicates if a console is allowed to print records. If false,
> + * the console also will not advance to later records.
> + * @CON_BOOT: Marks the console driver as early console driver which
> + * is used during boot before the real driver becomes available.
> + * It will be automatically unregistered unless the early console
> + * command line parameter for this console has the 'keep' option set.
This is more complicated. And it is a real mess, huh, huh, huh.
There are earlyprintk= and earlycon= parameters. They both register
boot consoles but earlyprintk= is supported only on some architectures.
I did not check it. My guess is that earlyprintk= was the first attempt
to show kernel messages as early as possible. There is also
early_printk() function that calls early_console->write()
directly. I think that it is used by Peter Zijlstra who
wants to avoid console_lock().
earlycon= does just the bare minimum to initialize driver
(no sysfs stuff, ...). Otherwise, the console works the same
way as a normal console driver defied by console=. It is called
from console_unlock().
Now, only earlyprintk= handle the "keep" parameter, see
setup_early_printk(). The CON_BOOT flag is not set when
"keep" parameter is used, see early_console_register().
earlycon= does not support the "keep" parameter. There is
the "keep_bootcon" option instead.
Lovely, isn't it?
OK, I suggest to write something like:
* @CON_BOOT: Marks the console driver as early console driver which
* is used during boot before the real driver becomes
* available. It will be automatically unregistered
* when the real console driver is registered unless
* "keep_bootcon" parameter is used.
> + * @CON_ANYTIME: A misnomed historical flag which tells the core code that the
s/misnomed/misnamed/ ?
> + * legacy @console::write callback can be invoked on a CPU which
> + * is marked OFFLINE. That's misleading as it suggests that there
> + * is no contextual limit for invoking the callback.
When I was digging the history, the motivation for this flag was
whether the per-CPU areas were initialized. Maybe, we should mention it here:
* @CON_ANYTIME: A misnamed historical flag which tells the core code
* that the legacy @console::write callback can be invoked
* on a CPU which is marked OFFLINE. That's misleading as
* it suggests that there is no contextual limit for
* invoking the callback. The original motivation was
* readiness of the per-CPU areas.
> + * @CON_BRL: Indicates a braille device which is exempt from receiving the
> + * printk spam for obvious reasons
> + * @CON_EXTENDED: The console supports the extended output format of /dev/kmesg
> + * which requires a larger output record buffer
> + */
> +enum cons_flags {
> + CON_PRINTBUFFER = BIT(0),
> + CON_CONSDEV = BIT(1),
> + CON_ENABLED = BIT(2),
> + CON_BOOT = BIT(3),
> + CON_ANYTIME = BIT(4),
> + CON_BRL = BIT(5),
> + CON_EXTENDED = BIT(6),
> +};
>
> +/**
> + * struct console - The console descriptor structure
> + * @name: The name of the console driver
> + * @write: Write callback to output messages (Optional)
I am surprised that write() callback is optional. But it seems
that, for example, ttynull_console does not have it defined.
> + * @read: Read callback for console input (Optional)
> + * @device: The underlying TTY device driver (Optional)
> + * @unblank: Callback to unblank the console (Optional)
> + * @setup: Callback for initializing the console (Optional)
Thanks a lot for this effort.
Best Regards,
Petr
On Sat 2022-09-24 02:10:51, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Create a data structure to replace the open coded separate buffers for
> regular and extended formatting.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> ---
> include/linux/console.h | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 8ec24fe097d3..05c7325e98f9 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -173,6 +173,20 @@ enum cons_flags {
> CON_EXTENDED = BIT(6),
> };
>
> +/**
> + * struct cons_text_buf - console output text buffer
> + * @ext_text: Buffer for extended log format text
> + * @dropped_text: Buffer for dropped text
> + * @text: Buffer for ringbuffer text
> + */
> +struct cons_text_buf {
Sigh, I feel bad to nit-pick about this. It seems that you have used
"cons" everywhere in the new API so any change might be painful.
But I personally find really handful when an API is predictable
and consistent.
I see that "cons" has already been used few times in tty subsystem,
especially tty/vt and tty/hvc.
But I do not see any single "cons_" under kernel/printk/ before
this patchset. Either "console_" or "con_" prefix was
used everywhere, including CON_XXX flags.
Is there any change to change this to either "console_"
or "con_", please? Or is there any particular reason why
this new API should be distinguished by the new prefix?
> + union {
> + char ext_text[CONSOLE_EXT_LOG_MAX];
> + char dropped_text[DROPPED_TEXT_MAX];
> + };
> + char text[CONSOLE_LOG_MAX];
We should explain in the commit message why we need
the separate ext_text buffer and why it can be shared
with dropped_text buffer. Something like:
<proposal>
Create a data structure to replace the open coded separate buffers for
regular and extended formatting.
Separate @ext_text buffer is needed because info_print_ext_header()
and msg_print_ext_body() are not able to add the needed extra
information inplace.
@ext_text and @dropped_text buffer can be shared because
they are never used at the same time.
</proposal>
Also I think about using pointers instead of the hard-coded
buffer size. For example, there is no need to have
the big ext_text buffer in the kthread when the related
console does not allow to allocated the extended text.
There is actually only one console that has this enabled.
I mean something like:
struct cons_text_buf {
char *text;
char *ext_text;
char *dropped_text;
unsigned int text_size;
unsigned int ext_text_size;
unsigned int dropped_text_size;
}
We might create a helper to define static buffer:
#define DEFINE_CONS_TEXT_BUF(name) \
static char _##name##_text[CONSOLE_LOG_MAX]; \
static char _##name##_ext_text[CONSOLE_EXT_LOG_MAX]; \
static struct const_text_buf name = { \
.text = _##name##_text, \
.ext_text = _##name##_ext_text, \
.dropped_text = _##name##_ext_text, \
\
.text_size = CONSOLE_LOG_MAX; \
.ext_text_size = CONSOLE_LOG_MAX; \
.dropped_text_size = DROPPED_TEXT_MAX; \
};
Another advantage would be that it looks like a more safe way to
pass the buffer size. The existing code hardcodes CONSOLE_LOG_MAX
and CONSOLE_EXT_LOG_MAX everywhere. And it is less obvious that
the buffer and size fits together. Especially that the names
do not match (text vs. LOG_MAX and ext_text vs. EXT_LOG_MAX).
Well, this might be out of scope of this patchset. I do not resist
on it. We might do this later.
Best Regards,
Petr
On Sat 2022-09-24 02:10:52, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> Replace the separately allocated output buffers with a single instance of
> struct cons_text_buf.
>
> Note that the buffer size of devkmsg_user.text_buf, when replaced with
> cons_text_buf.text, reduces from CONSOLE_EXT_LOG_MAX to CONSOLE_LOG_MAX.
> However, the buffer is only used to read ringbuffer records, which have
> a maximum size of LOG_LINE_MAX (CONSOLE_LOG_MAX - PREFIX_MAX).
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> ---
> kernel/printk/printk.c | 50 ++++++++++++++++++------------------------
> 1 file changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 65e9903d066f..9cbd44e9fc45 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -671,11 +671,9 @@ struct devkmsg_user {
> atomic64_t seq;
> struct ratelimit_state rs;
> struct mutex lock;
> - char buf[CONSOLE_EXT_LOG_MAX];
> -
> struct printk_info info;
> - char text_buf[CONSOLE_EXT_LOG_MAX];
> struct printk_record record;
> + struct cons_text_buf txtbuf;
I think about how to make it more clear that @txtbuf is not a simple
text buffer. It would help to better follow the code.
What about renaming "struct cons_text_buf", for example:
struct con_text_bufs;
struct con_bufs;
struct console_text_buffers;
struct console_buffers;
and use the variables, for example:
ctbufs, cbufs, ct_bufs, c_bufs, con_bufs
> };
>
> static __printf(3, 4) __cold
> @@ -758,6 +756,8 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
> {
> struct devkmsg_user *user = file->private_data;
> struct printk_record *r = &user->record;
> + char *outbuf = user->txtbuf.ext_text;
Please, use either "ext_text" or "ext_text_buf".
> + const int maxlen = sizeof(user->txtbuf.ext_text);
and "ext_text_size" or "ext_text_buf_size"
to follow the existing style, for example:
info_print_ext_header(buf, size, info).
prb_rec_init_rd(r, info, text_buf, text_buf_size).
> size_t len;
> ssize_t ret;
>
> @@ -2741,13 +2742,13 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
> goto skip;
> }
>
> - if (ext_text) {
> - write_text = ext_text;
> - len = info_print_ext_header(ext_text, CONSOLE_EXT_LOG_MAX, r.info);
> - len += msg_print_ext_body(ext_text + len, CONSOLE_EXT_LOG_MAX - len,
> + if (extmsg) {
We could get this from the console flags:
if (con->flags & CON_EXTENDED) {
> + write_text = txtbuf->ext_text;
> + len = info_print_ext_header(write_text, CONSOLE_EXT_LOG_MAX, r.info);
> + len += msg_print_ext_body(write_text + len, CONSOLE_EXT_LOG_MAX - len,
> &r.text_buf[0], r.info->text_len, &r.info->dev_info);
I would use this opportunity and get rid of the hardcoded *_LOG_MAX
lengts and something like:
write_text = txtbuf->ext_text;
write_text_size = sizeof(txtbuf->ext_text);
len = info_print_ext_header(write_text, write_text_size, r.info);
len += msg_print_ext_body(write_text + len, write_text_size - len,
&r.text_buf[0], r.info->text_len, &r.info->dev_info);
Using the hard coded size is error prone. It makes the review
complicated especially when we are going to pass the buffers
via some structures or generic pointers. I always have to check
if it is still the same buffer.
The only sane way is to use either sizeof(buf) or pass/store
@buf_size.
In addition, I would set here:
dropped_text = txtbuf->ext_text;
dropped_text_size = sizeof(txtbuf->ext_text);
As a result, we could define as:
struct con_text_bufs {
char ext_text[CONSOLE_EXT_LOG_MAX];
char text[CONSOLE_LOG_MAX];
} __no_randomize_layout;
and remove DROPPED_TEXT_MAX. I see that it is actually done later
anyway. Adding the union is just a temporary twist that complicates
the review.
> } else {
> - write_text = text;
> + write_text = txtbuf->text;
> len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
write_text = txtbuf->text;
write_text_size = sizeof(txtbuf->text);
len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
dropped_text = NULL;
dropped_text_size = 0;
> }
>
> @@ -2765,7 +2766,7 @@ static bool console_emit_next_record(struct console *con, char *text, char *ext_
> console_lock_spinning_enable();
>
> stop_critical_timings(); /* don't trace print latency */
> - call_console_driver(con, write_text, len, dropped_text);
> + call_console_driver(con, write_text, len, extmsg ? NULL : txtbuf->dropped_text);
call_console_driver(con, write_text, len, dropped_text, dropped_text_size);
> start_critical_timings();
>
> con->seq++;
Best Regards,
Petr
On Mon 2022-09-26 10:00:36, John Ogness wrote:
> On 2022-09-26, Sergey Senozhatsky <[email protected]> wrote:
> > On (22/09/24 02:10), John Ogness wrote:
> >> +/**
> >> + * cons_print_dropped - Print 'dropped' message if required
> >> + * @desc: Pointer to the output descriptor
> >> + *
> >> + * Prints the 'dropped' message info the output buffer if @desc->dropped is
> >> + * not 0 and the regular format is requested. Extended format does not
> >> + * need this message because it prints the sequence numbers.
> >> + *
> >> + * In regular format the extended message buffer is not in use.
> >> + * So print into it at the beginning and move the resulting string
> >> + * just in front of the regular text buffer so that the message can
> >> + * be printed in one go.
> >> + *
> >> + * In case of a message this returns with @desc->outbuf and @desc->len
> >> + * updated. If no message is required then @desc is not modified.
> >> + */
> >> +static void cons_print_dropped(struct cons_outbuf_desc *desc)
> >> +{
> >> + struct cons_text_buf *txtbuf = desc->txtbuf;
> >> + size_t len;
> >> +
> >> + if (!desc->dropped || desc->extmsg)
> >> + return;
> >> +
> >> + if (WARN_ON_ONCE(desc->outbuf != txtbuf->text))
> >> + return;
> >> +
> >> + /* Print it into ext_text which is unused */
> >> + len = snprintf(txtbuf->ext_text, DROPPED_TEXT_MAX,
> >> + "** %lu printk messages dropped **\n", desc->dropped);
> >> + desc->dropped = 0;
> >> +
> >> + /* Copy it just below text so it goes out with one write */
> >> + memcpy(txtbuf->text - len, txtbuf->ext_text, len);
> >> +
> >> + /* Update the descriptor */
> >> + desc->len += len;
> >> + desc->outbuf -= len;
> >
> > Oh, hmm. This does not look to me as a simplification. Quite
> > the opposite, moving cons_text_buf::text pointer to point to
> > cons_text_buf::text - strlen("... dropped messages...") looks
> > somewhat fragile.
>
> It relies on @ext_text and @text being packed together, which yes, may
> be fragile.
Yes, it is a nasty hack ;-)
I suggest to increase CONSOLE_LOG_MAX to 2048,
define LOG_LINE_MAX as 1024, and use the buffer for both
dropped message and normal message.
It would simplify the code. Also it would make enough
space for more potential line headers needed by more
lines in one record.
It would require moving the normal message to make a space for
the dropped messages. But the dropping should be rare. And we
do a lot of moving in record_print_text() anyway.
I think that I was against increasing the buffer size some time ago.
I was worried about small devices. But I think that the patch
just increased the buffer size without any bug report so that
the justification was weak.
But simplifying the code looks like a good justification to me.
And I really like the removal of the extra buffer.
Best Regards,
Petr
On Sat 2022-09-24 02:10:53, John Ogness wrote:
> From: Thomas Gleixner <[email protected]>
>
> To prepare for a new console infrastructure that is independent of the
> console BKL, wrap the output mode into a descriptor struct so the new
> infrastrucure can share the emit code that dumps the ringbuffer record
> into the output text buffers.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Signed-off-by: John Ogness <[email protected]>
> ---
> include/linux/console.h | 15 +++++++
> kernel/printk/printk.c | 88 ++++++++++++++++++++++++++++++-----------
> 2 files changed, 79 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/console.h b/include/linux/console.h
> index 05c7325e98f9..590ab62c01d9 100644
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -187,6 +187,21 @@ struct cons_text_buf {
> char text[CONSOLE_LOG_MAX];
> };
>
> +/**
> + * struct cons_outbuf_desc - console output buffer descriptor
> + * @txtbuf: Pointer to buffer for storing the text
> + * @outbuf: Pointer to the position in @buffer for
> + * writing it out to the device
This sounds like this pointer might point into the middle of
some buffer. It sounds scarry without providing the remaining
size of the buffer.
It seems that the pointer is actually used to point to
one of the buffers in txtbuf struct. Then it is again
a bit scarry without the size. I know that it is defined
by @len. But it is not obvious that it is related.
> + * @len: Message length
It is not clear that it is length of the outbuf.
> + * @extmsg: Select extended format printing
It would be nice to make it obvious (variable name)
that it is bool and not another buffer.
This actually defines which buffer will be used
in txtbuf.
> + */
> +struct cons_outbuf_desc {
> + struct cons_text_buf *txtbuf;
> + char *outbuf;
> + unsigned int len;
> + bool extmsg;
> +};
Sigh, I somehow do not like this structure. I think that the main
problem is that it combines both input and output values.
Also there is too much assignments here and there.
What about?
1. Storing "struct cons_text_buf *txtbuf" into struct console.
Normal consoles might point to a global txtbuf.
Atomic consoles might point to the allocated ones.
2. Create structure for writing the next record
on the console, for example:
struct console_record { /* like struct printk_record */
char *buf;
int size;
int len;
}
Then we could implement:
bool console_get_record(struct console *con,
struct console_record *cr)
{
struct cons_text_buf *txtbuf = con->txtbuf;
struct printk_info info;
struct printk_record r;
char *write_text;
size_t len;
cr->buf = NULL;
cr->size = 0;
cr->len = 0;
prb_rec_init_rd(&r, &info, txtbuf->text, sizeof(txtbuf->text);
if (!prb_read_valid(prb, desc->seq, &r))
return false;
/* Skip record that has level above the console loglevel. */
if (suppress_message_printing(r.info->level)) {
return true;
}
if (con->flags & CON_EXTENDED) {
cr->buf = txtbuf->ext_text;
cr->size = sizeof(txtbuf->ext_text);
info_print_ext_header(cr, r.info);
msg_print_ext_body(cr, &r);
} else {
cr->buf = txtbuf->text;
cr->size = sizeof(txtbuf->text);
record_print_text(cr, &r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time);
cons_print_dropped(cr, con);
}
return true;
}
and
static bool console_emit_next_record(struct console *con,
bool *handover)
{
struct console_record cr;
unsigned long flags;
*handover = false;
if (!console_get_next_record(con, cr))
return false;
/* supressed? */
if (!cr->buf) {
con->seq++;
return true;
}
/*
* While actively printing out messages, if another printk()
* were to occur on another CPU, it may wait for this one to
* finish. This task can not be preempted if there is a
* waiter waiting to take over.
*
* Interrupts are disabled because the hand over to a waiter
* must not be interrupted until the hand over is completed
* (@console_waiter is cleared).
*/
printk_safe_enter_irqsave(flags);
console_lock_spinning_enable();
/* don't trace print latency */
stop_critical_timings();
/* Write everything out to the hardware */
con->write(con, cr->buf, cr->len);
start_critical_timings();
con->seq++;
*handover = console_lock_spinning_disable_and_check();
printk_safe_exit_irqrestore(flags);
return true;
}
Advantages:
+ even less parameters
+ less assignments (read/write directly in struct console)
+ struct console_record has just output buffer =>
no confusion about the names
How does that sound, please?
Best Regards,
Petr