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

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

[Paul E. McKenney]

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>

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

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

[Masami Hiramatsu]

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


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

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

[Paul E. McKenney]

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>

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

[Masami Hiramatsu]

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>


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

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

[Masami Hiramatsu]

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>


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

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

[Paul E. McKenney]

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>

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

[Masami Hiramatsu]

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>


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

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

[Paul E. McKenney]

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

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

[Masami Hiramatsu]

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>

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

[Paul E. McKenney]

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>

[PATCH v2 fs/proc/bootconfig 0/2] remove redundant comments from /proc/bootconfig

[Paul E. McKenney]

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

[PATCH v2 fs/proc/bootconfig 1/2] fs/proc: remove redundant comments from /proc/bootconfig

[Paul E. McKenney]

From: Zhenhua Huang <quic_zhenhuah@quicinc.com>

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: <linux-trace-kernel@vger.kernel.org>
Cc: <linux-fsdevel@vger.kernel.org>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
 fs/proc/bootconfig.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

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);
-- 
2.40.1

[PATCH v2 fs/proc/bootconfig 2/2] fs/proc: Skip bootloader comment if no embedded kernel parameters

[Paul E. McKenney]

From: Masami Hiramatsu <mhiramat@kernel.org>

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>
---
 fs/proc/bootconfig.c       | 2 +-
 include/linux/bootconfig.h | 1 +
 init/main.c                | 5 +++++
 3 files changed, 7 insertions(+), 1 deletion(-)

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

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

[Masami Hiramatsu]

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


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

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

[Paul E. McKenney]

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>