[PATCH V6] panic: Move panic_print before kmsg dumpers

[PATCH V6] panic: Move panic_print before kmsg dumpers

[Guilherme G. Piccoli]

The panic_print setting allows users to collect more information in a
panic event, like memory stats, tasks, CPUs backtraces, etc.
This is an interesting debug mechanism, but currently the print event
happens *after* kmsg_dump(), meaning that pstore, for example, cannot
collect a dmesg with the panic_print extra information.

This patch changes that in 2 steps:

(a) The panic_print setting allows to replay the existing kernel log
buffer to the console (bit 5), besides the extra information dump.
This functionality makes sense only at the end of the panic() function.
So, we hereby allow to distinguish the two situations by a new boolean
parameter in the function panic_print_sys_info().

(b) With the above change, we can safely call panic_print_sys_info()
before kmsg_dump(), allowing to dump the extra information when using
pstore or other kmsg dumpers.

The additional messages from panic_print could overwrite the oldest
messages when the buffer is full. The only reasonable solution is to
use a large enough log buffer, hence we added an advice into the kernel
parameters documentation about that.

Cc: Feng Tang <feng.tang@intel.com>
Cc: Petr Mladek <pmladek@suse.com>
Acked-by: Baoquan He <bhe@redhat.com>
Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---


V6: Implemented a small suggestion from Baoquan in the commit
message; with that, added his Acked-By. (Thanks Baoquan!)
Notice that this is rebased against linux-next (next-20220211 branch).

V5: https://lore.kernel.org/lkml/20220211215539.822466-1-gpiccoli@igalia.com/

V4: https://lore.kernel.org/lkml/20220124203101.216051-1-gpiccoli@igalia.com/


Andrew, can we get that merged in the mm-tree, after getting [0]
removed from there? This one replaces [0]. Thanks!!

[0] https://ozlabs.org/~akpm/mmots/broken-out/panic-allow-printing-extra-panic-information-on-kdump.patch


 Documentation/admin-guide/kernel-parameters.txt |  4 ++++
 kernel/panic.c                                  | 13 +++++++++----
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 3c2b3e24e8f5..2cf7078eaa95 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3766,6 +3766,10 @@
 			bit 4: print ftrace buffer
 			bit 5: print all printk messages in buffer
 			bit 6: print all CPUs backtrace (if available in the arch)
+			*Be aware* that this option may print a _lot_ of lines,
+			so there are risks of losing older messages in the log.
+			Use this option carefully, maybe worth to setup a
+			bigger log buffer with "log_buf_len" along with this.
 
 	panic_on_taint=	Bitmask for conditionally calling panic() in add_taint()
 			Format: <hex>[,nousertaint]
diff --git a/kernel/panic.c b/kernel/panic.c
index 3c3fb36d8d41..eb4dfb932c85 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -148,10 +148,13 @@ void nmi_panic(struct pt_regs *regs, const char *msg)
 }
 EXPORT_SYMBOL(nmi_panic);
 
