From: Dave Young <dyoung@redhat.com> To: "Guilherme G. Piccoli" <gpiccoli@igalia.com> Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-doc@vger.kernel.org, mcgrof@kernel.org, keescook@chromium.org, yzaikin@google.com, akpm@linux-foundation.org, feng.tang@intel.com, siglesias@igalia.com, kernel@gpiccoli.net, kexec@lists.infradead.org Subject: Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump Date: Wed, 22 Dec 2021 19:45:18 +0800 [thread overview] Message-ID: <YcMPzs6t8MKpEacq@dhcp-128-65.nay.redhat.com> (raw) In-Reply-To: <20211109202848.610874-4-gpiccoli@igalia.com> Hi Guilherme, Thanks for you patch. Could you add kexec list for any following up patches? This could change kdump behavior so let's see if any comments from kexec list. Kudos for the lore+lei tool so that I can catch this by seeing this coming into Andrews tree :) On 11/09/21 at 05:28pm, Guilherme G. Piccoli wrote: > Currently we have the "panic_print" parameter/sysctl to allow some extra > information to be printed in a panic event. On the other hand, the kdump > mechanism allows to kexec a new kernel to collect a memory dump for the > running kernel in case of panic. > Right now these options are incompatible: the user either sets the kdump > or makes use of "panic_print". The code path of "panic_print" isn't > reached when kdump is configured. > > There are situations though in which this would be interesting: for > example, in systems that are very memory constrained, a handcrafted > tiny kernel/initrd for kdump might be used in order to only collect the > dmesg in kdump kernel. Even more common, systems with no disk space for > the full (compressed) memory dump might very well rely in this > functionality too, dumping only the dmesg with the additional information > provided by "panic_print". > > So, this is what the patch does: allows both functionality to co-exist; > if "panic_print" is set and the system performs a kdump, the extra > information is printed on dmesg before the kexec. Some notes about the > design choices here: > > (a) We could have introduced a sysctl or an extra bit on "panic_print" > to allow enabling the co-existence of kdump and "panic_print", but seems > that would be over-engineering; we have 3 cases, let's check how this > patch change things: > > - if the user have kdump set and not "panic_print", nothing changes; > - if the user have "panic_print" set and not kdump, nothing changes; > - if both are enabled, now we print the extra information before kdump, > which is exactly the goal of the patch (and should be the goal of the > user, since they enabled both options). People may enable kdump crashkernel and panic_print together but they are not aware the extra panic print could cause kdump not reliable (in theory). So at least some words in kernel-parameters.txt would help. > > (b) We assume that the code path won't return from __crash_kexec() > so we didn't guard against double execution of panic_print_sys_info(). > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > --- > kernel/panic.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/panic.c b/kernel/panic.c > index 5da71fa4e5f1..439dbf93b406 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -243,6 +243,13 @@ void panic(const char *fmt, ...) > */ > kgdb_panic(buf); > > + /* > + * If we have a kdump kernel loaded, give a chance to panic_print > + * show some extra information on kernel log if it was set... > + */ > + if (kexec_crash_loaded()) > + panic_print_sys_info(); > + > /* > * If we have crashed and we have a crash kernel loaded let it handle > * everything else. > -- > 2.33.1 > > Thanks Dave
WARNING: multiple messages have this Message-ID (diff)
From: Dave Young <dyoung@redhat.com> To: "Guilherme G. Piccoli" <gpiccoli@igalia.com> Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-doc@vger.kernel.org, mcgrof@kernel.org, keescook@chromium.org, yzaikin@google.com, akpm@linux-foundation.org, feng.tang@intel.com, siglesias@igalia.com, kernel@gpiccoli.net, kexec@lists.infradead.org Subject: Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump Date: Wed, 22 Dec 2021 19:45:18 +0800 [thread overview] Message-ID: <YcMPzs6t8MKpEacq@dhcp-128-65.nay.redhat.com> (raw) In-Reply-To: <20211109202848.610874-4-gpiccoli@igalia.com> Hi Guilherme, Thanks for you patch. Could you add kexec list for any following up patches? This could change kdump behavior so let's see if any comments from kexec list. Kudos for the lore+lei tool so that I can catch this by seeing this coming into Andrews tree :) On 11/09/21 at 05:28pm, Guilherme G. Piccoli wrote: > Currently we have the "panic_print" parameter/sysctl to allow some extra > information to be printed in a panic event. On the other hand, the kdump > mechanism allows to kexec a new kernel to collect a memory dump for the > running kernel in case of panic. > Right now these options are incompatible: the user either sets the kdump > or makes use of "panic_print". The code path of "panic_print" isn't > reached when kdump is configured. > > There are situations though in which this would be interesting: for > example, in systems that are very memory constrained, a handcrafted > tiny kernel/initrd for kdump might be used in order to only collect the > dmesg in kdump kernel. Even more common, systems with no disk space for > the full (compressed) memory dump might very well rely in this > functionality too, dumping only the dmesg with the additional information > provided by "panic_print". > > So, this is what the patch does: allows both functionality to co-exist; > if "panic_print" is set and the system performs a kdump, the extra > information is printed on dmesg before the kexec. Some notes about the > design choices here: > > (a) We could have introduced a sysctl or an extra bit on "panic_print" > to allow enabling the co-existence of kdump and "panic_print", but seems > that would be over-engineering; we have 3 cases, let's check how this > patch change things: > > - if the user have kdump set and not "panic_print", nothing changes; > - if the user have "panic_print" set and not kdump, nothing changes; > - if both are enabled, now we print the extra information before kdump, > which is exactly the goal of the patch (and should be the goal of the > user, since they enabled both options). People may enable kdump crashkernel and panic_print together but they are not aware the extra panic print could cause kdump not reliable (in theory). So at least some words in kernel-parameters.txt would help. > > (b) We assume that the code path won't return from __crash_kexec() > so we didn't guard against double execution of panic_print_sys_info(). > > Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> > --- > kernel/panic.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/panic.c b/kernel/panic.c > index 5da71fa4e5f1..439dbf93b406 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -243,6 +243,13 @@ void panic(const char *fmt, ...) > */ > kgdb_panic(buf); > > + /* > + * If we have a kdump kernel loaded, give a chance to panic_print > + * show some extra information on kernel log if it was set... > + */ > + if (kexec_crash_loaded()) > + panic_print_sys_info(); > + > /* > * If we have crashed and we have a crash kernel loaded let it handle > * everything else. > -- > 2.33.1 > > Thanks Dave _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2021-12-22 11:45 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-09 20:28 [PATCH 0/3] Some improvements on panic_print Guilherme G. Piccoli 2021-11-09 20:28 ` [PATCH 1/3] docs: sysctl/kernel: Add missing bit to panic_print Guilherme G. Piccoli 2021-11-30 5:09 ` Feng Tang 2021-11-09 20:28 ` [PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print Guilherme G. Piccoli 2021-11-30 5:12 ` Feng Tang 2021-12-03 15:09 ` Guilherme G. Piccoli 2021-12-19 20:11 ` Luis Chamberlain 2021-12-20 12:38 ` Guilherme G. Piccoli 2021-12-21 23:48 ` Andrew Morton 2021-12-22 12:37 ` Guilherme G. Piccoli 2022-01-13 9:31 ` Petr Mladek 2021-11-09 20:28 ` [PATCH 3/3] panic: Allow printing extra panic information on kdump Guilherme G. Piccoli 2021-12-22 11:45 ` Dave Young [this message] 2021-12-22 11:45 ` Dave Young 2021-12-22 12:34 ` Guilherme G. Piccoli 2021-12-22 12:34 ` Guilherme G. Piccoli 2021-12-24 1:35 ` Dave Young 2021-12-24 1:35 ` Dave Young 2021-12-25 19:21 ` Guilherme G. Piccoli 2021-12-25 19:21 ` Guilherme G. Piccoli 2021-12-27 1:45 ` Dave Young 2021-12-27 1:45 ` Dave Young 2021-12-27 3:14 ` Guilherme G. Piccoli 2021-12-27 3:14 ` Guilherme G. Piccoli 2022-01-13 9:02 ` Petr Mladek 2022-01-13 13:00 ` Guilherme G. Piccoli 2022-01-27 16:53 ` Guilherme G. Piccoli 2022-02-08 18:12 ` Guilherme G. Piccoli 2022-02-08 21:39 ` Stephen Rothwell 2022-02-09 15:06 ` Guilherme G. Piccoli 2022-02-09 23:26 ` Stephen Rothwell 2022-02-10 12:50 ` Guilherme G. Piccoli 2021-11-26 21:34 ` [PATCH 0/3] Some improvements on panic_print Guilherme G. Piccoli 2021-12-14 16:31 ` 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=YcMPzs6t8MKpEacq@dhcp-128-65.nay.redhat.com \ --to=dyoung@redhat.com \ --cc=akpm@linux-foundation.org \ --cc=feng.tang@intel.com \ --cc=gpiccoli@igalia.com \ --cc=keescook@chromium.org \ --cc=kernel@gpiccoli.net \ --cc=kexec@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mcgrof@kernel.org \ --cc=siglesias@igalia.com \ --cc=yzaikin@google.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.