From: "Guilherme G. Piccoli" <gpiccoli@igalia.com> To: Petr Mladek <pmladek@suse.com> Cc: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, kernel@gpiccoli.net, senozhatsky@chromium.org, rostedt@goodmis.org, john.ogness@linutronix.de, feng.tang@intel.com, kexec@lists.infradead.org, dyoung@redhat.com, keescook@chromium.org, anton@enomsg.org, ccross@android.com, tony.luck@intel.com Subject: Re: [PATCH V2] panic: Move panic_print before kmsg dumpers Date: Thu, 13 Jan 2022 09:34:01 -0300 [thread overview] Message-ID: <ba0e29ba-0e08-df6e-ade5-eb58ae2495e3@igalia.com> (raw) In-Reply-To: <Yd/0K1x7ILw3Qa46@alley> On 13/01/2022 08:50, Petr Mladek wrote: >> @@ -249,7 +252,7 @@ void panic(const char *fmt, ...) >> * show some extra information on kernel log if it was set... >> */ >> if (kexec_crash_loaded()) >> - panic_print_sys_info(); >> + panic_print_sys_info(false); > > panic_print_sys_info(false) will be called twice when both > kexec_crash_loaded() and _crash_kexec_post_notifiers are true. > > Do we really need to call panic_print_sys_info() here? All information > provided by panic_print_sys_info(false) can be found also in > the crash dump. > >> /* >> * If we have crashed and we have a crash kernel loaded let it handle >> @@ -283,6 +286,8 @@ void panic(const char *fmt, ...) >> */ >> atomic_notifier_call_chain(&panic_notifier_list, 0, buf); >> >> + panic_print_sys_info(false); > > This is where the info might be printed 2nd time. > >> + >> kmsg_dump(KMSG_DUMP_PANIC); >> >> /* > > Otherwise, the change makes sense to me. > > Best Regards, > Petr Hi Petr, thanks for your great review! I see you also commented in the thread of the patch introducing the panic_print_sys_info() before kdump. Thanks for catching this issue - indeed, if "_crash_kexec_post_notifiers" is true, with this patch we print stuff twice. I will submit a V3 that guards against that, using a bool, makes sense to you? The interesting question here is: > Do we really need to call panic_print_sys_info() here? All information > provided by panic_print_sys_info(false) can be found also in > the crash dump. So, we indeed need that in our use case. Crash is meant to be used post-mortem, i.e., you made a full vmcore collection and then, of course, you have basically all the data you need accessible though the crash tool. Problem is: in our use case, we want more data than a regular dmesg in a panic event (hence we use panic_print), but we don't collect a full crash dump, due to its big size. Also, as you can imagine, we do favor pstore over kdump, but it might fail due to a variety of reasons (like not having a free RAM buffer for ramoops), so kdump is our fallback. Hence, we'd like to be able to use panic_print with both kdump and pstore, and for that, both patches are needed. Cheers, Guilherme
WARNING: multiple messages have this Message-ID (diff)
From: Guilherme G. Piccoli <gpiccoli@igalia.com> To: kexec@lists.infradead.org Subject: [PATCH V2] panic: Move panic_print before kmsg dumpers Date: Thu, 13 Jan 2022 09:34:01 -0300 [thread overview] Message-ID: <ba0e29ba-0e08-df6e-ade5-eb58ae2495e3@igalia.com> (raw) In-Reply-To: <Yd/0K1x7ILw3Qa46@alley> On 13/01/2022 08:50, Petr Mladek wrote: >> @@ -249,7 +252,7 @@ void panic(const char *fmt, ...) >> * show some extra information on kernel log if it was set... >> */ >> if (kexec_crash_loaded()) >> - panic_print_sys_info(); >> + panic_print_sys_info(false); > > panic_print_sys_info(false) will be called twice when both > kexec_crash_loaded() and _crash_kexec_post_notifiers are true. > > Do we really need to call panic_print_sys_info() here? All information > provided by panic_print_sys_info(false) can be found also in > the crash dump. > >> /* >> * If we have crashed and we have a crash kernel loaded let it handle >> @@ -283,6 +286,8 @@ void panic(const char *fmt, ...) >> */ >> atomic_notifier_call_chain(&panic_notifier_list, 0, buf); >> >> + panic_print_sys_info(false); > > This is where the info might be printed 2nd time. > >> + >> kmsg_dump(KMSG_DUMP_PANIC); >> >> /* > > Otherwise, the change makes sense to me. > > Best Regards, > Petr Hi Petr, thanks for your great review! I see you also commented in the thread of the patch introducing the panic_print_sys_info() before kdump. Thanks for catching this issue - indeed, if "_crash_kexec_post_notifiers" is true, with this patch we print stuff twice. I will submit a V3 that guards against that, using a bool, makes sense to you? The interesting question here is: > Do we really need to call panic_print_sys_info() here? All information > provided by panic_print_sys_info(false) can be found also in > the crash dump. So, we indeed need that in our use case. Crash is meant to be used post-mortem, i.e., you made a full vmcore collection and then, of course, you have basically all the data you need accessible though the crash tool. Problem is: in our use case, we want more data than a regular dmesg in a panic event (hence we use panic_print), but we don't collect a full crash dump, due to its big size. Also, as you can imagine, we do favor pstore over kdump, but it might fail due to a variety of reasons (like not having a free RAM buffer for ramoops), so kdump is our fallback. Hence, we'd like to be able to use panic_print with both kdump and pstore, and for that, both patches are needed. Cheers, Guilherme
next prev parent reply other threads:[~2022-01-13 12:34 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-06 21:28 [PATCH V2] panic: Move panic_print before kmsg dumpers Guilherme G. Piccoli 2022-01-06 21:28 ` Guilherme G. Piccoli 2022-01-13 11:50 ` Petr Mladek 2022-01-13 11:50 ` Petr Mladek 2022-01-13 12:34 ` Guilherme G. Piccoli [this message] 2022-01-13 12:34 ` Guilherme G. Piccoli 2022-01-13 14:22 ` Petr Mladek 2022-01-13 14:22 ` Petr Mladek 2022-01-13 15:15 ` Guilherme G. Piccoli 2022-01-13 15:15 ` Guilherme G. Piccoli 2022-01-14 12:26 ` Petr Mladek 2022-01-14 12:26 ` Petr Mladek 2022-01-14 12:32 ` Guilherme G. Piccoli 2022-01-14 12:32 ` Guilherme G. Piccoli
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=ba0e29ba-0e08-df6e-ade5-eb58ae2495e3@igalia.com \ --to=gpiccoli@igalia.com \ --cc=akpm@linux-foundation.org \ --cc=anton@enomsg.org \ --cc=ccross@android.com \ --cc=dyoung@redhat.com \ --cc=feng.tang@intel.com \ --cc=john.ogness@linutronix.de \ --cc=keescook@chromium.org \ --cc=kernel@gpiccoli.net \ --cc=kexec@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=pmladek@suse.com \ --cc=rostedt@goodmis.org \ --cc=senozhatsky@chromium.org \ --cc=tony.luck@intel.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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.