-static void panic_print_sys_info(void)
+static void panic_print_sys_info(bool console_flush)
 {
-	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
-		console_flush_on_panic(CONSOLE_REPLAY_ALL);
+	if (console_flush) {
+		if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
+			console_flush_on_panic(CONSOLE_REPLAY_ALL);
+		return;
+	}
 
 	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
 		trigger_all_cpu_backtrace();
@@ -286,6 +289,8 @@ void panic(const char *fmt, ...)
 	 */
 	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
 
+	panic_print_sys_info(false);
+
 	kmsg_dump(KMSG_DUMP_PANIC);
 
 	/*
@@ -316,7 +321,7 @@ void panic(const char *fmt, ...)
 	debug_locks_off();
 	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
 
-	panic_print_sys_info();
+	panic_print_sys_info(true);
 
 	if (!panic_blink)
 		panic_blink = no_blink;
-- 
2.35.0

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Petr Mladek]

On Mon 2022-02-14 11:13:09, Guilherme G. Piccoli wrote:
> The panic_print setting allows users to collect more information in a
> panic event, like memory stats, tasks, CPUs backtraces, etc.
> This is an interesting debug mechanism, but currently the print event
> happens *after* kmsg_dump(), meaning that pstore, for example, cannot
> collect a dmesg with the panic_print extra information.
> 
> This patch changes that in 2 steps:
> 
> (a) The panic_print setting allows to replay the existing kernel log
> buffer to the console (bit 5), besides the extra information dump.
> This functionality makes sense only at the end of the panic() function.
> So, we hereby allow to distinguish the two situations by a new boolean
> parameter in the function panic_print_sys_info().
> 
> (b) With the above change, we can safely call panic_print_sys_info()
> before kmsg_dump(), allowing to dump the extra information when using
> pstore or other kmsg dumpers.
> 
> The additional messages from panic_print could overwrite the oldest
> messages when the buffer is full. The only reasonable solution is to
> use a large enough log buffer, hence we added an advice into the kernel
> parameters documentation about that.
> 
> Cc: Feng Tang <feng.tang@intel.com>
> Cc: Petr Mladek <pmladek@suse.com>
> Acked-by: Baoquan He <bhe@redhat.com>
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

Makes sense and looks good to me.

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

Best Regards,
Petr

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

On (22/02/14 11:13), Guilherme G. Piccoli wrote:
> -static void panic_print_sys_info(void)
> +static void panic_print_sys_info(bool console_flush)
>  {
> -	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> -		console_flush_on_panic(CONSOLE_REPLAY_ALL);
> +	if (console_flush) {
> +		if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> +			console_flush_on_panic(CONSOLE_REPLAY_ALL);
> +		return;
> +	}

Yeah, if Petr is fine with that then I'm OK. But at the same time,
we have `panic_print' which is a bit mask of what panic_print_sys_info()
should do. And now we also have a boolean `console_flush` flag that tells
panic_print_sys_info() to ignore some (one as of now) bits of `panic_print'.

So _maybe_ panic_print_sys_info() can just accept panic_print as
its parameter and then we can do something like this (as an example)

	panic_print_sys_info(panic_print & ~PANIC_PRINT_ALL_PRINTK_MSG);


>  	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
>  		trigger_all_cpu_backtrace();
> @@ -286,6 +289,8 @@ void panic(const char *fmt, ...)
>  	 */
>  	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);

[..]

> +	panic_print_sys_info(false);

Merely because `panic_print_sys_info(false);` doesn't tell much to a reader.
Like what is print sys info false?

Or did you already discuss this?

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

On (22/02/14 11:13), Guilherme G. Piccoli wrote:
> 
> The additional messages from panic_print could overwrite the oldest
> messages when the buffer is full. The only reasonable solution is to
> use a large enough log buffer, hence we added an advice into the kernel
> parameters documentation about that.

By additional panic_print messages you mean that panic_print_sys_info()
will print everything (except PANIC_PRINT_ALL_PRINTK_MSG) twice?

Do we really need to dump everything twice? show_mem(), show_state(),
ftrace_dump(DUMP_ALL). That's quite a bit of extra data.

Can instead this be something like (?):

@@ -286,6 +289,8 @@ void panic(const char *fmt, ...)
 	 */
 	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);


+	panic_print_sys_info(panic_print & ~PANIC_PRINT_ALL_PRINTK_MSG);


 	kmsg_dump(KMSG_DUMP_PANIC);
  
 	/*
@@ -316,7 +321,7 @@ void panic(const char *fmt, ...)
 	debug_locks_off();
 	console_flush_on_panic(CONSOLE_FLUSH_PENDING);


+	panic_print_sys_info(PANIC_PRINT_ALL_PRINTK_MSG);


 	if (!panic_blink)
 		panic_blink = no_blink;

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

A trivial typo:

On (22/02/22 11:06), Sergey Senozhatsky wrote:
> @@ -286,6 +289,8 @@ void panic(const char *fmt, ...)
>  	 */
>  	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
> 
> 
> +	panic_print_sys_info(panic_print & ~PANIC_PRINT_ALL_PRINTK_MSG);
> 
> 
>  	kmsg_dump(KMSG_DUMP_PANIC);
>   
>  	/*
> @@ -316,7 +321,7 @@ void panic(const char *fmt, ...)
>  	debug_locks_off();
>  	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> 

+	panic_print_sys_info(panic_print & PANIC_PRINT_ALL_PRINTK_MSG);



I also wonder if we want to CONSOLE_FLUSH_PENDING when
PANIC_PRINT_ALL_PRINTK_MSG set.

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Guilherme G. Piccoli]

On 21/02/2022 22:45, Sergey Senozhatsky wrote:
> [...]
> Yeah, if Petr is fine with that then I'm OK. But at the same time,
> we have `panic_print' which is a bit mask of what panic_print_sys_info()
> should do. And now we also have a boolean `console_flush` flag that tells
> panic_print_sys_info() to ignore some (one as of now) bits of `panic_print'.
> 
> So _maybe_ panic_print_sys_info() can just accept panic_print as
> its parameter and then we can do something like this (as an example)
> 
> 	panic_print_sys_info(panic_print & ~PANIC_PRINT_ALL_PRINTK_MSG);
> 
> 
>>  	if (panic_print & PANIC_PRINT_ALL_CPU_BT)
>>  		trigger_all_cpu_backtrace();
>> @@ -286,6 +289,8 @@ void panic(const char *fmt, ...)
>>  	 */
>>  	atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
> 
> [..]
> 
>> +	panic_print_sys_info(false);
> 
> Merely because `panic_print_sys_info(false);` doesn't tell much to a reader.
> Like what is print sys info false?
> 
> Or did you already discuss this?

