Hi all, What is the current status of this bug fix patch? I think it's OK if resending Hatayama-san's patch with Ingo's. Thanks, (2015/03/25 2:04), Vivek Goyal wrote: > On Tue, Mar 24, 2015 at 05:18:14PM +0100, Ingo Molnar wrote: >> >> * Vivek Goyal <vgoyal at redhat.com> wrote: >> >>>> Yet the actual bug is in that commit, 'crash_kexec_post_notifiers' >>>> was clearly not a no-op in the default case, against expectations. >>> >>> Hi Ingo, >>> >>> I did a quick test and in default case crash_kexec() runs before >>> panic notifiers. So it does look like crash_kexec_post_notifiers is >>> a no-op in default case. >>> >>> What am I missing. >> >> Well, look at f06e5153f4ae: >> >> diff --git a/kernel/panic.c b/kernel/panic.c >> index d02fa9fef46a..62e16cef9cc2 100644 >> --- a/kernel/panic.c >> +++ b/kernel/panic.c >> @@ -32,6 +32,7 @@ static unsigned long tainted_mask; >> static int pause_on_oops; >> static int pause_on_oops_flag; >> static DEFINE_SPINLOCK(pause_on_oops_lock); >> +static bool crash_kexec_post_notifiers; >> >> int panic_timeout = CONFIG_PANIC_TIMEOUT; >> EXPORT_SYMBOL_GPL(panic_timeout); >> @@ -112,9 +113,11 @@ void panic(const char *fmt, ...) >> /* >> * If we have crashed and we have a crash kernel loaded let it handle >> * everything else. >> - * Do we want to call this before we try to display a message? >> + * If we want to run this after calling panic_notifiers, pass >> + * the "crash_kexec_post_notifiers" option to the kernel. >> */ >> - crash_kexec(NULL); >> + if (!crash_kexec_post_notifiers) >> + crash_kexec(NULL); >> >> /* >> * Note smp_send_stop is the usual smp shutdown function, which >> @@ -131,6 +134,15 @@ void panic(const char *fmt, ...) >> >> kmsg_dump(KMSG_DUMP_PANIC); >> >> + /* >> + * If you doubt kdump always works fine in any situation, >> + * "crash_kexec_post_notifiers" offers you a chance to run >> + * panic_notifiers and dumping kmsg before kdump. >> + * Note: since some panic_notifiers can make crashed kernel >> + * more unstable, it can increase risks of the kdump failure too. >> + */ >> + crash_kexec(NULL); >> + >> bust_spinlocks(0); >> >> if (!panic_blink) >> >> >> Without knowing what crash_kexec() does, the patch looks buggy: it >> should preserve the old behavior by default, yet it will now execute a >> second crash_kexec() after the kmsg_dump() line. >> >> So the invariant change would have been to do: >> >> - crash_kexec(NULL); >> + if (!crash_kexec_post_notifiers) >> + crash_kexec(NULL); >> >> ... >> >> + if (crash_kexec_post_notifiers) >> + crash_kexec(NULL); >> >> Which in the !crash_kexec_post_notifiers flag case reduces to: >> >> crash_kexec(); >> >> ... >> >> /* NOP */ >> >> I.e. to exactly what the kernel was doing without the patch >> originally. >> >> Which is what my patch does. Nothing more, nothing less. > > Ok, I got it what you mean. > > crash_kexec() does not return if a kdump kernel is loaded. If kdump > kernel is not loaded, then crash_kexec() returns without doing anything. > > I think that explains why not making second call to crash_kexec() under > if, did not create problems. In first case it will never be called and > in second case, it will do nothing and simply return back. > > But anyway, we need your patch as that's right thing to do. > > Thanks > Vivek > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > -- Hidehiro Kawai Hitachi, Ltd. Research & Development Group
