[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <aJs7p_UjPIfb_XYd@pathway>
Date: Tue, 12 Aug 2025 15:03:35 +0200
From: Petr Mladek <pmladek@...e.com>
To: John Ogness <john.ogness@...utronix.de>
Cc: Sergey Senozhatsky <senozhatsky@...omium.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Feng Tang <feng.tang@...ux.alibaba.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] panic: remove redundant panic-cpu backtrace
On Thu 2025-07-31 09:51:07, John Ogness wrote:
> On 2025-07-31, Sergey Senozhatsky <senozhatsky@...omium.org> wrote:
> > On (25/07/31 09:15), John Ogness wrote:
> >> On 2025-07-31, Sergey Senozhatsky <senozhatsky@...omium.org> wrote:
> >> > SYS_INFO_ALL_CPU_BT sends NMI backtrace request to
> >> > all CPUs, which dumps an extra backtrace on panic CPU.
> >>
> >> Isn't this only true if CONFIG_DEBUG_BUGVERBOSE=y?
> >
> > Are you referring to vpanic()->dump_stack()?
>
> Yes.
>
> > Another way to get backtrace on panic CPU is via BUG(), which routes
> > through die()->__die_body(), which prints registers, stack trace,
> > and so on, before it calls into panic(). This might be x86 specific,
> > though.
>
> So in that case you see 2 stack traces if CONFIG_DEBUG_BUGVERBOSE=y?
>
> >> Also, the information is not the same. trigger_all_cpu_backtrace() will
> >> also dump the registers. For CONFIG_DEBUG_BUGVERBOSE=y on the panic CPU,
> >> only the stack is dumped.
IMHO, this is actually not true, see the following code:
void nmi_trigger_cpumask_backtrace(const cpumask_t *mask,
int exclude_cpu,
void (*raise)(cpumask_t *mask))
{
[...]
/*
* Don't try to send an NMI to this cpu; it may work on some
* architectures, but on others it may not, and we'll get
* information at least as useful just by doing a dump_stack() here.
* Note that nmi_cpu_backtrace(NULL) will clear the cpu bit.
*/
if (cpumask_test_cpu(this_cpu, to_cpumask(backtrace_mask)))
nmi_cpu_backtrace(NULL);
[...]
}
, where
bool nmi_cpu_backtrace(struct pt_regs *regs)
{
[...]
if (regs)
show_regs(regs);
else
dump_stack();
[...]
}
So, I think that the following patch should not make it worse:
diff --git a/kernel/panic.c b/kernel/panic.c
index 72fcbb5a071b..dfbfe1ce7bfc 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -67,6 +67,7 @@ static unsigned int warn_limit __read_mostly;
static bool panic_console_replay;
bool panic_triggering_all_cpu_backtrace;
+bool panic_this_cpu_backtrace_printed;
int panic_timeout = CONFIG_PANIC_TIMEOUT;
EXPORT_SYMBOL_GPL(panic_timeout);
@@ -328,6 +329,22 @@ void check_panic_on_warn(const char *origin)
origin, limit);
}
+static void panic_trigger_all_cpu_backtraces(void)
+{
+ /* Temporary allow non-panic CPUs to write their backtraces. */
+ panic_triggering_all_cpu_backtrace = true;
+
+ if (panic_this_cpu_backtrace_printed) {
+ int this_cpu = raw_smp_processor_id();
+
+ trigger_allbutcpu_cpu_backtrace(this_cpu);
+ } else {
+ trigger_all_cpu_backtrace();
+ }
+
+ panic_triggering_all_cpu_backtrace = false;
+}
+
/*
* Helper that triggers the NMI backtrace (if set in panic_print)
* and then performs the secondary CPUs shutdown - we cannot have
@@ -335,12 +352,8 @@ void check_panic_on_warn(const char *origin)
*/
static void panic_other_cpus_shutdown(bool crash_kexec)
{
- if (panic_print & SYS_INFO_ALL_CPU_BT) {
- /* Temporary allow non-panic CPUs to write their backtraces. */
- panic_triggering_all_cpu_backtrace = true;
- trigger_all_cpu_backtrace();
- panic_triggering_all_cpu_backtrace = false;
- }
+ if (panic_print & SYS_INFO_ALL_CPU_BT)
+ panic_trigger_all_cpu_backtraces();
/*
* Note that smp_send_stop() is the usual SMP shutdown function,
@@ -422,13 +435,15 @@ void vpanic(const char *fmt, va_list args)
buf[len - 1] = '\0';
pr_emerg("Kernel panic - not syncing: %s\n", buf);
-#ifdef CONFIG_DEBUG_BUGVERBOSE
/*
* Avoid nested stack-dumping if a panic occurs during oops processing
*/
- if (!test_taint(TAINT_DIE) && oops_in_progress <= 1)
+ if (test_taint(TAINT_DIE) || oops_in_progress > 1) {
+ panic_this_cpu_backtrace_printed = true;
+ } else if (IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE)) {
dump_stack();
-#endif
+ panic_this_cpu_backtrace_printed = true;
+ }
/*
* If kgdb is enabled, give it a chance to run before we stop all
> > Hmm, it's getting complicated, probably isn't worth it then.
>
> I think it is worth cleaning up, but it probably won't be such a simple
> fix. All call paths of redundant stack trace printing should be
> identified and then we can decide on a clean solution.
I feel that the check
+ if (test_taint(TAINT_DIE) || oops_in_progress > 1) {
is kind of a hack. It would be nice to make it cleaner. But I am not
sure how complicated it would be.
Anyway, I think that storing the information, whether the backtrace
was printed or not, into a global variable, is a step in the right
direction.
Best Regards,
Petr
Powered by blists - more mailing lists