panic_print was introduced to help debugging kernel panic by dumping
different kinds of system information like tasks' call stack, memory,
ftrace buffer etc. Acutually this function could help debugging cases
like task-hung, soft/hard lockup too, where user may need the snapshot
of system info at that time.

Extract sys_show_info() function out to be used by other kernel parts
for debugging.

Signed-off-by: Feng Tang <feng.tang@linux.alibaba.com>
---
 include/linux/panic.h | 11 ++++++++++
 kernel/panic.c        | 47 +++++++++++++++++++++++--------------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/include/linux/panic.h b/include/linux/panic.h
index 2494d51707ef..bb1796b64381 100644
--- a/include/linux/panic.h
+++ b/include/linux/panic.h
@@ -16,6 +16,17 @@ extern void oops_enter(void);
 extern void oops_exit(void);
 extern bool oops_may_print(void);
 
+#define SYS_PRINT_TASK_INFO		0x00000001
+#define SYS_PRINT_MEM_INFO		0x00000002
+#define SYS_PRINT_TIMER_INFO		0x00000004
+#define SYS_PRINT_LOCK_INFO		0x00000008
+#define SYS_PRINT_FTRACE_INFO		0x00000010
+#define SYS_PRINT_ALL_PRINTK_MSG	0x00000020
+#define SYS_PRINT_ALL_CPU_BT		0x00000040
+#define SYS_PRINT_BLOCKED_TASKS		0x00000080
+
+extern void sys_show_info(unsigned long info_mask);
+
 extern bool panic_triggering_all_cpu_backtrace;
 extern int panic_timeout;
 extern unsigned long panic_print;
diff --git a/kernel/panic.c b/kernel/panic.c
index a3889f38153d..2542ae3702f9 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -69,14 +69,6 @@ bool panic_triggering_all_cpu_backtrace;
 int panic_timeout = CONFIG_PANIC_TIMEOUT;
 EXPORT_SYMBOL_GPL(panic_timeout);
 
-#define PANIC_PRINT_TASK_INFO		0x00000001
-#define PANIC_PRINT_MEM_INFO		0x00000002
-#define PANIC_PRINT_TIMER_INFO		0x00000004
-#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
-#define PANIC_PRINT_BLOCKED_TASKS	0x00000080
 unsigned long panic_print;
 
 ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
@@ -208,33 +200,44 @@ void nmi_panic(struct pt_regs *regs, const char *msg)
 }
 EXPORT_SYMBOL(nmi_panic);
 
-static void panic_print_sys_info(bool console_flush)
+void sys_show_info(unsigned long info_mask)
 {
-	if (console_flush) {
-		if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
-			console_flush_on_panic(CONSOLE_REPLAY_ALL);
-		return;
-	}
-
-	if (panic_print & PANIC_PRINT_TASK_INFO)
+	if (info_mask & SYS_PRINT_TASK_INFO)
 		show_state();
 
-	if (panic_print & PANIC_PRINT_MEM_INFO)
+	if (info_mask & SYS_PRINT_MEM_INFO)
 		show_mem();
 
-	if (panic_print & PANIC_PRINT_TIMER_INFO)
+	if (info_mask & SYS_PRINT_TIMER_INFO)
 		sysrq_timer_list_show();
 
-	if (panic_print & PANIC_PRINT_LOCK_INFO)
+	if (info_mask & SYS_PRINT_LOCK_INFO)
 		debug_show_all_locks();
 
-	if (panic_print & PANIC_PRINT_FTRACE_INFO)
+	if (info_mask & SYS_PRINT_FTRACE_INFO)
 		ftrace_dump(DUMP_ALL);
 
-	if (panic_print & PANIC_PRINT_BLOCKED_TASKS)
+	if (panic_print & SYS_PRINT_ALL_PRINTK_MSG)
+		console_flush_on_panic(CONSOLE_REPLAY_ALL);
+
+	if (info_mask & SYS_PRINT_ALL_CPU_BT)
+		trigger_all_cpu_backtrace();
+
+	if (info_mask & SYS_PRINT_BLOCKED_TASKS)
 		show_state_filter(TASK_UNINTERRUPTIBLE);
 }
 
