Re: [PATCH v1 1/3] kernel/panic: generalize panic_print's function to show sys info

[Lance Yang <lance.yang@linux.dev>]

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