[PATCH printk 00/18] preparation for threaded/atomic printing

[PATCH printk 00/18] preparation for threaded/atomic printing

[John Ogness]

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/20220910221947.171557773@linutronix.de
[1] https://lore.kernel.org/lkml/875yheqh6v.fsf@jogness.linutronix.de

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

[PATCH printk 09/18] serial: kgdboc: Lock console list in probe function

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

Unprotected list walks are not necessarily safe.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 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

[PATCH printk 10/18] kgbd: Pretend that console list walk is safe

[John Ogness]

From: Thomas Gleixner <tglx@linutronix.de>

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 <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
---
 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

Re: [PATCH printk 00/18] preparation for threaded/atomic printing

[Greg Kroah-Hartman]

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 <gregkh@linuxfoundation.org>

Re: [PATCH printk 00/18] preparation for threaded/atomic printing

[Sergey Senozhatsky]

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()?

Re: [PATCH printk 00/18] preparation for threaded/atomic printing

[John Ogness]

On 2022-09-24, Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org>

Thanks!

John

Re: [PATCH printk 10/18] kgbd: Pretend that console list walk is safe

[Aaron Tomlin]

On Sat 2022-09-24 02:10 +0206, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> 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 <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  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 <atomlin@redhat.com>

-- 
Aaron Tomlin

Re: [PATCH printk 10/18] kgbd: Pretend that console list walk is safe

[Doug Anderson]

Hi,

On Fri, Sep 23, 2022 at 5:05 PM John Ogness <john.ogness@linutronix.de> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> 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 <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  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.

Re: [PATCH printk 09/18] serial: kgdboc: Lock console list in probe function

[Doug Anderson]

Hi,

On Fri, Sep 23, 2022 at 5:05 PM John Ogness <john.ogness@linutronix.de> wrote:
>
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Unprotected list walks are not necessarily safe.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
> ---
>  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 <dianders@chromium.org>

Re: [PATCH printk 00/18] preparation for threaded/atomic printing

[Petr Mladek]

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

Re: [PATCH printk 09/18] serial: kgdboc: Lock console list in probe function

[Petr Mladek]

On Sat 2022-09-24 02:10:45, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Unprotected list walks are not necessarily safe.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

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/20220924000454.3319186-7-john.ogness@linutronix.de

Depending on the result of the discussion, with either
console_list_lock() or console_lock():

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH printk 10/18] kgbd: Pretend that console list walk is safe

[Petr Mladek]

On Wed 2022-09-28 16:32:15, Doug Anderson wrote:
> Hi,
> 
> On Fri, Sep 23, 2022 at 5:05 PM John Ogness <john.ogness@linutronix.de> wrote:
> >
> > From: Thomas Gleixner <tglx@linutronix.de>
> >
> > 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

Re: [PATCH printk 10/18] kgbd: Pretend that console list walk is safe

[John Ogness]

On 2022-09-30, Petr Mladek <pmladek@suse.com> 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

Re: [PATCH printk 10/18] kgbd: Pretend that console list walk is safe

[Petr Mladek]

On Fri 2022-09-30 15:50:56, John Ogness wrote:
> On 2022-09-30, Petr Mladek <pmladek@suse.com> 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