Review request 0/2: init/main.c: sink the kernel_init to the bottom

Review request 0/2: init/main.c: sink the kernel_init to the bottom

[Liu Peibao]

It looks better that sinking the kernel_init() to bottom. And such the
statement of kernel_init_freeable() is redundant. Also there is a
warning found by the checkpatch.pl.

[PATCH 1/2] init/main.c: fix strings split across lines

[Liu Peibao]

Fix warning found by checkpatch.pl.

Signed-off-by: Liu Peibao <liupeibao@163.com>
---
 init/main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/init/main.c b/init/main.c
index 6feee7f11eaf..1e492de770c8 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1470,8 +1470,7 @@ static int __ref kernel_init(void *unused)
 	    !try_to_run_init_process("/bin/sh"))
 		return 0;
 
-	panic("No working init found.  Try passing init= option to kernel. "
-	      "See Linux Documentation/admin-guide/init.rst for guidance.");
+	panic("No working init found. Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.");
 }
 
 /* Open /dev/console, for stdin/stdout/stderr, this should never fail */
-- 
2.17.1

[PATCH 2/2] init/main.c: sink the kernel_init to the bottom

[Liu Peibao]

Remove the redundant kernel_init_freeable statement.

Signed-off-by: Liu Peibao <liupeibao@163.com>
---
 init/main.c | 132 ++++++++++++++++++++++++++--------------------------
 1 file changed, 65 insertions(+), 67 deletions(-)

diff --git a/init/main.c b/init/main.c
index 1e492de770c8..d5c2fa85ee54 100644
--- a/init/main.c
+++ b/init/main.c
@@ -1364,8 +1364,6 @@ static int try_to_run_init_process(const char *init_filename)
 	return ret;
 }
 
-static noinline void __init kernel_init_freeable(void);
-
 #if defined(CONFIG_STRICT_KERNEL_RWX) || defined(CONFIG_STRICT_MODULE_RWX)
 bool rodata_enabled __ro_after_init = true;
 static int __init set_debug_rodata(char *str)
@@ -1408,71 +1406,6 @@ void __weak free_initmem(void)
 	free_initmem_default(POISON_FREE_INITMEM);
 }
 
-static int __ref kernel_init(void *unused)
-{
-	int ret;
-
-	kernel_init_freeable();
-	/* need to finish all async __init code before freeing the memory */
-	async_synchronize_full();
-	kprobe_free_init_mem();
-	ftrace_free_init_mem();
-	free_initmem();
-	mark_readonly();
-
-	/*
-	 * Kernel mappings are now finalized - update the userspace page-table
-	 * to finalize PTI.
-	 */
-	pti_finalize();
-
-	system_state = SYSTEM_RUNNING;
-	numa_default_policy();
-
-	rcu_end_inkernel_boot();
-
-	do_sysctl_args();
-
-	if (ramdisk_execute_command) {
-		ret = run_init_process(ramdisk_execute_command);
-		if (!ret)
-			return 0;
-		pr_err("Failed to execute %s (error %d)\n",
-		       ramdisk_execute_command, ret);
-	}
-
-	/*
-	 * We try each of these until one succeeds.
-	 *
-	 * The Bourne shell can be used instead of init if we are
-	 * trying to recover a really broken machine.
-	 */
-	if (execute_command) {
-		ret = run_init_process(execute_command);
-		if (!ret)
-			return 0;
-		panic("Requested init %s failed (error %d).",
-		      execute_command, ret);
-	}
-
-	if (CONFIG_DEFAULT_INIT[0] != '\0') {
-		ret = run_init_process(CONFIG_DEFAULT_INIT);
-		if (ret)
-			pr_err("Default init %s failed (error %d)\n",
-			       CONFIG_DEFAULT_INIT, ret);
-		else
-			return 0;
-	}
-
-	if (!try_to_run_init_process("/sbin/init") ||
-	    !try_to_run_init_process("/etc/init") ||
-	    !try_to_run_init_process("/bin/init") ||
-	    !try_to_run_init_process("/bin/sh"))
-		return 0;
-
-	panic("No working init found. Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.");
-}
-
 /* Open /dev/console, for stdin/stdout/stderr, this should never fail */
 void __init console_on_rootfs(void)
 {
@@ -1554,3 +1487,68 @@ static noinline void __init kernel_init_freeable(void)
 
 	integrity_load_keys();
 }
+
+static int __ref kernel_init(void *unused)
+{
+	int ret;
+
+	kernel_init_freeable();
+	/* need to finish all async __init code before freeing the memory */
+	async_synchronize_full();
+	kprobe_free_init_mem();
+	ftrace_free_init_mem();
+	free_initmem();
+	mark_readonly();
+
+	/*
+	 * Kernel mappings are now finalized - update the userspace page-table
+	 * to finalize PTI.
+	 */
+	pti_finalize();
+
+	system_state = SYSTEM_RUNNING;
+	numa_default_policy();
+
+	rcu_end_inkernel_boot();
+
+	do_sysctl_args();
+
+	if (ramdisk_execute_command) {
+		ret = run_init_process(ramdisk_execute_command);
+		if (!ret)
+			return 0;
+		pr_err("Failed to execute %s (error %d)\n",
+		       ramdisk_execute_command, ret);
+	}
+
+	/*
+	 * We try each of these until one succeeds.
+	 *
+	 * The Bourne shell can be used instead of init if we are
+	 * trying to recover a really broken machine.
+	 */
+	if (execute_command) {
+		ret = run_init_process(execute_command);
+		if (!ret)
+			return 0;
+		panic("Requested init %s failed (error %d).",
+		      execute_command, ret);
+	}
+
+	if (CONFIG_DEFAULT_INIT[0] != '\0') {
+		ret = run_init_process(CONFIG_DEFAULT_INIT);
+		if (ret)
+			pr_err("Default init %s failed (error %d)\n",
+			       CONFIG_DEFAULT_INIT, ret);
+		else
+			return 0;
+	}
+
+	if (!try_to_run_init_process("/sbin/init") ||
+	    !try_to_run_init_process("/etc/init") ||
+	    !try_to_run_init_process("/bin/init") ||
+	    !try_to_run_init_process("/bin/sh"))
+		return 0;
+
+	panic("No working init found. Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.");
+}
-- 
2.17.1