+static void panic_print_sys_info(bool console_flush)
+{
+	if (console_flush) {
+		if (panic_print & SYS_PRINT_ALL_PRINTK_MSG)
+			console_flush_on_panic(CONSOLE_REPLAY_ALL);
+		return;
+	}
+
+	sys_show_info(panic_print & ~SYS_PRINT_ALL_PRINTK_MSG);
+}
+
 void check_panic_on_warn(const char *origin)
 {
 	unsigned int limit;
@@ -255,7 +258,7 @@ void check_panic_on_warn(const char *origin)
  */
 static void panic_other_cpus_shutdown(bool crash_kexec)
 {
-	if (panic_print & PANIC_PRINT_ALL_CPU_BT) {
+	if (panic_print & SYS_PRINT_ALL_CPU_BT) {
 		/* Temporary allow non-panic CPUs to write their backtraces. */
 		panic_triggering_all_cpu_backtrace = true;
 		trigger_all_cpu_backtrace();
-- 
2.39.5 (Apple Git-154)

On Sun 2025-05-11 16:52:52, Feng Tang wrote:
> panic_print was introduced to help debugging kernel panic by dumping
> different kinds of system information like tasks' call stack, memory,
> ftrace buffer etc. Acutually this function could help debugging cases
> like task-hung, soft/hard lockup too, where user may need the snapshot
> of system info at that time.
> 
> Extract sys_show_info() function out to be used by other kernel parts
> for debugging.
> 
> --- a/include/linux/panic.h
> +++ b/include/linux/panic.h
> @@ -16,6 +16,17 @@ extern void oops_enter(void);
>  extern void oops_exit(void);
>  extern bool oops_may_print(void);
>  
> +#define SYS_PRINT_TASK_INFO		0x00000001
> +#define SYS_PRINT_MEM_INFO		0x00000002
> +#define SYS_PRINT_TIMER_INFO		0x00000004
> +#define SYS_PRINT_LOCK_INFO		0x00000008
> +#define SYS_PRINT_FTRACE_INFO		0x00000010
> +#define SYS_PRINT_ALL_PRINTK_MSG	0x00000020

Please, remove this option from the generic interface. It is
controversial. In the current form, it makes sense to replay
the log only in panic() after all the other actions:

	console_verbose();
	bust_spinlocks(1);
	panic_other_cpus_shutdown(_crash_kexec_post_notifiers);
	printk_legacy_allow_panic_sync();
	console_unblank();
	debug_locks_off();
	console_flush_on_panic(CONSOLE_FLUSH_PENDING);

All these operations have some (side-)effect which increases
the chance to actually see the messages on the console.

I think that it was primary meant for graphics consoles. But there
it would need to be used together with printk_delay
(/proc/sys/kernel/printk_delay, sysctl printk_delay).

Note that it creates a hairy code. It is the only reason why we need to
call panic_print_sys_info() twice with "false" and "true"
parameter.


That said, I could imagine a generic use, for example, after forcing
ignore_loglevel or so. Otherwise, I do not see any advantage in
flushing the same messages again, for example, on serial or network
consoles where they are likely already stored. We could add this
later when there is a real-life demand.


> +#define SYS_PRINT_ALL_CPU_BT		0x00000040
> +#define SYS_PRINT_BLOCKED_TASKS		0x00000080

The generic approach might deserve a separate source file,
for example:

    include/linux/sys_info.h
    lib/sys_info.c

Also I always considered the bitmask as a horrible user interface.
It might be used internally. But it would be better to allow a human
readable parameter, for example:

	panic_sys_info=task,mem,timer,lock,ftrace,bt,all_bt,blocked_tasks

The console reply might be handled by a separate:

	panic_console_reply=1

And it would obsolete the existing "panic_print" which is an
ugly name and interface from my POV.

> +extern void sys_show_info(unsigned long info_mask);
> +
>  extern bool panic_triggering_all_cpu_backtrace;
>  extern int panic_timeout;
>  extern unsigned long panic_print;
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -208,33 +200,44 @@ void nmi_panic(struct pt_regs *regs, const char *msg)
>  }
>  EXPORT_SYMBOL(nmi_panic);
>  
> -static void panic_print_sys_info(bool console_flush)
> +void sys_show_info(unsigned long info_mask)
>  {
> -	if (console_flush) {
> -		if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> -			console_flush_on_panic(CONSOLE_REPLAY_ALL);
> -		return;
> -	}
> -
> -	if (panic_print & PANIC_PRINT_TASK_INFO)
> +	if (info_mask & SYS_PRINT_TASK_INFO)
>  		show_state();
>  
> -	if (panic_print & PANIC_PRINT_MEM_INFO)
> +	if (info_mask & SYS_PRINT_MEM_INFO)
>  		show_mem();
>  
> -	if (panic_print & PANIC_PRINT_TIMER_INFO)
> +	if (info_mask & SYS_PRINT_TIMER_INFO)
>  		sysrq_timer_list_show();
>  
> -	if (panic_print & PANIC_PRINT_LOCK_INFO)
> +	if (info_mask & SYS_PRINT_LOCK_INFO)
>  		debug_show_all_locks();
>  
> -	if (panic_print & PANIC_PRINT_FTRACE_INFO)
> +	if (info_mask & SYS_PRINT_FTRACE_INFO)
>  		ftrace_dump(DUMP_ALL);
>  
> -	if (panic_print & PANIC_PRINT_BLOCKED_TASKS)
> +	if (panic_print & SYS_PRINT_ALL_PRINTK_MSG)
> +		console_flush_on_panic(CONSOLE_REPLAY_ALL);

I do not see any advantage in replaying the log at this stage.
It might make sense to replay the messages printed before
the critical situation. But it makes sense only when they
might be filtered/blocked before and can be seen now (unblanked
consoles, forced  ignore_loglevel, or so).

I would keep this in the bitmap for backward compactibility.
But I would ignore it my the generic print_sys_info().

And I would replace panic_print_sys_info(true) call with:

static void panic_console_replay(void)
{
	if (panic_print & SYS_PRINT_ALL_PRINTK_MSG)
		console_flush_on_panic(CONSOLE_REPLAY_ALL);
}

> +	if (info_mask & SYS_PRINT_ALL_CPU_BT)
> +		trigger_all_cpu_backtrace();
> +
> +	if (info_mask & SYS_PRINT_BLOCKED_TASKS)
>  		show_state_filter(TASK_UNINTERRUPTIBLE);
>  }

Best Regards,
Petr

Hi Petr,

Thanks for the review!

On Tue, May 13, 2025 at 11:57:19AM +0200, Petr Mladek wrote:
> On Sun 2025-05-11 16:52:52, Feng Tang wrote:
> > panic_print was introduced to help debugging kernel panic by dumping
> > different kinds of system information like tasks' call stack, memory,
> > ftrace buffer etc. Acutually this function could help debugging cases
> > like task-hung, soft/hard lockup too, where user may need the snapshot
> > of system info at that time.
> > 
> > Extract sys_show_info() function out to be used by other kernel parts
> > for debugging.
> > 
> > --- a/include/linux/panic.h
> > +++ b/include/linux/panic.h
> > @@ -16,6 +16,17 @@ extern void oops_enter(void);
> >  extern void oops_exit(void);
> >  extern bool oops_may_print(void);
> >  
> > +#define SYS_PRINT_TASK_INFO		0x00000001
> > +#define SYS_PRINT_MEM_INFO		0x00000002
> > +#define SYS_PRINT_TIMER_INFO		0x00000004
> > +#define SYS_PRINT_LOCK_INFO		0x00000008
> > +#define SYS_PRINT_FTRACE_INFO		0x00000010
> > +#define SYS_PRINT_ALL_PRINTK_MSG	0x00000020
> 
> Please, remove this option from the generic interface. It is
> controversial. In the current form, it makes sense to replay
> the log only in panic() after all the other actions:
> 
> 	console_verbose();
> 	bust_spinlocks(1);
> 	panic_other_cpus_shutdown(_crash_kexec_post_notifiers);
> 	printk_legacy_allow_panic_sync();
> 	console_unblank();
> 	debug_locks_off();
> 	console_flush_on_panic(CONSOLE_FLUSH_PENDING);

OK, will do.

In my RFC version, I did reserved it for panic use only :) and
added the support to general case in v1 after some hesitation. 

https://lore.kernel.org/lkml/20250507104322.30700-2-feng.tang@linux.alibaba.com/

> 
> All these operations have some (side-)effect which increases
> the chance to actually see the messages on the console.
> 
> I think that it was primary meant for graphics consoles. But there
> it would need to be used together with printk_delay
> (/proc/sys/kernel/printk_delay, sysctl printk_delay).
> 
> Note that it creates a hairy code. It is the only reason why we need to
> call panic_print_sys_info() twice with "false" and "true"
> parameter.

Yes.

> 
> That said, I could imagine a generic use, for example, after forcing
> ignore_loglevel or so. Otherwise, I do not see any advantage in
> flushing the same messages again, for example, on serial or network
> consoles where they are likely already stored. We could add this
> later when there is a real-life demand.

You are right, one main usage is for some product which has only graphics
console and no serial one. And we also used for serial console sometimes,
when the console have a lot of user space message mixed with kernel ones,
and we'd like to see the full clean kernel messages.

> 
> > +#define SYS_PRINT_ALL_CPU_BT		0x00000040
> > +#define SYS_PRINT_BLOCKED_TASKS		0x00000080
> 
> The generic approach might deserve a separate source file,
> for example:
> 
>     include/linux/sys_info.h
>     lib/sys_info.c

Thanks for the suggestion! I'm really not good at naming. 

As panic.c is always built-in, I'll put sys_info.c as obj-y.

> Also I always considered the bitmask as a horrible user interface.
> It might be used internally. But it would be better to allow a human
> readable parameter, for example:
> 
> 	panic_sys_info=task,mem,timer,lock,ftrace,bt,all_bt,blocked_tasks

Yes, it's convenient for developers, as a cmdline parameter being parsed
at boot time.

But I think bitmask may be easier for runtime changing as a core parameter
under /proc/ or /sys/, or from sysctl interface. There are also some other
modules use debug bitmask controlling printking info for different
sub-components.

And we have similar control knobs for hung, lockup etc.

Or should I change the name from 'xxx_print_mask' to 'xxx_sysinfo_flag'
in patch 2/3 ?

> 
> The console reply might be handled by a separate:
> 
> 	panic_console_reply=1
> 
> And it would obsolete the existing "panic_print" which is an
> ugly name and interface from my POV.

Agree it's ugly :). But besides a kernel parameter,  'panic_print' is
also a sysctl interface, I'm afraid renaming it might break user ABI.

> > +extern void sys_show_info(unsigned long info_mask);
> > +
> >  extern bool panic_triggering_all_cpu_backtrace;
> >  extern int panic_timeout;
> >  extern unsigned long panic_print;
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -208,33 +200,44 @@ void nmi_panic(struct pt_regs *regs, const char *msg)
> >  }
> >  EXPORT_SYMBOL(nmi_panic);
> >  
> > -static void panic_print_sys_info(bool console_flush)
> > +void sys_show_info(unsigned long info_mask)
> >  {
> > -	if (console_flush) {
> > -		if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
> > -			console_flush_on_panic(CONSOLE_REPLAY_ALL);
> > -		return;
> > -	}
> > -
> > -	if (panic_print & PANIC_PRINT_TASK_INFO)
> > +	if (info_mask & SYS_PRINT_TASK_INFO)
> >  		show_state();
> >  
> > -	if (panic_print & PANIC_PRINT_MEM_INFO)
> > +	if (info_mask & SYS_PRINT_MEM_INFO)
> >  		show_mem();
> >  
> > -	if (panic_print & PANIC_PRINT_TIMER_INFO)
> > +	if (info_mask & SYS_PRINT_TIMER_INFO)
> >  		sysrq_timer_list_show();
> >  
> > -	if (panic_print & PANIC_PRINT_LOCK_INFO)
> > +	if (info_mask & SYS_PRINT_LOCK_INFO)
> >  		debug_show_all_locks();
> >  
> > -	if (panic_print & PANIC_PRINT_FTRACE_INFO)
> > +	if (info_mask & SYS_PRINT_FTRACE_INFO)
> >  		ftrace_dump(DUMP_ALL);
> >  
> > -	if (panic_print & PANIC_PRINT_BLOCKED_TASKS)
> > +	if (panic_print & SYS_PRINT_ALL_PRINTK_MSG)
> > +		console_flush_on_panic(CONSOLE_REPLAY_ALL);
> 
> I do not see any advantage in replaying the log at this stage.
> It might make sense to replay the messages printed before
> the critical situation. But it makes sense only when they
> might be filtered/blocked before and can be seen now (unblanked
> consoles, forced  ignore_loglevel, or so).
> 
> I would keep this in the bitmap for backward compactibility.
> But I would ignore it my the generic print_sys_info().

OK.

> And I would replace panic_print_sys_info(true) call with:
> 
> static void panic_console_replay(void)
> {
> 	if (panic_print & SYS_PRINT_ALL_PRINTK_MSG)
> 		console_flush_on_panic(CONSOLE_REPLAY_ALL);
> }

Nice cleanup! Will do. I'll fold this change with this patch.

Thanks,
Feng

> 
> > +	if (info_mask & SYS_PRINT_ALL_CPU_BT)
> > +		trigger_all_cpu_backtrace();
> > +
> > +	if (info_mask & SYS_PRINT_BLOCKED_TASKS)
> >  		show_state_filter(TASK_UNINTERRUPTIBLE);
> >  }
> 
> Best Regards,
> Petr

On Tue 2025-05-13 21:23:25, Feng Tang wrote:
> Hi Petr,
> 
> Thanks for the review!
> 
> On Tue, May 13, 2025 at 11:57:19AM +0200, Petr Mladek wrote:
> > On Sun 2025-05-11 16:52:52, Feng Tang wrote:
> > > panic_print was introduced to help debugging kernel panic by dumping
> > > different kinds of system information like tasks' call stack, memory,
> > > ftrace buffer etc. Acutually this function could help debugging cases
> > > like task-hung, soft/hard lockup too, where user may need the snapshot
> > > of system info at that time.
> > > 
> > The generic approach might deserve a separate source file,
> > for example:
> > 
> >     include/linux/sys_info.h
> >     lib/sys_info.c
> 
> Thanks for the suggestion! I'm really not good at naming. 
> 
> As panic.c is always built-in, I'll put sys_info.c as obj-y.

Makes sense.

> > Also I always considered the bitmask as a horrible user interface.
> > It might be used internally. But it would be better to allow a human
> > readable parameter, for example:
> > 
> > 	panic_sys_info=task,mem,timer,lock,ftrace,bt,all_bt,blocked_tasks
> 
> Yes, it's convenient for developers, as a cmdline parameter being parsed
> at boot time.
> 
> But I think bitmask may be easier for runtime changing as a core parameter
> under /proc/ or /sys/, or from sysctl interface. There are also some other
> modules use debug bitmask controlling printking info for different
> sub-components.

Good to know. Could you please give me a pointer to some other modules
using the bitmask? I believe that they exist but I can't find any.
I wonder how common it is...

Anyway, I personally find words/names easier to use. For example,
I like the following interfaces:

  #> cat /sys/power/pm_test
  [none] core processors platform devices freezer

  #> cat /sys/kernel/debug/tracing/available_tracers
  blk function_graph wakeup_dl wakeup_rt wakeup function nop

  #> cat /proc/sys/kernel/seccomp/actions_avail
  kill_process kill_thread trap errno user_notif trace log allow
  # cat /proc/sys/kernel/seccomp/actions_logged
  kill_process kill_thread trap errno user_notif trace log

> And we have similar control knobs for hung, lockup etc.
> 
> Or should I change the name from 'xxx_print_mask' to 'xxx_sysinfo_flag'
> in patch 2/3 ?
> 
> > 
> > The console reply might be handled by a separate:
> > 
> > 	panic_console_reply=1
> > 
> > And it would obsolete the existing "panic_print" which is an
> > ugly name and interface from my POV.
> 
> Agree it's ugly :). But besides a kernel parameter,  'panic_print' is
> also a sysctl interface, I'm afraid renaming it might break user ABI.

A solution would be to keep it and create "panic_sys_info="
with the human readable parameters in parallel. They would
store the request in the same bitmap.

We could print a message that "panic_print" has been obsoleted
by "panic_sys_info" when people use it.

Both parameters would override the current bitmap. So the later
used parameter or procfs/sysfs write would win.

Note:

One question is whether to use sysctl or module parameters.

An advantage of sysctl is the "systcl" userspace tool. Some people
might like it. But the API is very old and a bit cumbersome for
implementing.

The sysfs, aka include/linux/moduleparam.h, API looks cleaner to me.
But the parameters are hidden in the /sys/... jungle ;-)

