From: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
To: Vivek Goyal <vgoyal@redhat.com>
Cc: "Ingo Molnar" <mingo@kernel.org>,
"Masami Hiramatsu" <masami.hiramatsu.pt@hitachi.com>,
"Baoquan He" <bhe@redhat.com>,
"\"Hatayama, Daisuke/畑山 大輔\"" <d.hatayama@jp.fujitsu.com>,
ebiederm@xmission.com, linux-kernel@vger.kernel.org,
kexec@lists.infradead.org, akpm@linux-foundation.org,
mingo@redhat.com, bp@suse.de
Subject: Re: Re: Re: [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path
Date: Tue, 12 May 2015 17:43:03 +0900 [thread overview]
Message-ID: <5551BD17.6040606@hitachi.com> (raw)
In-Reply-To: <20150324170417.GA5030@redhat.com>
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@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@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
prev parent reply other threads:[~2015-05-12 8:43 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-06 16:31 [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path "Hatayama, Daisuke/畑山 大輔"
2015-03-06 18:08 ` Vivek Goyal
2015-03-23 3:47 ` Baoquan He
2015-03-23 7:19 ` Ingo Molnar
2015-03-23 13:37 ` Vivek Goyal
2015-03-23 13:50 ` Ingo Molnar
2015-03-23 14:31 ` Vivek Goyal
2015-03-23 16:01 ` Don Zickus
2015-03-24 3:58 ` Masami Hiramatsu
2015-03-23 15:36 ` Vivek Goyal
2015-03-24 3:30 ` Masami Hiramatsu
2015-03-24 7:11 ` Ingo Molnar
2015-03-24 10:27 ` Eric W. Biederman
2015-03-24 14:32 ` Vivek Goyal
2015-03-25 15:07 ` Hidehiro Kawai
2015-03-24 14:46 ` Vivek Goyal
2015-03-24 16:18 ` Ingo Molnar
2015-03-24 17:04 ` Vivek Goyal
2015-05-12 8:43 ` Hidehiro Kawai [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5551BD17.6040606@hitachi.com \
--to=hidehiro.kawai.ez@hitachi.com \
--cc=akpm@linux-foundation.org \
--cc=bhe@redhat.com \
--cc=bp@suse.de \
--cc=d.hatayama@jp.fujitsu.com \
--cc=ebiederm@xmission.com \
--cc=kexec@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=masami.hiramatsu.pt@hitachi.com \
--cc=mingo@kernel.org \
--cc=mingo@redhat.com \
--cc=vgoyal@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).