Re: [PATCH fs/proc/bootconfig] remove redundant comments from /proc/bootconfig

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

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)
>  {


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>