Re: [PATCH v3 1/2] printk: Add function to dump printk buffer directly to consoles

[Sreenath Vijayan]

On Thu, Jan 18, 2024 at 10:55:20AM +0106, John Ogness wrote:
> On 2024-01-17, Sreenath Vijayan <sreenath.vijayan@xxxxxxxx> 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 needs access to private items of printk like PRINTK_MESSAGE_MAX.
> > Add function in printk.c to accomplish this.
> >
> > Suggested-by: John Ogness <john.ogness@xxxxxxxxxxxxx>
> > Signed-off-by: Sreenath Vijayan <sreenath.vijayan@xxxxxxxx>
> > Signed-off-by: Shimoyashiki Taichi <taichi.shimoyashiki@xxxxxxxx>
> > ---
> >  include/linux/printk.h |  4 ++++
> >  kernel/printk/printk.c | 33 +++++++++++++++++++++++++++++++++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/include/linux/printk.h b/include/linux/printk.h
> > index 8ef499ab3c1e..0896745f31e2 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 inline void dump_printk_buffer(void)
> > +{
> > +}
> >  #endif
> >  
> >  #ifdef CONFIG_SMP
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index f2444b581e16..5b11fb377f8f 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -4259,6 +4259,39 @@ 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)
> > +{
> > +	struct kmsg_dump_iter iter;
> > +	struct console *con;
> > +	char *buf;
> > +	size_t len;
> > +	int cookie;
> > +
> > +	buf = kmalloc(PRINTK_MESSAGE_MAX, GFP_KERNEL);
> > +	if (!buf)
> > +		return;
> > +
> > +	kmsg_dump_rewind(&iter);
> > +	while (kmsg_dump_get_line(&iter, 1, buf, PRINTK_MESSAGE_MAX, &len)) {
> 
> Although using the kmsg_dump interface will provide you the messages,
> they will not necessarily be in the correct format. Consoles can be set
> to use extended format.
> 
> We probably should respect that console setting.

Thank you for reviewing the patch and pointing out the limitations
of ksmg_dump interface.

> 
> > +		/*
> > +		 * Since using printk() or pr_*() will append the message to the
> > +		 * printk ring buffer, they cannot be used to display the retrieved
> > +		 * message. Hence console_write() of serial drivers is used.
> > +		 */
> > +		console_lock();
> > +		cookie = console_srcu_read_lock();
> > +		for_each_console_srcu(con) {
> > +			if ((console_srcu_read_flags(con) & CON_ENABLED) && con->write)
> 
> console_is_usable() should be used instead. It makes the correct checks.
>

Ok, noted.
 
> > +				con->write(con, buf, len);
> > +		}
> > +		console_srcu_read_unlock(cookie);
> > +		console_unlock();
> > +	}
> > +	kfree(buf);
> > +}
> 
> We could do something like this:
> 
> void dump_printk_buffer(void)
> {
> 	console_lock();
> 	console_flush_on_panic(CONSOLE_REPLAY_ALL);
> 	console_unlock();
> }
> 
> This version respects all the console features (formatting, handovers),
> but console_flush_on_panic() does not to allow cond_resched(), which we
> would want in this case.
> 
> We could take the console sequence-resetting code out into its own
> helper function. Then it would look like this (comments removed to keep
> things short):
> 
> 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
> 			c->seq = seq;
> 	}
> 	console_srcu_read_unlock(cookie);
> }
> 
> void console_flush_on_panic(enum con_flush_mode mode)
> {
> 	bool handover;
> 	u64 next_seq;
> 
> 	console_may_schedule = 0;
> 
> 	if (mode == CONSOLE_REPLAY_ALL)
> 		console_rewind_all();
> 
> 	console_flush_all(false, &next_seq, &handover);
> }
> 
> void dump_printk_buffer(void)
> {
> 	bool handover;
> 	u64 next_seq;
> 
> 	console_lock();
> 	console_rewind_all();
> 	console_flush_all(true, &next_seq, &handover);
> 	console_unlock();
> }
> 
> Any thoughts?
> 
> John

Thank you for suggesting this new method. From initial tests,
this change looks ok. I will do more testing and send out the
next version.

Sreenath