2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

[Rafael J. Wysocki]

Hi,

The BUG_ON in khthread_bind (line 165 in kthread.c) triggers for me during
attempted suspend to disk, when disable_nonboot_cpus() calls _cpu_down()
(on x86_64).

This didn't happen with 2.6.21-rc6-mm1.

Greetings,
Rafael


-- 
If you don't have the time to read,
you don't have the time or the tools to write.
		- Stephen King

Re: 2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

[Andrew Morton]

On Thu, 26 Apr 2007 01:10:21 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:

> Hi,
> 
> The BUG_ON in khthread_bind (line 165 in kthread.c) triggers for me during
> attempted suspend to disk, when disable_nonboot_cpus() calls _cpu_down()
> (on x86_64).

I guess the backtrace would be pretty important here.

Guys, please don't add BUG_ONs unless there is simply no sane way to recover.

Because when someone goofs up, the BUG_ON will kill the whole machine and
everyone else who has code being tested in -mm loses a tester.

Plus a BUG_ON *greatly* decreases our chances of getting a trace from the
tester: dead box, nothing in the logs.


--- a/kernel/kthread.c~fix-kthread_create-vs-freezer-theoretical-race-dont-be-obnoxious
+++ a/kernel/kthread.c
@@ -162,7 +162,10 @@ EXPORT_SYMBOL(kthread_create);
  */
 void kthread_bind(struct task_struct *k, unsigned int cpu)
 {
-	BUG_ON(k->state != TASK_UNINTERRUPTIBLE);
+	if (k->state != TASK_UNINTERRUPTIBLE) {
+		WARN_ON(1);
+		return;
+	}
 	/* Must have done schedule() in kthread() before we set_task_cpu */
 	wait_task_inactive(k);
 	set_task_cpu(k, cpu);
_

Re: 2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

[Gautham R Shenoy]

On Wed, Apr 25, 2007 at 04:54:10PM -0700, Andrew Morton wrote:
> On Thu, 26 Apr 2007 01:10:21 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > Hi,
> > 
> > The BUG_ON in khthread_bind (line 165 in kthread.c) triggers for me during
> > attempted suspend to disk, when disable_nonboot_cpus() calls _cpu_down()
> > (on x86_64).
> 
Caused due to Oleg's patch http://lkml.org/lkml/2007/4/13/93.

Agreed that most of the time a kthread_create(p) is followed by a
kthread_bind(p), in which case the assertion 
WARN_ON(p->state != TASK_UNINTERRUPTIBLE) makes sense.

But, in cpu hotplug case, we need to rebind the stop_machine_run thread
from the cpu which has just been offlined to any online cpu.
(kernel/cpu.c line 180)
At this point, the thread would be in TASK_INTERRUPTIBLE waiting for us
to call a kthread_stop on it.(kernel/kthread.c line 161)

We only need to ensure in kthread_bind that the task which is being
bound is not running or exiting. Doesn't matter if it's sleeping in 
TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE state.

Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
 kernel/kthread.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.21-rc7/kernel/kthread.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/kthread.c
+++ linux-2.6.21-rc7/kernel/kthread.c
@@ -162,7 +162,8 @@ EXPORT_SYMBOL(kthread_create);
  */
 void kthread_bind(struct task_struct *k, unsigned int cpu)
 {
-	if (k->state != TASK_UNINTERRUPTIBLE) {
+	if (k->state != TASK_UNINTERRUPTIBLE &&
+	    k->state != TASK_INTERRUPTIBLE) {
 		WARN_ON(1);
 		return;
 	}
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: 2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

[Oleg Nesterov]

On 04/26, Gautham R Shenoy wrote:
>
> On Wed, Apr 25, 2007 at 04:54:10PM -0700, Andrew Morton wrote:
> > On Thu, 26 Apr 2007 01:10:21 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> > 
> > > Hi,
> > > 
> > > The BUG_ON in khthread_bind (line 165 in kthread.c) triggers for me during
> > > attempted suspend to disk, when disable_nonboot_cpus() calls _cpu_down()
> > > (on x86_64).
> > 
> Caused due to Oleg's patch http://lkml.org/lkml/2007/4/13/93.
> 
> Agreed that most of the time a kthread_create(p) is followed by a
> kthread_bind(p), in which case the assertion 
> WARN_ON(p->state != TASK_UNINTERRUPTIBLE) makes sense.
> 
> But, in cpu hotplug case, we need to rebind the stop_machine_run thread
> from the cpu which has just been offlined to any online cpu.
> (kernel/cpu.c line 180)

I can't understand why do we need to re-bind this thread. We are doing
kthread_stop()->wake_up() below, at this point move_task_off_dead_cpu()
has already cared about this task, no?

> We only need to ensure in kthread_bind that the task which is being
> bound is not running or exiting. Doesn't matter if it's sleeping in 
> TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE state.

We need to ensure that this task can't be woken after return from
wait_task_inactive(k), otherwise set_task_cpu() after that is not safe.

TASK_INTERRUPTIBLE doesn't protect us from freezing.

Couldn't we just remove this kthread_bind() ?

Oleg.

Re: 2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

[Eric W. Biederman]

Gautham R Shenoy <ego@in.ibm.com> writes:

> On Wed, Apr 25, 2007 at 04:54:10PM -0700, Andrew Morton wrote:
>> On Thu, 26 Apr 2007 01:10:21 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
>> 
>> > Hi,
>> > 
>> > The BUG_ON in khthread_bind (line 165 in kthread.c) triggers for me during
>> > attempted suspend to disk, when disable_nonboot_cpus() calls _cpu_down()
>> > (on x86_64).
>> 
> Caused due to Oleg's patch http://lkml.org/lkml/2007/4/13/93.
>
> Agreed that most of the time a kthread_create(p) is followed by a
> kthread_bind(p), in which case the assertion 
> WARN_ON(p->state != TASK_UNINTERRUPTIBLE) makes sense.
>
> But, in cpu hotplug case, we need to rebind the stop_machine_run thread
> from the cpu which has just been offlined to any online cpu.
> (kernel/cpu.c line 180)
> At this point, the thread would be in TASK_INTERRUPTIBLE waiting for us
> to call a kthread_stop on it.(kernel/kthread.c line 161)
>
> We only need to ensure in kthread_bind that the task which is being
> bound is not running or exiting. Doesn't matter if it's sleeping in 
> TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE state.

That will probably handle this problem.

However there is a weird interaction with process freezer.

The process freezer can come in and  wake up a kernel thread
to encourage it to call try_to_freeze_process while it is 
waiting to be bound.

How do we handle that evil race?

Eric

Re: 2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

[Gautham R Shenoy]

On Thu, Apr 26, 2007 at 02:15:47PM +0400, Oleg Nesterov wrote:
> 
> I can't understand why do we need to re-bind this thread. We are doing
> kthread_stop()->wake_up() below, at this point move_task_off_dead_cpu()
> has already cared about this task, no?
> 
> > We only need to ensure in kthread_bind that the task which is being
> > bound is not running or exiting. Doesn't matter if it's sleeping in 
> > TASK_INTERRUPTIBLE or TASK_UNINTERRUPTIBLE state.
> 
> We need to ensure that this task can't be woken after return from
> wait_task_inactive(k), otherwise set_task_cpu() after that is not safe.
> 
> TASK_INTERRUPTIBLE doesn't protect us from freezing.
> 
> Couldn't we just remove this kthread_bind() ?

Fair enough. We are anyway kthread_stop()ping other per-cpu kernel threads
after move_task_off_dead_cpu(), so we can do it with the stop_machine_run
thread as well.

I just checked with Vatsa if there was any subtle reason why they
had put in the kthread_bind() in cpu.c. Vatsa cannot seem to recollect
any and I can't see any. So let us just remove the kthread_bind.

Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
---
 kernel/cpu.c |    4 ----
 1 files changed, 4 deletions(-)

Index: linux-2.6.21-rc7/kernel/cpu.c
===================================================================
--- linux-2.6.21-rc7.orig/kernel/cpu.c
+++ linux-2.6.21-rc7/kernel/cpu.c
@@ -176,10 +176,6 @@ static int _cpu_down(unsigned int cpu, i
 	/* This actually kills the CPU. */
 	__cpu_die(cpu);
 
-	/* Move it here so it can run. */
-	kthread_bind(p, get_cpu());
-	put_cpu();
-
 	/* CPU is completely dead: tell everyone.  Too late to complain. */
 	if (raw_notifier_call_chain(&cpu_chain, CPU_DEAD | mod,
 				    hcpu) == NOTIFY_BAD)
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: 2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

[Rafael J. Wysocki]

On Thursday, 26 April 2007 01:54, Andrew Morton wrote:
> On Thu, 26 Apr 2007 01:10:21 +0200 "Rafael J. Wysocki" <rjw@sisk.pl> wrote:
> 
> > Hi,
> > 
> > The BUG_ON in khthread_bind (line 165 in kthread.c) triggers for me during
> > attempted suspend to disk, when disable_nonboot_cpus() calls _cpu_down()
> > (on x86_64).
> 
> I guess the backtrace would be pretty important here.

Yes, I didn't have the time to collect one yestarday.

Re: 2.6.21-rc7-mm1: BUG_ON in kthread_bind during _cpu_down

[Andrew Morton]

On Thu, 26 Apr 2007 18:28:38 +0530
Gautham R Shenoy <ego@in.ibm.com> wrote:

> I just checked with Vatsa if there was any subtle reason why they
> had put in the kthread_bind() in cpu.c. Vatsa cannot seem to recollect
> any and I can't see any. So let us just remove the kthread_bind.
> 
> Signed-off-by: Gautham R Shenoy <ego@in.ibm.com>
> ---
>  kernel/cpu.c |    4 ----
>  1 files changed, 4 deletions(-)
> 
> Index: linux-2.6.21-rc7/kernel/cpu.c
> ===================================================================
> --- linux-2.6.21-rc7.orig/kernel/cpu.c
> +++ linux-2.6.21-rc7/kernel/cpu.c
> @@ -176,10 +176,6 @@ static int _cpu_down(unsigned int cpu, i
>  	/* This actually kills the CPU. */
>  	__cpu_die(cpu);
>  
> -	/* Move it here so it can run. */
> -	kthread_bind(p, get_cpu());
> -	put_cpu();
> -
>  	/* CPU is completely dead: tell everyone.  Too late to complain. */
>  	if (raw_notifier_call_chain(&cpu_chain, CPU_DEAD | mod,
>  				    hcpu) == NOTIFY_BAD)

So I cooked up a changelog and queued up the diff.  But I have an uneasy
feeling that things are getting a bit close to guesswork here.

We have a huge amount of change pending in the kthread/workqueue/freezer
area, partly because I decided not to merge most of the workqueue changes
into 2.6.21.

It'd be good if people could take some time to sit down and re-review the
code which we presently have.  I plan on sending it all off for 2.6.22 and
there might be some glitches but it seems to have a good track record so
far.

[PATCH] add-suspend-related-notifications-for-cpu-hotplug-cleanup

[Oleg Nesterov]

On 04/27, Andrew Morton wrote:
>
> We have a huge amount of change pending in the kthread/workqueue/freezer
> area, partly because I decided not to merge most of the workqueue changes
> into 2.6.21.
> 
> It'd be good if people could take some time to sit down and re-review the
> code which we presently have.  I plan on sending it all off for 2.6.22 and
> there might be some glitches but it seems to have a good track record so
> far.

Oops. I completely misread the ->cpus_allowed check in try_to_wake_up(), and
so workqueue_cpu_callback() needs a simple fix: CPU_UP_CANCELED should rebind
cwq->thread. Or, better, we should not bind the task until CPU_ONLINE.

I'll send the patch soon, but it conflicts with Rafael's

	add-suspend-related-notifications-for-cpu-hotplug.patch

, so I am sending this cleanup first to make them independent.

Actually, I believe almost all subsystems could be simplified as well, most
of them don't care about CPU_TASKS_FROZEN bit.

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- OLD/kernel/workqueue.c~1_FROZEN	2007-05-01 22:13:10.000000000 +0400
+++ OLD/kernel/workqueue.c	2007-05-01 22:18:15.000000000 +0400
@@ -747,6 +747,8 @@ static int __devinit workqueue_cpu_callb
 	struct cpu_workqueue_struct *cwq;
 	struct workqueue_struct *wq;
 
+	action &= ~CPU_TASKS_FROZEN;
+
 	switch (action) {
 	case CPU_LOCK_ACQUIRE:
 		mutex_lock(&workqueue_mutex);
@@ -757,7 +759,6 @@ static int __devinit workqueue_cpu_callb
 		return NOTIFY_OK;
 
 	case CPU_UP_PREPARE:
-	case CPU_UP_PREPARE_FROZEN:
 		cpu_set(cpu, cpu_populated_map);
 	}
 
@@ -766,23 +767,19 @@ static int __devinit workqueue_cpu_callb
 
 		switch (action) {
 		case CPU_UP_PREPARE:
-		case CPU_UP_PREPARE_FROZEN:
 			if (!create_workqueue_thread(cwq, cpu))
 				break;
 			printk(KERN_ERR "workqueue for %i failed\n", cpu);
 			return NOTIFY_BAD;
 
 		case CPU_ONLINE:
-		case CPU_ONLINE_FROZEN:
 			wake_up_process(cwq->thread);
 			break;
 
 		case CPU_UP_CANCELED:
-		case CPU_UP_CANCELED_FROZEN:
 			if (cwq->thread)
 				wake_up_process(cwq->thread);
 		case CPU_DEAD:
-		case CPU_DEAD_FROZEN:
 			cleanup_workqueue_thread(cwq, cpu);
 			break;
 		}

libata-core.c: unsafe cancel_delayed_work() usage?

[Oleg Nesterov]

On 04/27, Andrew Morton wrote:
>
> It'd be good if people could take some time to sit down and re-review the
> code which we presently have.  I plan on sending it all off for 2.6.22 and
> there might be some glitches but it seems to have a good track record so
> far.

Alan, the usage of cancel_delayed_work() in drivers/ata/libata-core.c looks
suspicious to me, both ->hotplug_task and ->port_task can re-arm themselves,
so cancel_delayed_work + flush_workqueue (or cancel_work_sync) is not enough.

Could you confirm/nack my understanding?

Oleg.

Re: libata-core.c: unsafe cancel_delayed_work() usage?

[Jeff Garzik]

Oleg Nesterov wrote:
> On 04/27, Andrew Morton wrote:
>> It'd be good if people could take some time to sit down and re-review the
>> code which we presently have.  I plan on sending it all off for 2.6.22 and
>> there might be some glitches but it seems to have a good track record so
>> far.
> 
> Alan, the usage of cancel_delayed_work() in drivers/ata/libata-core.c looks
> suspicious to me, both ->hotplug_task and ->port_task can re-arm themselves,
> so cancel_delayed_work + flush_workqueue (or cancel_work_sync) is not enough.
> 
> Could you confirm/nack my understanding?

For all rearming workqueues, one must use cancel-rearming-blahblah.

	Jeff

Re: libata-core.c: unsafe cancel_delayed_work() usage?

[Oleg Nesterov]

On 05/01, Jeff Garzik wrote:
>
> Oleg Nesterov wrote:
> >
> >Alan, the usage of cancel_delayed_work() in drivers/ata/libata-core.c looks
> >suspicious to me, both ->hotplug_task and ->port_task can re-arm 
> >themselves,
> >so cancel_delayed_work + flush_workqueue (or cancel_work_sync) is not 
> >enough.
> >
> >Could you confirm/nack my understanding?
> 
> For all rearming workqueues, one must use cancel-rearming-blahblah.

Well, yes and no. cancel_rearming_delayed_work() requires that dwork
re-arms itself unconditionally, otherwise it just hangs (actually, the
patch to fix this shortcoming is ready). However, ata_pio_task() for
example, re-arms only when ATA_BUSY.

So it is very possible that the code is correct, if the caller of
ata_port_flush_task() ensures that ATA_BUSY is not possible. Otherwise
we should change cancel_rearming_delayed_work() and use it.

Oleg.

kernel/relay.c: a strange usage of delayed_work

[Oleg Nesterov]

relay_switch_subbuf() does schedule_delayed_work(&buf->wake_readers, 1),
wakeup_readers() only does wake_up_interruptible() and nothing more.

Why can't we use a plain timer for this?

In any case, this "wake_up ->read_wait after a minimal possible delay"
looks somewhat strange to me, could you explain? just curious.

Oleg.

Re: kernel/relay.c: a strange usage of delayed_work

[Tom Zanussi]

On Fri, 2007-05-04 at 01:38 +0400, Oleg Nesterov wrote:
> relay_switch_subbuf() does schedule_delayed_work(&buf->wake_readers, 1),
> wakeup_readers() only does wake_up_interruptible() and nothing more.
> 
> Why can't we use a plain timer for this?
> 
> In any case, this "wake_up ->read_wait after a minimal possible delay"
> looks somewhat strange to me, could you explain? just curious.
> 

The reason it's done that way is that if the event that causes the
relay_switch_subbuf() happens to be an event logged from schedule(), and
we directly call wake_up_interruptible() at that point, we lock up the
machine because it ends up back in schedule().  Deferring it avoids the
problem.

I don't see any problem with using a plain timer instead - I'll work up
a patch to make that change.

Tom