diff mbox series

[v4,1/2] printk: Add function to dump printk buffer directly to consoles

Message ID 8cb5936021c5811bd03a6bc18300b1384009ac26.1706772349.git.sreenath.vijayan@sony.com
State New
Headers
Series [v4,1/2] printk: Add function to dump printk buffer directly to consoles |

Commit Message

Sreenath Vijayan Feb. 1, 2024, 10:23 a.m. UTC 
  It is useful to be able to dump the printk buffer directly to
consoles in some situations so as to not flood the buffer.
To do this, we reuse the CONSOLE_REPLAY_ALL mode code in
console_flush_on_panic() by moving the code to a helper function
console_rewind_all(). This is done because console_flush_on_panic()
sets console_may_schedule to 0 but this should not be done in our
case. Then console_rewind_all() is called from the new function
dump_printk_buffer() with console lock held to set the console
sequence number to oldest record in the buffer for all consoles.
Releasing the console lock will flush the contents of printk buffer
to the consoles.

Suggested-by: John Ogness <john.ogness@linutronix.de>
Signed-off-by: Sreenath Vijayan <sreenath.vijayan@sony.com>
Signed-off-by: Shimoyashiki Taichi <taichi.shimoyashiki@sony.com>
---
 include/linux/printk.h |  4 +++
 kernel/printk/printk.c | 61 +++++++++++++++++++++++++-----------------
 2 files changed, 41 insertions(+), 24 deletions(-)

Comments

John Ogness Feb. 1, 2024, 11:28 a.m. UTC | #1 
On 2024-02-01, Sreenath Vijayan <sreenath.vijayan@sony.com> wrote:
> It is useful to be able to dump the printk buffer directly to
> consoles in some situations so as to not flood the buffer.
> To do this, we reuse the CONSOLE_REPLAY_ALL mode code in
> console_flush_on_panic() by moving the code to a helper function
> console_rewind_all(). This is done because console_flush_on_panic()
> sets console_may_schedule to 0 but this should not be done in our
> case. Then console_rewind_all() is called from the new function
> dump_printk_buffer() with console lock held to set the console
> sequence number to oldest record in the buffer for all consoles.
> Releasing the console lock will flush the contents of printk buffer
> to the consoles.
>
> Suggested-by: John Ogness <john.ogness@linutronix.de>
> Signed-off-by: Sreenath Vijayan <sreenath.vijayan@sony.com>
> Signed-off-by: Shimoyashiki Taichi <taichi.shimoyashiki@sony.com>

Reviewed-by: John Ogness <john.ogness@linutronix.de>
Petr Mladek Feb. 7, 2024, 2:43 p.m. UTC | #2 
On Thu 2024-02-01 15:53:40, Sreenath Vijayan wrote:
> It is useful to be able to dump the printk buffer directly to
> consoles in some situations so as to not flood the buffer.

This is not longer true. I think that it was valid for
the previous versions which used separate buffers with
the kmsg_dump API.

> To do this, we reuse the CONSOLE_REPLAY_ALL mode code in
> console_flush_on_panic() by moving the code to a helper function
> console_rewind_all(). This is done because console_flush_on_panic()
> sets console_may_schedule to 0 but this should not be done in our
> case.

Also the "c->seq = seq;" is not safe in the panic version.
But it will be safe when called under the console_lock.

> Then console_rewind_all() is called from the new function
> dump_printk_buffer() with console lock held to set the console
> sequence number to oldest record in the buffer for all consoles.
> Releasing the console lock will flush the contents of printk buffer
> to the consoles.

My proposed commit message is:

<proposal>
Add a generic function for replaying the kernel log on consoles.
It would allow seeing the the log on an unresponsive terminal
via sysrq interface.

Reuse the existing code from console_flush_on_panic() for
reseting the sequence numbers. It will be safe when called
under console_lock(). Also the console_unlock() will
automatically flush the messages on the consoles.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3134,6 +3134,32 @@ void console_unblank(void)
>  		pr_flush(1000, true);
>  }
>  

I would call this function __console_rewind_all(void)
because it is not safe on its own. Also It would
deserve a comment, something like:

/*
 * Rewind all consoles to the oldest available record.
 *
 * IMPORTANT: The function is safe only when called under
 *            console_lock(). It is not enforced because
 *	      it is used as a best effort in panic().
 */
static void __console_rewind_all(void)


This would deserve a comment because it is not safe by
default.

