Re: + workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch added to -mm tree

Re: + workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch added to -mm tree

[Oleg Nesterov]

On 07/22, Andrew Morton wrote:
> 
> From: Akinobu Mita <akinobu.mita@gmail.com>
> 
> 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.

Re: + workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch added to -mm tree

[Akinobu Mita]

On Tue, Jul 22, 2008 at 12:45:26PM +0400, Oleg Nesterov wrote:
> On 07/22, Andrew Morton wrote:
> > 
> > From: Akinobu Mita <akinobu.mita@gmail.com>
> > 
> > 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 <akinobu.mita@gmail.com>
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 <rusty@rustcorp.com.au>
    Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
    Cc: Gautham R Shenoy <ego@in.ibm.com>
    Cc: Oleg Nesterov <oleg@tv-sign.ru>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

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;

Re: + workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch added to -mm tree

[Oleg Nesterov]

On 07/22, Akinobu Mita wrote:
>
> On Tue, Jul 22, 2008 at 12:45:26PM +0400, Oleg Nesterov wrote:
> > > 
> > > From: Akinobu Mita <akinobu.mita@gmail.com>
> > > 
> > > --- 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 <akinobu.mita@gmail.com>
> 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)

Re: + workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch added to -mm tree

[Akinobu Mita]

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)
> 

Re: + workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch added to -mm tree

[Oleg Nesterov]

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.

[PATCH] workqueues: do CPU_UP_CANCELED if CPU_UP_PREPARE fails

[Oleg Nesterov]

(replaces workqueue-proper-error-unwinding-in-cpu-hotplug-error-path.patch)

The bug was pointed out by Akinobu Mita <akinobu.mita@gmail.com>, 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 <oleg@tv-sign.ru>

--- 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)