Skip to contentSkip to conversation

fix problem with reschenduling in rest_init (2.6.35-rc3)

PATCH In review Ilya Loginov 6 participants 15 messages Jun 23, 2010 – Jun 30, 2010

lore
Share & tools
lore

Ilya Loginov

author
#0 lore raw

PATCH 1/1 fix problem with reschenduling in rest_init (2.6.35-rc3)

There is a bug in rest_init function. The problem is that kernel_init
thread starts before initialization of kthreadd_task when CONFIG_PREEMPT_VOLUNTARY is enabled.

kernel_init thread do wake_up_process(kthreadd_task) and I have kernel Oops in try_to_wake_up when I try to get p->state.

I found this problem on 2.6.34 on FPGA. It is very slow, and
find_task_by_pid is done after reschenduling. I have no this problem on 2.6.35-rc3 because kernel code is moved. But if I write simple loop like

	volatile int tmp;
	for(i = 0; i < preset_lpj; i++)
		tmp++;

right after kernel_thread(kernel_init,.... , I have this problem on 2.6.35-rc3 too.

I understand that real problem is reschenduling in init code, but
I have no ability to fix it.

Signed-off-by Ilya Loginov <[email protected]>

---

init/main.c +1 -1
index 3bdb152..9febd69 100644
@@ -428,12 +428,12 @@ static noinline void __init_refok rest_init(void)
 	int pid;
 
 	rcu_scheduler_starting();
-	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
+	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	unlock_kernel();
 
 	/*
#1 lore raw
Quoted 10 lines · Ilya Loginov

There is a bug in rest_init function. The problem is that kernel_init
thread starts before initialization of kthreadd_task when CONFIG_PREEMPT_VOLUNTARY is enabled.

kernel_init thread do wake_up_process(kthreadd_task) and I have kernel Oops in try_to_wake_up when I try to get p->state.

I found this problem on 2.6.34 on FPGA. It is very slow, and
find_task_by_pid is done after reschenduling. I have no this problem on 2.6.35-rc3 because kernel code is moved. But if I write simple loop like

Quoted 8 lines
	volatile int tmp;
	for(i = 0; i < preset_lpj; i++)
		tmp++;

right after kernel_thread(kernel_init,.... , I have this problem on 2.6.35-rc3 too.

I understand that real problem is reschenduling in init code, but
I have no ability to fix it.

Ooh, interesting problem, so kernel_init() will indeed spawn all kinds of kernel threads, and doing so before we create the kthreadd will result in the oops you observed.

However I suspect the ordering is like it is because we want init to have pid 1, if we were to re-order like you suggest kthreadd will end up with pid 1 and init with pid 2.

Humm, how to fix this...

Quoted 20 lines
Signed-off-by Ilya Loginov <[email protected]>

---

init/main.c +1 -1
index 3bdb152..9febd69 100644
@@ -428,12 +428,12 @@ static noinline void __init_refok rest_init(void)
 	int pid;
 
 	rcu_scheduler_starting();
-	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
+	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	unlock_kernel();
 
 	/*
author
#2 lore raw
Quoted 3 lines · On Thu, 24 Jun 2010 15:11:36 +0200 Peter Zijlstra <[email protected]> wrote:

However I suspect the ordering is like it is because we want init to have pid 1, if we were to re-order like you suggest kthreadd will end up with pid 1 and init with pid 2.

Strange, but init does not die after I did this. Fix me if I wrong, but it wants to have pid 1, and die in other case.

Interesting...

Signature · 1 line
Ilya Loginov <[email protected]>
#3 lore raw
Quoted 4 lines · On Thu, 24 Jun 2010 17:23:34 +0400 Ilya Loginov <[email protected]> wrote:

Strange, but init does not die after I did this. Fix me if I wrong, but it wants to have pid 1, and die in other case.

Interesting...

Yeah, of course, I am wrong. To make it clear I watched sources of sysvinit and founded out that if pid not equal to 1 init does

exit(telinit(p, argc, argv));

So, it does not die. Anyway, I would have kernel panic.

Signature · 2 lines
Илья Логинов <[email protected]>
ЪТХ╨{.nг+┴╥÷╝┴╜├+%┼кЪ╠Ищ╤╔┼wЪ╨{.nг+┴╥╔┼{╠ЧG╚²ИЪ┼{ay╨й┤з≥К,j╜╒fё╒╥h ▐О│ЙЪ▒ЙГz_Х╝(╜И ▌┼щ╒j"²З╤m╖ЪЪ╬╚ЧG╚²ИЪ╒╦?≥╗Х╜з&ёЬ╖~▐А╤iO∙Ф╛z╥ vь^╤m╖ЪЪцЪ╤ЛЪ╒╦?√I╔
#4 lore raw

PATCH 1/1 init: Fix race between init and kthreadd

In reply to Ilya Loginov
Quoted 9 lines · Ilya Loginov
Quoted 3 lines · On Thu, 24 Jun 2010 15:11:36 +0200 Peter Zijlstra <[email protected]> wrote:

However I suspect the ordering is like it is because we want init to have pid 1, if we were to re-order like you suggest kthreadd will end up with pid 1 and init with pid 2.

Strange, but init does not die after I did this. Fix me if I wrong, but it wants to have pid 1, and die in other case.

Does something like this work for you?

---
Subject: init: Fix race between init and kthreadd
From: Peter Zijlstra <[email protected]>
Date: Mon Jun 28 10:49:09 CEST 2010

Ilya reported that on a very slow machine he could reliably reproduce a race between forking init and kthreadd. We first fork init so that it obtains pid-1, however since the scheduler is already fully running at this point it can preempt and run the init thread before we spawn and set kthreadd_task.

The init thread can then attempt spawning kthreads without kthreadd being present which results in an OOPS.

Cure this in a crude way by having the init task spin-wait on kthreadd_task. Nicer solutions are more complex and have more overhead which doesn't appear worth it.

Reported-by Ilya Loginov <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>
---
 init/main.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
init/main.c +19 -0
Index: linux-2.6/init/main.c
===================================================================
@@ -426,6 +426,17 @@ static noinline void __init_refok rest_i
 	int pid;
 
 	rcu_scheduler_starting();
+	/*
+	 * Here we first fork the init thread and then the kthreadd so that
+	 * init ends up with pid-1.
+	 *
+	 * Since the scheduler is already fully active we can end up
+	 * running the init thread for long enough to start spawning kthreads
+	 * before this thread continues and spawns/sets kthreadd, which
+	 * would result in an OOPS.
+	 *
+	 * See the serialization against kthreadd_task in kernel_init().
+	 */
 	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);