I would slightly prefer "sysctl" because these parameters are easier
to find.

Best Regards,
Petr

On Thu, May 15, 2025 at 12:32:04PM +0200, Petr Mladek wrote:
> On Tue 2025-05-13 21:23:25, Feng Tang wrote:
> > Hi Petr,
> > 
> > Thanks for the review!
> > 
> > On Tue, May 13, 2025 at 11:57:19AM +0200, Petr Mladek wrote:
> > > On Sun 2025-05-11 16:52:52, Feng Tang wrote:
> > > > panic_print was introduced to help debugging kernel panic by dumping
> > > > different kinds of system information like tasks' call stack, memory,
> > > > ftrace buffer etc. Acutually this function could help debugging cases
> > > > like task-hung, soft/hard lockup too, where user may need the snapshot
> > > > of system info at that time.
> > > > 
> > > The generic approach might deserve a separate source file,
> > > for example:
> > > 
> > >     include/linux/sys_info.h
> > >     lib/sys_info.c
> > 
> > Thanks for the suggestion! I'm really not good at naming. 
> > 
> > As panic.c is always built-in, I'll put sys_info.c as obj-y.
> 
> Makes sense.
> 
> > > Also I always considered the bitmask as a horrible user interface.
> > > It might be used internally. But it would be better to allow a human
> > > readable parameter, for example:
> > > 
> > > 	panic_sys_info=task,mem,timer,lock,ftrace,bt,all_bt,blocked_tasks
> > 
> > Yes, it's convenient for developers, as a cmdline parameter being parsed
> > at boot time.
> > 
> > But I think bitmask may be easier for runtime changing as a core parameter
> > under /proc/ or /sys/, or from sysctl interface. There are also some other
> > modules use debug bitmask controlling printking info for different
> > sub-components.
> 
> Good to know. Could you please give me a pointer to some other modules
> using the bitmask? I believe that they exist but I can't find any.
> I wonder how common it is...

