On 07/22, Andrew Morton wrote:
>
> From: Akinobu Mita <[email protected]>
>
> Add proper error unwinding in error path in CPU_UP_PREPARE notifier.
Could you clarify?
> --- a/kernel/workqueue.c~workqueue-proper-error-unwinding-in-cpu-hotplug-error-path
> +++ a/kernel/workqueue.c
> @@ -928,6 +928,15 @@ static int __devinit workqueue_cpu_callb
> break;
> printk(KERN_ERR "workqueue [%s] for %i failed\n",
> wq->name, cpu);
> +
> + list_for_each_entry_continue_reverse(wq, &workqueues,
> + list) {
> + cwq = per_cpu_ptr(wq->cpu_wq, cpu);
> + start_workqueue_thread(cwq, -1);
> + cleanup_workqueue_thread(cwq);
> + }
> + cpu_clear(cpu, cpu_populated_map);
> +
> return NOTIFY_BAD;
If CPU_UP_PREPARE fails, _cpu_up() sends CPU_UP_CANCELED, and afaics
workqueue_cpu_callback() correctly cleanups cwq->thread's.
Oleg.
On Tue, Jul 22, 2008 at 12:45:26PM +0400, Oleg Nesterov wrote:
> On 07/22, Andrew Morton wrote:
> >
> > From: Akinobu Mita <[email protected]>
> >
> > Add proper error unwinding in error path in CPU_UP_PREPARE notifier.
>
> Could you clarify?
Sure.
> > --- a/kernel/workqueue.c~workqueue-proper-error-unwinding-in-cpu-hotplug-error-path
> > +++ a/kernel/workqueue.c
> > @@ -928,6 +928,15 @@ static int __devinit workqueue_cpu_callb
> > break;
> > printk(KERN_ERR "workqueue [%s] for %i failed\n",
> > wq->name, cpu);
> > +
> > + list_for_each_entry_continue_reverse(wq, &workqueues,
> > + list) {
> > + cwq = per_cpu_ptr(wq->cpu_wq, cpu);
> > + start_workqueue_thread(cwq, -1);
> > + cleanup_workqueue_thread(cwq);
> > + }
> > + cpu_clear(cpu, cpu_populated_map);
> > +
> > return NOTIFY_BAD;
>
> If CPU_UP_PREPARE fails, _cpu_up() sends CPU_UP_CANCELED, and afaics
> workqueue_cpu_callback() correctly cleanups cwq->thread's.
_cpu_up() does not send CPU_UP_CANCELED to the callback which has
returned NOTIFY_BAD.
The behavior was changed by this commit:
commit a0d8cdb652d35af9319a9e0fb7134de2a276c636
Author: Akinobu Mita <[email protected]>
Date: Thu Oct 18 03:05:12 2007 -0700
cpu hotplug: cpu: deliver CPU_UP_CANCELED only to NOTIFY_OKed callbacks with CPU_UP_PREPARE
The functions in a CPU notifier chain is called with CPU_UP_PREPARE event
before making the CPU online. If one of the callback returns NOTIFY_BAD, it
stops to deliver CPU_UP_PREPARE event, and CPU online operation is canceled.
Then CPU_UP_CANCELED event is delivered to the functions in a CPU notifier
chain again.
This CPU_UP_CANCELED event is delivered to the functions which have been
called with CPU_UP_PREPARE, not delivered to the functions which haven't been
called with CPU_UP_PREPARE.
The problem that makes existing cpu hotplug error handlings complex is that
the CPU_UP_CANCELED event is delivered to the function that has returned
NOTIFY_BAD, too.
Usually we don't expect to call destructor function against the object that
has failed to initialize. It is like:
err = register_something();
if (err) {
unregister_something();
return err;
}
So it is natural to deliver CPU_UP_CANCELED event only to the functions that
have returned NOTIFY_OK with CPU_UP_PREPARE event and not to call the function
that have returned NOTIFY_BAD. This is what this patch is doing.
Otherwise, every cpu hotplug notifiler has to track whether notifiler event is
failed or not for each cpu. (drivers/base/topology.c is doing this with
topology_dev_map)
Similary this patch makes same thing with CPU_DOWN_PREPARE and CPU_DOWN_FAILED
evnets.
Acked-by: Rusty Russell <[email protected]>
Signed-off-by: Akinobu Mita <[email protected]>
Cc: Gautham R Shenoy <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
Signed-off-by: Linus Torvalds <[email protected]>
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 38033db..a21f71a 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -150,6 +150,7 @@ static int _cpu_down(unsigned int cpu, int tasks_frozen)
err = __raw_notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE | mod,
hcpu, -1, &nr_calls);
if (err == NOTIFY_BAD) {
+ nr_calls--;
__raw_notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED | mod,
hcpu, nr_calls, NULL);
printk("%s: attempt to take down CPU %u failed\n",
@@ -233,6 +234,7 @@ static int __cpuinit _cpu_up(unsigned int cpu, int tasks_frozen)
ret = __raw_notifier_call_chain(&cpu_chain, CPU_UP_PREPARE | mod, hcpu,
-1, &nr_calls);
if (ret == NOTIFY_BAD) {
+ nr_calls--;
printk("%s: attempt to bring up CPU %u failed\n",
__FUNCTION__, cpu);
ret = -EINVAL;
On 07/22, Akinobu Mita wrote:
>
> On Tue, Jul 22, 2008 at 12:45:26PM +0400, Oleg Nesterov wrote:
> > >
> > > From: Akinobu Mita <[email protected]>
> > >
> > > --- a/kernel/workqueue.c~workqueue-proper-error-unwinding-in-cpu-hotplug-error-path
> > > +++ a/kernel/workqueue.c
> > > @@ -928,6 +928,15 @@ static int __devinit workqueue_cpu_callb
> > > break;
> > > printk(KERN_ERR "workqueue [%s] for %i failed\n",
> > > wq->name, cpu);
> > > +
> > > + list_for_each_entry_continue_reverse(wq, &workqueues,
> > > + list) {
> > > + cwq = per_cpu_ptr(wq->cpu_wq, cpu);
> > > + start_workqueue_thread(cwq, -1);
> > > + cleanup_workqueue_thread(cwq);
> > > + }
> > > + cpu_clear(cpu, cpu_populated_map);
> > > +
> > > return NOTIFY_BAD;
> >
> > If CPU_UP_PREPARE fails, _cpu_up() sends CPU_UP_CANCELED, and afaics
> > workqueue_cpu_callback() correctly cleanups cwq->thread's.
>
> _cpu_up() does not send CPU_UP_CANCELED to the callback which has
> returned NOTIFY_BAD.
>
> The behavior was changed by this commit:
>
> commit a0d8cdb652d35af9319a9e0fb7134de2a276c636
> Author: Akinobu Mita <[email protected]>
> Date: Thu Oct 18 03:05:12 2007 -0700
>
> cpu hotplug: cpu: deliver CPU_UP_CANCELED only to NOTIFY_OKed callbacks with CPU_UP_PREPARE
Thanks Akinobu!
Can't we simplify the fix? I don't like the fact that the CPU_UP_CANCELED
logic is duplicated.
What do you think about the patch below?
Oleg.
--- 26-rc2/kernel/workqueue.c~WQ_CPU_UP_PREPARE 2008-07-12 19:40:57.000000000 +0400
+++ 26-rc2/kernel/workqueue.c 2008-07-22 13:15:16.000000000 +0400
@@ -911,6 +911,7 @@ static int __devinit workqueue_cpu_callb
unsigned int cpu = (unsigned long)hcpu;
struct cpu_workqueue_struct *cwq;
struct workqueue_struct *wq;
+ int ret = NOTIFY_OK;
action &= ~CPU_TASKS_FROZEN;
@@ -919,6 +920,7 @@ static int __devinit workqueue_cpu_callb
cpu_set(cpu, cpu_populated_map);
}
+cancel:
list_for_each_entry(wq, &workqueues, list) {
cwq = per_cpu_ptr(wq->cpu_wq, cpu);
@@ -928,7 +930,9 @@ static int __devinit workqueue_cpu_callb
break;
printk(KERN_ERR "workqueue [%s] for %i failed\n",
wq->name, cpu);
- return NOTIFY_BAD;
+ action = CPU_UP_CANCELED;
+ ret = NOTIFY_BAD;
+ goto cancel;
case CPU_ONLINE:
start_workqueue_thread(cwq, cpu);
@@ -948,7 +952,7 @@ static int __devinit workqueue_cpu_callb
cpu_clear(cpu, cpu_populated_map);
}
- return NOTIFY_OK;
+ return ret;
}
void __init init_workqueues(void)
On Tue, Jul 22, 2008 at 01:16:36PM +0400, Oleg Nesterov wrote:
> Thanks Akinobu!
You are welcome!
> Can't we simplify the fix? I don't like the fact that the CPU_UP_CANCELED
> logic is duplicated.
>
> What do you think about the patch below?
Yes, it is no duplication. But the goto usage in this patch looks bit
tricky to me. Maybe it is better to factor out the list_for_each() block.
>
> Oleg.
>
> --- 26-rc2/kernel/workqueue.c~WQ_CPU_UP_PREPARE 2008-07-12 19:40:57.000000000 +0400
> +++ 26-rc2/kernel/workqueue.c 2008-07-22 13:15:16.000000000 +0400
> @@ -911,6 +911,7 @@ static int __devinit workqueue_cpu_callb
> unsigned int cpu = (unsigned long)hcpu;
> struct cpu_workqueue_struct *cwq;
> struct workqueue_struct *wq;
> + int ret = NOTIFY_OK;
>
> action &= ~CPU_TASKS_FROZEN;
>
> @@ -919,6 +920,7 @@ static int __devinit workqueue_cpu_callb
> cpu_set(cpu, cpu_populated_map);
> }
>
> +cancel:
> list_for_each_entry(wq, &workqueues, list) {
> cwq = per_cpu_ptr(wq->cpu_wq, cpu);
>
> @@ -928,7 +930,9 @@ static int __devinit workqueue_cpu_callb
> break;
> printk(KERN_ERR "workqueue [%s] for %i failed\n",
> wq->name, cpu);
> - return NOTIFY_BAD;
> + action = CPU_UP_CANCELED;
> + ret = NOTIFY_BAD;
> + goto cancel;
>
> case CPU_ONLINE:
> start_workqueue_thread(cwq, cpu);
> @@ -948,7 +952,7 @@ static int __devinit workqueue_cpu_callb
> cpu_clear(cpu, cpu_populated_map);
> }
>
> - return NOTIFY_OK;
> + return ret;
> }
>
> void __init init_workqueues(void)
>
On 07/22, Akinobu Mita wrote:
>
> On Tue, Jul 22, 2008 at 01:16:36PM +0400, Oleg Nesterov wrote:
>
> > Can't we simplify the fix? I don't like the fact that the CPU_UP_CANCELED
> > logic is duplicated.
> >
> > What do you think about the patch below?
>
> Yes, it is no duplication. But the goto usage in this patch looks bit
> tricky to me.
Well, the kernel is full of "goto again" / "goto retry". Not that I
claim this patch improves the readability ;)
But the duplication is really bad, imho.
> Maybe it is better to factor out the list_for_each() block.
Not sure I understand... do you mean add another function which
starts with list_for_each_entry ? This is not necessary, instead
of goto we can just do
workqueue_cpu_callback(CPU_UP_CANCELED);
return NOTIFY_BAD;
, in that case the patch will be one-liner.
Akinobu, Andrew, I'd suggest to drop
workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch
, I'll send the new patch. If nothing else, it is simpler.
Oleg.
(replaces workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch)
The bug was pointed out by Akinobu Mita <[email protected]>, and this
patch is based on his original patch.
workqueue_cpu_callback(CPU_UP_PREPARE) expects that if it returns NOTIFY_BAD,
_cpu_up() will send CPU_UP_CANCELED then.
However, this is not true since
"cpu hotplug: cpu: deliver CPU_UP_CANCELED only to NOTIFY_OKed callbacks with CPU_UP_PREPARE"
commit: a0d8cdb652d35af9319a9e0fb7134de2a276c636
The callback which has returned NOTIFY_BAD will not receive CPU_UP_CANCELED.
Change the code to fulfil the CPU_UP_CANCELED logic if CPU_UP_PREPARE fails.
Signed-off-by: Oleg Nesterov <[email protected]>
--- 26-rc2/kernel/workqueue.c~WQ_CPU_UP_PREPARE 2008-07-12 19:40:57.000000000 +0400
+++ 26-rc2/kernel/workqueue.c 2008-07-22 13:43:47.000000000 +0400
@@ -911,6 +911,7 @@ static int __devinit workqueue_cpu_callb
unsigned int cpu = (unsigned long)hcpu;
struct cpu_workqueue_struct *cwq;
struct workqueue_struct *wq;
+ int ret = NOTIFY_OK;
action &= ~CPU_TASKS_FROZEN;
@@ -918,7 +919,7 @@ static int __devinit workqueue_cpu_callb
case CPU_UP_PREPARE:
cpu_set(cpu, cpu_populated_map);
}
-
+undo:
list_for_each_entry(wq, &workqueues, list) {
cwq = per_cpu_ptr(wq->cpu_wq, cpu);
@@ -928,7 +929,9 @@ static int __devinit workqueue_cpu_callb
break;
printk(KERN_ERR "workqueue [%s] for %i failed\n",
wq->name, cpu);
- return NOTIFY_BAD;
+ action = CPU_UP_CANCELED;
+ ret = NOTIFY_BAD;
+ goto undo;
case CPU_ONLINE:
start_workqueue_thread(cwq, cpu);
@@ -948,7 +951,7 @@ static int __devinit workqueue_cpu_callb
cpu_clear(cpu, cpu_populated_map);
}
- return NOTIFY_OK;
+ return ret;
}
void __init init_workqueues(void)