Re: + panic-avoid-the-extra-noise-dmesg.patch added to -mm tree

[Feng Tang <feng.tang@intel.com>]

Hi Petr,

On Tue, Dec 04, 2018 at 05:01:52PM +0100, Petr Mladek wrote:
> On Tue 2018-12-04 23:49:36, Feng Tang wrote:
> > + Sasha 
> > 
> > Thanks Petr and Sergey for the reviews.
> > 
> > 
> > On Tue, Dec 04, 2018 at 11:20:33AM +0100, Petr Mladek wrote:
> > > On Mon 2018-12-03 23:15:31, Andrew Morton wrote:
> > > > 
> > > > ------------------------------------------------------
> > > > From: Feng Tang <feng.tang@intel.com>
> > > > Subject: panic: Avoid the extra noise dmesg
> > > > 
> > > > 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:
> > > > 
> > > > 	WARNING: CPU: 1 PID: 280 at kernel/sched/core.c:1198 set_task_cpu+0x183/0x190
> > > > 	Call Trace:
> > > > 	<IRQ>
> > > > 	try_to_wake_up
> > > > 	default_wake_function
> > > > 	autoremove_wake_function
> > > > 	__wake_up_common
> > > > 	__wake_up_common_lock
> > > > 	__wake_up
> > > > 	wake_up_klogd_work_func
> > > > 	irq_work_run_list
> > > > 	irq_work_tick
> > > > 	update_process_times
> > > > 	tick_sched_timer
> > > > 	__hrtimer_run_queues
> > > > 	hrtimer_interrupt
> > > > 	smp_apic_timer_interrupt
> > > > 	apic_timer_interrupt
> > > 
> > > I guess that it is a warning about migrating tasks to an offline CPU.
> >  
> > My v1 patch was trying to add some hacky code into architecture code to
> > address several WARN()s directly, but that turns out to be very hacky and
> > involve much code for many archs.
> > 
> > > > For people working in console mode, the screen will first show the panic
> > > > call stack, but immediately overridded by these noisy extra messages, which
> > > > makes debugging much more difficult, as the original context gets lost on
> > > > screen.
> > > > 
> > > > Also these noisy messages will confuse some users, as I have seen many bug
> > > > reporters posted the noisy message into bugzilla, instead of the real panic
> > > > call stack and context.
> > > > 
> > > > Removing the "local_irq_enable" will avoid the noisy message.
> > > > 
> > > > The justification for the removing is: when code runs to this point, it
> > > > means user has chosed to not reboot, or do any special handling by using
> > > > the panic notifier method, no much point in re-enabling the interrupt.
> > > > 
> > > > Link: http://lkml.kernel.org/r/1543902228-23834-1-git-send-email-feng.tang@intel.com
> > > > Signed-off-by: Feng Tang <feng.tang@intel.com>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Borislav Petkov <bp@suse.de>
> > > > Cc: Petr Mladek <pmladek@suse.com>
> > > > Cc: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> > > > Cc: <stable@vger.kernel.org>
> > > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > > ---
> > > > 
> > > > 
> > > > --- a/kernel/panic.c~panic-avoid-the-extra-noise-dmesg
> > > > +++ a/kernel/panic.c
> > > > @@ -322,7 +322,6 @@ void panic(const char *fmt, ...)
> > > >  	}
> > > >  #endif
> > > >  	pr_emerg("---[ end Kernel panic - not syncing: %s ]---\n", buf);
> > > > -	local_irq_enable();
> > > >  	for (i = 0; ; i += PANIC_TIMER_STEP) {
> > > >  		touch_softlockup_watchdog();
> > > >  		if (i >= i_next) {
> > > 
> > > Hmm, this calls panic_blink(). It seems that it depends on workqueues
> > > and the scheduler:
> > > 
> > >   + led_panic_blink()
> > >     + led_trigger_event()
> > >       + led_set_brightness()
> > >         + schedule_work(set_brightness_work)
> > > 
> > > I guess that blinking might be important in some situations and
> > > on some devices. On the other hand, we are interested only into
> > > blinking from this point on.
> > > 
> > > The easiest solution seems to be to make a noop from printk().
> > > For example, we could add a global flag:
> > > 
> > > int panic_blinking;
> > > 
> > > and add the following into vprintk_func()
> > > 
> > > 	/*
> > > 	 * Do not push away real panic() message by warnings from led
> > > 	 * blinking code.
> > > 	 */
> > > 	if (panic_blinking)
> > > 		return 0;
> > > 
> > > How does that sound?
> > 
> > This should be able to achieve the same goal. 
> > 
> > One thing I can think of is what mentioned by Sergey that some sysrq
> > handler may want to print out something, but it should mostly be
> > covered by 2 other panic debug print patches, which will print out
> > task/mem/timer/lock/ftrace info runtime on demand.
> 
> I think that we could simply clear panic_blinking from
> __handle_sysrq(). The user will still be able to capture the screen
> before touching the keyboard. But it will keep the things simple.
> 
> I hope that we did not miss anything else. Anyway, the approach with
> making printk a nop still looks like the best maintainable solution
> to me.
 
I will setup a platform which can handle sysrq request and try your
suggestion. thanks,

- Feng