> +static void console_rewind_all(void)
> +{
> +	struct console *c;
> +	short flags;
> +	int cookie;
> +	u64 seq;
> +
> +	seq = prb_first_valid_seq(prb);
> +
> +	cookie = console_srcu_read_lock();
> +	for_each_console_srcu(c) {
> +		flags = console_srcu_read_flags(c);
> +
> +		if (flags & CON_NBCON) {
> +			nbcon_seq_force(c, seq);
> +		} else {
> +			/*
> +			 * This is an unsynchronized assignment. On
> +			 * panic legacy consoles are only best effort.
> +			 */

We should change this to something like:

			/*
			 * This assigment is safe only when called under
			 * console_lock(). */
			 */

> +			c->seq = seq;
> +		}
> +	}
> +	console_srcu_read_unlock(cookie);
> +}
> +
>  /**
>   * console_flush_on_panic - flush console content on panic
>   * @mode: flush all messages in buffer or just the pending ones
> @@ -3162,30 +3188,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
>  	 */
>  	console_may_schedule = 0;
>  
> -	if (mode == CONSOLE_REPLAY_ALL) {
> -		struct console *c;
> -		short flags;
> -		int cookie;
> -		u64 seq;
> -
> -		seq = prb_first_valid_seq(prb);
> -
> -		cookie = console_srcu_read_lock();
> -		for_each_console_srcu(c) {
> -			flags = console_srcu_read_flags(c);
> -
> -			if (flags & CON_NBCON) {
> -				nbcon_seq_force(c, seq);
> -			} else {
> -				/*
> -				 * This is an unsynchronized assignment. On
> -				 * panic legacy consoles are only best effort.
> -				 */
> -				c->seq = seq;
> -			}
> -		}
> -		console_srcu_read_unlock(cookie);
> -	}
> +	if (mode == CONSOLE_REPLAY_ALL)
> +		console_rewind_all();
>  
>  	console_flush_all(false, &next_seq, &handover);
>  }
> @@ -4259,6 +4263,15 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
>  }
>  EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
>  
> +/**
> + * Dump the printk ring buffer directly to consoles
> + */
> +void dump_printk_buffer(void)

I would call this function console_replay_all(). IMHO, it better describes
what it does.

> +{
> +	console_lock();
> +	console_rewind_all();

I would add a comment:

	/* Consoles are flushed as part of console_unlock(). */

> +	console_unlock();
> +}
>  #endif

Best Regards,
Petr
Sreenath Vijayan Feb. 14, 2024, 10:33 a.m. UTC | #3 
On Wed, Feb 07, 2024 at 03:43:52PM +0100, Petr Mladek wrote:
> On Thu 2024-02-01 15:53:40, Sreenath Vijayan wrote:
> > It is useful to be able to dump the printk buffer directly to
> > consoles in some situations so as to not flood the buffer.
> 
> This is not longer true. I think that it was valid for
> the previous versions which used separate buffers with
> the kmsg_dump API.
> 
> > To do this, we reuse the CONSOLE_REPLAY_ALL mode code in
> > console_flush_on_panic() by moving the code to a helper function
> > console_rewind_all(). This is done because console_flush_on_panic()
> > sets console_may_schedule to 0 but this should not be done in our
> > case.
> 
> Also the "c->seq = seq;" is not safe in the panic version.
> But it will be safe when called under the console_lock.
> 
> > Then console_rewind_all() is called from the new function
> > dump_printk_buffer() with console lock held to set the console
> > sequence number to oldest record in the buffer for all consoles.
> > Releasing the console lock will flush the contents of printk buffer
> > to the consoles.
> 
> My proposed commit message is:
> 
> <proposal>
> Add a generic function for replaying the kernel log on consoles.
> It would allow seeing the the log on an unresponsive terminal
> via sysrq interface.
> 
> Reuse the existing code from console_flush_on_panic() for
> reseting the sequence numbers. It will be safe when called
> under console_lock(). Also the console_unlock() will
> automatically flush the messages on the consoles.
> 
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -3134,6 +3134,32 @@ void console_unblank(void)
> >  		pr_flush(1000, true);
> >  }
> >  
> 
> I would call this function __console_rewind_all(void)
> because it is not safe on its own. Also It would
> deserve a comment, something like:
> 
> /*
>  * Rewind all consoles to the oldest available record.
>  *
>  * IMPORTANT: The function is safe only when called under
>  *            console_lock(). It is not enforced because
>  *	      it is used as a best effort in panic().
>  */
> static void __console_rewind_all(void)
> 
> 
> This would deserve a comment because it is not safe by
> default.
> 
> > +static void console_rewind_all(void)
> > +{
> > +	struct console *c;
> > +	short flags;
> > +	int cookie;
> > +	u64 seq;
> > +
> > +	seq = prb_first_valid_seq(prb);
> > +
> > +	cookie = console_srcu_read_lock();
> > +	for_each_console_srcu(c) {
> > +		flags = console_srcu_read_flags(c);
> > +
> > +		if (flags & CON_NBCON) {
> > +			nbcon_seq_force(c, seq);
> > +		} else {
> > +			/*
> > +			 * This is an unsynchronized assignment. On
> > +			 * panic legacy consoles are only best effort.
> > +			 */
> 
> We should change this to something like:
> 
> 			/*
> 			 * This assigment is safe only when called under
> 			 * console_lock(). */
> 			 */
> 
> > +			c->seq = seq;
> > +		}
> > +	}
> > +	console_srcu_read_unlock(cookie);
> > +}
> > +
> >  /**
> >   * console_flush_on_panic - flush console content on panic
> >   * @mode: flush all messages in buffer or just the pending ones
> > @@ -3162,30 +3188,8 @@ void console_flush_on_panic(enum con_flush_mode mode)
> >  	 */
> >  	console_may_schedule = 0;
> >  
> > -	if (mode == CONSOLE_REPLAY_ALL) {
> > -		struct console *c;
> > -		short flags;
> > -		int cookie;
> > -		u64 seq;
> > -
> > -		seq = prb_first_valid_seq(prb);
> > -
> > -		cookie = console_srcu_read_lock();
> > -		for_each_console_srcu(c) {
> > -			flags = console_srcu_read_flags(c);
> > -
> > -			if (flags & CON_NBCON) {
> > -				nbcon_seq_force(c, seq);
> > -			} else {
> > -				/*
> > -				 * This is an unsynchronized assignment. On
> > -				 * panic legacy consoles are only best effort.
> > -				 */
> > -				c->seq = seq;
> > -			}
> > -		}
> > -		console_srcu_read_unlock(cookie);
> > -	}
> > +	if (mode == CONSOLE_REPLAY_ALL)
> > +		console_rewind_all();
> >  
> >  	console_flush_all(false, &next_seq, &handover);
> >  }
> > @@ -4259,6 +4263,15 @@ void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
> >  }
> >  EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
> >  
> > +/**
> > + * Dump the printk ring buffer directly to consoles
> > + */
> > +void dump_printk_buffer(void)
> 
> I would call this function console_replay_all(). IMHO, it better describes
> what it does.
> 
> > +{
> > +	console_lock();
> > +	console_rewind_all();
> 
> I would add a comment:
> 
> 	/* Consoles are flushed as part of console_unlock(). */
> 
> > +	console_unlock();
> > +}
> >  #endif
> 
> Best Regards,
> Petr