Definitely not common :) I only know one: ACPI, which has 2 debug knobs,
'debug_layer' is a bigmap to control which sub-component's msg to be
dumped, and 'debug_level'.

> Anyway, I personally find words/names easier to use.

True. For me, I have some notes sticked on my monitor: one about characters
for /proc/sysrq-trigger, one for panic_print, one for struct page's flag :)

> For example,
> I like the following interfaces:
> 
>   #> cat /sys/power/pm_test
>   [none] core processors platform devices freezer
> 
>   #> cat /sys/kernel/debug/tracing/available_tracers
>   blk function_graph wakeup_dl wakeup_rt wakeup function nop
> 
>   #> cat /proc/sys/kernel/seccomp/actions_avail
>   kill_process kill_thread trap errno user_notif trace log allow
>   # cat /proc/sys/kernel/seccomp/actions_logged
>   kill_process kill_thread trap errno user_notif trace log

Thanks for the info, will check them.

> > And we have similar control knobs for hung, lockup etc.
> > 
> > Or should I change the name from 'xxx_print_mask' to 'xxx_sysinfo_flag'
> > in patch 2/3 ?
> > 
> > > 
> > > The console reply might be handled by a separate:
> > > 
> > > 	panic_console_reply=1
> > > 
> > > And it would obsolete the existing "panic_print" which is an
> > > ugly name and interface from my POV.
> > 
> > Agree it's ugly :). But besides a kernel parameter,  'panic_print' is
> > also a sysctl interface, I'm afraid renaming it might break user ABI.
> 
> A solution would be to keep it and create "panic_sys_info="
> with the human readable parameters in parallel. They would
> store the request in the same bitmap.
> 
> We could print a message that "panic_print" has been obsoleted
> by "panic_sys_info" when people use it.
> 
> Both parameters would override the current bitmap. So the later
> used parameter or procfs/sysfs write would win.

Reasonalbe.

> Note:
> 
> One question is whether to use sysctl or module parameters.
> 
> An advantage of sysctl is the "systcl" userspace tool. Some people
> might like it. But the API is very old and a bit cumbersome for
> implementing.
> 
> The sysfs, aka include/linux/moduleparam.h, API looks cleaner to me.
> But the parameters are hidden in the /sys/... jungle ;-)
> 
> I would slightly prefer "sysctl" because these parameters are easier
> to find.

I will think about the string parsing in sys_info.c, and in the backend,
a bitmap is still needed to save the parsing result, and as the parameter
for sys_show_info().

Also if we go 'sysctl' way, in the future, some exising interface like
'sysctl_hung_task_all_cpu_backtrace' could be deprecated and integrated
into the 'hung_task_sys_info'? 

Thanks,
Feng

> Best Regards,
> Petr

On Fri, May 16, 2025 at 01:38:02PM +0800, Feng Tang wrote:
> On Thu, May 15, 2025 at 12:32:04PM +0200, Petr Mladek wrote:
[...]
> > > > The console reply might be handled by a separate:
> > > > 
> > > > 	panic_console_reply=1
> > > > 
> > > > And it would obsolete the existing "panic_print" which is an
> > > > ugly name and interface from my POV.
> > > 
> > > Agree it's ugly :). But besides a kernel parameter,  'panic_print' is
> > > also a sysctl interface, I'm afraid renaming it might break user ABI.
> > 
> > A solution would be to keep it and create "panic_sys_info="
> > with the human readable parameters in parallel. They would
> > store the request in the same bitmap.
> > 
> > We could print a message that "panic_print" has been obsoleted
> > by "panic_sys_info" when people use it.
> > 
> > Both parameters would override the current bitmap. So the later
> > used parameter or procfs/sysfs write would win.
> 
> Reasonalbe.
> 
> > Note:
> > 
> > One question is whether to use sysctl or module parameters.
> > 
> > An advantage of sysctl is the "systcl" userspace tool. Some people
> > might like it. But the API is very old and a bit cumbersome for
> > implementing.
> > 
> > The sysfs, aka include/linux/moduleparam.h, API looks cleaner to me.
> > But the parameters are hidden in the /sys/... jungle ;-)
> > 
> > I would slightly prefer "sysctl" because these parameters are easier
> > to find.
> 
> I will think about the string parsing in sys_info.c, and in the backend,
> a bitmap is still needed to save the parsing result, and as the parameter
> for sys_show_info().

Hi Petr

I tried further this way, and with below patch on top of current 1/3
patch, the 'panic_sys_info' sysctl interface basically works, as parsing
user-input, and save it in 'panic_print' bitmap. 

It has  one problem that it doesn't support the string parsing as a the
kernel command line parameter (auto-derived from sysctl interface), I'm
not sure if we should add a __setup() or early_param() for it, or it's
fine?

Thanks,
Feng

---
diff --git a/include/linux/sys_info.h b/include/linux/sys_info.h
index 79bf4a942e5f..d6d55646e25a 100644
--- a/include/linux/sys_info.h
+++ b/include/linux/sys_info.h
@@ -17,4 +17,8 @@
 
 extern void sys_show_info(unsigned long info_mask);
 
+struct ctl_table;
+extern int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
+					  void *buffer, size_t *lenp,
+					  loff_t *ppos);
 #endif	/* _LINUX_SYS_INFO_H */
diff --git a/kernel/panic.c b/kernel/panic.c
index 3d9cf8063242..8ca9b30f0fe4 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -88,6 +88,13 @@ static const struct ctl_table kern_panic_table[] = {
 		.extra2         = SYSCTL_ONE,
 	},
 #endif