Hi Sergey, thanks for your feedback. So, personally I prefer having the
flag - for me it's clear, it's just a matter of reading the prototype -
either we print the info _or_ we console_flush.

But let's see if others have a preference - if the preference is to use
the bitmask as you suggest, we can do it in a next version.

Cheers,


Guilherme

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Guilherme G. Piccoli]

On 21/02/2022 23:06, Sergey Senozhatsky wrote:
> On (22/02/14 11:13), Guilherme G. Piccoli wrote:
> [...]
> By additional panic_print messages you mean that panic_print_sys_info()
> will print everything (except PANIC_PRINT_ALL_PRINTK_MSG) twice?
> 
> Do we really need to dump everything twice? show_mem(), show_state(),
> ftrace_dump(DUMP_ALL). That's quite a bit of extra data.
> 

Oh no, we don't print everything twice, that'd be insane heh

The patch only anticipates the call site for the panic_print_sys_info()
- but as discussed in the first iterations, we couldn't just bring it
earlier due to the console replay thing. Hence, we needed to split
calls...the first call dumps the information, the 2nd call only does the
console replaying if that is set.

Cheers,


Guilherme

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

On (22/02/22 11:10), Guilherme G. Piccoli wrote:
> On 21/02/2022 23:06, Sergey Senozhatsky wrote:
> > On (22/02/14 11:13), Guilherme G. Piccoli wrote:
> > [...]
> > By additional panic_print messages you mean that panic_print_sys_info()
> > will print everything (except PANIC_PRINT_ALL_PRINTK_MSG) twice?
> > 
> > Do we really need to dump everything twice? show_mem(), show_state(),
> > ftrace_dump(DUMP_ALL). That's quite a bit of extra data.
> > 
> 
> Oh no, we don't print everything twice, that'd be insane heh

My bad! I did not spot the `return` at the end of the new branch.

+       if (console_flush) {
+               if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
+                       console_flush_on_panic(CONSOLE_REPLAY_ALL);
+               return;
+       }

Hmm. Yeah, well, that's a bit of a tricky interface now

	panic()
		// everything (if corresponding bits set), no console flush
		panic_print_sys_info(false)
		...
		// console flush only if corresponding bit set
		panic_print_sys_info(true)



If everyone is fine then OK.

But I _personally_ would look into changing this to something like this:

	#define EARLY_PANIC_MASK (PANIC_PRINT_FOO | PANIC_PRINT_BAR | ...)
	#define LATE_PANIC_MASK (PANIC_PRINT_ALL_PRINTK_MSG)
	panic()
		panic_print_sys_info(panic_print & EARLY_PANIC_MASK)
		...
		panic_print_sys_info(panic_print & LATE_PANIC_MASK)

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

On (22/02/22 11:08), Guilherme G. Piccoli wrote:
> Hi Sergey, thanks for your feedback. So, personally I prefer having the
> flag - for me it's clear, it's just a matter of reading the prototype -
> either we print the info _or_ we console_flush.
> 
> But let's see if others have a preference - if the preference is to use
> the bitmask as you suggest, we can do it in a next version.

Sounds good to me, thanks.

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

On (22/02/15 17:14), Petr Mladek wrote:
> Makes sense and looks good to me.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

FWIW

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Guilherme G. Piccoli]

On 23/02/2022 08:41, Sergey Senozhatsky wrote:
> On (22/02/15 17:14), Petr Mladek wrote:
>> Makes sense and looks good to me.
>>
>> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> FWIW
> 
> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Thanks a lot Sergey, for your review =)

Andrew, do I need to send a V7 with the above tag or this is
incorporated when patch gets into your tree?

Thanks!

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Guilherme G. Piccoli]

