There is no need to keep a custom structure just for the need of
a plain array of strings. Replace struct sys_info_name with plain
array of strings.
With that done, simplify the code, in particular, naturally use
for_each_set_bit() when iterating over si_bits_global bitmap.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
lib/sys_info.c | 48 +++++++++++++++++++++---------------------------
1 file changed, 21 insertions(+), 27 deletions(-)
diff --git a/lib/sys_info.c b/lib/sys_info.c
index 6b0188b30227..5aecf4b6025f 100644
--- a/lib/sys_info.c
+++ b/lib/sys_info.c
@@ -1,30 +1,29 @@
// SPDX-License-Identifier: GPL-2.0-only
-#include <linux/sched/debug.h>
+#include <linux/bitops.h>
#include <linux/console.h>
+#include <linux/log2.h>
#include <linux/kernel.h>
#include <linux/ftrace.h>
-#include <linux/sysctl.h>
#include <linux/nmi.h>
+#include <linux/sched/debug.h>
+#include <linux/string.h>
+#include <linux/sysctl.h>
#include <linux/sys_info.h>
-struct sys_info_name {
- unsigned long bit;
- const char *name;
-};
-
/*
* When 'si_names' gets updated, please make sure the 'sys_info_avail'
* below is updated accordingly.
*/
-static const struct sys_info_name si_names[] = {
- { SYS_INFO_TASKS, "tasks" },
- { SYS_INFO_MEM, "mem" },
- { SYS_INFO_TIMERS, "timers" },
- { SYS_INFO_LOCKS, "locks" },
- { SYS_INFO_FTRACE, "ftrace" },
- { SYS_INFO_ALL_BT, "all_bt" },
- { SYS_INFO_BLOCKED_TASKS, "blocked_tasks" },
+static const char * const si_names[] = {
+ [ilog2(SYS_INFO_TASKS)] = "tasks",
+ [ilog2(SYS_INFO_MEM)] = "mem",
+ [ilog2(SYS_INFO_TIMERS)] = "timers",
+ [ilog2(SYS_INFO_LOCKS)] = "locks",
+ [ilog2(SYS_INFO_FTRACE)] = "ftrace",
+ [ilog2(SYS_INFO_PANIC_CONSOLE_REPLAY)] = "",
+ [ilog2(SYS_INFO_ALL_BT)] = "all_bt",
+ [ilog2(SYS_INFO_BLOCKED_TASKS)] = "blocked_tasks",
};
/* Expecting string like "xxx_sys_info=tasks,mem,timers,locks,ftrace,..." */
@@ -36,12 +35,9 @@ unsigned long sys_info_parse_param(char *str)
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;
- }
- }
+ i = match_string(si_names, ARRAY_SIZE(si_names), name);
+ if (i >= 0)
+ __set_bit(i, &si_bits);
}
return si_bits;
@@ -85,12 +81,10 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
si_bits = READ_ONCE(*si_bits_global);
names[0] = '\0';
- for (i = 0; i < ARRAY_SIZE(si_names); i++) {
- if (si_bits & si_names[i].bit) {
- len += scnprintf(names + len, sizeof(names) - len,
- "%s%s", delim, si_names[i].name);
- delim = ",";
- }
+ for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
+ len += scnprintf(names + len, sizeof(names) - len,
+ "%s%s", delim, si_names[i]);
+ delim = ",";
}
table = *ro_table;
--
2.50.1On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> There is no need to keep a custom structure just for the need of
> a plain array of strings. Replace struct sys_info_name with plain
> array of strings.
>
> With that done, simplify the code, in particular, naturally use
> for_each_set_bit() when iterating over si_bits_global bitmap.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> lib/sys_info.c | 48 +++++++++++++++++++++---------------------------
> 1 file changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/lib/sys_info.c b/lib/sys_info.c
> index 6b0188b30227..5aecf4b6025f 100644
> --- a/lib/sys_info.c
> +++ b/lib/sys_info.c
> @@ -1,30 +1,29 @@
> // SPDX-License-Identifier: GPL-2.0-only
> -#include <linux/sched/debug.h>
> +#include <linux/bitops.h>
> #include <linux/console.h>
> +#include <linux/log2.h>
> #include <linux/kernel.h>
> #include <linux/ftrace.h>
> -#include <linux/sysctl.h>
> #include <linux/nmi.h>
> +#include <linux/sched/debug.h>
> +#include <linux/string.h>
> +#include <linux/sysctl.h>
>
> #include <linux/sys_info.h>
>
> -struct sys_info_name {
> - unsigned long bit;
> - const char *name;
> -};
> -
> /*
> * When 'si_names' gets updated, please make sure the 'sys_info_avail'
> * below is updated accordingly.
> */
> -static const struct sys_info_name si_names[] = {
> - { SYS_INFO_TASKS, "tasks" },
> - { SYS_INFO_MEM, "mem" },
> - { SYS_INFO_TIMERS, "timers" },
> - { SYS_INFO_LOCKS, "locks" },
> - { SYS_INFO_FTRACE, "ftrace" },
> - { SYS_INFO_ALL_BT, "all_bt" },
> - { SYS_INFO_BLOCKED_TASKS, "blocked_tasks" },
> +static const char * const si_names[] = {
> + [ilog2(SYS_INFO_TASKS)] = "tasks",
> + [ilog2(SYS_INFO_MEM)] = "mem",
> + [ilog2(SYS_INFO_TIMERS)] = "timers",
> + [ilog2(SYS_INFO_LOCKS)] = "locks",
> + [ilog2(SYS_INFO_FTRACE)] = "ftrace",
> + [ilog2(SYS_INFO_PANIC_CONSOLE_REPLAY)] = "",
> + [ilog2(SYS_INFO_ALL_BT)] = "all_bt",
> + [ilog2(SYS_INFO_BLOCKED_TASKS)] = "blocked_tasks",
> };
>
> /* Expecting string like "xxx_sys_info=tasks,mem,timers,locks,ftrace,..." */
> @@ -36,12 +35,9 @@ unsigned long sys_info_parse_param(char *str)
>
> 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;
> - }
> - }
> + i = match_string(si_names, ARRAY_SIZE(si_names), name);
> + if (i >= 0)
> + __set_bit(i, &si_bits);
> }
>
> return si_bits;
> @@ -85,12 +81,10 @@ int sysctl_sys_info_handler(const struct ctl_table *ro_table, int write,
> si_bits = READ_ONCE(*si_bits_global);
>
> names[0] = '\0';
> - for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> - if (si_bits & si_names[i].bit) {
> - len += scnprintf(names + len, sizeof(names) - len,
> - "%s%s", delim, si_names[i].name);
> - delim = ",";
> - }
> + for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> + len += scnprintf(names + len, sizeof(names) - len,
> + "%s%s", delim, si_names[i]);
> + delim = ",";
For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
an extra ','?
Thanks,
Feng
On Thu, Oct 30, 2025 at 10:01:49AM +0800, Feng Tang wrote:
> On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> > There is no need to keep a custom structure just for the need of
> > a plain array of strings. Replace struct sys_info_name with plain
> > array of strings.
> >
> > With that done, simplify the code, in particular, naturally use
> > for_each_set_bit() when iterating over si_bits_global bitmap.
...
> > names[0] = '\0';
> > - for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > - if (si_bits & si_names[i].bit) {
> > - len += scnprintf(names + len, sizeof(names) - len,
> > - "%s%s", delim, si_names[i].name);
> > - delim = ",";
> > - }
> > + for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> > + len += scnprintf(names + len, sizeof(names) - len,
> > + "%s%s", delim, si_names[i]);
> > + delim = ",";
>
> For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
> an extra ','?
If you look closer to the original code, the behaviour is the same. Feel free
to update behaviour separately as I tried to keep the functionality to be not
changed with my series (with the exceptions of the fetching issue).
--
With Best Regards,
Andy Shevchenko
On Thu, Oct 30, 2025 at 09:36:42AM +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 10:01:49AM +0800, Feng Tang wrote:
> > On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> > > There is no need to keep a custom structure just for the need of
> > > a plain array of strings. Replace struct sys_info_name with plain
> > > array of strings.
> > >
> > > With that done, simplify the code, in particular, naturally use
> > > for_each_set_bit() when iterating over si_bits_global bitmap.
>
> ...
>
> > > names[0] = '\0';
> > > - for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > > - if (si_bits & si_names[i].bit) {
> > > - len += scnprintf(names + len, sizeof(names) - len,
> > > - "%s%s", delim, si_names[i].name);
> > > - delim = ",";
> > > - }
> > > + for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> > > + len += scnprintf(names + len, sizeof(names) - len,
> > > + "%s%s", delim, si_names[i]);
> > > + delim = ",";
> >
> > For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
> > an extra ','?
>
> If you look closer to the original code, the behaviour is the same. Feel free
> to update behaviour separately as I tried to keep the functionality to be not
> changed with my series (with the exceptions of the fetching issue).
I gave the comment by code-reading.
And to double check it, I just run a simple test by adding "panic_print=0xff"
to cmdline, with the current kernel, by running "sysctl kernel.panic_sys_info"
on current kernel, it will get:
'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks'
And after applying your first 3 patches, it will show:
'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,,all_bt,blocked_task'
Thanks,
Feng
On Thu, Oct 30, 2025 at 04:45:24PM +0800, Feng Tang wrote:
> On Thu, Oct 30, 2025 at 09:36:42AM +0200, Andy Shevchenko wrote:
> > On Thu, Oct 30, 2025 at 10:01:49AM +0800, Feng Tang wrote:
> > > On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> > > > There is no need to keep a custom structure just for the need of
> > > > a plain array of strings. Replace struct sys_info_name with plain
> > > > array of strings.
> > > >
> > > > With that done, simplify the code, in particular, naturally use
> > > > for_each_set_bit() when iterating over si_bits_global bitmap.
...
> > > > names[0] = '\0';
> > > > - for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > > > - if (si_bits & si_names[i].bit) {
> > > > - len += scnprintf(names + len, sizeof(names) - len,
> > > > - "%s%s", delim, si_names[i].name);
> > > > - delim = ",";
> > > > - }
> > > > + for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> > > > + len += scnprintf(names + len, sizeof(names) - len,
> > > > + "%s%s", delim, si_names[i]);
> > > > + delim = ",";
> > >
> > > For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
> > > an extra ','?
> >
> > If you look closer to the original code, the behaviour is the same. Feel free
> > to update behaviour separately as I tried to keep the functionality to be not
> > changed with my series (with the exceptions of the fetching issue).
>
> I gave the comment by code-reading.
>
> And to double check it, I just run a simple test by adding "panic_print=0xff"
> to cmdline, with the current kernel, by running "sysctl kernel.panic_sys_info"
> on current kernel, it will get:
>
> 'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks'
>
> And after applying your first 3 patches, it will show:
>
> 'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,,all_bt,blocked_task'
Thanks for the test, now I see the issue! Before si_names has no entry for that bit.
I will address this in next version. Sorry for the confusion.
--
With Best Regards,
Andy Shevchenko
On Thu, Oct 30, 2025 at 11:05:59AM +0200, Andy Shevchenko wrote:
> On Thu, Oct 30, 2025 at 04:45:24PM +0800, Feng Tang wrote:
> > On Thu, Oct 30, 2025 at 09:36:42AM +0200, Andy Shevchenko wrote:
> > > On Thu, Oct 30, 2025 at 10:01:49AM +0800, Feng Tang wrote:
> > > > On Wed, Oct 29, 2025 at 12:07:38PM +0100, Andy Shevchenko wrote:
> > > > > There is no need to keep a custom structure just for the need of
> > > > > a plain array of strings. Replace struct sys_info_name with plain
> > > > > array of strings.
> > > > >
> > > > > With that done, simplify the code, in particular, naturally use
> > > > > for_each_set_bit() when iterating over si_bits_global bitmap.
>
> ...
>
> > > > > names[0] = '\0';
> > > > > - for (i = 0; i < ARRAY_SIZE(si_names); i++) {
> > > > > - if (si_bits & si_names[i].bit) {
> > > > > - len += scnprintf(names + len, sizeof(names) - len,
> > > > > - "%s%s", delim, si_names[i].name);
> > > > > - delim = ",";
> > > > > - }
> > > > > + for_each_set_bit(i, &si_bits, ARRAY_SIZE(si_names)) {
> > > > > + len += scnprintf(names + len, sizeof(names) - len,
> > > > > + "%s%s", delim, si_names[i]);
> > > > > + delim = ",";
> > > >
> > > > For SYS_INFO_PANIC_CONSOLE_REPLAY bit, it is a NULL string, no need for
> > > > an extra ','?
> > >
> > > If you look closer to the original code, the behaviour is the same. Feel free
> > > to update behaviour separately as I tried to keep the functionality to be not
> > > changed with my series (with the exceptions of the fetching issue).
> >
> > I gave the comment by code-reading.
> >
> > And to double check it, I just run a simple test by adding "panic_print=0xff"
> > to cmdline, with the current kernel, by running "sysctl kernel.panic_sys_info"
> > on current kernel, it will get:
> >
> > 'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,all_bt,blocked_tasks'
> >
> > And after applying your first 3 patches, it will show:
> >
> > 'kernel.panic_sys_info = tasks,mem,timers,locks,ftrace,,all_bt,blocked_task'
>
> Thanks for the test, now I see the issue! Before si_names has no entry for that bit.
> I will address this in next version. Sorry for the confusion.
No problem. Thanks for helping to improve the code!
- Feng
© 2016 - 2026 Red Hat, Inc.