[PATCH 0/3] Some improvements on panic_print

[PATCH 0/3] Some improvements on panic_print

[Guilherme G. Piccoli]

Hey everybody, this is a mix of a documentation fix with some additions
to the "panic_print" syscall / parameter. The goal here is being able
to collect all CPUs backtraces during a panic event and also
to enable "panic_print" in a kdump event - details of the reasoning
and design choices in the patches.

Thanks in advance for reviews!
Cheers,


Guilherme


Guilherme G. Piccoli (3):
  docs: sysctl/kernel: Add missing bit to panic_print
  panic: Add option to dump all CPUs backtraces in panic_print
  panic: Allow printing extra panic information on kdump

 Documentation/admin-guide/kernel-parameters.txt |  1 +
 Documentation/admin-guide/sysctl/kernel.rst     |  2 ++
 kernel/panic.c                                  | 11 +++++++++++
 3 files changed, 14 insertions(+)

-- 
2.33.1

[PATCH 1/3] docs: sysctl/kernel: Add missing bit to panic_print

[Guilherme G. Piccoli]

Commit de6da1e8bcf0 ("panic: add an option to replay all the printk message in buffer")
added a new bit to the sysctl/kernel parameter "panic_print", but the
documentation was added only in kernel-parameters.txt, not in the sysctl guide.

Fix it here by adding bit 5 to sysctl admin-guide documentation.

Cc: Feng Tang <feng.tang@intel.com>
Fixes: de6da1e8bcf0 ("panic: add an option to replay all the printk message in buffer")
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 Documentation/admin-guide/sysctl/kernel.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index 426162009ce9..70b7df9b081a 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -795,6 +795,7 @@ bit 1  print system memory info
 bit 2  print timer info
 bit 3  print locks info if ``CONFIG_LOCKDEP`` is on
 bit 4  print ftrace buffer
+bit 5: print all printk messages in buffer
 =====  ============================================
 
 So for example to print tasks and memory info on panic, user can::
-- 
2.33.1

Re: [PATCH 1/3] docs: sysctl/kernel: Add missing bit to panic_print

[Feng Tang]

On Tue, Nov 09, 2021 at 05:28:46PM -0300, Guilherme G. Piccoli wrote:
> Commit de6da1e8bcf0 ("panic: add an option to replay all the printk message in buffer")
> added a new bit to the sysctl/kernel parameter "panic_print", but the
> documentation was added only in kernel-parameters.txt, not in the sysctl guide.
> 
> Fix it here by adding bit 5 to sysctl admin-guide documentation.
 
Thanks for the fix!

Reviewed-by: Feng Tang <feng.tang@intel.com>

- Feng

> Cc: Feng Tang <feng.tang@intel.com>
> Fixes: de6da1e8bcf0 ("panic: add an option to replay all the printk message in buffer")
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>  Documentation/admin-guide/sysctl/kernel.rst | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index 426162009ce9..70b7df9b081a 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -795,6 +795,7 @@ bit 1  print system memory info
>  bit 2  print timer info
>  bit 3  print locks info if ``CONFIG_LOCKDEP`` is on
>  bit 4  print ftrace buffer
> +bit 5: print all printk messages in buffer
>  =====  ============================================
>  
>  So for example to print tasks and memory info on panic, user can::
> -- 
> 2.33.1

[PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print

[Guilherme G. Piccoli]

Currently the "panic_print" parameter/sysctl allows some interesting debug
information to be printed during a panic event. This is useful for example
in cases the user cannot kdump due to resource limits, or if the user
collects panic logs in a serial output (or pstore) and prefers a fast
reboot instead of a kdump.

Happens that currently there's no way to see all CPUs backtraces in
a panic using "panic_print" on architectures that support that. We do
have "oops_all_cpu_backtrace" sysctl, but although partially overlapping
in the functionality, they are orthogonal in nature: "panic_print" is
a panic tuning (and we have panics without oopses, like direct calls to
panic() or maybe other paths that don't go through oops_enter()
function), and the original purpose of "oops_all_cpu_backtrace" is to
provide more information on oopses for cases in which the users desire
to continue running the kernel even after an oops, i.e., used in
non-panic scenarios.

So, we hereby introduce an additional bit for "panic_print" to allow
dumping the CPUs backtraces during a panic event.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 1 +
 Documentation/admin-guide/sysctl/kernel.rst     | 1 +
 kernel/panic.c                                  | 4 ++++
 3 files changed, 6 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 0905d2cdb2d5..569d035c4332 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3690,6 +3690,7 @@
 			bit 3: print locks info if CONFIG_LOCKDEP is on
 			bit 4: print ftrace buffer
 			bit 5: print all printk messages in buffer
+			bit 6: print all CPUs backtrace (if available in the arch)
 
 	panic_on_taint=	Bitmask for conditionally calling panic() in add_taint()
 			Format: <hex>[,nousertaint]
diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index 70b7df9b081a..1666c1a9dbba 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -796,6 +796,7 @@ bit 2  print timer info
 bit 3  print locks info if ``CONFIG_LOCKDEP`` is on
 bit 4  print ftrace buffer
 bit 5: print all printk messages in buffer
+bit 6: print all CPUs backtrace (if available in the arch)
 =====  ============================================
 
 So for example to print tasks and memory info on panic, user can::
diff --git a/kernel/panic.c b/kernel/panic.c
index cefd7d82366f..5da71fa4e5f1 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -65,6 +65,7 @@ EXPORT_SYMBOL_GPL(panic_timeout);
 #define PANIC_PRINT_LOCK_INFO		0x00000008
 #define PANIC_PRINT_FTRACE_INFO		0x00000010
 #define PANIC_PRINT_ALL_PRINTK_MSG	0x00000020
+#define PANIC_PRINT_ALL_CPU_BT		0x00000040
 unsigned long panic_print;
 
 ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
@@ -151,6 +152,9 @@ static void panic_print_sys_info(void)
 	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
 		console_flush_on_panic(CONSOLE_REPLAY_ALL);
 
+	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
+		trigger_all_cpu_backtrace();
+
 	if (panic_print & PANIC_PRINT_TASK_INFO)
 		show_state();
 
-- 
2.33.1

Re: [PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print

[Feng Tang]

On Tue, Nov 09, 2021 at 05:28:47PM -0300, Guilherme G. Piccoli wrote:
> Currently the "panic_print" parameter/sysctl allows some interesting debug
> information to be printed during a panic event. This is useful for example
> in cases the user cannot kdump due to resource limits, or if the user
> collects panic logs in a serial output (or pstore) and prefers a fast
> reboot instead of a kdump.
> 
> Happens that currently there's no way to see all CPUs backtraces in
> a panic using "panic_print" on architectures that support that. We do
> have "oops_all_cpu_backtrace" sysctl, but although partially overlapping
> in the functionality, they are orthogonal in nature: "panic_print" is
> a panic tuning (and we have panics without oopses, like direct calls to
> panic() or maybe other paths that don't go through oops_enter()
> function), and the original purpose of "oops_all_cpu_backtrace" is to
> provide more information on oopses for cases in which the users desire
> to continue running the kernel even after an oops, i.e., used in
> non-panic scenarios.
> 
> So, we hereby introduce an additional bit for "panic_print" to allow
> dumping the CPUs backtraces during a panic event.

This looks to be helpful for debugging panic.

Reviewed-by: Feng Tang <feng.tang@intel.com>

Thanks,
Feng


> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
> ---
>  Documentation/admin-guide/kernel-parameters.txt | 1 +
>  Documentation/admin-guide/sysctl/kernel.rst     | 1 +
>  kernel/panic.c                                  | 4 ++++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 0905d2cdb2d5..569d035c4332 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -3690,6 +3690,7 @@
>  			bit 3: print locks info if CONFIG_LOCKDEP is on
>  			bit 4: print ftrace buffer
>  			bit 5: print all printk messages in buffer
> +			bit 6: print all CPUs backtrace (if available in the arch)
>  
>  	panic_on_taint=	Bitmask for conditionally calling panic() in add_taint()
>  			Format: <hex>[,nousertaint]
> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index 70b7df9b081a..1666c1a9dbba 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -796,6 +796,7 @@ bit 2  print timer info
>  bit 3  print locks info if ``CONFIG_LOCKDEP`` is on
>  bit 4  print ftrace buffer
>  bit 5: print all printk messages in buffer
> +bit 6: print all CPUs backtrace (if available in the arch)
>  =====  ============================================
>  
>  So for example to print tasks and memory info on panic, user can::
> diff --git a/kernel/panic.c b/kernel/panic.c
> index cefd7d82366f..5da71fa4e5f1 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -65,6 +65,7 @@ EXPORT_SYMBOL_GPL(panic_timeout);
>  #define PANIC_PRINT_LOCK_INFO		0x00000008
>  #define PANIC_PRINT_FTRACE_INFO		0x00000010
>  #define PANIC_PRINT_ALL_PRINTK_MSG	0x00000020
> +#define PANIC_PRINT_ALL_CPU_BT		0x00000040
>  unsigned long panic_print;
>  
>  ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
> @@ -151,6 +152,9 @@ static void panic_print_sys_info(void)
>  	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
>  		console_flush_on_panic(CONSOLE_REPLAY_ALL);
>  
> +	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
> +		trigger_all_cpu_backtrace();
> +
>  	if (panic_print & PANIC_PRINT_TASK_INFO)
>  		show_state();
>  
> -- 
> 2.33.1

Re: [PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print

[Guilherme G. Piccoli]

On 30/11/2021 02:12, Feng Tang wrote:
> On Tue, Nov 09, 2021 at 05:28:47PM -0300, Guilherme G. Piccoli wrote:
>> [...]
> This looks to be helpful for debugging panic.
> 
> Reviewed-by: Feng Tang <feng.tang@intel.com>
> 
> Thanks,
> Feng

Thanks a lot Feng, for both your reviews! Do you have any opinions about
patch 3?

Also, as a generic question to all CCed, what is the way forward with
this thread?
Cheers,


Guilherme

Re: [PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print

[Luis Chamberlain]

On Fri, Dec 03, 2021 at 12:09:06PM -0300, Guilherme G. Piccoli wrote:
> On 30/11/2021 02:12, Feng Tang wrote:
> > On Tue, Nov 09, 2021 at 05:28:47PM -0300, Guilherme G. Piccoli wrote:
> >> [...]
> > This looks to be helpful for debugging panic.
> > 
> > Reviewed-by: Feng Tang <feng.tang@intel.com>
> > 
> > Thanks,
> > Feng
> 
> Thanks a lot Feng, for both your reviews! Do you have any opinions about
> patch 3?
> 
> Also, as a generic question to all CCed, what is the way forward with
> this thread?
> Cheers,

mcgrof@sumo ~/linux-next (git::master)$ ./scripts/get_maintainer.pl
kernel/printk/
Petr Mladek <pmladek@suse.com> (maintainer:PRINTK)
Sergey Senozhatsky <senozhatsky@chromium.org> (maintainer:PRINTK)
Steven Rostedt <rostedt@goodmis.org> (reviewer:PRINTK)
John Ogness <john.ogness@linutronix.de> (reviewer:PRINTK)
linux-kernel@vger.kernel.org (open list)    

So I suggest you email the patches to those.

  Luis

Re: [PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print

[Guilherme G. Piccoli]

On 19/12/2021 17:11, Luis Chamberlain wrote:
> mcgrof@sumo ~/linux-next (git::master)$ ./scripts/get_maintainer.pl
> kernel/printk/
> Petr Mladek <pmladek@suse.com> (maintainer:PRINTK)
> Sergey Senozhatsky <senozhatsky@chromium.org> (maintainer:PRINTK)
> Steven Rostedt <rostedt@goodmis.org> (reviewer:PRINTK)
> John Ogness <john.ogness@linutronix.de> (reviewer:PRINTK)
> linux-kernel@vger.kernel.org (open list)    
> 
> So I suggest you email the patches to those.
> 
>   Luis
> 

Hi Luis, thank you! But I confess I'm very confused with this series. I
saw emails from Andrew that the patches had been accepted and were
available in -mm tree ([0] for example) but I'm not seeing them in
linux-next nor mmotm/mmots (although I saw them in mmotm directory for a
while before).

Andrew, could you clarify the state of them?
Appreciate that!

Cheers,


Guilherme


[0]
https://lore.kernel.org/mm-commits/20211214182909._sQRtXv89%25akpm@linux-foundation.org/

Re: [PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print

[Andrew Morton]

On Mon, 20 Dec 2021 09:38:23 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote:

> On 19/12/2021 17:11, Luis Chamberlain wrote:
> > mcgrof@sumo ~/linux-next (git::master)$ ./scripts/get_maintainer.pl
> > kernel/printk/
> > Petr Mladek <pmladek@suse.com> (maintainer:PRINTK)
> > Sergey Senozhatsky <senozhatsky@chromium.org> (maintainer:PRINTK)
> > Steven Rostedt <rostedt@goodmis.org> (reviewer:PRINTK)
> > John Ogness <john.ogness@linutronix.de> (reviewer:PRINTK)
> > linux-kernel@vger.kernel.org (open list)    
> > 
> > So I suggest you email the patches to those.
> > 
> >   Luis
> > 
> 
> Hi Luis, thank you! But I confess I'm very confused with this series. I
> saw emails from Andrew that the patches had been accepted and were
> available in -mm tree ([0] for example) but I'm not seeing them in
> linux-next nor mmotm/mmots (although I saw them in mmotm directory for a
> while before).
> 
> Andrew, could you clarify the state of them?

They'll turn up on ozlabs after I've tested it all then uploaded.  I do
this a couple of times a week, approx.

Re: [PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print

[Guilherme G. Piccoli]

On 21/12/2021 20:48, Andrew Morton wrote:
>[...]
> 
> They'll turn up on ozlabs after I've tested it all then uploaded.  I do
> this a couple of times a week, approx.
> 

OK, thank you Andrew. Will I get some ping when that happens, through
some bot or anything? I'm waiting them to show up in linux-next tree so
to start a backport to an in-house kernel and starting using them.

Cheers,


Guilherme

Re: [PATCH 2/3] panic: Add option to dump all CPUs backtraces in panic_print

[Petr Mladek]

On Tue 2021-11-09 17:28:47, Guilherme G. Piccoli wrote:
> Currently the "panic_print" parameter/sysctl allows some interesting debug
> information to be printed during a panic event. This is useful for example
> in cases the user cannot kdump due to resource limits, or if the user
> collects panic logs in a serial output (or pstore) and prefers a fast
> reboot instead of a kdump.

Yes, I have missed this possibility many times.

> Happens that currently there's no way to see all CPUs backtraces in
> a panic using "panic_print" on architectures that support that. We do
> have "oops_all_cpu_backtrace" sysctl, but although partially overlapping
> in the functionality, they are orthogonal in nature: "panic_print" is
> a panic tuning (and we have panics without oopses, like direct calls to
> panic() or maybe other paths that don't go through oops_enter()
> function), and the original purpose of "oops_all_cpu_backtrace" is to
> provide more information on oopses for cases in which the users desire
> to continue running the kernel even after an oops, i.e., used in
> non-panic scenarios.

panic() already prevents double backtrace of the CPU that Oopsed, see:

#ifdef CONFIG_DEBUG_BUGVERBOSE
	/*
	 * Avoid nested stack-dumping if a panic occurs during oops processing
	 */
	if (!test_taint(TAINT_DIE) && oops_in_progress <= 1)
		dump_stack();
#endif

It should be possible to do something similar also for backtraces
on all CPUs.

There are more situation when the backtraces are printed and panic()
is called, for example: softlockup_panic and
softlockup_all_cpu_backtrace.

Well, it is just nice to have. People probably will not use these
options together. And it is better to have the backtraces twice
than do not have them at all.

> So, we hereby introduce an additional bit for "panic_print" to allow
> dumping the CPUs backtraces during a panic event.
>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

[PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

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).

(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

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Dave Young]

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

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

On 22/12/2021 08:45, Dave Young wrote:
> 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 :)

Hi Dave, I'm really sorry for not adding the kexec list, I forgot. But I
will do next time for sure, my apologies. And thanks for taking a look
after you noticed that on lore, I appreciate your feedback!

> [...]
> 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.
>  

That makes sense, I'll improve that in a follow-up patch, how about
that? Indeed it's a good idea to let people be sure that panic_print
might affect kdump reliability, although I consider the risk to be
pretty low. And I'll loop the kexec list for sure!

Cheers,


Guilherme

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Dave Young]

Hi Guilherme,
On 12/22/21 at 09:34am, Guilherme G. Piccoli wrote:
> On 22/12/2021 08:45, Dave Young wrote:
> > 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 :)
> 
> Hi Dave, I'm really sorry for not adding the kexec list, I forgot. But I
> will do next time for sure, my apologies. And thanks for taking a look
> after you noticed that on lore, I appreciate your feedback!

Thanks!

> 
> > [...]
> > 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.
> >  
> 
> That makes sense, I'll improve that in a follow-up patch, how about
> that? Indeed it's a good idea to let people be sure that panic_print
> might affect kdump reliability, although I consider the risk to be
> pretty low. And I'll loop the kexec list for sure!

If only the doc update, I think it is fine to be another follup-up
patch.

About your 1st option in patch log, there is crash_kexec_post_notifiers
kernel param which can be used to switch on panic notifiers before kdump
bootup.   Another way probably you can try to move panic print to be
panic notifier. Have this been discussed before? 

> 
> Cheers,
> 
> 
> Guilherme

Thanks
Dave

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

On 23/12/2021 22:35, Dave Young wrote:
> Hi Guilherme,
> [...]
> If only the doc update, I think it is fine to be another follup-up
> patch.
> 
> About your 1st option in patch log, there is crash_kexec_post_notifiers
> kernel param which can be used to switch on panic notifiers before kdump
> bootup.   Another way probably you can try to move panic print to be
> panic notifier. Have this been discussed before? 
> 

Hey Dave, thanks for the suggestion. I've considered that but didn't
like the idea. My reasoning was: allowing post notifiers on kdump will
highly compromise the reliability, whereas the panic_print is a solo
option, and not very invasive.

To mix it with all panic notifiers would just increase a lot the risk of
a kdump failure. Put in other words: if I'm a kdump user and in order to
have this panic_print setting I'd also need to enable post notifiers,
certainly I'll not use the feature, 'cause I don't wanna risk kdump too
much.

One other option I've considered however, and I'd appreciate your
opinion here, would be a new option on crash_kexec_post_notifiers that
allows the users to select *which few notifiers* they want to enable.
Currently it's all or nothing, and this approach is too heavy/risky I
believe. Allowing customization on which post notifiers the user wants
would be much better and in this case, having a post notifier for
panic_print makes a lot of sense. What do you think?

Thanks!

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Dave Young]

On 12/25/21 at 04:21pm, Guilherme G. Piccoli wrote:
> On 23/12/2021 22:35, Dave Young wrote:
> > Hi Guilherme,
> > [...]
> > If only the doc update, I think it is fine to be another follup-up
> > patch.
> > 
> > About your 1st option in patch log, there is crash_kexec_post_notifiers
> > kernel param which can be used to switch on panic notifiers before kdump
> > bootup.   Another way probably you can try to move panic print to be
> > panic notifier. Have this been discussed before? 
> > 
> 
> Hey Dave, thanks for the suggestion. I've considered that but didn't
> like the idea. My reasoning was: allowing post notifiers on kdump will
> highly compromise the reliability, whereas the panic_print is a solo
> option, and not very invasive.
> 
> To mix it with all panic notifiers would just increase a lot the risk of
> a kdump failure. Put in other words: if I'm a kdump user and in order to
> have this panic_print setting I'd also need to enable post notifiers,
> certainly I'll not use the feature, 'cause I don't wanna risk kdump too
> much.

Hi Guilherme, yes, I have the same concern.  But there could be more
things like the panic_print in the future, it looks odd to have more
kernel cmdline params though.

> 
> One other option I've considered however, and I'd appreciate your
> opinion here, would be a new option on crash_kexec_post_notifiers that
> allows the users to select *which few notifiers* they want to enable.
> Currently it's all or nothing, and this approach is too heavy/risky I
> believe. Allowing customization on which post notifiers the user wants
> would be much better and in this case, having a post notifier for
> panic_print makes a lot of sense. What do you think?

It is definitely a good idea, I'm more than glad to see this if you
would like to work on this! 

> 
> Thanks!
> 

Thanks
Dave

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

On 26/12/2021 22:45, Dave Young wrote:
> On 12/25/21 at 04:21pm, Guilherme G. Piccoli wrote:
> [...] 
> Hi Guilherme, yes, I have the same concern.  But there could be more
> things like the panic_print in the future, it looks odd to have more
> kernel cmdline params though.
> 

Agreed! We're on the same page here, definitely.


> [...] 
> It is definitely a good idea, I'm more than glad to see this if you
> would like to work on this! 
> 

Awesome! I'll try to give it a shot this week, let's see how complex
that'd be.

Thanks again for the great feedback!
Cheers,


Guilherme

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Petr Mladek]

On Tue 2021-11-09 17:28:48, 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".

Is anyone really using this approach? kmsg_dump() looks like a better
choice when there are memory constrains. It does not need to reserve
memory for booting the crash kernel.

I would not mind much but this change depends on a not fully reliable
assumption, see below.

Also it will also complicate the solution for the kmsg_dump() code path.
It would be better to discuss this togeter with the other patch
https://lore.kernel.org/r/20220106212835.119409-1-gpiccoli@igalia.com


> 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).
> 
> (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().

This sounds suspiciously. There is small race window but it actually works.
__crash_kexec() really never returns when @kexec_crash_image is
loaded. Well, it might break in the future if the code is modified.

Best Regards,
Petr

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

On 13/01/2022 06:02, Petr Mladek wrote:
> [...]
> Is anyone really using this approach? kmsg_dump() looks like a better
> choice when there are memory constrains. It does not need to reserve
> memory for booting the crash kernel.
> 
> I would not mind much but this change depends on a not fully reliable
> assumption, see below.
> 
> Also it will also complicate the solution for the kmsg_dump() code path.
> It would be better to discuss this togeter with the other patch
> https://lore.kernel.org/r/20220106212835.119409-1-gpiccoli@igalia.com

Hi Petr, thanks for your analysis here. Indeed, our use case benefits
from both this and the other patch in the thread you mentioned above -
see [0].


> [...]
>> (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().
> 
> This sounds suspiciously. There is small race window but it actually works.
> __crash_kexec() really never returns when @kexec_crash_image is
> loaded. Well, it might break in the future if the code is modified.
> 
> Best Regards,
> Petr

OK, so since this patch is already on linux-next and is relevant for our
use case, how about if we explicitly guard against the double print, as
I suggested in [0]?

Cheers,

Guilherme


[0]
https://lore.kernel.org/lkml/ba0e29ba-0e08-df6e-ade5-eb58ae2495e3@igalia.com/

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

Hi Andrew, can I ask you to please remove this patch from linux-next?

It shows here:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=56439cb78293

Baoquan has concerns, and we're discussing that in another thread [0],
after I submit another change on top of this one. So, I guess it's
simpler to just drop it.

My apologies for this, I should have definitely loop the kexec list in
this one , but I forgot.

Cheers,


Guilherme


[0]
https://lore.kernel.org/lkml/7b93afff-66a0-44ee-3bb7-3d1e12dd47c2@igalia.com/

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

On 27/01/2022 13:53, Guilherme G. Piccoli wrote:
> Hi Andrew, can I ask you to please remove this patch from linux-next?
> 
> It shows here:
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=56439cb78293
> 
> Baoquan has concerns, and we're discussing that in another thread [0],
> after I submit another change on top of this one. So, I guess it's
> simpler to just drop it.
> 
> My apologies for this, I should have definitely loop the kexec list in
> this one , but I forgot.
> 
> Cheers,
> 
> 
> Guilherme
> 
> 
> [0]
> https://lore.kernel.org/lkml/7b93afff-66a0-44ee-3bb7-3d1e12dd47c2@igalia.com/


Hi Stephen / Andrew, sorry for the annoyance, but can you please remove
this patch from linux-next?

Today it shows as commit
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d691651735f1
- this commit is subject to concern from Baoquan and we are discussing
better ways to achieve that, through a refactor.

Thanks in advance,


Guilherme

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

Hi Guilherme,

On Tue, 8 Feb 2022 15:12:04 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote:
>
> Hi Stephen / Andrew, sorry for the annoyance, but can you please remove
> this patch from linux-next?
> 
> Today it shows as commit
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=d691651735f1
> - this commit is subject to concern from Baoquan and we are discussing
> better ways to achieve that, through a refactor.

Dropped from linux-next today.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

On 08/02/2022 18:39, Stephen Rothwell wrote:
> Hi Guilherme,
> [...] 
> Dropped from linux-next today.
> 

Hi Stephen, thanks! I'm still seeing this patch over there, though - I'm
not sure if takes a while to show up in the tree...

Notice this request is only for patch 3/3 in this series - patches 1 and
2 are fine, were reviewed and accepted =)

Cheers,


Guilherme

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 632 bytes --]

Hi Guilherme,

On Wed, 9 Feb 2022 12:06:03 -0300 "Guilherme G. Piccoli" <gpiccoli@igalia.com> wrote:
>
> On 08/02/2022 18:39, Stephen Rothwell wrote:
> > Hi Guilherme,
> > [...] 
> > Dropped from linux-next today.
> >   
> 
> Hi Stephen, thanks! I'm still seeing this patch over there, though - I'm
> not sure if takes a while to show up in the tree...

Andrew did another mmotm release which put it back in.  I have removed
it again for today.

> Notice this request is only for patch 3/3 in this series - patches 1 and
> 2 are fine, were reviewed and accepted =)