Thank you for the review comments. Will fix all these points in the
next version.

Regards,
Sreenath
diff mbox series

Patch

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 8ef499ab3c1e..861ff5a545ff 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -192,6 +192,7 @@  void show_regs_print_info(const char *log_lvl);
 extern asmlinkage void dump_stack_lvl(const char *log_lvl) __cold;
 extern asmlinkage void dump_stack(void) __cold;
 void printk_trigger_flush(void);
+void dump_printk_buffer(void);
 #else
 static inline __printf(1, 0)
 int vprintk(const char *s, va_list args)
@@ -271,6 +272,9 @@  static inline void dump_stack(void)
 static inline void printk_trigger_flush(void)
 {
 }
+static void dump_printk_buffer(void)
+{
+}
 #endif
 
 #ifdef CONFIG_SMP
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index f2444b581e16..b05ca9f98e53 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3134,6 +3134,32 @@  void console_unblank(void)
 		pr_flush(1000, true);
 }
 
+static void console_rewind_all(void)
+{
+	struct console *c;
+	short flags;
+	int cookie;
+	u64 seq;
+
+	seq = prb_first_valid_seq(prb);
+
+	cookie = console_srcu_read_lock();
+	for_each_console_srcu(c) {
+		flags = console_srcu_read_flags(c);
+
+		if (flags & CON_NBCON) {
+			nbcon_seq_force(c, seq);
+		} else {
+			/*
+			 * This is an unsynchronized assignment. On
+			 * panic legacy consoles are only best effort.
+			 */
+			c->seq = seq;
+		}
+	}
+	console_srcu_read_unlock(cookie);
+}
+
 /**
  * console_flush_on_panic - flush console content on panic
  * @mode: flush all messages in buffer or just the pending ones
@@ -3162,30 +3188,8 @@  void console_flush_on_panic(enum con_flush_mode mode)
 	 */
 	console_may_schedule = 0;
 
-	if (mode == CONSOLE_REPLAY_ALL) {
-		struct console *c;
-		short flags;
-		int cookie;
-		u64 seq;
-
-		seq = prb_first_valid_seq(prb);
-
-		cookie = console_srcu_read_lock();
-		for_each_console_srcu(c) {
-			flags = console_srcu_read_flags(c);
-
-			if (flags & CON_NBCON) {
-				nbcon_seq_force(c, seq);
-			} else {
-				/*
-				 * This is an unsynchronized assignment. On
-				 * panic legacy consoles are only best effort.
-				 */
-				c->seq = seq;
-			}
-		}
-		console_srcu_read_unlock(cookie);
-	}
+	if (mode == CONSOLE_REPLAY_ALL)
+		console_rewind_all();
 
 	console_flush_all(false, &next_seq, &handover);
 }
@@ -4259,6 +4263,15 @@  void kmsg_dump_rewind(struct kmsg_dump_iter *iter)
 }
 EXPORT_SYMBOL_GPL(kmsg_dump_rewind);
 
+/**
+ * Dump the printk ring buffer directly to consoles
+ */
+void dump_printk_buffer(void)
+{
+	console_lock();
+	console_rewind_all();
+	console_unlock();
+}
 #endif
 
 #ifdef CONFIG_SMP