@@ -847,6 +858,14 @@ static noinline int init_post(void)
 
 static int __init kernel_init(void * unused)
 {
+	/*
+	 * Synchronize against setting kthreadd_task in rest_init().
+	 * Using a mutex would have been a lot nicer, but since its a very
+	 * rare race don't bother wasting the space overhead.
+	 */
+	while (!kthreadd_task)
+		yield();
+
 	lock_kernel();
 
 	/*
#5 lore raw

PATCH 1/1 init: Fix race between init and kthreadd -v2

Review

Quoted 11 lines · Peter Zijlstra

static int __init kernel_init(void * unused)
{

(quoted diff) +8 -0
+	/*
+	 * Synchronize against setting kthreadd_task in rest_init().
+	 * Using a mutex would have been a lot nicer, but since its a very
+	 * rare race don't bother wasting the space overhead.
+	 */
+	while (!kthreadd_task)
+		yield();
+
 	lock_kernel();

I just realized its all __init code so its all 'free' anyway, how about the nicer version:

---
Subject: init: Fix race between init and kthreadd -v2

Ilya reported that on a very slow machine he could reliably reproduce a race between forking init and kthreadd. We first fork init so that it obtains pid-1, however since the scheduler is already fully running at this point it can preempt and run the init thread before we spawn and set kthreadd_task.

The init thread can then attempt spawning kthreads without kthreadd being present which results in an OOPS.

Signed-off-by Peter Zijlstra <[email protected]>
---
 init/main.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)
