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

Feng Tang
2018-12-05T01:53:38Z

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