On Fri, 21 Feb 2025 16:23:07 -0500, Hamza Mahfooz wrote: >On Fri, Feb 21, 2025 at 11:23:28AM +0900, Ryo Takakura wrote: >> On Thu, 20 Feb 2025 17:53:00 -0500, Hamza Mahfooz wrote: >> >Since, the panic handlers may require certain cpus to be online to panic >> >gracefully, we should call them before turning off SMP. Without this >> >re-ordering, on Hyper-V hv_panic_vmbus_unload() times out, because the >> >vmbus channel is bound to VMBUS_CONNECT_CPU and unless the crashing cpu >> >is the same as VMBUS_CONNECT_CPU, VMBUS_CONNECT_CPU will be offlined by >> >crash_smp_send_stop() before the vmbus channel can be deconstructed. >> > >> >Signed-off-by: Hamza Mahfooz <hamzamahfooz@xxxxxxxxxxxxxxxxxxx> >> >--- >> > kernel/panic.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> >diff --git a/kernel/panic.c b/kernel/panic.c >> >index fbc59b3b64d0..9712a46dfe27 100644 >> >--- a/kernel/panic.c >> >+++ b/kernel/panic.c >> >@@ -372,8 +372,6 @@ void panic(const char *fmt, ...) >> > if (!_crash_kexec_post_notifiers) >> > __crash_kexec(NULL); >> > >> >- panic_other_cpus_shutdown(_crash_kexec_post_notifiers); >> >- >> > printk_legacy_allow_panic_sync(); >> >> I think printk_legacy_allow_panic_sync() is placed after >> panic_other_cpus_shutdown() so that it flushes the stored >> cpus backtraces as described [0]. >> >> > /* >> >@@ -382,6 +380,8 @@ void panic(const char *fmt, ...) >> > */ >> > atomic_notifier_call_chain(&panic_notifier_list, 0, buf); >> > >> >+ panic_other_cpus_shutdown(_crash_kexec_post_notifiers); >> >+ >> >> So maybe panic_other_cpus_shutdown() should be palced after >> atomic_notifier_call_chain() along with printk_legacy_allow_panic_sync() >> like below? >> >> ----- BEGIN ----- >> diff --git a/kernel/panic.c b/kernel/panic.c >> index d8635d5cecb2..7ac40e85ee27 100644 >> --- a/kernel/panic.c >> +++ b/kernel/panic.c >> @@ -372,16 +372,16 @@ void panic(const char *fmt, ...) >> if (!_crash_kexec_post_notifiers) >> __crash_kexec(NULL); >> >> - panic_other_cpus_shutdown(_crash_kexec_post_notifiers); >> - >> - printk_legacy_allow_panic_sync(); >> - >> /* >> * Run any panic handlers, including those that might need to >> * add information to the kmsg dump output. >> */ >> atomic_notifier_call_chain(&panic_notifier_list, 0, buf); >> >> + panic_other_cpus_shutdown(_crash_kexec_post_notifiers); >> + >> + printk_legacy_allow_panic_sync(); >> + >> panic_print_sys_info(false); >> >> kmsg_dump_desc(KMSG_DUMP_PANIC, buf); >> ----- END ----- > >Ya, that looks fine to me, that's actually how I had it initally, but I >wasn't sure if it had to go before the panic handlers. So, I erred on >the side of caution. I see, sorry that I was only speaking in relation to stored backtraces. It seems that printk_legacy_allow_panic_sync() is placed before atomic_notifier_call_chain() so that it can handle flushing before calling any panic handlers as described [0]. I'm not really familar with the problems associated with panic handlers so I hope maybe John and Petr can help on this matter... Sincerely, Ryo Takakura >BR, >Hamza [0] https://lore.kernel.org/lkml/ZeHSgZs9I3Ihvpye@alley/