Re: [PATCH 1/2] init/main.c: fix strings split across lines

[Steven Rostedt]

On Wed, 30 Dec 2020 14:04:23 +0800
Liu Peibao <liupeibao@163.com> wrote:

> Fix warning found by checkpatch.pl.
> 
> Signed-off-by: Liu Peibao <liupeibao@163.com>
> ---
>  init/main.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/init/main.c b/init/main.c
> index 6feee7f11eaf..1e492de770c8 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -1470,8 +1470,7 @@ static int __ref kernel_init(void *unused)
>  	    !try_to_run_init_process("/bin/sh"))
>  		return 0;
>  
> -	panic("No working init found.  Try passing init= option to kernel. "
> -	      "See Linux Documentation/admin-guide/init.rst for guidance.");
> +	panic("No working init found. Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.");

Sorry, we don't add changes to the kernel that checkpatch warns about.
checkpatch should only be run on new code. Please do not submit any patches
on existing code because checkpatch warns about it.

-- Steve


>  }
>  
>  /* Open /dev/console, for stdin/stdout/stderr, this should never fail */

Re: [PATCH 2/2] init/main.c: sink the kernel_init to the bottom

[Steven Rostedt]

On Wed, 30 Dec 2020 14:04:24 +0800
Liu Peibao <liupeibao@163.com> wrote:

> Remove the redundant kernel_init_freeable statement.

Why? This change only adds churn to the git history and adds more noise to
git blame. We do not need cosmetic changes like this.

-- Steve


> 
> Signed-off-by: Liu Peibao <liupeibao@163.com>

Re: [PATCH 1/2] init/main.c: fix strings split across lines

[Liu Peibao]

On 1/12/21 8:02 AM, Steven Rostedt wrote:
> On Wed, 30 Dec 2020 14:04:23 +0800
> Liu Peibao <liupeibao@163.com> wrote:
> 
>> Fix warning found by checkpatch.pl.
>>
>> Signed-off-by: Liu Peibao <liupeibao@163.com>
>> ---
>>   init/main.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/init/main.c b/init/main.c
>> index 6feee7f11eaf..1e492de770c8 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -1470,8 +1470,7 @@ static int __ref kernel_init(void *unused)
>>   	    !try_to_run_init_process("/bin/sh"))
>>   		return 0;
>>   
>> -	panic("No working init found.  Try passing init= option to kernel. "
>> -	      "See Linux Documentation/admin-guide/init.rst for guidance.");
>> +	panic("No working init found. Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.");
> 
> Sorry, we don't add changes to the kernel that checkpatch warns about.
> checkpatch should only be run on new code. Please do not submit any patches
> on existing code because checkpatch warns about it.
> 
> -- Steve
> 
Thanks for your replay! I get it.
But I still feel a little confused that we use different standard to 
measure the existing code and the new code. I also checked some commits, 
there are similar patches too.

BR,
Peibao
> 
>>   }
>>   
>>   /* Open /dev/console, for stdin/stdout/stderr, this should never fail */

Re: [PATCH 1/2] init/main.c: fix strings split across lines

[Steven Rostedt]

On Tue, 12 Jan 2021 22:26:21 +0800
Liu Peibao <liupeibao@163.com> wrote:

> Thanks for your replay! I get it.
> But I still feel a little confused that we use different standard to 
> measure the existing code and the new code. I also checked some commits, 
> there are similar patches too.

For the reason of different standards for existing code to new code. Think
of it as a "grandfather clause". Where rules change for new instantiations,
but if you already have something, you can still use the old rules. Hmm,
it's kind of like how RCU works!

As for some commits getting it. They sometimes get pulled in by various
maintainers, and also may happen if you are changing the code around
something. With the "one commit does one thing", you can have a "clean up
code" patch followed by a "change the code" patch. matters what the context
is.

-- Steve

Re: [PATCH 1/2] init/main.c: fix strings split across lines

[Liu Peibao]

On 1/13/21 5:11 AM, Steven Rostedt wrote:
> On Tue, 12 Jan 2021 22:26:21 +0800
> Liu Peibao <liupeibao@163.com> wrote:
> 
>> Thanks for your replay! I get it.
>> But I still feel a little confused that we use different standard to
>> measure the existing code and the new code. I also checked some commits,
>> there are similar patches too.
> 
> For the reason of different standards for existing code to new code. Think
> of it as a "grandfather clause". Where rules change for new instantiations,
> but if you already have something, you can still use the old rules. Hmm,
> it's kind of like how RCU works!
> 
> As for some commits getting it. They sometimes get pulled in by various
> maintainers, and also may happen if you are changing the code around
> something. With the "one commit does one thing", you can have a "clean up
> code" patch followed by a "change the code" patch. matters what the context
> is.
> 
> -- Steve
> 
Whoa, the RCU example is really amazing!
Thanks for your detailed explanation.

BR,
Peibao