On 22/02/2022 22:27, Sergey Senozhatsky wrote:
> [...]
> Hmm. Yeah, well, that's a bit of a tricky interface now
> 
> 	panic()
> 		// everything (if corresponding bits set), no console flush
> 		panic_print_sys_info(false)
> 		...
> 		// console flush only if corresponding bit set
> 		panic_print_sys_info(true)
> 
> 
> 
> If everyone is fine then OK.
> 
> But I _personally_ would look into changing this to something like this:
> 
> 	#define EARLY_PANIC_MASK (PANIC_PRINT_FOO | PANIC_PRINT_BAR | ...)
> 	#define LATE_PANIC_MASK (PANIC_PRINT_ALL_PRINTK_MSG)
> 	panic()
> 		panic_print_sys_info(panic_print & EARLY_PANIC_MASK)
> 		...
> 		panic_print_sys_info(panic_print & LATE_PANIC_MASK)

Hi Sergey, notice that panic_print_sys_info() currently doesn't have a
parameter! The prototype (without this patch) is:

static void panic_print_sys_info(void);

So, it consumes the "panic_print" global variable (which matches the
command-line parameter / sysctl), hence to implement your suggestion
either we need a refactor in panic_print_sys_info(), adding a parameter
(more or less what the patch is already doing, but with a bit more
changes) or we override the global variable twice in panic(), before the
function calls.

As you said, it's possible and a matter of personal coding style. I'd be
fine if more people ask that, but if everyone is fine with the current
implementation, I'd rather get this patch merged as is, since we need it
and couldn't even make it for 5.17 heh

Cheers,


Guilherme

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

Hi,

On (22/02/23 10:15), Guilherme G. Piccoli wrote:
> On 22/02/2022 22:27, Sergey Senozhatsky wrote:
> > [...]
> > Hmm. Yeah, well, that's a bit of a tricky interface now
> > 
> > 	panic()
> > 		// everything (if corresponding bits set), no console flush
> > 		panic_print_sys_info(false)
> > 		...
> > 		// console flush only if corresponding bit set
> > 		panic_print_sys_info(true)
> > 
> > 
> > 
> > If everyone is fine then OK.
> > 
> > But I _personally_ would look into changing this to something like this:
> > 
> > 	#define EARLY_PANIC_MASK (PANIC_PRINT_FOO | PANIC_PRINT_BAR | ...)
> > 	#define LATE_PANIC_MASK (PANIC_PRINT_ALL_PRINTK_MSG)
> > 	panic()
> > 		panic_print_sys_info(panic_print & EARLY_PANIC_MASK)
> > 		...
> > 		panic_print_sys_info(panic_print & LATE_PANIC_MASK)
> 
> Hi Sergey, notice that panic_print_sys_info() currently doesn't have a
> parameter! The prototype (without this patch) is:

Correct.

> static void panic_print_sys_info(void);
>
> So, it consumes the "panic_print" global variable (which matches the
> command-line parameter / sysctl), hence to implement your suggestion
> either we need a refactor in panic_print_sys_info(), adding a parameter

Correct. That's the idea. Since you are already adding a parameter,
what I'm talking is turning that parameter from true/false to something
more verbose.

> (more or less what the patch is already doing, but with a bit more
> changes) or we override the global variable twice in panic(), before the
> function calls.

We don't need to overwrite the global var. We pass "permitted bits at
this stage of panic" mask to panic_print_sys_info(). The global var
stays intact.

> As you said, it's possible and a matter of personal coding style. I'd be
> fine if more people ask that, but if everyone is fine with the current
> implementation, I'd rather get this patch merged as is, since we need it
> and couldn't even make it for 5.17 heh

Sure, works for me.

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Petr Mladek]

On Wed 2022-02-23 01:27:35, Sergey Senozhatsky wrote:
> On (22/02/22 11:10), Guilherme G. Piccoli wrote:
> > On 21/02/2022 23:06, Sergey Senozhatsky wrote:
> > > On (22/02/14 11:13), Guilherme G. Piccoli wrote:
> > > [...]
> > > By additional panic_print messages you mean that panic_print_sys_info()
> > > will print everything (except PANIC_PRINT_ALL_PRINTK_MSG) twice?
> > > 
> > > Do we really need to dump everything twice? show_mem(), show_state(),
> > > ftrace_dump(DUMP_ALL). That's quite a bit of extra data.
> > > 
> > 
> > Oh no, we don't print everything twice, that'd be insane heh
> 
> My bad! I did not spot the `return` at the end of the new branch.
> 
> +       if (console_flush) {
> +               if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> +                       console_flush_on_panic(CONSOLE_REPLAY_ALL);
> +               return;
> +       }
> 
> Hmm. Yeah, well, that's a bit of a tricky interface now
> 
> 	panic()
> 		// everything (if corresponding bits set), no console flush
> 		panic_print_sys_info(false)
> 		...
> 		// console flush only if corresponding bit set
> 		panic_print_sys_info(true)