+	{
+		.procname	= "panic_sys_info",
+		.data		= &panic_print,
+		.maxlen         = sizeof(panic_print),
+		.mode		= 0644,
+		.proc_handler	= sysctl_sys_info_handler,
+	},
 	{
 		.procname       = "warn_limit",
 		.data           = &warn_limit,
diff --git a/lib/sys_info.c b/lib/sys_info.c
index 4090b2e0515e..27de6f0d0a4d 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -4,6 +4,121 @@
 #include <linux/console.h>
 #include <linux/nmi.h>
 
+struct sys_info_name {
+	unsigned long bit;
+	const char *name;
+};
+
+static const char sys_info_avail[] = " tasks mem timer lock ftrace all_bt blocked_tasks ";
+
+static const struct sys_info_name  si_names[] = {
+	{ SYS_SHOW_TASK_INFO,	"tasks" },
+	{ SYS_SHOW_MEM_INFO,	"mem" },
+	{ SYS_SHOW_TIMER_INFO,	"timer" },
+	{ SYS_SHOW_LOCK_INFO,	"lock" },
+	{ SYS_SHOW_FTRACE_INFO, "ftrace" },
+	{ SYS_SHOW_ALL_CPU_BT,	"all_bt" },
+	{ SYS_SHOW_BLOCKED_TASKS, "blocked_tasks" },
+};
+
+
+/* Expecting string like "xxx_sys_info=tasks,mem,timer,lock" */
+static int write_handler(const struct ctl_table *ro_table, void *buffer,
+				size_t *lenp, loff_t *ppos)
+{
+	char names[sizeof(sys_info_avail)];
+	char *buf, *name;
+	struct ctl_table table;
+	unsigned long *si_flag;
+	int i, len, ret;
+
+	si_flag = ro_table->data;
+
+	/* Clear it first */
+	*si_flag = 0;
+
+	table = *ro_table;
+	table.data = names;
+	table.maxlen = sizeof(names);
+	ret = proc_dostring(&table, 1, buffer, lenp, ppos);
+	if (ret)
+		return ret;
+
+	buf = names;
+	while ((name = strsep(&buf, ",")) && *name) {
+		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
+			if (!strcmp(name, si_names[i].name))
+				*si_flag |= si_names[i].bit;
+		}
+	}
+
+	return 0;
+}
+
+int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
+					  void *buffer, size_t *lenp,
+					  loff_t *ppos)
+{
+	char names[sizeof(sys_info_avail) + 1];
+	char *buf, *name;
+	struct ctl_table table;
+	unsigned long *si_flag;
+	int i, ret, len;
+
+	si_flag = ro_table->data;
+
+	if (write) {
+		/* Clear it first */
+		*si_flag = 0;
+
+		table = *ro_table;
+		table.data = names;
+		table.maxlen = sizeof(names);
+		ret = proc_dostring(&table, 1, buffer, lenp, ppos);
+		if (ret)
+			return ret;
+
+		/* Expecting string like "xxx_sys_info=tasks,mem,timer,lock" */
+		buf = names;
+		while ((name = strsep(&buf, ",")) && *name) {
+			for (i = 0; i < ARRAY_SIZE(si_names); i++) {
+				if (!strcmp(name, si_names[i].name))
+					*si_flag |= si_names[i].bit;
+			}
+		}
+
+		return 0;
+	} else {
+		bool first = true;
+
+		memset(names, 0, sizeof(names));
+
+		buf = names;
+		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
+			if (*si_flag & si_names[i].bit) {
+
+				if (first) {
+					first = false;
+				} else {
+					*buf = ',';
+					buf++;
+				}
+
+				len = strlen(si_names[i].name);
+				strncpy(buf, si_names[i].name, len);
+				buf += len;
+			}
+
+		}
+		*buf = '\0';
+
+		table = *ro_table;
+		table.data = names;
+		table.maxlen = sizeof(names);
+		return proc_dostring(&table, 0, buffer, lenp, ppos);
+	}
+}
+
 void sys_show_info(unsigned long info_flag)
 {
 	if (info_flag & SYS_SHOW_TASK_INFO)

On Wed 2025-05-21 13:31:20, Feng Tang wrote:
> On Fri, May 16, 2025 at 01:38:02PM +0800, Feng Tang wrote:
> > On Thu, May 15, 2025 at 12:32:04PM +0200, Petr Mladek wrote:
> [...]
> > > > > The console reply might be handled by a separate:
> > > > > 
> > > > > 	panic_console_reply=1
> > > > > 
> > > > > And it would obsolete the existing "panic_print" which is an
> > > > > ugly name and interface from my POV.
> > > > 
> > > > Agree it's ugly :). But besides a kernel parameter,  'panic_print' is
> > > > also a sysctl interface, I'm afraid renaming it might break user ABI.
> > > 
> > > A solution would be to keep it and create "panic_sys_info="
> > > with the human readable parameters in parallel. They would
> > > store the request in the same bitmap.
> > > 
> > > We could print a message that "panic_print" has been obsoleted
> > > by "panic_sys_info" when people use it.
> > > 
> > > Both parameters would override the current bitmap. So the later
> > > used parameter or procfs/sysfs write would win.
> > 
> > Reasonalbe.
> > 
> > > Note:
> > > 
> > > One question is whether to use sysctl or module parameters.
> > > 
> > > An advantage of sysctl is the "systcl" userspace tool. Some people
> > > might like it. But the API is very old and a bit cumbersome for
> > > implementing.
> > > 
> > > The sysfs, aka include/linux/moduleparam.h, API looks cleaner to me.
> > > But the parameters are hidden in the /sys/... jungle ;-)
> > > 
> > > I would slightly prefer "sysctl" because these parameters are easier
> > > to find.
> > 
> > I will think about the string parsing in sys_info.c, and in the backend,
> > a bitmap is still needed to save the parsing result, and as the parameter
> > for sys_show_info().
> 
> Hi Petr
> 
> I tried further this way, and with below patch on top of current 1/3
> patch, the 'panic_sys_info' sysctl interface basically works, as parsing
> user-input, and save it in 'panic_print' bitmap. 

It does not apply. It seems that it depends on another change which
crated lib/sys_info.c...

> It has  one problem that it doesn't support the string parsing as a the
> kernel command line parameter (auto-derived from sysctl interface), I'm
> not sure if we should add a __setup() or early_param() for it, or it's
> fine?

Ah, I was not aware of this. We need to make it working from the
command line, definitely. I would go with __setup() for now. We could
always switch it to early_param() when anyone requires it.

See some more comments, below.

> ---
> diff --git a/include/linux/sys_info.h b/include/linux/sys_info.h
> index 79bf4a942e5f..d6d55646e25a 100644
> --- a/include/linux/sys_info.h
> +++ b/include/linux/sys_info.h
> @@ -17,4 +17,8 @@
>  
>  extern void sys_show_info(unsigned long info_mask);
>  
> +struct ctl_table;
> +extern int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> +					  void *buffer, size_t *lenp,
> +					  loff_t *ppos);
>  #endif	/* _LINUX_SYS_INFO_H */
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 3d9cf8063242..8ca9b30f0fe4 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -88,6 +88,13 @@ static const struct ctl_table kern_panic_table[] = {
>  		.extra2         = SYSCTL_ONE,
>  	},
>  #endif
> +	{
> +		.procname	= "panic_sys_info",
> +		.data		= &panic_print,
> +		.maxlen         = sizeof(panic_print),
> +		.mode		= 0644,
> +		.proc_handler	= sysctl_sys_info_handler,
> +	},
>  	{
>  		.procname       = "warn_limit",
>  		.data           = &warn_limit,
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index 4090b2e0515e..27de6f0d0a4d 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -4,6 +4,121 @@
>  #include <linux/console.h>
>  #include <linux/nmi.h>
>  
> +struct sys_info_name {
> +	unsigned long bit;
> +	const char *name;
> +};
> +
> +static const char sys_info_avail[] = " tasks mem timer lock ftrace all_bt blocked_tasks ";

It is a bit confusing to have it space-separated when the parameter
is comma-separated. Also I am not sure why there is the leading and
trailing space.

I would expect:

static const char sys_info_avail[] = "tasks,mem,timer,lock,ftrace,all_bt,blocked_tasks";

> +static const struct sys_info_name  si_names[] = {
> +	{ SYS_SHOW_TASK_INFO,	"tasks" },
> +	{ SYS_SHOW_MEM_INFO,	"mem" },
> +	{ SYS_SHOW_TIMER_INFO,	"timer" },
> +	{ SYS_SHOW_LOCK_INFO,	"lock" },
> +	{ SYS_SHOW_FTRACE_INFO, "ftrace" },
> +	{ SYS_SHOW_ALL_CPU_BT,	"all_bt" },
> +	{ SYS_SHOW_BLOCKED_TASKS, "blocked_tasks" },
> +};

I guess that this is just an RFC. Anyway, I would expect that
SYS_SHOW_* values would be defined in sys_info.h.

> +
> +/* Expecting string like "xxx_sys_info=tasks,mem,timer,lock" */
> +static int write_handler(const struct ctl_table *ro_table, void *buffer,
> +				size_t *lenp, loff_t *ppos)
> +{
> +	char names[sizeof(sys_info_avail)];
> +	char *buf, *name;
> +	struct ctl_table table;
> +	unsigned long *si_flag;
> +	int i, len, ret;
> +
> +	si_flag = ro_table->data;
> +
> +	/* Clear it first */
> +	*si_flag = 0;
> +
> +	table = *ro_table;
> +	table.data = names;
> +	table.maxlen = sizeof(names);
> +	ret = proc_dostring(&table, 1, buffer, lenp, ppos);
> +	if (ret)
> +		return ret;
> +
> +	buf = names;
> +	while ((name = strsep(&buf, ",")) && *name) {
> +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> +			if (!strcmp(name, si_names[i].name))
> +				*si_flag |= si_names[i].bit;
> +		}
> +	}
> +
> +	return 0;
> +}

