Message-ID: <aCXCpGkXJ1x9ncHS@pathway.suse.cz>
Date: Thu, 15 May 2025 12:32:04 +0200
From: Petr Mladek <pmladek@...e.com>
To: Feng Tang <feng.tang@...ux.alibaba.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Lance Yang <lance.yang@...ux.dev>, linux-kernel@...r.kernel.org,
	mhiramat@...nel.org, llong@...hat.com
Subject: Re: [PATCH v1 1/3] kernel/panic: generalize panic_print's function
 to show sys info

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

Powered by blists - more mailing lists