| Message ID | f036c5b0-20cc-40c1-85f9-69fa9edd0c95@paulmck-laptop (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Masami Hiramatsu |
| Headers | show |
| Series | [fs/proc/bootconfig] remove redundant comments from /proc/bootconfig | expand |
On Wed, 3 Apr 2024 12:16:28 -0700 "Paul E. McKenney" <paulmck@kernel.org> wrote: > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > /proc/bootconfig shows boot_command_line[] multiple times following > every xbc key value pair, that's duplicated and not necessary. > Remove redundant ones. > > Output before and after the fix is like: > key1 = value1 > *bootloader argument comments* > key2 = value2 > *bootloader argument comments* > key3 = value3 > *bootloader argument comments* > ... > > key1 = value1 > key2 = value2 > key3 = value3 > *bootloader argument comments* > ... > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > Cc: Masami Hiramatsu <mhiramat@kernel.org> > Cc: <linux-trace-kernel@vger.kernel.org> > Cc: <linux-fsdevel@vger.kernel.org> OOps, good catch! Let me pick it. Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thank you! > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > index 902b326e1e560..e5635a6b127b0 100644 > --- a/fs/proc/bootconfig.c > +++ b/fs/proc/bootconfig.c > @@ -62,12 +62,12 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > break; > dst += ret; > } > - if (ret >= 0 && boot_command_line[0]) { > - ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > - boot_command_line); > - if (ret > 0) > - dst += ret; > - } > + } > + if (ret >= 0 && boot_command_line[0]) { > + ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > + boot_command_line); > + if (ret > 0) > + dst += ret; > } > out: > kfree(key);
On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote: > On Wed, 3 Apr 2024 12:16:28 -0700 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > > > /proc/bootconfig shows boot_command_line[] multiple times following > > every xbc key value pair, that's duplicated and not necessary. > > Remove redundant ones. > > > > Output before and after the fix is like: > > key1 = value1 > > *bootloader argument comments* > > key2 = value2 > > *bootloader argument comments* > > key3 = value3 > > *bootloader argument comments* > > ... > > > > key1 = value1 > > key2 = value2 > > key3 = value3 > > *bootloader argument comments* > > ... > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > Cc: <linux-trace-kernel@vger.kernel.org> > > Cc: <linux-fsdevel@vger.kernel.org> > > OOps, good catch! Let me pick it. > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Thank you, and I have applied your ack and pulled this into its own bootconfig.2024.04.04a. My guess is that you will push this via your own tree, and so I will drop my copy as soon as yours hits -next. Thanx, Paul > Thank you! > > > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > index 902b326e1e560..e5635a6b127b0 100644 > > --- a/fs/proc/bootconfig.c > > +++ b/fs/proc/bootconfig.c > > @@ -62,12 +62,12 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > break; > > dst += ret; > > } > > - if (ret >= 0 && boot_command_line[0]) { > > - ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > - boot_command_line); > > - if (ret > 0) > > - dst += ret; > > - } > > + } > > + if (ret >= 0 && boot_command_line[0]) { > > + ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > + boot_command_line); > > + if (ret > 0) > > + dst += ret; > > } > > out: > > kfree(key); > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Thu, 4 Apr 2024 10:43:14 -0700 "Paul E. McKenney" <paulmck@kernel.org> wrote: > On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote: > > On Wed, 3 Apr 2024 12:16:28 -0700 > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > > > > > /proc/bootconfig shows boot_command_line[] multiple times following > > > every xbc key value pair, that's duplicated and not necessary. > > > Remove redundant ones. > > > > > > Output before and after the fix is like: > > > key1 = value1 > > > *bootloader argument comments* > > > key2 = value2 > > > *bootloader argument comments* > > > key3 = value3 > > > *bootloader argument comments* > > > ... > > > > > > key1 = value1 > > > key2 = value2 > > > key3 = value3 > > > *bootloader argument comments* > > > ... > > > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > > Cc: <linux-trace-kernel@vger.kernel.org> > > > Cc: <linux-fsdevel@vger.kernel.org> > > > > OOps, good catch! Let me pick it. > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Thank you, and I have applied your ack and pulled this into its own > bootconfig.2024.04.04a. > > My guess is that you will push this via your own tree, and so I will > drop my copy as soon as yours hits -next. Thanks! I would like to make PR this soon as bootconfig fixes for v6.9-rc2. Thank you, > > Thanx, Paul > > > Thank you! > > > > > > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > > index 902b326e1e560..e5635a6b127b0 100644 > > > --- a/fs/proc/bootconfig.c > > > +++ b/fs/proc/bootconfig.c > > > @@ -62,12 +62,12 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > > break; > > > dst += ret; > > > } > > > - if (ret >= 0 && boot_command_line[0]) { > > > - ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > - boot_command_line); > > > - if (ret > 0) > > > - dst += ret; > > > - } > > > + } > > > + if (ret >= 0 && boot_command_line[0]) { > > > + ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > + boot_command_line); > > > + if (ret > 0) > > > + dst += ret; > > > } > > > out: > > > kfree(key); > > > > > > -- > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Fri, 5 Apr 2024 10:23:24 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > On Thu, 4 Apr 2024 10:43:14 -0700 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote: > > > On Wed, 3 Apr 2024 12:16:28 -0700 > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > > > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > > > > > > > /proc/bootconfig shows boot_command_line[] multiple times following > > > > every xbc key value pair, that's duplicated and not necessary. > > > > Remove redundant ones. > > > > > > > > Output before and after the fix is like: > > > > key1 = value1 > > > > *bootloader argument comments* > > > > key2 = value2 > > > > *bootloader argument comments* > > > > key3 = value3 > > > > *bootloader argument comments* > > > > ... > > > > > > > > key1 = value1 > > > > key2 = value2 > > > > key3 = value3 > > > > *bootloader argument comments* > > > > ... > > > > > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > > > Cc: <linux-trace-kernel@vger.kernel.org> > > > > Cc: <linux-fsdevel@vger.kernel.org> > > > > > > OOps, good catch! Let me pick it. > > > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > Thank you, and I have applied your ack and pulled this into its own > > bootconfig.2024.04.04a. > > > > My guess is that you will push this via your own tree, and so I will > > drop my copy as soon as yours hits -next. > > Thanks! I would like to make PR this soon as bootconfig fixes for v6.9-rc2. > Hmm I found that this always shows the command line comment in /proc/bootconfig even without "bootconfig" option. I think that is easier for user-tools but changes the behavior and a bit redundant. We should skip showing this original argument comment if bootconfig is not initialized (no "bootconfig" in cmdline) as it is now. Thank you, > Thank you, > > > > > Thanx, Paul > > > > > Thank you! > > > > > > > > > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > > > index 902b326e1e560..e5635a6b127b0 100644 > > > > --- a/fs/proc/bootconfig.c > > > > +++ b/fs/proc/bootconfig.c > > > > @@ -62,12 +62,12 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > > > break; > > > > dst += ret; > > > > } > > > > - if (ret >= 0 && boot_command_line[0]) { > > > > - ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > > - boot_command_line); > > > > - if (ret > 0) > > > > - dst += ret; > > > > - } > > > > + } > > > > + if (ret >= 0 && boot_command_line[0]) { > > > > + ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > > + boot_command_line); > > > > + if (ret > 0) > > > > + dst += ret; > > > > } > > > > out: > > > > kfree(key); > > > > > > > > > -- > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Fri, Apr 05, 2024 at 11:57:45AM +0900, Masami Hiramatsu wrote: > On Fri, 5 Apr 2024 10:23:24 +0900 > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > On Thu, 4 Apr 2024 10:43:14 -0700 > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote: > > > > On Wed, 3 Apr 2024 12:16:28 -0700 > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > > > > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > > > > > > > > > /proc/bootconfig shows boot_command_line[] multiple times following > > > > > every xbc key value pair, that's duplicated and not necessary. > > > > > Remove redundant ones. > > > > > > > > > > Output before and after the fix is like: > > > > > key1 = value1 > > > > > *bootloader argument comments* > > > > > key2 = value2 > > > > > *bootloader argument comments* > > > > > key3 = value3 > > > > > *bootloader argument comments* > > > > > ... > > > > > > > > > > key1 = value1 > > > > > key2 = value2 > > > > > key3 = value3 > > > > > *bootloader argument comments* > > > > > ... > > > > > > > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > > > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > > > > Cc: <linux-trace-kernel@vger.kernel.org> > > > > > Cc: <linux-fsdevel@vger.kernel.org> > > > > > > > > OOps, good catch! Let me pick it. > > > > > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > Thank you, and I have applied your ack and pulled this into its own > > > bootconfig.2024.04.04a. > > > > > > My guess is that you will push this via your own tree, and so I will > > > drop my copy as soon as yours hits -next. > > > > Thanks! I would like to make PR this soon as bootconfig fixes for v6.9-rc2. > > Hmm I found that this always shows the command line comment in > /proc/bootconfig even without "bootconfig" option. > I think that is easier for user-tools but changes the behavior and > a bit redundant. > > We should skip showing this original argument comment if bootconfig is > not initialized (no "bootconfig" in cmdline) as it is now. So something like this folded into that patch? ------------------------------------------------------------------------ diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c index e5635a6b127b0..7d2520378f5f2 100644 --- a/fs/proc/bootconfig.c +++ b/fs/proc/bootconfig.c @@ -63,7 +63,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) dst += ret; } } - if (ret >= 0 && boot_command_line[0]) { + if (bootconfig_is_present() && ret >= 0 && boot_command_line[0]) { ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", boot_command_line); if (ret > 0) diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h index ca73940e26df8..ef70d1b381421 100644 --- a/include/linux/bootconfig.h +++ b/include/linux/bootconfig.h @@ -10,6 +10,7 @@ #ifdef __KERNEL__ #include <linux/kernel.h> #include <linux/types.h> +int bootconfig_is_present(void); #else /* !__KERNEL__ */ /* * NOTE: This is only for tools/bootconfig, because tools/bootconfig will diff --git a/init/main.c b/init/main.c index 2ca52474d0c30..720a669b1493d 100644 --- a/init/main.c +++ b/init/main.c @@ -1572,3 +1572,8 @@ static noinline void __init kernel_init_freeable(void) integrity_load_keys(); } + +int bootconfig_is_present(void) +{ + return bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE); +} ------------------------------------------------------------------------ Give or take placement of the bootconfig_is_present() function's declaration and definition. Thanx, Paul Thanx, Paul > Thank you, > > > > Thank you, > > > > > > > > Thanx, Paul > > > > > > > Thank you! > > > > > > > > > > > > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > > > > index 902b326e1e560..e5635a6b127b0 100644 > > > > > --- a/fs/proc/bootconfig.c > > > > > +++ b/fs/proc/bootconfig.c > > > > > @@ -62,12 +62,12 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > > > > break; > > > > > dst += ret; > > > > > } > > > > > - if (ret >= 0 && boot_command_line[0]) { > > > > > - ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > > > - boot_command_line); > > > > > - if (ret > 0) > > > > > - dst += ret; > > > > > - } > > > > > + } > > > > > + if (ret >= 0 && boot_command_line[0]) { > > > > > + ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > > > + boot_command_line); > > > > > + if (ret > 0) > > > > > + dst += ret; > > > > > } > > > > > out: > > > > > kfree(key); > > > > > > > > > > > > -- > > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > -- > > Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Thu, 4 Apr 2024 21:25:41 -0700 "Paul E. McKenney" <paulmck@kernel.org> wrote: > On Fri, Apr 05, 2024 at 11:57:45AM +0900, Masami Hiramatsu wrote: > > On Fri, 5 Apr 2024 10:23:24 +0900 > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > > > On Thu, 4 Apr 2024 10:43:14 -0700 > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote: > > > > > On Wed, 3 Apr 2024 12:16:28 -0700 > > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > > > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > > > > > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > > > > > > > > > > > /proc/bootconfig shows boot_command_line[] multiple times following > > > > > > every xbc key value pair, that's duplicated and not necessary. > > > > > > Remove redundant ones. > > > > > > > > > > > > Output before and after the fix is like: > > > > > > key1 = value1 > > > > > > *bootloader argument comments* > > > > > > key2 = value2 > > > > > > *bootloader argument comments* > > > > > > key3 = value3 > > > > > > *bootloader argument comments* > > > > > > ... > > > > > > > > > > > > key1 = value1 > > > > > > key2 = value2 > > > > > > key3 = value3 > > > > > > *bootloader argument comments* > > > > > > ... > > > > > > > > > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > > > > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > > > > > Cc: <linux-trace-kernel@vger.kernel.org> > > > > > > Cc: <linux-fsdevel@vger.kernel.org> > > > > > > > > > > OOps, good catch! Let me pick it. > > > > > > > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > > > Thank you, and I have applied your ack and pulled this into its own > > > > bootconfig.2024.04.04a. > > > > > > > > My guess is that you will push this via your own tree, and so I will > > > > drop my copy as soon as yours hits -next. > > > > > > Thanks! I would like to make PR this soon as bootconfig fixes for v6.9-rc2. > > > > Hmm I found that this always shows the command line comment in > > /proc/bootconfig even without "bootconfig" option. > > I think that is easier for user-tools but changes the behavior and > > a bit redundant. > > > > We should skip showing this original argument comment if bootconfig is > > not initialized (no "bootconfig" in cmdline) as it is now. > > So something like this folded into that patch? Hm, I expected just checking it in the loop as below. ------------------------------------------------------------------------ diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c index e5635a6b127b..98e0780f7e07 100644 --- a/fs/proc/bootconfig.c +++ b/fs/proc/bootconfig.c @@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) { struct xbc_node *leaf, *vnode; char *key, *end = dst + size; + bool empty = true; const char *val; char q; int ret = 0; @@ -62,8 +63,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) break; dst += ret; } + empty = false; } - if (ret >= 0 && boot_command_line[0]) { + if (!empty && ret >= 0 && boot_command_line[0]) { ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", boot_command_line); if (ret > 0) ------------------------------------------------------------------------ The difference is checking "bootconfig" cmdline option or checking the "bootconfig" is actually empty. So the behaviors are different when the "bootconfig" is specified but there is no bootconfig data. Another idea is to check whether the cmdline is actually updated by bootconfig and show original one only if it is updated. (I think this fits the purpose of the original patch better.) ------------------------------------------------------------------------ diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c index e5635a6b127b..95d6a231210c 100644 --- a/fs/proc/bootconfig.c +++ b/fs/proc/bootconfig.c @@ -10,6 +10,9 @@ #include <linux/bootconfig.h> #include <linux/slab.h> +/* defined in main/init.c */ +bool __init cmdline_has_extra_options(void); + static char *saved_boot_config; static int boot_config_proc_show(struct seq_file *m, void *v) @@ -63,7 +66,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) dst += ret; } } - if (ret >= 0 && boot_command_line[0]) { + if (cmdline_has_extra_options() && ret >= 0 && boot_command_line[0]) { ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", boot_command_line); if (ret > 0) diff --git a/init/main.c b/init/main.c index 2ca52474d0c3..881f6230ee59 100644 --- a/init/main.c +++ b/init/main.c @@ -487,6 +487,11 @@ static int __init warn_bootconfig(char *str) early_param("bootconfig", warn_bootconfig); +bool __init cmdline_has_extra_options(void) +{ + return extra_command_line || extra_init_args; +} + /* Change NUL term back to "=", to make "param" the whole string. */ static void __init repair_env_string(char *param, char *val) { ------------------------------------------------------------------------ Thank you, > > ------------------------------------------------------------------------ > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > index e5635a6b127b0..7d2520378f5f2 100644 > --- a/fs/proc/bootconfig.c > +++ b/fs/proc/bootconfig.c > @@ -63,7 +63,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > dst += ret; > } > } > - if (ret >= 0 && boot_command_line[0]) { > + if (bootconfig_is_present() && ret >= 0 && boot_command_line[0]) { > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > boot_command_line); > if (ret > 0) > diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h > index ca73940e26df8..ef70d1b381421 100644 > --- a/include/linux/bootconfig.h > +++ b/include/linux/bootconfig.h > @@ -10,6 +10,7 @@ > #ifdef __KERNEL__ > #include <linux/kernel.h> > #include <linux/types.h> > +int bootconfig_is_present(void); > #else /* !__KERNEL__ */ > /* > * NOTE: This is only for tools/bootconfig, because tools/bootconfig will > diff --git a/init/main.c b/init/main.c > index 2ca52474d0c30..720a669b1493d 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -1572,3 +1572,8 @@ static noinline void __init kernel_init_freeable(void) > > integrity_load_keys(); > } > + > +int bootconfig_is_present(void) > +{ > + return bootconfig_found || IS_ENABLED(CONFIG_BOOT_CONFIG_FORCE); > +} > > ------------------------------------------------------------------------ > > Give or take placement of the bootconfig_is_present() function's > declaration and definition. > > Thanx, Paul > > Thanx, Paul > > > Thank you, > > > > > > > Thank you, > > > > > > > > > > > Thanx, Paul > > > > > > > > > Thank you! > > > > > > > > > > > > > > > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > > > > > index 902b326e1e560..e5635a6b127b0 100644 > > > > > > --- a/fs/proc/bootconfig.c > > > > > > +++ b/fs/proc/bootconfig.c > > > > > > @@ -62,12 +62,12 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > > > > > break; > > > > > > dst += ret; > > > > > > } > > > > > > - if (ret >= 0 && boot_command_line[0]) { > > > > > > - ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > > > > - boot_command_line); > > > > > > - if (ret > 0) > > > > > > - dst += ret; > > > > > > - } > > > > > > + } > > > > > > + if (ret >= 0 && boot_command_line[0]) { > > > > > > + ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > > > > + boot_command_line); > > > > > > + if (ret > 0) > > > > > > + dst += ret; > > > > > > } > > > > > > out: > > > > > > kfree(key); > > > > > > > > > > > > > > > -- > > > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > > > > -- > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > -- > > Masami Hiramatsu (Google) <mhiramat@kernel.org>
On Sat, Apr 06, 2024 at 11:11:08AM +0900, Masami Hiramatsu wrote: > On Thu, 4 Apr 2024 21:25:41 -0700 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > On Fri, Apr 05, 2024 at 11:57:45AM +0900, Masami Hiramatsu wrote: > > > On Fri, 5 Apr 2024 10:23:24 +0900 > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > > > > > On Thu, 4 Apr 2024 10:43:14 -0700 > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > > > On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote: > > > > > > On Wed, 3 Apr 2024 12:16:28 -0700 > > > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > > > > > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > > > > > > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > > > > > > > > > > > > > /proc/bootconfig shows boot_command_line[] multiple times following > > > > > > > every xbc key value pair, that's duplicated and not necessary. > > > > > > > Remove redundant ones. > > > > > > > > > > > > > > Output before and after the fix is like: > > > > > > > key1 = value1 > > > > > > > *bootloader argument comments* > > > > > > > key2 = value2 > > > > > > > *bootloader argument comments* > > > > > > > key3 = value3 > > > > > > > *bootloader argument comments* > > > > > > > ... > > > > > > > > > > > > > > key1 = value1 > > > > > > > key2 = value2 > > > > > > > key3 = value3 > > > > > > > *bootloader argument comments* > > > > > > > ... > > > > > > > > > > > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > > > > > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > > > > > > Cc: <linux-trace-kernel@vger.kernel.org> > > > > > > > Cc: <linux-fsdevel@vger.kernel.org> > > > > > > > > > > > > OOps, good catch! Let me pick it. > > > > > > > > > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > > > > > Thank you, and I have applied your ack and pulled this into its own > > > > > bootconfig.2024.04.04a. > > > > > > > > > > My guess is that you will push this via your own tree, and so I will > > > > > drop my copy as soon as yours hits -next. > > > > > > > > Thanks! I would like to make PR this soon as bootconfig fixes for v6.9-rc2. > > > > > > Hmm I found that this always shows the command line comment in > > > /proc/bootconfig even without "bootconfig" option. > > > I think that is easier for user-tools but changes the behavior and > > > a bit redundant. > > > > > > We should skip showing this original argument comment if bootconfig is > > > not initialized (no "bootconfig" in cmdline) as it is now. > > > > So something like this folded into that patch? > > Hm, I expected just checking it in the loop as below. > > ------------------------------------------------------------------------ > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > index e5635a6b127b..98e0780f7e07 100644 > --- a/fs/proc/bootconfig.c > +++ b/fs/proc/bootconfig.c > @@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > { > struct xbc_node *leaf, *vnode; > char *key, *end = dst + size; > + bool empty = true; > const char *val; > char q; > int ret = 0; > @@ -62,8 +63,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > break; > dst += ret; > } > + empty = false; > } > - if (ret >= 0 && boot_command_line[0]) { > + if (!empty && ret >= 0 && boot_command_line[0]) { > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > boot_command_line); > if (ret > 0) > > ------------------------------------------------------------------------ > > The difference is checking "bootconfig" cmdline option or checking > the "bootconfig" is actually empty. So the behaviors are different > when the "bootconfig" is specified but there is no bootconfig data. Ah, understood, the point is to avoid the comment in cases where its content would be identical to /proc/cmdline. > Another idea is to check whether the cmdline is actually updated by > bootconfig and show original one only if it is updated. > (I think this fits the purpose of the original patch better.) > > ------------------------------------------------------------------------ > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > index e5635a6b127b..95d6a231210c 100644 > --- a/fs/proc/bootconfig.c > +++ b/fs/proc/bootconfig.c > @@ -10,6 +10,9 @@ > #include <linux/bootconfig.h> > #include <linux/slab.h> > > +/* defined in main/init.c */ > +bool __init cmdline_has_extra_options(void); > + > static char *saved_boot_config; > > static int boot_config_proc_show(struct seq_file *m, void *v) > @@ -63,7 +66,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > dst += ret; > } > } > - if (ret >= 0 && boot_command_line[0]) { > + if (cmdline_has_extra_options() && ret >= 0 && boot_command_line[0]) { > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > boot_command_line); > if (ret > 0) > diff --git a/init/main.c b/init/main.c > index 2ca52474d0c3..881f6230ee59 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -487,6 +487,11 @@ static int __init warn_bootconfig(char *str) > > early_param("bootconfig", warn_bootconfig); > > +bool __init cmdline_has_extra_options(void) > +{ > + return extra_command_line || extra_init_args; > +} > + > /* Change NUL term back to "=", to make "param" the whole string. */ > static void __init repair_env_string(char *param, char *val) > { > ------------------------------------------------------------------------ This one looks good to me! I had to move the declaration from /fs/proc/bootconfig.c to include/linux/bootconfig.h in order to avoid a build error. (The build system wants the declaration and definition to be visible as a cross-check.) Does the resulting patch below work for you? Thanx, Paul ------------------------------------------------------------------------ commit 8d95b50c523fba7133368650b3b5f71b169c76b5 Author: Masami Hiramatsu <mhiramat@kernel.org> Date: Mon Apr 8 12:10:38 2024 -0700 fs/proc: Skip bootloader comment if no embedded kernel parameters If the "bootconfig" kernel command-line argument was specified or if the kernel was built with CONFIG_BOOT_CONFIG_FORCE, but if there are no embedded kernel parameter, omit the "# Parameters from bootloader:" comment from the /proc/bootconfig file. This will cause automation to fall back to the /proc/cmdline file, which will be identical to the comment in this no-embedded-kernel-parameters case. Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> Signed-off-by: Paul E. McKenney <paulmck@kernel.org> diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c index e5635a6b127b0..87dcaae32ff87 100644 --- a/fs/proc/bootconfig.c +++ b/fs/proc/bootconfig.c @@ -63,7 +63,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) dst += ret; } } - if (ret >= 0 && boot_command_line[0]) { + if (cmdline_has_extra_options() && ret >= 0 && boot_command_line[0]) { ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", boot_command_line); if (ret > 0) diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h index ca73940e26df8..e5ee2c694401e 100644 --- a/include/linux/bootconfig.h +++ b/include/linux/bootconfig.h @@ -10,6 +10,7 @@ #ifdef __KERNEL__ #include <linux/kernel.h> #include <linux/types.h> +bool __init cmdline_has_extra_options(void); #else /* !__KERNEL__ */ /* * NOTE: This is only for tools/bootconfig, because tools/bootconfig will diff --git a/init/main.c b/init/main.c index 2ca52474d0c30..881f6230ee59e 100644 --- a/init/main.c +++ b/init/main.c @@ -487,6 +487,11 @@ static int __init warn_bootconfig(char *str) early_param("bootconfig", warn_bootconfig); +bool __init cmdline_has_extra_options(void) +{ + return extra_command_line || extra_init_args; +} + /* Change NUL term back to "=", to make "param" the whole string. */ static void __init repair_env_string(char *param, char *val) {
On Mon, 8 Apr 2024 12:18:19 -0700 "Paul E. McKenney" <paulmck@kernel.org> wrote: > On Sat, Apr 06, 2024 at 11:11:08AM +0900, Masami Hiramatsu wrote: > > On Thu, 4 Apr 2024 21:25:41 -0700 > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > On Fri, Apr 05, 2024 at 11:57:45AM +0900, Masami Hiramatsu wrote: > > > > On Fri, 5 Apr 2024 10:23:24 +0900 > > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > > > > > > > On Thu, 4 Apr 2024 10:43:14 -0700 > > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > > > > > On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote: > > > > > > > On Wed, 3 Apr 2024 12:16:28 -0700 > > > > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > > > > > > > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > > > > > > > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > > > > > > > > > > > > > > > /proc/bootconfig shows boot_command_line[] multiple times following > > > > > > > > every xbc key value pair, that's duplicated and not necessary. > > > > > > > > Remove redundant ones. > > > > > > > > > > > > > > > > Output before and after the fix is like: > > > > > > > > key1 = value1 > > > > > > > > *bootloader argument comments* > > > > > > > > key2 = value2 > > > > > > > > *bootloader argument comments* > > > > > > > > key3 = value3 > > > > > > > > *bootloader argument comments* > > > > > > > > ... > > > > > > > > > > > > > > > > key1 = value1 > > > > > > > > key2 = value2 > > > > > > > > key3 = value3 > > > > > > > > *bootloader argument comments* > > > > > > > > ... > > > > > > > > > > > > > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > > > > > > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > > > > > > > Cc: <linux-trace-kernel@vger.kernel.org> > > > > > > > > Cc: <linux-fsdevel@vger.kernel.org> > > > > > > > > > > > > > > OOps, good catch! Let me pick it. > > > > > > > > > > > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > > > > > > > Thank you, and I have applied your ack and pulled this into its own > > > > > > bootconfig.2024.04.04a. > > > > > > > > > > > > My guess is that you will push this via your own tree, and so I will > > > > > > drop my copy as soon as yours hits -next. > > > > > > > > > > Thanks! I would like to make PR this soon as bootconfig fixes for v6.9-rc2. > > > > > > > > Hmm I found that this always shows the command line comment in > > > > /proc/bootconfig even without "bootconfig" option. > > > > I think that is easier for user-tools but changes the behavior and > > > > a bit redundant. > > > > > > > > We should skip showing this original argument comment if bootconfig is > > > > not initialized (no "bootconfig" in cmdline) as it is now. > > > > > > So something like this folded into that patch? > > > > Hm, I expected just checking it in the loop as below. > > > > ------------------------------------------------------------------------ > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > index e5635a6b127b..98e0780f7e07 100644 > > --- a/fs/proc/bootconfig.c > > +++ b/fs/proc/bootconfig.c > > @@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > { > > struct xbc_node *leaf, *vnode; > > char *key, *end = dst + size; > > + bool empty = true; > > const char *val; > > char q; > > int ret = 0; > > @@ -62,8 +63,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > break; > > dst += ret; > > } > > + empty = false; > > } > > - if (ret >= 0 && boot_command_line[0]) { > > + if (!empty && ret >= 0 && boot_command_line[0]) { > > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > boot_command_line); > > if (ret > 0) > > > > ------------------------------------------------------------------------ > > > > The difference is checking "bootconfig" cmdline option or checking > > the "bootconfig" is actually empty. So the behaviors are different > > when the "bootconfig" is specified but there is no bootconfig data. > > Ah, understood, the point is to avoid the comment in cases where its > content would be identical to /proc/cmdline. > > > Another idea is to check whether the cmdline is actually updated by > > bootconfig and show original one only if it is updated. > > (I think this fits the purpose of the original patch better.) > > > > ------------------------------------------------------------------------ > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > index e5635a6b127b..95d6a231210c 100644 > > --- a/fs/proc/bootconfig.c > > +++ b/fs/proc/bootconfig.c > > @@ -10,6 +10,9 @@ > > #include <linux/bootconfig.h> > > #include <linux/slab.h> > > > > +/* defined in main/init.c */ > > +bool __init cmdline_has_extra_options(void); > > + > > static char *saved_boot_config; > > > > static int boot_config_proc_show(struct seq_file *m, void *v) > > @@ -63,7 +66,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > dst += ret; > > } > > } > > - if (ret >= 0 && boot_command_line[0]) { > > + if (cmdline_has_extra_options() && ret >= 0 && boot_command_line[0]) { > > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > boot_command_line); > > if (ret > 0) > > diff --git a/init/main.c b/init/main.c > > index 2ca52474d0c3..881f6230ee59 100644 > > --- a/init/main.c > > +++ b/init/main.c > > @@ -487,6 +487,11 @@ static int __init warn_bootconfig(char *str) > > > > early_param("bootconfig", warn_bootconfig); > > > > +bool __init cmdline_has_extra_options(void) > > +{ > > + return extra_command_line || extra_init_args; > > +} > > + > > /* Change NUL term back to "=", to make "param" the whole string. */ > > static void __init repair_env_string(char *param, char *val) > > { > > ------------------------------------------------------------------------ > > This one looks good to me! > > I had to move the declaration from /fs/proc/bootconfig.c to > include/linux/bootconfig.h in order to avoid a build error. (The > build system wants the declaration and definition to be visible as > a cross-check.) > > Does the resulting patch below work for you? Yes, this looks good to me. BTW, shouldn't we push this with the previous fix (from the viewpoint of memory usage, this is a kind of fix for redundant memory usage)? Thank you! > > Thanx, Paul > > ------------------------------------------------------------------------ > > commit 8d95b50c523fba7133368650b3b5f71b169c76b5 > Author: Masami Hiramatsu <mhiramat@kernel.org> > Date: Mon Apr 8 12:10:38 2024 -0700 > > fs/proc: Skip bootloader comment if no embedded kernel parameters > > If the "bootconfig" kernel command-line argument was specified or if > the kernel was built with CONFIG_BOOT_CONFIG_FORCE, but if there are > no embedded kernel parameter, omit the "# Parameters from bootloader:" > comment from the /proc/bootconfig file. This will cause automation > to fall back to the /proc/cmdline file, which will be identical to the > comment in this no-embedded-kernel-parameters case. > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > index e5635a6b127b0..87dcaae32ff87 100644 > --- a/fs/proc/bootconfig.c > +++ b/fs/proc/bootconfig.c > @@ -63,7 +63,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > dst += ret; > } > } > - if (ret >= 0 && boot_command_line[0]) { > + if (cmdline_has_extra_options() && ret >= 0 && boot_command_line[0]) { > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > boot_command_line); > if (ret > 0) > diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h > index ca73940e26df8..e5ee2c694401e 100644 > --- a/include/linux/bootconfig.h > +++ b/include/linux/bootconfig.h > @@ -10,6 +10,7 @@ > #ifdef __KERNEL__ > #include <linux/kernel.h> > #include <linux/types.h> > +bool __init cmdline_has_extra_options(void); > #else /* !__KERNEL__ */ > /* > * NOTE: This is only for tools/bootconfig, because tools/bootconfig will > diff --git a/init/main.c b/init/main.c > index 2ca52474d0c30..881f6230ee59e 100644 > --- a/init/main.c > +++ b/init/main.c > @@ -487,6 +487,11 @@ static int __init warn_bootconfig(char *str) > > early_param("bootconfig", warn_bootconfig); > > +bool __init cmdline_has_extra_options(void) > +{ > + return extra_command_line || extra_init_args; > +} > + > /* Change NUL term back to "=", to make "param" the whole string. */ > static void __init repair_env_string(char *param, char *val) > {
On Tue, Apr 09, 2024 at 09:25:40AM +0900, Masami Hiramatsu wrote: > On Mon, 8 Apr 2024 12:18:19 -0700 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > On Sat, Apr 06, 2024 at 11:11:08AM +0900, Masami Hiramatsu wrote: > > > On Thu, 4 Apr 2024 21:25:41 -0700 > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > On Fri, Apr 05, 2024 at 11:57:45AM +0900, Masami Hiramatsu wrote: > > > > > On Fri, 5 Apr 2024 10:23:24 +0900 > > > > > Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > > > > > > > > > > On Thu, 4 Apr 2024 10:43:14 -0700 > > > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > > > > > > > On Thu, Apr 04, 2024 at 08:55:22AM +0900, Masami Hiramatsu wrote: > > > > > > > > On Wed, 3 Apr 2024 12:16:28 -0700 > > > > > > > > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > > > > > > > > > > > > > > > commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to > > > > > > > > > /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. > > > > > > > > > > > > > > > > > > /proc/bootconfig shows boot_command_line[] multiple times following > > > > > > > > > every xbc key value pair, that's duplicated and not necessary. > > > > > > > > > Remove redundant ones. > > > > > > > > > > > > > > > > > > Output before and after the fix is like: > > > > > > > > > key1 = value1 > > > > > > > > > *bootloader argument comments* > > > > > > > > > key2 = value2 > > > > > > > > > *bootloader argument comments* > > > > > > > > > key3 = value3 > > > > > > > > > *bootloader argument comments* > > > > > > > > > ... > > > > > > > > > > > > > > > > > > key1 = value1 > > > > > > > > > key2 = value2 > > > > > > > > > key3 = value3 > > > > > > > > > *bootloader argument comments* > > > > > > > > > ... > > > > > > > > > > > > > > > > > > Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") > > > > > > > > > Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> > > > > > > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > > > > > > Cc: Masami Hiramatsu <mhiramat@kernel.org> > > > > > > > > > Cc: <linux-trace-kernel@vger.kernel.org> > > > > > > > > > Cc: <linux-fsdevel@vger.kernel.org> > > > > > > > > > > > > > > > > OOps, good catch! Let me pick it. > > > > > > > > > > > > > > > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > > > > > > > > > > > > > Thank you, and I have applied your ack and pulled this into its own > > > > > > > bootconfig.2024.04.04a. > > > > > > > > > > > > > > My guess is that you will push this via your own tree, and so I will > > > > > > > drop my copy as soon as yours hits -next. > > > > > > > > > > > > Thanks! I would like to make PR this soon as bootconfig fixes for v6.9-rc2. > > > > > > > > > > Hmm I found that this always shows the command line comment in > > > > > /proc/bootconfig even without "bootconfig" option. > > > > > I think that is easier for user-tools but changes the behavior and > > > > > a bit redundant. > > > > > > > > > > We should skip showing this original argument comment if bootconfig is > > > > > not initialized (no "bootconfig" in cmdline) as it is now. > > > > > > > > So something like this folded into that patch? > > > > > > Hm, I expected just checking it in the loop as below. > > > > > > ------------------------------------------------------------------------ > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > > index e5635a6b127b..98e0780f7e07 100644 > > > --- a/fs/proc/bootconfig.c > > > +++ b/fs/proc/bootconfig.c > > > @@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > > { > > > struct xbc_node *leaf, *vnode; > > > char *key, *end = dst + size; > > > + bool empty = true; > > > const char *val; > > > char q; > > > int ret = 0; > > > @@ -62,8 +63,9 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > > break; > > > dst += ret; > > > } > > > + empty = false; > > > } > > > - if (ret >= 0 && boot_command_line[0]) { > > > + if (!empty && ret >= 0 && boot_command_line[0]) { > > > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > boot_command_line); > > > if (ret > 0) > > > > > > ------------------------------------------------------------------------ > > > > > > The difference is checking "bootconfig" cmdline option or checking > > > the "bootconfig" is actually empty. So the behaviors are different > > > when the "bootconfig" is specified but there is no bootconfig data. > > > > Ah, understood, the point is to avoid the comment in cases where its > > content would be identical to /proc/cmdline. > > > > > Another idea is to check whether the cmdline is actually updated by > > > bootconfig and show original one only if it is updated. > > > (I think this fits the purpose of the original patch better.) > > > > > > ------------------------------------------------------------------------ > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > > index e5635a6b127b..95d6a231210c 100644 > > > --- a/fs/proc/bootconfig.c > > > +++ b/fs/proc/bootconfig.c > > > @@ -10,6 +10,9 @@ > > > #include <linux/bootconfig.h> > > > #include <linux/slab.h> > > > > > > +/* defined in main/init.c */ > > > +bool __init cmdline_has_extra_options(void); > > > + > > > static char *saved_boot_config; > > > > > > static int boot_config_proc_show(struct seq_file *m, void *v) > > > @@ -63,7 +66,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > > dst += ret; > > > } > > > } > > > - if (ret >= 0 && boot_command_line[0]) { > > > + if (cmdline_has_extra_options() && ret >= 0 && boot_command_line[0]) { > > > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > > boot_command_line); > > > if (ret > 0) > > > diff --git a/init/main.c b/init/main.c > > > index 2ca52474d0c3..881f6230ee59 100644 > > > --- a/init/main.c > > > +++ b/init/main.c > > > @@ -487,6 +487,11 @@ static int __init warn_bootconfig(char *str) > > > > > > early_param("bootconfig", warn_bootconfig); > > > > > > +bool __init cmdline_has_extra_options(void) > > > +{ > > > + return extra_command_line || extra_init_args; > > > +} > > > + > > > /* Change NUL term back to "=", to make "param" the whole string. */ > > > static void __init repair_env_string(char *param, char *val) > > > { > > > ------------------------------------------------------------------------ > > > > This one looks good to me! > > > > I had to move the declaration from /fs/proc/bootconfig.c to > > include/linux/bootconfig.h in order to avoid a build error. (The > > build system wants the declaration and definition to be visible as > > a cross-check.) > > > > Does the resulting patch below work for you? > > Yes, this looks good to me. BTW, shouldn't we push this with the > previous fix (from the viewpoint of memory usage, this is a kind > of fix for redundant memory usage)? Makes sense to me! Let me run a few tests on it. For whatever it is worth, I have them both queued on their own branch on -rcu: 4facfdfaec7a ("fs/proc: remove redundant comments from /proc/bootconfig") 8d95b50c523f ("fs/proc: Skip bootloader comment if no embedded kernel parameters") Thanx, Paul > Thank you! > > > > > Thanx, Paul > > > > ------------------------------------------------------------------------ > > > > commit 8d95b50c523fba7133368650b3b5f71b169c76b5 > > Author: Masami Hiramatsu <mhiramat@kernel.org> > > Date: Mon Apr 8 12:10:38 2024 -0700 > > > > fs/proc: Skip bootloader comment if no embedded kernel parameters > > > > If the "bootconfig" kernel command-line argument was specified or if > > the kernel was built with CONFIG_BOOT_CONFIG_FORCE, but if there are > > no embedded kernel parameter, omit the "# Parameters from bootloader:" > > comment from the /proc/bootconfig file. This will cause automation > > to fall back to the /proc/cmdline file, which will be identical to the > > comment in this no-embedded-kernel-parameters case. > > > > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org> > > > > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c > > index e5635a6b127b0..87dcaae32ff87 100644 > > --- a/fs/proc/bootconfig.c > > +++ b/fs/proc/bootconfig.c > > @@ -63,7 +63,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) > > dst += ret; > > } > > } > > - if (ret >= 0 && boot_command_line[0]) { > > + if (cmdline_has_extra_options() && ret >= 0 && boot_command_line[0]) { > > ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", > > boot_command_line); > > if (ret > 0) > > diff --git a/include/linux/bootconfig.h b/include/linux/bootconfig.h > > index ca73940e26df8..e5ee2c694401e 100644 > > --- a/include/linux/bootconfig.h > > +++ b/include/linux/bootconfig.h > > @@ -10,6 +10,7 @@ > > #ifdef __KERNEL__ > > #include <linux/kernel.h> > > #include <linux/types.h> > > +bool __init cmdline_has_extra_options(void); > > #else /* !__KERNEL__ */ > > /* > > * NOTE: This is only for tools/bootconfig, because tools/bootconfig will > > diff --git a/init/main.c b/init/main.c > > index 2ca52474d0c30..881f6230ee59e 100644 > > --- a/init/main.c > > +++ b/init/main.c > > @@ -487,6 +487,11 @@ static int __init warn_bootconfig(char *str) > > > > early_param("bootconfig", warn_bootconfig); > > > > +bool __init cmdline_has_extra_options(void) > > +{ > > + return extra_command_line || extra_init_args; > > +} > > + > > /* Change NUL term back to "=", to make "param" the whole string. */ > > static void __init repair_env_string(char *param, char *val) > > { > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
Hello! This series removes redundant comments from /proc/bootconfig: 1. fs/proc: remove redundant comments from /proc/bootconfig, courtesy of Zhenhua Huang. 2. fs/proc: Skip bootloader comment if no embedded kernel parameters, courtesy of Masami Hiramatsu. Thanx, Paul ------------------------------------------------------------------------ b/fs/proc/bootconfig.c | 12 ++++++------ b/include/linux/bootconfig.h | 1 + b/init/main.c | 5 +++++ fs/proc/bootconfig.c | 2 +- 4 files changed, 13 insertions(+), 7 deletions(-)
Hi Paul, Thanks, both looks good to me. Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> Let me update bootconfig/fixes. On Mon, 8 Apr 2024 21:42:49 -0700 "Paul E. McKenney" <paulmck@kernel.org> wrote: > Hello! > > This series removes redundant comments from /proc/bootconfig: > > 1. fs/proc: remove redundant comments from /proc/bootconfig, > courtesy of Zhenhua Huang. > > 2. fs/proc: Skip bootloader comment if no embedded kernel parameters, > courtesy of Masami Hiramatsu. > > Thanx, Paul > > ------------------------------------------------------------------------ > > b/fs/proc/bootconfig.c | 12 ++++++------ > b/include/linux/bootconfig.h | 1 + > b/init/main.c | 5 +++++ > fs/proc/bootconfig.c | 2 +- > 4 files changed, 13 insertions(+), 7 deletions(-) >
On Tue, Apr 09, 2024 at 11:32:48PM +0900, Masami Hiramatsu wrote: > Hi Paul, > > Thanks, both looks good to me. > > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > > Let me update bootconfig/fixes. Thank you very much! As soon as I see them in -next from you, I will drop them from -rcu. Thanx, Paul > On Mon, 8 Apr 2024 21:42:49 -0700 > "Paul E. McKenney" <paulmck@kernel.org> wrote: > > > Hello! > > > > This series removes redundant comments from /proc/bootconfig: > > > > 1. fs/proc: remove redundant comments from /proc/bootconfig, > > courtesy of Zhenhua Huang. > > > > 2. fs/proc: Skip bootloader comment if no embedded kernel parameters, > > courtesy of Masami Hiramatsu. > > > > Thanx, Paul > > > > ------------------------------------------------------------------------ > > > > b/fs/proc/bootconfig.c | 12 ++++++------ > > b/include/linux/bootconfig.h | 1 + > > b/init/main.c | 5 +++++ > > fs/proc/bootconfig.c | 2 +- > > 4 files changed, 13 insertions(+), 7 deletions(-) > > > > > -- > Masami Hiramatsu (Google) <mhiramat@kernel.org>
diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c index 902b326e1e560..e5635a6b127b0 100644 --- a/fs/proc/bootconfig.c +++ b/fs/proc/bootconfig.c @@ -62,12 +62,12 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size) break; dst += ret; } - if (ret >= 0 && boot_command_line[0]) { - ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", - boot_command_line); - if (ret > 0) - dst += ret; - } + } + if (ret >= 0 && boot_command_line[0]) { + ret = snprintf(dst, rest(dst, end), "# Parameters from bootloader:\n# %s\n", + boot_command_line); + if (ret > 0) + dst += ret; } out: kfree(key);
commit 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") adds bootloader argument comments into /proc/bootconfig. /proc/bootconfig shows boot_command_line[] multiple times following every xbc key value pair, that's duplicated and not necessary. Remove redundant ones. Output before and after the fix is like: key1 = value1 *bootloader argument comments* key2 = value2 *bootloader argument comments* key3 = value3 *bootloader argument comments* ... key1 = value1 key2 = value2 key3 = value3 *bootloader argument comments* ... Fixes: 717c7c894d4b ("fs/proc: Add boot loader arguments as comment to /proc/bootconfig") Signed-off-by: Zhenhua Huang <quic_zhenhuah@quicinc.com> Signed-off-by: Paul E. McKenney <paulmck@kernel.org> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: <linux-trace-kernel@vger.kernel.org> Cc: <linux-fsdevel@vger.kernel.org>