init/main.c +16 -0
index e2a2bf3..8f2acf5 100644
@@ -420,18 +420,27 @@ static void __init setup_command_line(char *command_line)
  * gcc-3.4 accidentally inlines this function, so use noinline.
  */
 
+static __initdata DEFINE_MUTEX(kthreadd_lock);
+
 static noinline void __init_refok rest_init(void)
 	__releases(kernel_lock)
 {
 	int pid;
 
 	rcu_scheduler_starting();
+	/*
+	 * We need to spawn init first so that it obtains pid-1, however
+	 * the init task will end up wanting to create kthreads, which
+	 * if we schedule it before we create kthreadd, will OOPS.
+	 */
+	mutex_lock(&kthreadd_lock);
 	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
+	mutex_unlock(&kthreadd_lock);
 	unlock_kernel();
 
 	/*

@@ -847,6 +856,13 @@ static noinline int init_post(void)
 
 static int __init kernel_init(void * unused)
 {
+	/*
+	 * We spawned this thread while holding this lock, ensure the
+	 * locked section in rest_init() is complete before proceeding.
+	 */
+	mutex_lock(&kthreadd_lock);
+	mutex_unlock(&kthreadd_lock);
+
 	lock_kernel();
 
 	/*
#6 lore raw

Re: [PATCH] init: Fix race between init and kthreadd -v2

Quoted 19 lines · Peter Zijlstra
init/main.c +8 -0
+static __initdata DEFINE_MUTEX(kthreadd_lock);
 
+	/*
+	 * We need to spawn init first so that it obtains pid-1, however
+	 * the init task will end up wanting to create kthreads, which
+	 * if we schedule it before we create kthreadd, will OOPS.
+	 */
+	mutex_lock(&kthreadd_lock);
 	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
+	mutex_unlock(&kthreadd_lock);
 	unlock_kernel();
 
 	/*

init/main.c static noinline int init_post(void) · line 864

Quoted 9 lines
init/main.c +6 -0
@@ -847,6 +856,13 @@ static noinline int init_post(void)
 
 static int __init kernel_init(void * unused)
 {
+	/*
+	 * We spawned this thread while holding this lock, ensure the
+	 * locked section in rest_init() is complete before proceeding.
+	 */
+	mutex_lock(&kthreadd_lock);
+	mutex_unlock(&kthreadd_lock);

I think you may be using a mutex as a completion in essence. Why not use completions instead?

Ingo

#7 lore raw

PATCH 1/1 init: Fix race between init and kthreadd -v3

Quoted 2 lines · Ingo Molnar

I think you may be using a mutex as a completion in essence. Why not use completions instead?

Totally forgot about those things.. Yes they fit perfectly.

---
Subject: init: Fix race between init and kthreadd -v2

Ilya reported that on a very slow machine he could reliably reproduce a race between forking init and kthreadd. We first fork init so that it obtains pid-1, however since the scheduler is already fully running at this point it can preempt and run the init thread before we spawn and set kthreadd_task.

The init thread can then attempt spawning kthreads without kthreadd being present which results in an OOPS.

Reported-by Ilya Loginov <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>
---
 init/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
init/main.c +12 -0
index e2a2bf3..2280f63 100644
@@ -420,18 +420,26 @@ static void __init setup_command_line(char *command_line)
  * gcc-3.4 accidentally inlines this function, so use noinline.
  */
 
+static __initdata DECLARE_COMPLETION(kthreadd_done);
+
 static noinline void __init_refok rest_init(void)
 	__releases(kernel_lock)
 {
 	int pid;
 
 	rcu_scheduler_starting();
+	/*
+	 * We need to spawn init first so that it obtains pid-1, however
+	 * the init task will end up wanting to create kthreads, which, if
+	 * we schedule it before we create kthreadd, will OOPS.
+	 */
 	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
+	complete(&kthreadd_done);
 	unlock_kernel();
 
 	/*

@@ -847,6 +855,10 @@ static noinline int init_post(void)
 
 static int __init kernel_init(void * unused)
 {
+	/*
+	 * Wait until kthreadd is all set-up.
+	 */
+	wait_for_completion(&kthreadd_done);
 	lock_kernel();
 
 	/*
#8 lore raw

Re: [PATCH] init: Fix race between init and kthreadd -v3

Quoted 8 lines · Peter Zijlstra

 static int __init kernel_init(void * unused)
 {
+       /*
+        * Wait until kthreadd is all set-up.
+        */
+       wait_for_completion(&kthreadd_done);
       lock_kernel();

Ack. Looks sane.

Linus

#9 lore raw

Re: [PATCH] init: Fix race between init and kthreadd -v3

In reply to Peter Zijlstra
Quoted 33 lines · On Mon, 28 Jun 2010 16:51:01 +0200 Peter Zijlstra wrote:
Quoted 2 lines · Ingo Molnar

I think you may be using a mutex as a completion in essence. Why not use completions instead?

Totally forgot about those things.. Yes they fit perfectly.

---
Subject: init: Fix race between init and kthreadd -v2

Ilya reported that on a very slow machine he could reliably reproduce a race between forking init and kthreadd. We first fork init so that it obtains pid-1, however since the scheduler is already fully running at this point it can preempt and run the init thread before we spawn and set kthreadd_task.

The init thread can then attempt spawning kthreads without kthreadd being present which results in an OOPS.

Reported-by Ilya Loginov <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>
---
 init/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
init/main.c +0 -0
index e2a2bf3..2280f63 100644
@@ -420,18 +420,26 @@ static void __init setup_command_line(char *command_line)
  * gcc-3.4 accidentally inlines this function, so use noinline.
  */

init/main.c static void __init setup_command_line(char *command_line) · line 432

Quoted 10 lines
init/main.c +4 -0
+static __initdata DECLARE_COMPLETION(kthreadd_done);
+
 static noinline void __init_refok rest_init(void)
 	__releases(kernel_lock)
 {
 	int pid;
 
 	rcu_scheduler_starting();
+	/*
+	 * We need to spawn init first so that it obtains pid-1, however

Would be better to say "pid 1" or "pid=1".
"pid-1" seems odd to me.

Quoted 16 lines
init/main.c +4 -0
+	 * the init task will end up wanting to create kthreads, which, if
+	 * we schedule it before we create kthreadd, will OOPS.
+	 */
 	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
+	complete(&kthreadd_done);
 	unlock_kernel();
 
 	/*

@@ -847,6 +855,10 @@ static noinline int init_post(void)
 
 static int __init kernel_init(void * unused)

init/main.c static noinline int init_post(void)

Quoted 10 lines
init/main.c +4 -1
 {
+	/*
+	 * Wait until kthreadd is all set-up.
+	 */
+	wait_for_completion(&kthreadd_done);
 	lock_kernel();
 
 	/*
 
--

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

#10 lore raw

[tip:sched/urgent] init, sched: Fix race between init and kthreadd

In reply to Peter Zijlstra
Commit-ID:  b433c3d4549ae74935b585115f076c6fb7bc48fe
Gitweb:     http://git.kernel.org/tip/b433c3d4549ae74935b585115f076c6fb7bc48fe
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Mon, 28 Jun 2010 16:51:01 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Mon, 28 Jun 2010 18:21:30 +0200

init, sched: Fix race between init and kthreadd

Ilya reported that on a very slow machine he could reliably reproduce a race between forking init and kthreadd. We first fork init so that it obtains pid-1, however since the scheduler is already fully running at this point it can preempt and run the init thread before we spawn and set kthreadd_task.

The init thread can then attempt spawning kthreads without kthreadd being present which results in an OOPS.

Reported-by Ilya Loginov <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>
Acked-by Linus Torvalds <[email protected]>

LKML-Reference: <1277736661.3561.110.camel@laptop>

Signed-off-by Ingo Molnar <[email protected]>
---
 init/main.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
init/main.c +12 -0
index 3bdb152..633442f 100644
@@ -422,18 +422,26 @@ static void __init setup_command_line(char *command_line)
  * gcc-3.4 accidentally inlines this function, so use noinline.
  */
 
+static __initdata DECLARE_COMPLETION(kthreadd_done);
+
 static noinline void __init_refok rest_init(void)
 	__releases(kernel_lock)
 {
 	int pid;
 
 	rcu_scheduler_starting();
+	/*
+	 * We need to spawn init first so that it obtains pid-1, however
+	 * the init task will end up wanting to create kthreads, which, if
+	 * we schedule it before we create kthreadd, will OOPS.
+	 */
 	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
+	complete(&kthreadd_done);
 	unlock_kernel();
 
 	/*

@@ -855,6 +863,10 @@ static noinline int init_post(void)
 
 static int __init kernel_init(void * unused)
 {
+	/*
+	 * Wait until kthreadd is all set-up.
+	 */
+	wait_for_completion(&kthreadd_done);
 	lock_kernel();
 
 	/*
#12 lore raw

PATCH 1/1 init: Fix comment

Quoted 4 lines · Ilya Loginov

I have tested patch v3. All works fine for me.

I agree with Randy. The commentary should be fixed.

By popular request..

---
Subject: init: Fix comment

Apparently "pid-1" confuses people...

Requested-by Randy Dunlap <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>
---
 init/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
init/main.c +1 -1
index 9eed0f3..4ab5124 100644
@@ -431,7 +431,7 @@ static noinline void __init_refok rest_init(void)
 
 	rcu_scheduler_starting();
 	/*
-	 * We need to spawn init first so that it obtains pid-1, however
+	 * We need to spawn init first so that it obtains pid 1, however
 	 * the init task will end up wanting to create kthreads, which, if
 	 * we schedule it before we create kthreadd, will OOPS.
 	 */
#13 lore raw

[tip:sched/urgent] init: Fix comment

Commit-ID:  9715856922bf8475f5428c29b6f4a9eebc97d391
Gitweb:     http://git.kernel.org/tip/9715856922bf8475f5428c29b6f4a9eebc97d391
Author:     Peter Zijlstra <[email protected]>
AuthorDate: Wed, 30 Jun 2010 10:37:11 +0200
Committer:  Ingo Molnar <[email protected]>
CommitDate: Wed, 30 Jun 2010 10:42:44 +0200

init: Fix comment

Apparently "pid-1" confuses people...

Requested-by Randy Dunlap <[email protected]>
Signed-off-by Peter Zijlstra <[email protected]>
Cc [email protected]
Cc [email protected]
Cc Ilya Loginov <[email protected]>

LKML-Reference: <1277887031.1868.82.camel@laptop>

Signed-off-by Ingo Molnar <[email protected]>
---
 init/main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
init/main.c +1 -1
index 633442f..1692df3 100644
@@ -431,7 +431,7 @@ static noinline void __init_refok rest_init(void)
 
 	rcu_scheduler_starting();
 	/*
-	 * We need to spawn init first so that it obtains pid-1, however
+	 * We need to spawn init first so that it obtains pid 1, however
 	 * the init task will end up wanting to create kthreads, which, if
 	 * we schedule it before we create kthreadd, will OOPS.
 	 */
author
#14 lore raw

Re: [PATCH] init: Fix race between init and kthreadd -v2

In reply to Peter Zijlstra
Quoted 2 lines · On Mon, 28 Jun 2010 13:53:17 +0200 Peter Zijlstra <[email protected]> wrote:

I just realized its all __init code so its all 'free' anyway, how about the nicer version:

I will try both versions and write about results. But I think, that there is some logical inconsistent in these patches (for me). I like v2 more than v1. But, I would move mutex_lock/unlock. Anyway, I will try your patches first.

Many thanks.

Signature · 1 line
Ilya Loginov <[email protected]>