Understood.

-- 
Cheers,
Stephen Rothwell

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 3/3] panic: Allow printing extra panic information on kdump

[Guilherme G. Piccoli]

On 09/02/2022 20:26, Stephen Rothwell wrote:
> [...]
>> Hi Stephen, thanks! I'm still seeing this patch over there, though - I'm
>> not sure if takes a while to show up in the tree...
> 
> Andrew did another mmotm release which put it back in.  I have removed
> it again for today.
> 
>> Notice this request is only for patch 3/3 in this series - patches 1 and
>> 2 are fine, were reviewed and accepted =)
> 
> Understood.
> 

Thanks a bunch! So it seems Andrew needs to remove patch 3 from mmotm right?

Cheers,


Guilherme

Re: [PATCH 0/3] Some improvements on panic_print

[Guilherme G. Piccoli]

On 09/11/2021 17:28, Guilherme G. Piccoli wrote:
> Hey everybody, this is a mix of a documentation fix with some additions
> to the "panic_print" syscall / parameter. The goal here is being able
> to collect all CPUs backtraces during a panic event and also
> to enable "panic_print" in a kdump event - details of the reasoning
> and design choices in the patches.
> 
> Thanks in advance for reviews!
> Cheers,
> 
> 
> Guilherme
> 

Hi everybody, is there any feedback for this series?
Thanks in advance,


Guilherme

Re: [PATCH 0/3] Some improvements on panic_print

[Guilherme G. Piccoli]

On 26/11/2021 18:34, Guilherme G. Piccoli wrote:
> Hi everybody, is there any feedback for this series?
> Thanks in advance,
> 
> 
> Guilherme

Hey folks, this is a(nother) bi-weekly ping - if anybody has any
suggestions on how could we move forward with this series, that'd
greatly appreciated!

Thanks in advance,


Guilherme