Re: [PATCH] printk: CPU backtrace not printing on panic

[Petr Mladek <pmladek@suse.com>]

On Thu 2024-08-01 17:27:21, takakura@valinux.co.jp wrote:
> Hi Petr and John,
> 
> On 2024-07-30, Petr Mladek <pmladek@suse.com> wrote:
> >On Fri 2024-07-26 16:02:45, John Ogness wrote:
> >> On 2024-07-26, Petr Mladek <pmladek@suse.com> wrote:
> >> > I would do it the other way and enable printing from other CPUs only
> >> > when triggring the backtrace. We could do it because
> >> > trigger_all_cpu_backtrace() waits until all backtraces are
> >> > printed.
> >> >
> >> > Something like:
> >> >
> >> > diff --git a/include/linux/panic.h b/include/linux/panic.h
> >> > index 3130e0b5116b..980bacbdfcfc 100644
> >> > --- a/include/linux/panic.h
> >> > +++ b/include/linux/panic.h
> >> > @@ -16,6 +16,7 @@ extern void oops_enter(void);
> >> >  extern void oops_exit(void);
> >> >  extern bool oops_may_print(void);
> >> >  
> >> > +extern int panic_triggering_all_cpu_backtrace;
> >> >  extern int panic_timeout;
> >> >  extern unsigned long panic_print;
> >> >  extern int panic_on_oops;
> >> > diff --git a/kernel/panic.c b/kernel/panic.c
> >> > index f861bedc1925..7e9e97d59b1e 100644
> >> > --- a/kernel/panic.c
> >> > +++ b/kernel/panic.c
> >> > @@ -64,6 +64,8 @@ unsigned long panic_on_taint;
> >> >  bool panic_on_taint_nousertaint = false;
> >> >  static unsigned int warn_limit __read_mostly;
> >> >  
> >> > +int panic_triggering_all_cpu_backtrace;
> >> > +
> >> >  int panic_timeout = CONFIG_PANIC_TIMEOUT;
> >> >  EXPORT_SYMBOL_GPL(panic_timeout);
> >> >  
> >> > @@ -253,8 +255,12 @@ void check_panic_on_warn(const char *origin)
> >> >   */
> >> >  static void panic_other_cpus_shutdown(bool crash_kexec)
> >> >  {
> >> > -	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
> >> > +	if (panic_print & PANIC_PRINT_ALL_CPU_BT) {
> >> > +		/* Temporary allow printing messages on non-panic CPUs. */
> >> > +		panic_triggering_all_cpu_backtrace = true;
> >> >  		trigger_all_cpu_backtrace();
> >> > +		panic_triggering_all_cpu_backtrace = false;
> >> 
> >> Note, here we should also add
> >> 
> >> 		nbcon_atomic_flush_pending();
> >> 
> >> Your suggestion allows the other CPUs to dump their backtrace into the
> >> ringbuffer, but they are still forbidden from acquiring the nbcon
> >> console contexts for printing. That is a necessary requirement of
> >> nbcon_waiter_matches().
> >
> >Great catch!
> >
> >I would prefer to solve this in a separate patch. This problem existed
> >even before the commit 779dbc2e78d7 ("printk: Avoid non-panic CPUs writing
> >to ringbuffer"). In fact, the problem existed very long time even for
> >the legacy consoles.
> >
> 
> Good point! I guess the problem existed since the commit 51a1d258e50e 
> ("printk: Keep non-panic-CPUs out of console lock") as it forbade the 
> acquisition of console lock for non-panic cpus?

It most likely existed since the commit 7acac3445acde1c94
("printk: always use deferred printk when flush printk_safe lines")

These were times when printk() serialized access to the log buffer
using a spin lock. The backtraces from other CPUs were stored in
temporary per-CPU buffers and later copied to the main log buffer.
The above mentioned commit caused that printk_safe_flush_line()
did not longer try to flush the messages to the console after
copying the temporary stored messages.

Well, the above commit was in Jan 2017. It was before panic
allowed to show the backtraces.

In practice, the problem with flushing bracktraces in panic has
existed since the option to print the backtraces was added by
the commit 60c958d8df9cfc40b ("panic: add sysctl to dump
all CPUs backtraces on oops event") in Jun 2020.

Best Regards,
Petr