The above function is defined but not used. The same code is
copy&pasted in the if (write) section below.

I think that we would need a helper function which could be used
in both sysctl_sys_info_handler() and in the __setup() callback.

Something like:

static unsigned long sys_info_parse_flags(char *str)
{
	unsigned long si_bits = 0;
	char *s, *name;
	int i;

	s = str;
	while ((name = strsep(&s, ",")) && *name) {
		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
			if (!strcmp(name, si_names[i].name)) {
				*si_bits |= si_names[i].bit;
				break;
			}
		}
	}

	return si_bits;
}

> +
> +int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> +					  void *buffer, size_t *lenp,
> +					  loff_t *ppos)
> +{
> +	char names[sizeof(sys_info_avail) + 1];
> +	char *buf, *name;
> +	struct ctl_table table;
> +	unsigned long *si_flag;

Nit: I would call this "si_bits_global" to make it more clear that
     this is pointer to the global bitmask.


> +	int i, ret, len;
> +
> +	si_flag = ro_table->data;
> +
> +	if (write) {
> +		/* Clear it first */
> +		*si_flag = 0;

There is no synchronization against readers. IMHO, it is not worth it.
But we should at least update the global value only once.

We should define a local variable, e.g.

		unsigned long si_bits;

and do the following:

> +		table = *ro_table;
> +		table.data = names;
> +		table.maxlen = sizeof(names);
> +		ret = proc_dostring(&table, 1, buffer, lenp, ppos);

I would pass the "write" parameter here instead of the hard-coded "1".
Do we know that it should be exactly '1'?

> +		if (ret)
> +			return ret;

		si_bits = sys_info_parse_param(flags);
		/*
		 * The access to the global value is not synchronized.
		 * Update it at once at least.
		 */
		WRITE_ONCE(*si_bits_global, si_bits);

> +		/* Expecting string like "xxx_sys_info=tasks,mem,timer,lock" */
> +		buf = names;
> +		while ((name = strsep(&buf, ",")) && *name) {
> +			for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> +				if (!strcmp(name, si_names[i].name))
> +					*si_flag |= si_names[i].bit;
> +			}
> +		}
> +
> +		return 0;
> +	} else {
> +		bool first = true;
> +
> +		memset(names, 0, sizeof(names));

I guess that you took this from read_actions_logged().

It looks too paranoid to me. I do not see it anywhere else.
IMHO, if the proc_dostring() does not stop at the trailing '\0'
then most interfaces would leak data.

> +		buf = names;
> +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> +			if (*si_flag & si_names[i].bit) {
> +
> +				if (first) {
> +					first = false;
> +				} else {
> +					*buf = ',';
> +					buf++;
> +				}
> +
> +				len = strlen(si_names[i].name);
> +				strncpy(buf, si_names[i].name, len);
> +				buf += len;
> +			}
> +
> +		}
> +		*buf = '\0';

IMHO, always adding this trailing '\0' should be enough.

> +		table = *ro_table;
> +		table.data = names;
> +		table.maxlen = sizeof(names);
> +		return proc_dostring(&table, 0, buffer, lenp, ppos);

I would pass the "write" parameter here instead of the hard coded 0.
But it is a matter of taste.

> +	}
> +}
> +
>  void sys_show_info(unsigned long info_flag)
>  {
>  	if (info_flag & SYS_SHOW_TASK_INFO)

Best Regards,
Petr

PS: I am sorry for the late reply. Too many things have accumulated
over the few last weeks.

On Tue, Jun 10, 2025 at 05:44:34PM +0200, Petr Mladek wrote:
[...]
> > > > Note:
> > > > 
> > > > One question is whether to use sysctl or module parameters.
> > > > 
> > > > An advantage of sysctl is the "systcl" userspace tool. Some people
> > > > might like it. But the API is very old and a bit cumbersome for
> > > > implementing.
> > > > 
> > > > The sysfs, aka include/linux/moduleparam.h, API looks cleaner to me.
> > > > But the parameters are hidden in the /sys/... jungle ;-)
> > > > 
> > > > I would slightly prefer "sysctl" because these parameters are easier
> > > > to find.
> > > 
> > > I will think about the string parsing in sys_info.c, and in the backend,
> > > a bitmap is still needed to save the parsing result, and as the parameter
> > > for sys_show_info().
> > 
> > Hi Petr
> > 
> > I tried further this way, and with below patch on top of current 1/3
> > patch, the 'panic_sys_info' sysctl interface basically works, as parsing
> > user-input, and save it in 'panic_print' bitmap. 
> 
> It does not apply. It seems that it depends on another change which
> crated lib/sys_info.c...

My bad. It could be another my local change which follows your suggestion
of using a panic_console_replay() cleanup.

> > It has  one problem that it doesn't support the string parsing as a the
> > kernel command line parameter (auto-derived from sysctl interface), I'm
> > not sure if we should add a __setup() or early_param() for it, or it's
> > fine?
> 
> Ah, I was not aware of this. We need to make it working from the
> command line, definitely. I would go with __setup() for now. We could
> always switch it to early_param() when anyone requires it.

OK.