I agree that self-explaining names are always better than true/false.
It is pity that replay the log is handled in panic_print at all.

I sometimes hide these tricks into wrappers. We could rename:

    panic_printk_sys_info() -> panic_print_handler()

and add wrappers:

void panic_print_sys_info()
{
	panic_printk_handler(false);
}

void panic_print_log_replay()
{
	panic_printk_handler(true);
}

Or just split panic_printk_sys_info() into these two functions.

> If everyone is fine then OK.
> 
> But I _personally_ would look into changing this to something like this:
> 
> 	#define EARLY_PANIC_MASK (PANIC_PRINT_FOO | PANIC_PRINT_BAR | ...)
> 	#define LATE_PANIC_MASK (PANIC_PRINT_ALL_PRINTK_MSG)

These lists cause merge and backporting conflicts. I vote to avoid
this approach ;-)


> 	panic()
> 		panic_print_sys_info(panic_print & EARLY_PANIC_MASK)
> 		...
> 		panic_print_sys_info(panic_print & LATE_PANIC_MASK)

That said, I could live with the patch as is. I leave the decision
to Andrew. Feel free to use:

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

Best Regards,
Petr

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Guilherme G. Piccoli]

On 24/02/2022 11:33, Petr Mladek wrote:
> [...]
> That said, I could live with the patch as is. I leave the decision
> to Andrew. Feel free to use:
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> Best Regards,
> Petr

Thanks a bunch Petr and Sergey for the reviews (and the tags).

I also vote to keep the patch as-is and get it merged, maybe eventually
improving that along with the complex panic notifiers task [0]. I could
definitely do it over there...

Andrew, is this fine for you?
Thanks,


Guilherme

[0] https://lore.kernel.org/lkml/YfPxvzSzDLjO5ldp@alley/

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

On (22/02/24 15:33), Petr Mladek wrote:
> > My bad! I did not spot the `return` at the end of the new branch.
> > 
> > +       if (console_flush) {
> > +               if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> > +                       console_flush_on_panic(CONSOLE_REPLAY_ALL);
> > +               return;
> > +       }
> > 
> > Hmm. Yeah, well, that's a bit of a tricky interface now
> > 
> > 	panic()
> > 		// everything (if corresponding bits set), no console flush
> > 		panic_print_sys_info(false)
> > 		...
> > 		// console flush only if corresponding bit set
> > 		panic_print_sys_info(true)
> 
> I agree that self-explaining names are always better than true/false.
> It is pity that replay the log is handled in panic_print at all.
> 
> I sometimes hide these tricks into wrappers. We could rename:
> 
>     panic_printk_sys_info() -> panic_print_handler()
> 
> and add wrappers:
> 
> void panic_print_sys_info()
> {
> 	panic_printk_handler(false);
> }
> 
> void panic_print_log_replay()
> {
> 	panic_printk_handler(true);
> }
> 
> Or just split panic_printk_sys_info() into these two functions.

Agreed. I also tend to think that panic_printk_sys_info() is needed anyway,
just because now we do

	debug_locks_off();
	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
		console_flush_on_panic(CONSOLE_REPLAY_ALL);

It probably would be better if we do

	debug_locks_off();
	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
		console_flush_on_panic(CONSOLE_REPLAY_ALL);
	else
		console_flush_on_panic(CONSOLE_FLUSH_PENDING);

instead.

IOW move console_flush_on_panic() handling out of panic_print_sys_info().
console_flush_on_panic() isn't really related to "print sys info" stuff
that panic_print_sys_info() does.

Something like this may be:

---
 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();
 
@@ -196,6 +193,23 @@ static void panic_print_sys_info(void)
 		ftrace_dump(DUMP_ALL);
 }
 
