Re: [PATCH v4] panic: Avoid the extra noise dmesg

[Feng Tang]

Hi Petr,

Thanks for your review.

On Wed, Feb 20, 2019 at 02:43:44PM +0100, Petr Mladek wrote:
> On Fri 2019-02-15 13:56:54, Feng Tang wrote:
> > Hi all,
> > 
> > On Tue, Dec 11, 2018 at 09:32:30AM +0100, Petr Mladek wrote:
> > > On Mon 2018-12-10 10:49:22, Kees Cook wrote:
> > > > On Mon, Dec 10, 2018 at 10:17 AM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri,  7 Dec 2018 17:51:19 +0800
> > > > > Feng Tang <feng.tang@xxxxxxxxx> wrote:
> > > > >
> > > > > > When kernel panic happens, it will first print the panic call stack,
> > > > > > then the ending msg like:
> > > > > >
> > > > > > [   35.743249] ---[ end Kernel panic - not syncing: Fatal exception
> > > > > > [   35.749975] ------------[ cut here ]------------
> > > > > >
> > > > > > The above message are very useful for debugging.
> > > > > >
> > > > > > But if system is configured to not reboot on panic, say the "panic_timeout"
> > > > > > parameter equals 0, it will likely print out many noisy message like
> > > > > > WARN() call stack for each and every CPU except the panic one, messages
> > > > > > like below:
> > 
> > So currently, there are 2 proposals:
> > 1. this v4 patch of "panic_keep_irq_on" flag (default off to be same
> >    as the current kernel behavior)
> > 2. Petr's suggestion of adding a flag to suppress printk after enterring
> >    late panic phase (blinking time), while keeping the sysrq printk
> >    working.
> > 
> > Following is the draft patch based on Petr's suggestion:
> > 
> > Please review, thanks. I'm fine with both solutions.
> > 
> > - Feng
> > 
> > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
> > index 1f03078..8921fed 100644
> > --- a/drivers/tty/sysrq.c
> > +++ b/drivers/tty/sysrq.c
> > @@ -528,6 +528,11 @@ void __handle_sysrq(int key, bool check_mask)
> >  	struct sysrq_key_op *op_p;
> >  	int orig_log_level;
> >  	int i;
> > +	int old_val;
> > +
> > +	/* save the old panic printk flag */
> 
> The comment is not needed. It is obvious.

ok, will remove.

> 
> > +	old_val = panic_suppress_printk;
> 
> s/old_val/orig_panic_suppress_printk/ to follow
> the naming of orig_log_level.

Ok.

> 
> > +	panic_suppress_printk = 1;
> 
> We want to enable the messages in sysrq. This should be:
> 
> 	panic_suppress_printk = 0;

Yes, will do.

>
> >  	rcu_sysrq_start();
> >  	rcu_read_lock();
> > @@ -574,6 +579,8 @@ void __handle_sysrq(int key, bool check_mask)
> >  	}
> >  	rcu_read_unlock();
> >  	rcu_sysrq_end();
> > +
> > +	panic_suppress_printk = old_val;
> >  }
> >  
> >  void handle_sysrq(int key)
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index f121e6b..0cd3a1b 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -326,6 +328,7 @@ void panic(const char *fmt, ...)
> >  	}
> >  #endif
> >  	pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
> 
> /* Do not scroll important messages with errors from blinking code. */

Will add the comment, with one minor chanage that the noisy messages may
also come from other places.

> 
> > +	panic_suppress_printk = 1;
> >  	local_irq_enable();
> >  	for (i = 0; ; i += PANIC_TIMER_STEP) {
> >  		touch_softlockup_watchdog();
> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index d3d1703..c27bbf5 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -1987,6 +1987,9 @@ asmlinkage __visible int printk(const char *fmt, ...)
> >  	va_list args;
> >  	int r;
> 
> 	/* Suppress messages from panic blinking code. */

Ditto.
 
> > +	if (unlikely(panic_suppress_printk))
> > +		return 0;
> 
> This should go to vprintk_emit() so that it works for all
> printk() interfaces.

Ok, will do.

Thanks,
Feng
> 
> > +
> >  	va_start(args, fmt);
> >  	r = vprintk_func(fmt, args);
> >  	va_end(args);
> 
> Best Regards,
> Petr