> See some more comments, below.
> 
> > ---
> > diff --git a/include/linux/sys_info.h b/include/linux/sys_info.h
> > index 79bf4a942e5f..d6d55646e25a 100644
> > --- a/include/linux/sys_info.h
> > +++ b/include/linux/sys_info.h
> > @@ -17,4 +17,8 @@
> >  
> >  extern void sys_show_info(unsigned long info_mask);
> >  
> > +struct ctl_table;
> > +extern int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> > +					  void *buffer, size_t *lenp,
> > +					  loff_t *ppos);
> >  #endif	/* _LINUX_SYS_INFO_H */
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index 3d9cf8063242..8ca9b30f0fe4 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -88,6 +88,13 @@ static const struct ctl_table kern_panic_table[] = {
> >  		.extra2         = SYSCTL_ONE,
> >  	},
> >  #endif
> > +	{
> > +		.procname	= "panic_sys_info",
> > +		.data		= &panic_print,
> > +		.maxlen         = sizeof(panic_print),
> > +		.mode		= 0644,
> > +		.proc_handler	= sysctl_sys_info_handler,
> > +	},
> >  	{
> >  		.procname       = "warn_limit",
> >  		.data           = &warn_limit,
> > diff --git a/lib/sys_info.c b/lib/sys_info.c
> > index 4090b2e0515e..27de6f0d0a4d 100644
> > --- a/lib/sys_info.c
> > +++ b/lib/sys_info.c
> > @@ -4,6 +4,121 @@
> >  #include <linux/console.h>
> >  #include <linux/nmi.h>
> >  
> > +struct sys_info_name {
> > +	unsigned long bit;
> > +	const char *name;
> > +};
> > +
> > +static const char sys_info_avail[] = " tasks mem timer lock ftrace all_bt blocked_tasks ";
> 
> It is a bit confusing to have it space-separated when the parameter
> is comma-separated.

Aha, right.

> Also I am not sure why there is the leading and
> trailing space.

I tried to give more space to avoid 'char names[strlen()+1]' 

> I would expect:
> 
> static const char sys_info_avail[] = "tasks,mem,timer,lock,ftrace,all_bt,blocked_tasks";

OK.

> > +static const struct sys_info_name  si_names[] = {
> > +	{ SYS_SHOW_TASK_INFO,	"tasks" },
> > +	{ SYS_SHOW_MEM_INFO,	"mem" },
> > +	{ SYS_SHOW_TIMER_INFO,	"timer" },
> > +	{ SYS_SHOW_LOCK_INFO,	"lock" },
> > +	{ SYS_SHOW_FTRACE_INFO, "ftrace" },
> > +	{ SYS_SHOW_ALL_CPU_BT,	"all_bt" },
> > +	{ SYS_SHOW_BLOCKED_TASKS, "blocked_tasks" },
> > +};
> 
> I guess that this is just an RFC. Anyway, I would expect that
> SYS_SHOW_* values would be defined in sys_info.h.

Yes, in 0001 patch, they are defined in sys_info.h   

> > +
> > +/* Expecting string like "xxx_sys_info=tasks,mem,timer,lock" */
> > +static int write_handler(const struct ctl_table *ro_table, void *buffer,
> > +				size_t *lenp, loff_t *ppos)
> > +{
> > +	char names[sizeof(sys_info_avail)];
> > +	char *buf, *name;
> > +	struct ctl_table table;
> > +	unsigned long *si_flag;
> > +	int i, len, ret;
> > +
> > +	si_flag = ro_table->data;
> > +
> > +	/* Clear it first */
> > +	*si_flag = 0;
> > +
> > +	table = *ro_table;
> > +	table.data = names;
> > +	table.maxlen = sizeof(names);
> > +	ret = proc_dostring(&table, 1, buffer, lenp, ppos);
> > +	if (ret)
> > +		return ret;
> > +
> > +	buf = names;
> > +	while ((name = strsep(&buf, ",")) && *name) {
> > +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > +			if (!strcmp(name, si_names[i].name))
> > +				*si_flag |= si_names[i].bit;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> 
> The above function is defined but not used. The same code is
> copy&pasted in the if (write) section below.

I forgot to remove it in code clean :)

> I think that we would need a helper function which could be used
> in both sysctl_sys_info_handler() and in the __setup() callback.
> 
> Something like:
> 
> static unsigned long sys_info_parse_flags(char *str)
> {
> 	unsigned long si_bits = 0;
> 	char *s, *name;
> 	int i;
> 
> 	s = str;
> 	while ((name = strsep(&s, ",")) && *name) {
> 		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> 			if (!strcmp(name, si_names[i].name)) {
> 				*si_bits |= si_names[i].bit;
> 				break;

Good catch! thanks

> 			}
> 		}
> 	}
> 
> 	return si_bits;
> }

Will do.

> > +
> > +int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> > +					  void *buffer, size_t *lenp,
> > +					  loff_t *ppos)
> > +{
> > +	char names[sizeof(sys_info_avail) + 1];
> > +	char *buf, *name;
> > +	struct ctl_table table;
> > +	unsigned long *si_flag;
> 
> Nit: I would call this "si_bits_global" to make it more clear that
>      this is pointer to the global bitmask.
 
Makes sense.

> 
> > +	int i, ret, len;
> > +
> > +	si_flag = ro_table->data;
> > +
> > +	if (write) {
> > +		/* Clear it first */
> > +		*si_flag = 0;
> 
> There is no synchronization against readers. IMHO, it is not worth it.
> But we should at least update the global value only once.
> 
> We should define a local variable, e.g.
> 
> 		unsigned long si_bits;
> 
> and do the following:

Will do.

> > +		table = *ro_table;
> > +		table.data = names;
> > +		table.maxlen = sizeof(names);
> > +		ret = proc_dostring(&table, 1, buffer, lenp, ppos);
> 
> I would pass the "write" parameter here instead of the hard-coded "1".
> Do we know that it should be exactly '1'?
> 
> > +		if (ret)
> > +			return ret;
> 
> 		si_bits = sys_info_parse_param(flags);
> 		/*
> 		 * The access to the global value is not synchronized.
> 		 * Update it at once at least.
> 		 */
> 		WRITE_ONCE(*si_bits_global, si_bits);

Thanks for the suggestion.


> > +		/* Expecting string like "xxx_sys_info=tasks,mem,timer,lock" */
> > +		buf = names;
> > +		while ((name = strsep(&buf, ",")) && *name) {
> > +			for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > +				if (!strcmp(name, si_names[i].name))
> > +					*si_flag |= si_names[i].bit;
> > +			}
> > +		}
> > +
> > +		return 0;
> > +	} else {
> > +		bool first = true;
> > +
> > +		memset(names, 0, sizeof(names));
> 
> I guess that you took this from read_actions_logged().

Yes, I referred the seccomp.c in many places.

> It looks too paranoid to me. I do not see it anywhere else.
> IMHO, if the proc_dostring() does not stop at the trailing '\0'
> then most interfaces would leak data.
> 
> > +		buf = names;
> > +		for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > +			if (*si_flag & si_names[i].bit) {
> > +
> > +				if (first) {
> > +					first = false;
> > +				} else {
> > +					*buf = ',';
> > +					buf++;
> > +				}
> > +
> > +				len = strlen(si_names[i].name);
> > +				strncpy(buf, si_names[i].name, len);
> > +				buf += len;
> > +			}
> > +
> > +		}
> > +		*buf = '\0';
> 
> IMHO, always adding this trailing '\0' should be enough.

OK.

> 
> > +		table = *ro_table;
> > +		table.data = names;
> > +		table.maxlen = sizeof(names);
> > +		return proc_dostring(&table, 0, buffer, lenp, ppos);
> 
> I would pass the "write" parameter here instead of the hard coded 0.
> But it is a matter of taste.

I think it's obviously better :). will change.

