kthread: make kthread_create() killable

A kernel bug report was opened against Ubuntu[0]. We performed a kernel bisect, and found that reverting the following commit resolved this bug:

From c6562e5d774dd1f36724197dbcb8976cccfaab53 Mon Sep 17 00:00:00 2001

Therefore, I'd like to propose this patch for 3.14-final and 3.13-stable.

Commit 786235ee "kthread: make kthread_create() killable" changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures if systemd-udevd received SIGKILL (probably due to timeout) while loading SCSI controller drivers using finit_module() [1].

kernel/kthread.c +1 -0

@@ -292,6 +292,17 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 	 * new kernel thread.
 	 */
 	if (unlikely(wait_for_completion_killable(&done))) {
+		int i = 0;

Quoted 10 lines

kernel/kthread.c +6 -0

@@ -292,6 +292,17 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 	 * new kernel thread.
 	 */
 	if (unlikely(wait_for_completion_killable(&done))) {
+		int i = 0;
+
+		/*
+		 * I got SIGKILL, but wait for 10 more seconds for completion
+		 * unless chosen by the OOM killer. This delay is there as a
+		 * workaround for boot failure caused by SIGKILL upon device

kernel/kthread.c struct task_struct *kthread_create_on_node(int (*threadfn)(void *data), · line 305

Quoted 5 lines

kernel/kthread.c +5 -0

+		 * driver initialization timeout.
+		 */
+		while (i++ < 10 && !test_tsk_thread_flag(current, TIF_MEMDIE))
+			if (wait_for_completion_timeout(&done, HZ))
+				goto ready;

Personally I really dislike this hack. And btw, why we return -ENOMEM if SIGKILL'ed? Why not EINTR ?

Quoted 5 lines

Commit 786235ee "kthread: make kthread_create() killable" changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures if systemd-udevd received SIGKILL (probably due to timeout) while loading SCSI controller drivers using finit_module() [1].

Shouldn't we fix the caller instead? It should handle the error from kthread_create() correctly.

And could you tell who is the caller which doesn't do this? If it can't be fixed, then, say, it can use workqueue to create a kernel thread.

Quoted 3 lines · Oleg Nesterov

Personally I really dislike this hack. And btw, why we return -ENOMEM if SIGKILL'ed? Why not EINTR ?

I chose -ENOMEM because -ENOMEM looked better for conveying that current thread was SIGKILLed by the OOM killer in order to solve no memory state. But I forgot that -ENOMEM will not convey the reason properly if current thread was SIGKILLed by other than the OOM killer. Maybe

  return test_tsk_thread_flag(current, TIF_MEMDIE) ? -ENOMEM : -EINTR;

rather than

return -ENOMEM;

Quoted 6 lines

Quoted 5 lines

Commit 786235ee "kthread: make kthread_create() killable" changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures if systemd-udevd received SIGKILL (probably due to timeout) while loading SCSI controller drivers using finit_module() [1].

Quoted 6 lines

Shouldn't we fix the caller instead? It should handle the error from kthread_create() correctly.

And could you tell who is the caller which doesn't do this? If it can't be fixed, then, say, it can use workqueue to create a kernel thread.

There are many callers.

One of them is scsi_host_alloc() which is called upon bootup in order to recognize SCSI storage devices.

To my surprise, systemd-udevd process sends SIGKILL to worker systemd-udevd processes if they did not complete their jobs within 30 seconds. On some machines, it takes more than 30 seconds to recognize SCSI storage devices.

On such machines, scsi_host_alloc() is called after the worker process received SIGKILL. Since commit 786235ee "kthread: make kthread_create() killable" broke all callers of kthread_create() who had been able to survive SIGKILL,

I think fixing this regression at kthread_create() is the appropriate response.

Given that said, which one do we prefer?

(a) Wait for completion forever after receiving SIGKILL, unless chosen by the OOM killer.

(b) Wait for completion for only limited duration after receiving SIGKILL.

If we need the urgent hack to fix the regression, then I suggest to change scsi_host_alloc() temporary until mptsas (or whatever) is fixed.

drivers/scsi/hosts.c +5 -2

@@ -447,8 +447,18 @@ struct Scsi_Host *scsi_host_alloc(struct
 	dev_set_name(&shost->shost_dev, "host%d", shost->host_no);
 	shost->shost_dev.groups = scsi_sysfs_shost_attr_groups;
 
-	shost->ehandler = kthread_run(scsi_error_handler, shost,
-			"scsi_eh_%d", shost->host_no);
+	/*
+	 * HUGE COMMENT. and kthread_create() needs s/ENOMEM/EINTR/.
+	 */
+	for (;;) {
+		shost->ehandler = kthread_run(scsi_error_handler, shost,

Quoted 2 lines · Oleg Nesterov

If we need the urgent hack to fix the regression, then I suggest to change scsi_host_alloc() temporary until mptsas (or whatever) is fixed.

Device initialization taking longer than 30 seconds is possible and is not a hang up. It is systemd which needs to be fixed.

Quoted 15 lines

drivers/scsi/hosts.c +5 -2

@@ -447,8 +447,18 @@ struct Scsi_Host *scsi_host_alloc(struct
 	dev_set_name(&shost->shost_dev, "host%d", shost->host_no);
 	shost->shost_dev.groups = scsi_sysfs_shost_attr_groups;
 
-	shost->ehandler = kthread_run(scsi_error_handler, shost,
-			"scsi_eh_%d", shost->host_no);
+	/*
+	 * HUGE COMMENT. and kthread_create() needs s/ENOMEM/EINTR/.
+	 */
+	for (;;) {
+		shost->ehandler = kthread_run(scsi_error_handler, shost,

drivers/scsi/hosts.c struct Scsi_Host *scsi_host_alloc(struct · line 464

Quoted 10 lines

drivers/scsi/hosts.c +7 -0

+						"scsi_eh_%d", shost->host_no);
+		if (!IS_ERR(shost->ehandler) || PTR_ERR(shost->ehandler) != -EINTR)
+			break;
+		clear_thread_flag(TIF_SIGPENDING);
+	}
+	recalc_sigpending();
+
 	if (IS_ERR(shost->ehandler)) {
 		printk(KERN_WARNING "scsi%d: error handler thread failed to spawn, error = %ld\n",
 			shost->host_no, PTR_ERR(shost->ehandler));

I think we need a bit different version, in order to take TIF_MEMDIE flag into account at the caller of kthread_create(), for the purpose of commit 786235ee is "try to die as soon as possible if chosen by the OOM killer".

	for (;;) {
		shost->ehandler = kthread_run(scsi_error_handler, shost,
					      "scsi_eh_%d", shost->host_no);
		if (PTR_ERR(shost->ehandler) != -EINTR ||
		    test_thread_flag(TIF_MEMDIE))

(1) Changing return code from -ENOMEM to -EINTR may not be sufficient.

If kmalloc(GFP_KERNEL) in kthread_create_on_node() does something that calls recalc_sigpending(),

TIF_SIGPENDING will be set on the second call
to kthread_run(). This will make wait_for_completion_killable() return -EINTR immediately because the second call to kthread_run() happens only when current thread already received SIGKILL (by other than the OOM killer). This may form an infinite busy loop.

As I think it is difficult to prove that kmalloc(GFP_KERNEL) never sets TIF_SIGPENDING flag

(2) I don't like scattering around test_thread_flag(TIF_MEMDIE), might be other drivers who receive SIGKILL by systemd's 30 seconds timeout.

Quoted 5 lines

Quoted 2 lines

If we need the urgent hack to fix the regression, then I suggest to change scsi_host_alloc() temporary until mptsas (or whatever) is fixed.

Device initialization taking longer than 30 seconds is possible and is not a hang up. It is systemd which needs to be fixed.

Perhaps systemd needs the fix too, I do not know. But this is irrelevant, I think. Or at least this should be discussed separately.

kthread_run() can fail anyway, mptsas_probe() should not crash the kernel.

And btw, it is not clear to me if in this case device initialization really needs more than 30 seconds... My understanding is probably wrong, so please correct me. But it seems that before your "make kthread_create() killable"

- probe hangs

- SIGKILL wakes it up

- so I assume that the probe was interrupted and didn't finish correctly ???

- initialization continues, does scsi_host_alloc(), etc, and everything works fine even if probe was interrupted?

So perhaps that probe should not hang and this should be fixed too ? Do you know where exactly it hangs? And where it is woken up by SIGKILL ? Or I totally misunderstood ?

Quoted 9 lines

I think we need a bit different version, in order to take TIF_MEMDIE flag into account at the caller of kthread_create(), for the purpose of commit 786235ee is "try to die as soon as possible if chosen by the OOM killer".

	for (;;) {
		shost->ehandler = kthread_run(scsi_error_handler, shost,
					      "scsi_eh_%d", shost->host_no);
		if (PTR_ERR(shost->ehandler) != -EINTR ||
		    test_thread_flag(TIF_MEMDIE))

Well, personally I do not care about TIF_MEMDIE/oom at all. We need the temporary hack (unless we have the "right" fix right now) which should be reverted later.

Not sure I understand... Yes, wait_for_completion_killable() can return immediately if TIF_SIGPENDING will be set again for any reason. Say, another signal. But the next iteration will clear TIF_SIGPENDING ?

Quoted 2 lines

As I think it is difficult to prove that kmalloc(GFP_KERNEL) never sets TIF_SIGPENDING flag

Ah, I see, you mean that kmalloc() can do this every time. No, this should not happen or we have another problem.

Anyway. I agree with any hack in scsi_host_alloc/etc, this is up to maintainers. I still think that your change uncovered the problems in drivers/message/fusion/, these problems should be fixed somehow.

Dear maintainers, we need your help.

Quoted 6 lines · Oleg Nesterov

Quoted 4 lines

Quoted 2 lines

If we need the urgent hack to fix the regression, then I suggest to change scsi_host_alloc() temporary until mptsas (or whatever) is fixed.

Device initialization taking longer than 30 seconds is possible and is not a hang up. It is systemd which needs to be fixed.

Perhaps systemd needs the fix too, I do not know. But this is irrelevant, I think. Or at least this should be discussed separately.

I confirmed that this problem goes away if systemd-udevd supports longer timeout.

Quoted 1 line

kthread_run() can fail anyway, mptsas_probe() should not crash the kernel.

Right. But mptsas_probe() triggering an OOPS is irrelevant to kthread_run() ( comment #27 ).

Quoted 6 lines

And btw, it is not clear to me if in this case device initialization really needs more than 30 seconds... My understanding is probably wrong, so please correct me. But it seems that before your "make kthread_create() killable"

- probe hangs

Quoted 8 lines

- SIGKILL wakes it up

- so I assume that the probe was interrupted and didn't finish correctly ???

- initialization continues, does scsi_host_alloc(), etc, and everything works fine even if probe was interrupted?

I confirmed that device initialization really took more than 30 seconds ( comments #51 and #52 ).

Quoted 3 lines

So perhaps that probe should not hang and this should be fixed too ? Do you know where exactly it hangs? And where it is woken up by SIGKILL ? Or I totally misunderstood ?

The probe did not hang. SIGKILL affected only wait_for_completion_killable() in kthread_create_on_node() called by mptsas_probe() via scsi_host_alloc(). Thus, the probe was interrupted because kthread_run() returned an error.

Quoted 4 lines

Quoted 4 lines

I think we need a bit different version, in order to take TIF_MEMDIE flag into account at the caller of kthread_create(), for the purpose of commit 786235ee is "try to die as soon as possible if chosen by the OOM killer".

Quoted 8 lines

Quoted 5 lines

	for (;;) {
		shost->ehandler = kthread_run(scsi_error_handler, shost,
					      "scsi_eh_%d", shost->host_no);
		if (PTR_ERR(shost->ehandler) != -EINTR ||
		    test_thread_flag(TIF_MEMDIE))

Well, personally I do not care about TIF_MEMDIE/oom at all. We need the temporary hack (unless we have the "right" fix right now) which should be reverted later.

I do seriously care about TIF_MEMDIE/oom. Last week I respond to a trouble which hit "kernel: request_module() OOM local DoS" (RHBZ #853474) without any malice.

Quoted 8 lines

Not sure I understand... Yes, wait_for_completion_killable() can return immediately if TIF_SIGPENDING will be set again for any reason. Say, another signal. But the next iteration will clear TIF_SIGPENDING ?

Quoted 2 lines

As I think it is difficult to prove that kmalloc(GFP_KERNEL) never sets TIF_SIGPENDING flag

Ah, I see, you mean that kmalloc() can do this every time. No, this should not happen or we have another problem.

Then, what happens if somebody does

  while (1)
      kill(pid, SIGKILL);

where pid is the process calling kthread_run() from the "for (;;)" loop in scsi_host_alloc()? Theoretically, it will form an infinite retry loop. Clearing TIF_SIGPENDING does not guarantee that next wait_for_completion_killable() does not return immediately. Doing retry decision at scsi_host_alloc() will make things worse than doing it at kthread_create_on_node().

Quoted 4 lines

Anyway. I agree with any hack in scsi_host_alloc/etc, this is up to maintainers. I still think that your change uncovered the problems in drivers/message/fusion/, these problems should be fixed somehow.

Quoted 2 lines

Dear maintainers, we need your help.

Right. We found that we can fix this problem by updating systemd-udevd to support longer timeout ( comment #53 ). Joseph, would you consult systemd maintainers?

On 03/19/2014 07:49 AM, Tetsuo Handa wrote:

Quoted 10 lines

Quoted 8 lines · Oleg Nesterov

And btw, it is not clear to me if in this case device initialization really needs more than 30 seconds... My understanding is probably wrong, so please correct me. But it seems that before your "make kthread_create() killable"

- probe hangs

- SIGKILL wakes it up

Quoted 8 lines

Quoted 6 lines

- so I assume that the probe was interrupted and didn't finish correctly ???

- initialization continues, does scsi_host_alloc(), etc, and everything works fine even if probe was interrupted?

I confirmed that device initialization really took more than 30 seconds ( comments #51 and #52 ).

Quoted 4 lines

Quoted 3 lines

So perhaps that probe should not hang and this should be fixed too ? Do you know where exactly it hangs? And where it is woken up by SIGKILL ? Or I totally misunderstood ?

The probe did not hang.

Quoted 2 lines

SIGKILL affected only wait_for_completion_killable()
in kthread_create_on_node() called by mptsas_probe() via scsi_host_alloc().

Quoted 1 line

Thus, the probe was interrupted because kthread_run() returned an error.

Quoted 9 lines

Quoted 2 lines

Ah, I see, you mean that kmalloc() can do this every time. No, this should not happen or we have another problem.

Then, what happens if somebody does

  while (1)
      kill(pid, SIGKILL);

where pid is the process calling kthread_run() from the "for (;;)" loop in scsi_host_alloc()?

Quoted 5 lines

Quoted 2 lines

Dear maintainers, we need your help.

Right. We found that we can fix this problem by updating systemd-udevd to support longer timeout ( comment #53 ). Joseph, would you consult systemd maintainers?

Thanks everyone for reviewing this bug. Message sent to systemd mailing list:
http://lists.freedesktop.org/archives/systemd-devel/2014-March/018006.html

But please do not forget that the kernel crashes. Whatever else we do, this should be fixed anyway. And this should be fixed in driver.

Quoted 3 lines · Oleg Nesterov

But please do not forget that the kernel crashes. Whatever else we do, this should be fixed anyway. And this should be fixed in driver.

drivers/message/fusion/ is obviously buggy.

Quoted 4 lines · Oleg Nesterov

Quoted 2 lines · Oleg Nesterov

But please do not forget that the kernel crashes. Whatever else we do, this should be fixed anyway. And this should be fixed in driver.

drivers/message/fusion/ is obviously buggy.

Perhaps this is the only problem and Tetsuo is right, this driver really needs more than 30 secs to probe...

But if you have a bit of free time, perhaps you can try the stupid debugging patch below ;) Not sure it will help, but who knows.

Oleg.

drivers/message/fusion/mptsas.c +9 -2

index 00d339c..5ecc27e 100644
@@ -5400,12 +5400,16 @@ mptsas_init(void)
 {
 	int error;
 
+	printk(KERN_CRIT "mptsas_init start\n");
+	current->flags |= 0x1;
 	show_mptmod_ver(my_NAME, my_VERSION);
 
 	mptsas_transport_template =
 	    sas_attach_transport(&mptsas_transport_functions);
-	if (!mptsas_transport_template)
-		return -ENODEV;
+	if (!mptsas_transport_template) {
+		error = -ENODEV;
+		goto out;
+	}
 	mptsas_transport_template->eh_timed_out = mptsas_eh_timed_out;
 
 	mptsasDoneCtx = mpt_register(mptscsih_io_done, MPTSAS_DRIVER,

@@ -5428,6 +5432,9 @@ mptsas_init(void)
 	if (error)
 		sas_release_transport(mptsas_transport_template);
 
+out:
+	current->flags &= ~0x1;
+	printk(KERN_CRIT "mptsas_init end\n");
 	return error;
 }
 

kernel/kthread.c +8 -0

index b5ae3ee..78e643d 100644
@@ -291,6 +291,13 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 	 * the OOM killer while kthreadd is trying to allocate memory for
 	 * new kernel thread.
 	 */
+
+	if (current->flags & 1) {
+		pr_crit("mptsas no killable wait: %d %d\n",
+			signal_pending(current), __fatal_signal_pending(current));
+		goto wait;
+	}
+
 	if (unlikely(wait_for_completion_killable(&done))) {
 		/*
 		 * If I was SIGKILLed before kthreadd (or new kernel thread)

@@ -303,6 +310,7 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 		 * kthreadd (or new kernel thread) will call complete()
 		 * shortly.
 		 */
+wait:
 		wait_for_completion(&done);
 	}
 	task = create->result;

kernel/sched/core.c +13 -0

index b46131e..2b202bd 100644
@@ -2655,6 +2655,14 @@ static void __sched __schedule(void)
 	unsigned long *switch_count;
 	struct rq *rq;
 	int cpu;
+	bool trace;
+
+	trace = (current->flags & 1) && current->state && !(preempt_count() & PREEMPT_ACTIVE);
+	if (trace) {
+		pr_crit("mptsas sched: %lx %d %d\n", current->state,
+			signal_pending(current), __fatal_signal_pending(current));
+		show_stack(NULL, NULL);
+	}
 
 need_resched:
 	preempt_disable();

@@ -2733,6 +2741,11 @@ need_resched:
 	sched_preempt_enable_no_resched();
 	if (need_resched())
 		goto need_resched;
+
+	if (trace) {
+		pr_crit("mptsas wake: %d %d\n",
+			signal_pending(current), __fatal_signal_pending(current));
+	}
 }
 
 static inline void sched_submit_work(struct task_struct *tsk)

kernel/signal.c +0 -0

index 52f881d..d121944 100644
@@ -1152,6 +1152,11 @@ static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
 {

Quoted 3 lines · Oleg Nesterov

Quoted 2 lines · Oleg Nesterov

But please do not forget that the kernel crashes. Whatever else we do, this should be fixed anyway. And this should be fixed in driver.

Quoted 1 line

drivers/message/fusion/ is obviously buggy.

Perhaps this is the only problem and Tetsuo is right, this driver really needs more than 30 secs to probe...

But if you have a bit of free time, perhaps you can try the stupid debugging patch below ;) Not sure it will help, but who knows.

Oleg.

drivers/message/fusion/mptsas.c +9 -2

index 00d339c..5ecc27e 100644
@@ -5400,12 +5400,16 @@ mptsas_init(void)
 {
 	int error;
 
+	printk(KERN_CRIT "mptsas_init start\n");
+	current->flags |= 0x1;
 	show_mptmod_ver(my_NAME, my_VERSION);
 
 	mptsas_transport_template =
 	    sas_attach_transport(&mptsas_transport_functions);
-	if (!mptsas_transport_template)
-		return -ENODEV;
+	if (!mptsas_transport_template) {
+		error = -ENODEV;
+		goto out;
+	}
 	mptsas_transport_template->eh_timed_out = mptsas_eh_timed_out;
 
 	mptsasDoneCtx = mpt_register(mptscsih_io_done, MPTSAS_DRIVER,

@@ -5428,6 +5432,9 @@ mptsas_init(void)
 	if (error)
 		sas_release_transport(mptsas_transport_template);
 
+out:
+	current->flags &= ~0x1;
+	printk(KERN_CRIT "mptsas_init end\n");
 	return error;
 }
 

kernel/kthread.c +8 -0

index b5ae3ee..78e643d 100644
@@ -291,6 +291,13 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 	 * the OOM killer while kthreadd is trying to allocate memory for
 	 * new kernel thread.
 	 */
+
+	if (current->flags & 1) {
+		pr_crit("mptsas no killable wait: %d %d\n",
+			signal_pending(current), __fatal_signal_pending(current));
+		goto wait;
+	}
+
 	if (unlikely(wait_for_completion_killable(&done))) {
 		/*
 		 * If I was SIGKILLed before kthreadd (or new kernel thread)

@@ -303,6 +310,7 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 		 * kthreadd (or new kernel thread) will call complete()
 		 * shortly.
 		 */
+wait:
 		wait_for_completion(&done);
 	}
 	task = create->result;

kernel/sched/core.c +13 -0

index b46131e..2b202bd 100644
@@ -2655,6 +2655,14 @@ static void __sched __schedule(void)
 	unsigned long *switch_count;
 	struct rq *rq;
 	int cpu;
+	bool trace;
+
+	trace = (current->flags & 1) && current->state && !(preempt_count() & PREEMPT_ACTIVE);
+	if (trace) {
+		pr_crit("mptsas sched: %lx %d %d\n", current->state,
+			signal_pending(current), __fatal_signal_pending(current));
+		show_stack(NULL, NULL);
+	}
 
 need_resched:
 	preempt_disable();

@@ -2733,6 +2741,11 @@ need_resched:
 	sched_preempt_enable_no_resched();
 	if (need_resched())
 		goto need_resched;
+
+	if (trace) {
+		pr_crit("mptsas wake: %d %d\n",
+			signal_pending(current), __fatal_signal_pending(current));
+	}
 }
 
 static inline void sched_submit_work(struct task_struct *tsk)

kernel/signal.c +5 -0

index 52f881d..d121944 100644
@@ -1152,6 +1152,11 @@ static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
 {
 	int from_ancestor_ns = 0;
 
+	if (t->flags & 1) {
+		pr_crit("mptsas killed %d\n", sig);
+		sched_show_task(t);
+	}
+
 #ifdef CONFIG_PID_NS
 	from_ancestor_ns = si_fromuser(info) &&
 			   !task_pid_nr_ns(current, task_active_pid_ns(t));

Quoted 8 lines · Oleg Nesterov

Quoted 4 lines · Oleg Nesterov

Quoted 2 lines · Oleg Nesterov

But please do not forget that the kernel crashes. Whatever else we do, this should be fixed anyway. And this should be fixed in driver.

drivers/message/fusion/ is obviously buggy.

Perhaps this is the only problem and Tetsuo is right, this driver really needs more than 30 secs to probe...

Quoted 4 lines

But if you have a bit of free time, perhaps you can try the stupid debugging patch below ;) Not sure it will help, but who knows.

Oleg.

There was some testing done with your test kernel. The data collected while running your kernel is available in the bug report:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705/comments/58

Quoted 4 lines · Joseph Salisbury

There was some testing done with your test kernel. The data collected while running your kernel is available in the bug report:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705/comments/58

Joseph, thanks a lot.

I'll try to read the logs tomorrow, but at first glance Tetsuo was right, I do not see a "long" sleep in that log.

Yes, it seems that it actually needs > 30 secs. It spends most of the time (30.13286 seconds) in [..]

Quoted 3 lines · Oleg Nesterov

Yes, it seems that it actually needs > 30 secs. It spends most of the time (30.13286 seconds) in [..]

So how about taking a completely different approach:

- just say that waiting for devices in the module init sequence for
over 30 seconds is really really wrong.

- make the damn mptsas driver just register the controller from the
init sequence, and then do device discovery asynchronously.

The ATA layer does this correctly: it synchronously finds each host, but then it does

        /* perform each probe asynchronously */
        for (i = 0; i < host->n_ports; i++) {
                struct ata_port *ap = host->ports[i];
                async_schedule(async_port_probe, ap);
        }

and I really think SCSI drivers should do the same if they have this kind of "ports can take forever to probe" behavior.

What would be the equivalent magic to do this for SCSI? Could we just make something like scsi_probe_and_add_lun() just always do this, the same way ata_host_register() does it?

Linus

Quoted 3 lines · Oleg Nesterov

Yes, it seems that it actually needs > 30 secs. It spends most of the time (30.13286 seconds) in [..]

So how about taking a completely different approach:

- just say that waiting for devices in the module init sequence for
over 30 seconds is really really wrong.

- make the damn mptsas driver just register the controller from the
init sequence, and then do device discovery asynchronously.

The ATA layer does this correctly: it synchronously finds each host, but then it does

        /* perform each probe asynchronously */
        for (i = 0; i < host->n_ports; i++) {
                struct ata_port *ap = host->ports[i];
                async_schedule(async_port_probe, ap);
        }

and I really think SCSI drivers should do the same if they have this kind of "ports can take forever to probe" behavior.

What would be the equivalent magic to do this for SCSI? Could we just make something like scsi_probe_and_add_lun() just always do this, the same way ata_host_register() does it?

Tetsuo, what do you think?

From 731f1f6dec7efaa132f751c432079b9b1fdb78d2 Mon Sep 17 00:00:00 2001

Quoted 1 line · Oleg Nesterov

Tetsuo, what do you think?

I don't like blocking SIGKILL while doing operations that depend on memory allocation by other processes. If the OOM killer is triggered and it chose the process blocking SIGKILL in mptsas_init() (I know it unlikely happens), it generates the OOM killer deadlock.

My preference is to fix kthread_create() to handle SIGKILL gracefully.

Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage.

Commit 786235ee "kthread: make kthread_create() killable" changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1].

Therefore, this patch changes kthread_create() from "wait forever in killable state" to "wait for 10 seconds in unkillable state, check for the OOM killer every second".

This patch also changes the return value of timeout case from -ENOMEM to -EINTR because -ENOMEM could make sense for only TIF_MEMDIE case.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705

Signed-off-by Tetsuo Handa <[email protected]>

---
 kernel/kthread.c |   37 +++++++++++++++++++++----------------
 1 files changed, 21 insertions(+), 16 deletions(-)

kernel/kthread.c +21 -17

index b5ae3ee..6a25a9f 100644
@@ -269,6 +269,7 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 					   const char namefmt[],
 					   ...)
 {
+	int i = 0;
 	DECLARE_COMPLETION_ONSTACK(done);
 	struct task_struct *task;
 	struct kthread_create_info *create = kmalloc(sizeof(*create),

@@ -287,24 +288,28 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 
 	wake_up_process(kthreadd_task);
 	/*
-	 * Wait for completion in killable state, for I might be chosen by
-	 * the OOM killer while kthreadd is trying to allocate memory for
-	 * new kernel thread.
+	 * Wait for completion with 10 seconds timeout. Unless the kthreadd is
+	 * blocked for direct memory reclaim path, the kthreadd will be able to
+	 * complete the request within 10 seconds. Also, check every second if
+	 * I was chosen by the OOM killer in order to avoid the OOM killer
+	 * deadlock.
 	 */
-	if (unlikely(wait_for_completion_killable(&done))) {
-		/*
-		 * If I was SIGKILLed before kthreadd (or new kernel thread)
-		 * calls complete(), leave the cleanup of this structure to
-		 * that thread.
-		 */
-		if (xchg(&create->done, NULL))
-			return ERR_PTR(-ENOMEM);
-		/*
-		 * kthreadd (or new kernel thread) will call complete()
-		 * shortly.
-		 */
-		wait_for_completion(&done);
+	do {
+		if (likely(wait_for_completion_timeout(&done, HZ)))
+			goto ready;
+	} while (i++ < 10 && !test_thread_flag(TIF_MEMDIE));
+	/*
+	 * The kthreadd was unable to complete the request within 10 seconds
+	 * (or I was chosen by the OOM killer). Thus, give up and leave the
+	 * cleanup of this structure to the kthreadd (or new kernel thread).
+	 */
+	if (xchg(&create->done, NULL)) {
+		WARN(1, "Gave up waiting for kthreadd.\n");
+		return ERR_PTR(-EINTR);
 	}
+	/* kthreadd (or new kernel thread) will call complete() shortly. */
+	wait_for_completion(&done);
+ready:
 	task = create->result;
 	if (!IS_ERR(task)) {
 		static const struct sched_param param = { .sched_priority = 0 };
--

1.7.1

Quoted 8 lines · Tetsuo Handa

Quoted 1 line · Oleg Nesterov

Tetsuo, what do you think?

I don't like blocking SIGKILL while doing operations that depend on memory allocation by other processes. If the OOM killer is triggered and it chose the process blocking SIGKILL in mptsas_init() (I know it unlikely happens), it generates the OOM killer deadlock.

It seems that we do not understand each other.

I do not like this hack too. And it is even wrong, you can't really block SIGKILL. And it is unnecessary in a sense that (I think) it is fine that module_init() reacts to SIGKILL and aborts, just the fact it crashes the kernel in the error paths is not fine.

The driver should be fixed anyway. As for timeout, either userspace/systemd should be changed to not send SIGKILL after 30 secs, or (better) the driver should be changed to not waste 30 secs.

The hack I sent can only serve as a short term solution, it should be reverted once we have something better. And, otoh, this hack only affects the problematic driver which should be fixed in any case.

Do you see my point? I can be wrong, but I think that you constantly misunderstand the intent.

Quoted 1 line

My preference is to fix kthread_create() to handle SIGKILL gracefully.

And now I do not understand you too. I do not understand why should we "fix" kthread_create().

Quoted 3 lines

Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage.

Personally I do not really think so, but OK. In this case lets revert 786235ee.

Quoted 4 lines

Commit 786235ee "kthread: make kthread_create() killable" changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1].

And I still think that 786235ee simply uncovered the problems in this driver. Perhaps we should change kthread_create() for some reason, but (imho) not because we need to help the buggy code.

Quoted 3 lines

Therefore, this patch changes kthread_create() from "wait forever in killable state" to "wait for 10 seconds in unkillable state, check for the OOM killer every second".

Personally I dislike this change. In fact I think it is ugly. But this is only my opinion.

If you convince someone to take this patch I won't argue.

OK, the fix from the SCSI point of view is to make the mpt teardown path actually work. The whole thing looks to be a complete mess because there's another place where it will do the wrong thing in mptscsih_remove(). You always have to call mpt_detach() otherwise the device doesn't get removed from the lists. In theory this patch fixes both bugs in the driver.

Quoted 3 lines · Tetsuo Handa

Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage.

Personally I do not really think so, but OK. In this case lets revert 786235ee.

Quoted 4 lines

Commit 786235ee "kthread: make kthread_create() killable" changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1].

And I still think that 786235ee simply uncovered the problems in this driver. Perhaps we should change kthread_create() for some reason, but (imho) not because we need to help the buggy code.

Quoted 3 lines

Therefore, this patch changes kthread_create() from "wait forever in killable state" to "wait for 10 seconds in unkillable state, check for the OOM killer every second".

Personally I dislike this change. In fact I think it is ugly. But this is only my opinion.

Quoted 2 lines · Oleg Nesterov

Personally I dislike this change. In fact I think it is ugly. But this is only my opinion.

It's not only ugly, it's activly wrong. It's as wrong as 786235ee itself. And 786235ee needs to be reverted and the revert must go into 3.13.stable as well. I'll send a revert request in separate mail.

But then systemd/udev mutters:

"You migh be able to work around the timeout with udev rules and
OPTIONS+="event_timeout=120", but that code was maybe never used or tested, so it might not work correctly." [1]

AFAICT from the ubuntu bug system [2] nobody bothered even to try that.

And if the udev/systemd event_timeout option is broken it's way better to fix that one instead of hacking random heuristics into the kernel.

Quoted 10 lines · Thomas Gleixner

But then systemd/udev mutters:

"You migh be able to work around the timeout with udev rules and
OPTIONS+="event_timeout=120", but that code was maybe never used or tested, so it might not work correctly." [1]

AFAICT from the ubuntu bug system [2] nobody bothered even to try that.

And if the udev/systemd event_timeout option is broken it's way better to fix that one instead of hacking random heuristics into the kernel.

I haven't tried the event_timeout= option but I think it will not work. The timeout is hard coded as shown below and there will be no chance for taking the event_timeout= option into account.

---------- systemd-204/src/udev/udevd.c start ---------- (...snipped...)

                        /* check for hanging events */
                        udev_list_node_foreach(loop, &worker_list) {
                                struct worker *worker = node_to_worker(loop);

                                if (worker->state != WORKER_RUNNING)
                                        continue;

                                if ((now(CLOCK_MONOTONIC) - worker->event_start_usec) > 30 * 1000 * 1000) {

Quoted 18 lines · Tetsuo Handa

Quoted 10 lines · Thomas Gleixner

But then systemd/udev mutters:

"You migh be able to work around the timeout with udev rules and
OPTIONS+="event_timeout=120", but that code was maybe never used or tested, so it might not work correctly." [1]

AFAICT from the ubuntu bug system [2] nobody bothered even to try that.

And if the udev/systemd event_timeout option is broken it's way better to fix that one instead of hacking random heuristics into the kernel.

I haven't tried the event_timeout= option but I think it will not work. The timeout is hard coded as shown below and there will be no chance for taking the event_timeout= option into account.

---------- systemd-204/src/udev/udevd.c start ---------- (...snipped...)

Quoted 8 lines

                        /* check for hanging events */
                        udev_list_node_foreach(loop, &worker_list) {
                                struct worker *worker = node_to_worker(loop);

                                if (worker->state != WORKER_RUNNING)
                                        continue;

                                if ((now(CLOCK_MONOTONIC) - worker->event_start_usec) > 30 * 1000 * 1000) {

And because systemd has an immutable hardcoded random timeout we add another hardcoded random timeout into kthread_create() to work around that.

How broken is that?

And it seems other people have solved it:

    http://www.redhat.com/archives/lvm-devel/2013-September/msg00036.html

Quoted 35 lines · Thomas Gleixner

Quoted 18 lines · Tetsuo Handa

Quoted 10 lines · Thomas Gleixner

But then systemd/udev mutters:

"You migh be able to work around the timeout with udev rules and
OPTIONS+="event_timeout=120", but that code was maybe never used or tested, so it might not work correctly." [1]

AFAICT from the ubuntu bug system [2] nobody bothered even to try that.

And if the udev/systemd event_timeout option is broken it's way better to fix that one instead of hacking random heuristics into the kernel.

I haven't tried the event_timeout= option but I think it will not work. The timeout is hard coded as shown below and there will be no chance for taking the event_timeout= option into account.

---------- systemd-204/src/udev/udevd.c start ---------- (...snipped...)

Quoted 8 lines

                        /* check for hanging events */
                        udev_list_node_foreach(loop, &worker_list) {
                                struct worker *worker = node_to_worker(loop);

                                if (worker->state != WORKER_RUNNING)
                                        continue;

                                if ((now(CLOCK_MONOTONIC) - worker->event_start_usec) > 30 * 1000 * 1000) {

And because systemd has an immutable hardcoded random timeout we add another hardcoded random timeout into kthread_create() to work around that.

How broken is that?

Quoted 3 lines

And it seems other people have solved it:

    http://www.redhat.com/archives/lvm-devel/2013-September/msg00036.html

I agree with Thomas. A hardcoded timeout is a systemd bug. However, could I get confirmation, while you can use this bug to do it, that the patch back in this thread actually fixes the crash when scsi_alloc_host fails, that's the serious SCSI bug, in my view?

Quoted 38 lines · James Bottomley

Quoted 37 lines · Thomas Gleixner

Quoted 18 lines · Tetsuo Handa

Quoted 10 lines · Thomas Gleixner

But then systemd/udev mutters:

"You migh be able to work around the timeout with udev rules and
OPTIONS+="event_timeout=120", but that code was maybe never used or tested, so it might not work correctly." [1]

AFAICT from the ubuntu bug system [2] nobody bothered even to try that.

And if the udev/systemd event_timeout option is broken it's way better to fix that one instead of hacking random heuristics into the kernel.

I haven't tried the event_timeout= option but I think it will not work. The timeout is hard coded as shown below and there will be no chance for taking the event_timeout= option into account.

---------- systemd-204/src/udev/udevd.c start ---------- (...snipped...)

Quoted 8 lines

                        /* check for hanging events */
                        udev_list_node_foreach(loop, &worker_list) {
                                struct worker *worker = node_to_worker(loop);

                                if (worker->state != WORKER_RUNNING)
                                        continue;

                                if ((now(CLOCK_MONOTONIC) - worker->event_start_usec) > 30 * 1000 * 1000) {

And because systemd has an immutable hardcoded random timeout we add another hardcoded random timeout into kthread_create() to work around that.

How broken is that?

And it seems other people have solved it:

Quoted 6 lines

Quoted 1 line

    http://www.redhat.com/archives/lvm-devel/2013-September/msg00036.html

I agree with Thomas. A hardcoded timeout is a systemd bug. However, could I get confirmation, while you can use this bug to do it, that the patch back in this thread actually fixes the crash when scsi_alloc_host fails, that's the serious SCSI bug, in my view?

Which patch, the one to kthread_create() or the one to SCSI?

Quoted 8 lines · Tetsuo Handa

Quoted 1 line · Oleg Nesterov

Tetsuo, what do you think?

I don't like blocking SIGKILL while doing operations that depend on memory allocation by other processes. If the OOM killer is triggered and it chose the process blocking SIGKILL in mptsas_init() (I know it unlikely happens), it generates the OOM killer deadlock.

It seems that we do not understand each other.

I do not like this hack too. And it is even wrong, you can't really block SIGKILL. And it is unnecessary in a sense that (I think) it is fine that module_init() reacts to SIGKILL and aborts, just the fact it crashes the kernel in the error paths is not fine.

The driver should be fixed anyway. As for timeout, either userspace/systemd should be changed to not send SIGKILL after 30 secs, or (better) the driver should be changed to not waste 30 secs.

The hack I sent can only serve as a short term solution, it should be reverted once we have something better. And, otoh, this hack only affects the problematic driver which should be fixed in any case.

Do you see my point? I can be wrong, but I think that you constantly misunderstand the intent.

Quoted 1 line

My preference is to fix kthread_create() to handle SIGKILL gracefully.

And now I do not understand you too. I do not understand why should we "fix" kthread_create().

Quoted 4 lines

Many kernel operations (e.g. mutex_lock() wait_event() wait_for_completion()) ignore SIGKILL and the current users depend on SIGKILL being ignored. Thus, commit 786235ee sounds like a kernel API breakage.

Personally I do not really think so, but OK. In this case lets revert 786235ee.

Quoted 4 lines

Commit 786235ee "kthread: make kthread_create() killable" changed to leave kthread_create() as soon as receiving SIGKILL. But this change caused boot failures because systemd-udevd receives SIGKILL due to timeout while loading SCSI controller drivers using finit_module() [1].

And I still think that 786235ee simply uncovered the problems in this driver. Perhaps we should change kthread_create() for some reason, but (imho) not because we need to help the buggy code.

Quoted 3 lines

Therefore, this patch changes kthread_create() from "wait forever in killable state" to "wait for 10 seconds in unkillable state, check for the OOM killer every second".

Personally I dislike this change. In fact I think it is ugly. But this is only my opinion.

If you convince someone to take this patch I won't argue.

Hi Tetsuo,

A kernel bug report was opened against Ubuntu[0]. We performed a kernel bisect, and found that reverting the following commit resolved this bug:

commit 786235eeba0e1e85e5cbbb9f97d1087ad03dfa21

Author: Tetsuo Handa <[email protected]>
Date:   Tue Nov 12 15:06:45 2013 -0800

kthread: make kthread_create() killable

The regression was introduced as of v3.13-rc1.

The bug indicates an issue with the SAS controller during initialization, which prevents the system from booting. Additional details are available in the bug report or on request.

I was hoping to get your feedback, since you are the patch author. Do you think gathering any additional data will help diagnose this issue, or would it be best to submit a revert request?

[0] http://pad.lv/1276705

Quoted 22 lines · On Fri, 14 Mar 2014 16:46:26 -0400 Joseph Salisbury…

Hi Tetsuo,

A kernel bug report was opened against Ubuntu[0]. We performed a kernel bisect, and found that reverting the following commit resolved this bug:

commit 786235eeba0e1e85e5cbbb9f97d1087ad03dfa21

Author: Tetsuo Handa <[email protected]>
Date:   Tue Nov 12 15:06:45 2013 -0800

kthread: make kthread_create() killable

The regression was introduced as of v3.13-rc1.

The bug indicates an issue with the SAS controller during initialization, which prevents the system from booting. Additional details are available in the bug report or on request.

I was hoping to get your feedback, since you are the patch author. Do you think gathering any additional data will help diagnose this issue, or would it be best to submit a revert request?

Quoted 1 line

[0] http://pad.lv/1276705

What process is running here? Presumably modprobe.

A possible explanation is that modprobe has genuinely received a SIGKILL. Can you identify anything in this setup which might send a SIGKILL to the modprobe process?

kthread_create_on_node() thinks that SIGKILL came from the oom-killer and it cheerfully returns -ENOMEM, which is incorrect if that signal came from userspace.

And I don't _think_ we prevent
userspace-originated signals from unblocking
wait_for_completion_killable()?

Root cause time: it's wrong for the oom-killer to use SIGKILL.

In fact
it's basically always wrong to send signals from in-kernel.

Signals
are a userspace IPC mechanism and using them in-kernel a) makes it hard (or impossible) to distinguish them from userspace-originated signals and b) permits userspace to produce surprising results in the kernel, which I suspect is what we're seeing here.

Quoted 3 lines

kthread_create_on_node() thinks that SIGKILL came from the oom-killer and it cheerfully returns -ENOMEM, which is incorrect if that signal came from userspace.

Yes, I think it should return -EINTR.

Quoted 3 lines

And I don't _think_ we prevent
userspace-originated signals from unblocking
wait_for_completion_killable()?

And we should not.

Quoted 1 line

Root cause time: it's wrong for the oom-killer to use SIGKILL.

Not sure... what else?

Quoted 2 lines

In fact
it's basically always wrong to send signals from in-kernel.

Well, SIGSEGV, SIGIO...

Quoted 5 lines

Signals
are a userspace IPC mechanism and using them in-kernel a) makes it hard (or impossible) to distinguish them from userspace-originated signals and b) permits userspace to produce surprising results in the kernel, which I suspect is what we're seeing here.

Well, I think in this case it doesn't matter who/why sends a signal. The task is killed, it should react and exit asap. And kthread_create() can fail in any case, at least the kernel should not crash.

Quoted 3 lines · On Mon, 17 Mar 2014 21:19:19 +0100 Oleg Nesterov <[email protected]> wrote:

Quoted 1 line

Root cause time: it's wrong for the oom-killer to use SIGKILL.

Not sure... what else?

If we really really have to use userspace IPC in the kernel then I suppose we could add a new signal type (SIGKERNEL?) which userspace can neither receive nor send.

Quoted 4 lines

Quoted 2 lines

In fact
it's basically always wrong to send signals from in-kernel.

Well, SIGSEGV, SIGIO...

Those are kernel->userspace, not kernel->kernel.

We have all sorts of cross-task mechanisms available in kernel (set_bit and wake_up would suffice here) so we don't need to use signals with all the baggage they bring along.

Quoted 3 lines

Well, I think in this case it doesn't matter who/why sends a signal. The task is killed, it should react and exit asap. And kthread_create() can fail in any case, at least the kernel should not crash.

Well yes. In this "bug", userspace tried to kill udev and surprise surprise, that's what happened. The only real kernel bug here is the incorrect errno.

But in the real world, the kernel changed and systems broke - we should try to unbreak them.

From c41fa0b9294900a86167d6c8db392d99dde09774 Mon Sep 17 00:00:00 2001

Quoted 1 line

From c41fa0b9294900a86167d6c8db392d99dde09774 Mon Sep 17 00:00:00 2001

From: Tetsuo Handa <[email protected]>
Date: Tue, 3 Jun 2014 21:46:24 +0900
Subject: [PATCH] kthread: Fix return value of kthread_create() upon SIGKILL.

Commit 786235eeb "kthread: make kthread_create() killable" meant for allowing kthread_create() to abort as soon as killed by the OOM-killer. But returning -ENOMEM is wrong if killed by SIGKILL from userspace. Change kthread_create() to return -EINTR upon SIGKILL.

Signed-off-by Tetsuo Handa <[email protected]>

Cc Oleg Nesterov <[email protected]>

Cc David Rientjes <[email protected]>

Cc stable <[email protected]> [3.13+]

What process is running here? Presumably modprobe.

A possible explanation is that modprobe has genuinely received a SIGKILL. Can you identify anything in this setup which might send a SIGKILL to the modprobe process?

kthread_create_on_node() thinks that SIGKILL came from the oom-killer and it cheerfully returns -ENOMEM, which is incorrect if that signal came from userspace. And I don't _think_ we prevent userspace-originated signals from unblocking wait_for_completion_killable()?

Root cause time: it's wrong for the oom-killer to use SIGKILL. In fact

it's basically always wrong to send signals from in-kernel. Signals are a userspace IPC mechanism and using them in-kernel a) makes it hard (or impossible) to distinguish them from userspace-originated signals

and b) permits userspace to produce surprising results in the kernel, which I suspect is what we're seeing here.

Quoted 3 lines

and b) permits userspace to produce surprising results in the kernel, which I suspect is what we're seeing here.

There is enough information for kernel side code to decide whether a signal came from kernel or userspace.

Then again - it's not clear that it
should ever have to.