+static void panic_console_flush(void)
+{
+	/*
+	 * We may have ended up stopping the CPU holding the lock (in
+	 * smp_send_stop()) while still having some valuable data in the console
+	 * buffer.  Try to acquire the lock then release it regardless of the
+	 * result.  The release will also print the buffers out.  Locks debug
+	 * should be disabled to avoid reporting bad unlock balance when
+	 * panic() is not being callled from OOPS.
+	 */
+	debug_locks_off();
+	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
+		console_flush_on_panic(CONSOLE_REPLAY_ALL);
+	else
+		console_flush_on_panic(CONSOLE_FLUSH_PENDING);
+}
+
 /**
  *	panic - halt the system
  *	@fmt: The text string to print
@@ -329,17 +343,7 @@ void panic(const char *fmt, ...)
 #endif
 	console_unblank();
 
-	/*
-	 * We may have ended up stopping the CPU holding the lock (in
-	 * smp_send_stop()) while still having some valuable data in the console
-	 * buffer.  Try to acquire the lock then release it regardless of the
-	 * result.  The release will also print the buffers out.  Locks debug
-	 * should be disabled to avoid reporting bad unlock balance when
-	 * panic() is not being callled from OOPS.
-	 */
-	debug_locks_off();
-	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
-
+	panic_console_flush();
 	panic_print_sys_info();
 
 	if (!panic_blink)
---

> > If everyone is fine then OK.
> > 
> > But I _personally_ would look into changing this to something like this:
> > 
> > 	#define EARLY_PANIC_MASK (PANIC_PRINT_FOO | PANIC_PRINT_BAR | ...)
> > 	#define LATE_PANIC_MASK (PANIC_PRINT_ALL_PRINTK_MSG)
> 
> These lists cause merge and backporting conflicts. I vote to avoid
> this approach ;-)

OK :)

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

Correction:

On (22/02/25 14:18), Sergey Senozhatsky wrote:
> > 
> > Or just split panic_printk_sys_info() into these two functions.
> 
> Agreed. I also tend to think that panic_printk_sys_info() is needed anyway,

							^^^^ split

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Petr Mladek]

On Fri 2022-02-25 14:18:22, Sergey Senozhatsky wrote:
> IOW move console_flush_on_panic() handling out of panic_print_sys_info().
> console_flush_on_panic() isn't really related to "print sys info" stuff
> that panic_print_sys_info() does.
> 
> Something like this may be:
> 
>  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();
>  
> @@ -196,6 +193,23 @@ static void panic_print_sys_info(void)
>  		ftrace_dump(DUMP_ALL);
>  }
>  
> +static void panic_console_flush(void)
> +{
> +	/*
> +	 * We may have ended up stopping the CPU holding the lock (in
> +	 * smp_send_stop()) while still having some valuable data in the console
> +	 * buffer.  Try to acquire the lock then release it regardless of the
> +	 * result.  The release will also print the buffers out.  Locks debug
> +	 * should be disabled to avoid reporting bad unlock balance when
> +	 * panic() is not being callled from OOPS.
> +	 */
> +	debug_locks_off();
> +	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> +		console_flush_on_panic(CONSOLE_REPLAY_ALL);
> +	else
> +		console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> +}
> +
>  /**
>   *	panic - halt the system
>   *	@fmt: The text string to print
> @@ -329,17 +343,7 @@ void panic(const char *fmt, ...)
>  #endif
>  	console_unblank();
>  
> -	/*
> -	 * We may have ended up stopping the CPU holding the lock (in
> -	 * smp_send_stop()) while still having some valuable data in the console
> -	 * buffer.  Try to acquire the lock then release it regardless of the
> -	 * result.  The release will also print the buffers out.  Locks debug
> -	 * should be disabled to avoid reporting bad unlock balance when
> -	 * panic() is not being callled from OOPS.
> -	 */
> -	debug_locks_off();
> -	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> -
> +	panic_console_flush();
>  	panic_print_sys_info();
>  
>  	if (!panic_blink)

The result looks good to me. We could do it as a followup patch.

Best Regards,
Petr

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Sergey Senozhatsky]

On (22/02/25 13:16), Petr Mladek wrote:
> 
> The result looks good to me. We could do it as a followup patch.
> 

Yup, sounds good to me. Thanks.

Re: [PATCH V6] panic: Move panic_print before kmsg dumpers

[Guilherme G. Piccoli]

On 25/02/2022 23:23, Sergey Senozhatsky wrote:
> On (22/02/25 13:16), Petr Mladek wrote:
>>
>> The result looks good to me. We could do it as a followup patch.
> 
> Yup, sounds good to me. Thanks.

Cool, I also agree.

So Andrew: is there anything missing in order to get this patch merged?
If you prefer, I can send a new version...with the reviews' tags.
Lemme know =D

Thanks,


Guilherme