> 
> > +	}
> > +}
> > +
> >  void sys_show_info(unsigned long info_flag)
> >  {
> >  	if (info_flag & SYS_SHOW_TASK_INFO)
> 
> Best Regards,
> Petr
> 
> PS: I am sorry for the late reply. Too many things have accumulated
> over the few last weeks.

No problem! I really appreciate your review and suggestions, like
your help on reviewing my early 'panic_print' patches years ago.

Thanks,
Feng

On 2025/5/16 13:38, Feng Tang wrote:
> On Thu, May 15, 2025 at 12:32:04PM +0200, Petr Mladek wrote:
>> On Tue 2025-05-13 21:23:25, Feng Tang wrote:
>>> Hi Petr,
>>>
>>> Thanks for the review!
>>>
>>> On Tue, May 13, 2025 at 11:57:19AM +0200, Petr Mladek wrote:
>>>> On Sun 2025-05-11 16:52:52, Feng Tang wrote:
>>>>> panic_print was introduced to help debugging kernel panic by dumping
>>>>> different kinds of system information like tasks' call stack, memory,
>>>>> ftrace buffer etc. Acutually this function could help debugging cases
>>>>> like task-hung, soft/hard lockup too, where user may need the snapshot
>>>>> of system info at that time.
>>>>>
>>>> The generic approach might deserve a separate source file,
>>>> for example:
>>>>
>>>>      include/linux/sys_info.h
>>>>      lib/sys_info.c
>>>
>>> Thanks for the suggestion! I'm really not good at naming.
>>>
>>> As panic.c is always built-in, I'll put sys_info.c as obj-y.
>>
>> Makes sense.
>>
>>>> Also I always considered the bitmask as a horrible user interface.
>>>> It might be used internally. But it would be better to allow a human
>>>> readable parameter, for example:
>>>>
>>>> 	panic_sys_info=task,mem,timer,lock,ftrace,bt,all_bt,blocked_tasks
>>>
>>> Yes, it's convenient for developers, as a cmdline parameter being parsed
>>> at boot time.
>>>
>>> But I think bitmask may be easier for runtime changing as a core parameter
>>> under /proc/ or /sys/, or from sysctl interface. There are also some other
>>> modules use debug bitmask controlling printking info for different
>>> sub-components.
>>
>> Good to know. Could you please give me a pointer to some other modules
>> using the bitmask? I believe that they exist but I can't find any.
>> I wonder how common it is...
> 
> Definitely not common :) I only know one: ACPI, which has 2 debug knobs,
> 'debug_layer' is a bigmap to control which sub-component's msg to be
> dumped, and 'debug_level'.
> 
>> Anyway, I personally find words/names easier to use.
> 
> True. For me, I have some notes sticked on my monitor: one about characters
> for /proc/sysrq-trigger, one for panic_print, one for struct page's flag :)
> 
>> For example,
>> I like the following interfaces:
>>
>>    #> cat /sys/power/pm_test
>>    [none] core processors platform devices freezer
>>
>>    #> cat /sys/kernel/debug/tracing/available_tracers
>>    blk function_graph wakeup_dl wakeup_rt wakeup function nop
>>
>>    #> cat /proc/sys/kernel/seccomp/actions_avail
>>    kill_process kill_thread trap errno user_notif trace log allow
>>    # cat /proc/sys/kernel/seccomp/actions_logged
>>    kill_process kill_thread trap errno user_notif trace log
> 
> Thanks for the info, will check them.
> 
>>> And we have similar control knobs for hung, lockup etc.
>>>
>>> Or should I change the name from 'xxx_print_mask' to 'xxx_sysinfo_flag'
>>> in patch 2/3 ?
>>>
>>>>
>>>> The console reply might be handled by a separate:
>>>>
>>>> 	panic_console_reply=1
>>>>
>>>> And it would obsolete the existing "panic_print" which is an
>>>> ugly name and interface from my POV.
>>>
>>> Agree it's ugly :). But besides a kernel parameter,  'panic_print' is
>>> also a sysctl interface, I'm afraid renaming it might break user ABI.
>>
>> A solution would be to keep it and create "panic_sys_info="
>> with the human readable parameters in parallel. They would
>> store the request in the same bitmap.
>>
>> We could print a message that "panic_print" has been obsoleted
>> by "panic_sys_info" when people use it.
>>
>> Both parameters would override the current bitmap. So the later
>> used parameter or procfs/sysfs write would win.
> 
> Reasonalbe.
> 
>> Note:
>>
>> One question is whether to use sysctl or module parameters.
>>
>> An advantage of sysctl is the "systcl" userspace tool. Some people
>> might like it. But the API is very old and a bit cumbersome for
>> implementing.
>>
>> The sysfs, aka include/linux/moduleparam.h, API looks cleaner to me.
>> But the parameters are hidden in the /sys/... jungle ;-)
>>
>> I would slightly prefer "sysctl" because these parameters are easier
>> to find.
> 
> I will think about the string parsing in sys_info.c, and in the backend,
> a bitmap is still needed to save the parsing result, and as the parameter
> for sys_show_info().
> 
> Also if we go 'sysctl' way, in the future, some exising interface like
> 'sysctl_hung_task_all_cpu_backtrace' could be deprecated and integrated
> into the 'hung_task_sys_info'?

Works for me. Let's go with 'sysctl' approach, and 
'hung_task_all_cpu_backtrace'
could be deprecated, though it won't be removed anytime soon. IMHO,
'hung_task_sys_info' is more organized and easier to expand - plus these
parameters are much easier to find as Petr said ;)

Thanks,
Lance

> 
> Thanks,
> Feng
> 
>> Best Regards,
>> Petr

On 2025/5/15 18:32, Petr Mladek wrote:
> On Tue 2025-05-13 21:23:25, Feng Tang wrote:
>> Hi Petr,
>>
>> Thanks for the review!
>>
>> On Tue, May 13, 2025 at 11:57:19AM +0200, Petr Mladek wrote:
>>> On Sun 2025-05-11 16:52:52, Feng Tang wrote:
>>>> panic_print was introduced to help debugging kernel panic by dumping
>>>> different kinds of system information like tasks' call stack, memory,
>>>> ftrace buffer etc. Acutually this function could help debugging cases
>>>> like task-hung, soft/hard lockup too, where user may need the snapshot
>>>> of system info at that time.
>>>>
>>> The generic approach might deserve a separate source file,
>>> for example:
>>>
>>>      include/linux/sys_info.h
>>>      lib/sys_info.c
>>
>> Thanks for the suggestion! I'm really not good at naming.
>>
>> As panic.c is always built-in, I'll put sys_info.c as obj-y.
> 
> Makes sense.
> 
>>> Also I always considered the bitmask as a horrible user interface.
>>> It might be used internally. But it would be better to allow a human
>>> readable parameter, for example:
>>>
>>> 	panic_sys_info=task,mem,timer,lock,ftrace,bt,all_bt,blocked_tasks
>>
>> Yes, it's convenient for developers, as a cmdline parameter being parsed
>> at boot time.
>>
>> But I think bitmask may be easier for runtime changing as a core parameter
>> under /proc/ or /sys/, or from sysctl interface. There are also some other
>> modules use debug bitmask controlling printking info for different
>> sub-components.
> 
> Good to know. Could you please give me a pointer to some other modules
> using the bitmask? I believe that they exist but I can't find any.
> I wonder how common it is...
> 
> Anyway, I personally find words/names easier to use. For example,
> I like the following interfaces:
> 
>    #> cat /sys/power/pm_test
>    [none] core processors platform devices freezer
> 
>    #> cat /sys/kernel/debug/tracing/available_tracers
>    blk function_graph wakeup_dl wakeup_rt wakeup function nop
> 
>    #> cat /proc/sys/kernel/seccomp/actions_avail
>    kill_process kill_thread trap errno user_notif trace log allow
>    # cat /proc/sys/kernel/seccomp/actions_logged
>    kill_process kill_thread trap errno user_notif trace log

Clean and straightforward! I prefer this too ;p

> 
>> And we have similar control knobs for hung, lockup etc.
>>
>> Or should I change the name from 'xxx_print_mask' to 'xxx_sysinfo_flag'
>> in patch 2/3 ?
>>
>>>
>>> The console reply might be handled by a separate:
>>>
>>> 	panic_console_reply=1
>>>
>>> And it would obsolete the existing "panic_print" which is an
>>> ugly name and interface from my POV.
>>
>> Agree it's ugly :). But besides a kernel parameter,  'panic_print' is
>> also a sysctl interface, I'm afraid renaming it might break user ABI.
> 
> A solution would be to keep it and create "panic_sys_info="
> with the human readable parameters in parallel. They would
> store the request in the same bitmap.

Nice! Seems like a well-balanced solution ;)

> 
> We could print a message that "panic_print" has been obsoleted
> by "panic_sys_info" when people use it.

Indeed, we should add a deprecation warning. Perhaps the message could say:

"'panic_print=' is deprecated and will be removed. Use 'panic_sys_info=' 
instead."

e.g. pr_warn_ratelimited("block device autoloading is deprecated and will be
removed.\n") in blkdev_get_no_open().

> 
> Both parameters would override the current bitmap. So the later
> used parameter or procfs/sysfs write would win.

Makes sense to me. BTW, this override behavior needs to be documented
in kernel-doc.

> 
> Note:
> 
> One question is whether to use sysctl or module parameters.
> 
> An advantage of sysctl is the "systcl" userspace tool. Some people
> might like it. But the API is very old and a bit cumbersome for
> implementing.
> 
> The sysfs, aka include/linux/moduleparam.h, API looks cleaner to me.
> But the parameters are hidden in the /sys/... jungle ;-)
> 
> I would slightly prefer "sysctl" because these parameters are easier
> to find.

+1 for sysctl.

Thanks,
Lance

> 
> Best Regards,
> Petr