[PATCH] Fix deadlock in __create_workqueue

[PATCH] Fix deadlock in __create_workqueue

[Srivatsa Vaddagiri]

Noticed a possible deadlock in __create_workqueue when CONFIG_HOTPLUG_CPU is
set.  This can happen when create_workqueue_thread fails to create a worker
thread. In that case, we call destroy_workqueue with cpu hotplug lock held.
destroy_workqueue however also attempts to take the same lock.

Patch below address this deadlock as well as a kthread_stop race. 
Its tested against 2.6.6-rc2-mm2 on a 4-way Intel SMP box.

---

 linux-2.6.6-rc2-mm2-root/kernel/workqueue.c |   58 +++++++++++++++++-----------
 1 files changed, 37 insertions(+), 21 deletions(-)

diff -puN kernel/workqueue.c~__create_workqueue_fix kernel/workqueue.c
--- linux-2.6.6-rc2-mm2/kernel/workqueue.c~__create_workqueue_fix	2004-04-29 18:53:03.722704312 +0530
+++ linux-2.6.6-rc2-mm2-root/kernel/workqueue.c	2004-04-29 18:53:09.623807208 +0530
@@ -201,8 +201,9 @@ static int worker_thread(void *__cwq)
 	siginitset(&sa.sa.sa_mask, sigmask(SIGCHLD));
 	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
 
+	set_task_state(current, TASK_INTERRUPTIBLE);
+
 	while (!kthread_should_stop()) {
-		set_task_state(current, TASK_INTERRUPTIBLE);
 
 		add_wait_queue(&cwq->more_work, &wait);
 		if (list_empty(&cwq->worklist))
@@ -213,7 +214,11 @@ static int worker_thread(void *__cwq)
 
 		if (!list_empty(&cwq->worklist))
 			run_workqueue(cwq);
+
+		set_task_state(current, TASK_INTERRUPTIBLE);
 	}
+
+	__set_current_state(TASK_RUNNING);
 	return 0;
 }
 
@@ -297,6 +302,21 @@ static struct task_struct *create_workqu
 	return p;
 }
 
+static void cleanup_workqueue_thread(struct workqueue_struct *wq, int cpu)
+{
+	struct cpu_workqueue_struct *cwq;
+	unsigned long flags;
+	struct task_struct *p;
+
+	cwq = wq->cpu_wq + cpu;
+	spin_lock_irqsave(&cwq->lock, flags);
+	p = cwq->thread;
+	cwq->thread = NULL;
+	spin_unlock_irqrestore(&cwq->lock, flags);
+	if (p)
+		kthread_stop(p);
+}
+
 struct workqueue_struct *__create_workqueue(const char *name,
 					    int singlethread)
 {
@@ -322,16 +342,27 @@ struct workqueue_struct *__create_workqu
 		else
 			wake_up_process(p);
 	} else {
-		spin_lock(&workqueue_lock);
-		list_add(&wq->list, &workqueues);
-		spin_unlock_irq(&workqueue_lock);
 		for_each_online_cpu(cpu) {
 			p = create_workqueue_thread(wq, cpu);
 			if (p) {
 				kthread_bind(p, cpu);
 				wake_up_process(p);
-			} else
+			} else {
+				int i;
+
+				for (i=cpu-1; i>=0; --i)
+					if (cpu_online(i))
+						cleanup_workqueue_thread(wq, i);
+
 				destroy = 1;
+				break;
+			}
+		}
+
+		if (!destroy) {
+			spin_lock(&workqueue_lock);
+			list_add(&wq->list, &workqueues);
+			spin_unlock_irq(&workqueue_lock);
 		}
 	}
 
@@ -339,28 +370,13 @@ struct workqueue_struct *__create_workqu
 	 * Was there any error during startup? If yes then clean up:
 	 */
 	if (destroy) {
-		destroy_workqueue(wq);
+		kfree(wq);
 		wq = NULL;
 	}
 	unlock_cpu_hotplug();
 	return wq;
 }
 
-static void cleanup_workqueue_thread(struct workqueue_struct *wq, int cpu)
-{
-	struct cpu_workqueue_struct *cwq;
-	unsigned long flags;
-	struct task_struct *p;
-
-	cwq = wq->cpu_wq + cpu;
-	spin_lock_irqsave(&cwq->lock, flags);
-	p = cwq->thread;
-	cwq->thread = NULL;
-	spin_unlock_irqrestore(&cwq->lock, flags);
-	if (p)
-		kthread_stop(p);
-}
-
 void destroy_workqueue(struct workqueue_struct *wq)
 {
 	int cpu;

_


-- 


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

Re: [PATCH] Fix deadlock in __create_workqueue

[Andrew Morton]

Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:
>
> Noticed a possible deadlock in __create_workqueue when CONFIG_HOTPLUG_CPU is
> set.  This can happen when create_workqueue_thread fails to create a worker
> thread. In that case, we call destroy_workqueue with cpu hotplug lock held.
> destroy_workqueue however also attempts to take the same lock.
> 
> Patch below address this deadlock as well as a kthread_stop race. 

Fixing a kthread_stop() race is a quite different thing from fixing a
create-workqueue() error-path deadlock and hence should be a separate
patch.

And the description of that separate patch should explain the race which
it is fixing!  Yes, the logic in worker_thread() is a bit dorky, but I
don't believe that there is a race in there.

I dropped that part of your patch.  Please resend, with justification, if
you disagree.

Thanks.

Re: [PATCH] Fix deadlock in __create_workqueue

[Andrew Morton]

Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:
>
> Noticed a possible deadlock in __create_workqueue when CONFIG_HOTPLUG_CPU is
>  set.  This can happen when create_workqueue_thread fails to create a worker
>  thread. In that case, we call destroy_workqueue with cpu hotplug lock held.
>  destroy_workqueue however also attempts to take the same lock.

Can we not simply do:


diff -puN kernel/workqueue.c~a kernel/workqueue.c
--- 25/kernel/workqueue.c~a	2004-04-30 19:26:32.003303600 -0700
+++ 25-akpm/kernel/workqueue.c	2004-04-30 19:26:44.492404968 -0700
@@ -334,6 +334,7 @@ struct workqueue_struct *__create_workqu
 				destroy = 1;
 		}
 	}
+	unlock_cpu_hotplug();
 
 	/*
 	 * Was there any error during startup? If yes then clean up:
@@ -342,7 +343,6 @@ struct workqueue_struct *__create_workqu
 		destroy_workqueue(wq);
 		wq = NULL;
 	}
-	unlock_cpu_hotplug();
 	return wq;
 }
 

_

Re: [PATCH] Fix deadlock in __create_workqueue

[Srivatsa Vaddagiri]

On Fri, Apr 30, 2004 at 07:27:12PM -0700, Andrew Morton wrote:
> Can we not simply do:
> 
> 
> diff -puN kernel/workqueue.c~a kernel/workqueue.c
> --- 25/kernel/workqueue.c~a	2004-04-30 19:26:32.003303600 -0700
> +++ 25-akpm/kernel/workqueue.c	2004-04-30 19:26:44.492404968 -0700
> @@ -334,6 +334,7 @@ struct workqueue_struct *__create_workqu
>  				destroy = 1;
>  		}
>  	}
> +	unlock_cpu_hotplug();
>  
>  	/*
>  	 * Was there any error during startup? If yes then clean up:
> @@ -342,7 +343,6 @@ struct workqueue_struct *__create_workqu
>  		destroy_workqueue(wq);
>  		wq = NULL;
>  	}
> -	unlock_cpu_hotplug();
>  	return wq;
>  }

I didn't do this because I introduced a break at the first instance
when create_workqueue_thread failed. Breaking out of the loop
like that appeared to be more efficient rather than going back and
trying to create threads for rest of the online cpus, because most
likely thread creation will fail for other cpus also and anyway
the workqueue will be destroyed down the line.

If the break is introduced, I dont think we can rely on destroy_workqueue
because some of the per-cpu structures (spinlocks for one) will not be 
initialized for all online cpus.


-- 


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

Re: [PATCH] Fix deadlock in __create_workqueue

[Srivatsa Vaddagiri]

On Fri, Apr 30, 2004 at 07:19:01PM -0700, Andrew Morton wrote:
> Yes, the logic in worker_thread() is a bit dorky, but I
> don't believe that there is a race in there.

worker_thread examines kthread_should_stop() while its state
is TASK_RUNNING, after which it sets its state to TASK_INTERRUPTIBLE.
If kthread_stop were to come after kthread_should_stop and before
worker_thread has set its state to TASK_INTERRUPTIBLE (which is possible
because of a CPU going dead), wouldn't kthread_stop block forever? 
Note that in case of CPU going dead, it is possible that a worker
thread bound to the dead cpu continues executing on a different cpu 
before it is killed in CPU_DEAD processing.

Am I missing something here?

-- 


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

Re: [PATCH] Fix deadlock in __create_workqueue

[Andrew Morton]

Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:
>
> On Fri, Apr 30, 2004 at 07:27:12PM -0700, Andrew Morton wrote:
> > Can we not simply do:
> > 
> > 
> > diff -puN kernel/workqueue.c~a kernel/workqueue.c
> > --- 25/kernel/workqueue.c~a	2004-04-30 19:26:32.003303600 -0700
> > +++ 25-akpm/kernel/workqueue.c	2004-04-30 19:26:44.492404968 -0700
> > @@ -334,6 +334,7 @@ struct workqueue_struct *__create_workqu
> >  				destroy = 1;
> >  		}
> >  	}
> > +	unlock_cpu_hotplug();
> >  
> >  	/*
> >  	 * Was there any error during startup? If yes then clean up:
> > @@ -342,7 +343,6 @@ struct workqueue_struct *__create_workqu
> >  		destroy_workqueue(wq);
> >  		wq = NULL;
> >  	}
> > -	unlock_cpu_hotplug();
> >  	return wq;
> >  }
> 
> I didn't do this because I introduced a break at the first instance
> when create_workqueue_thread failed. Breaking out of the loop
> like that appeared to be more efficient rather than going back and
> trying to create threads for rest of the online cpus, because most
> likely thread creation will fail for other cpus also and anyway
> the workqueue will be destroyed down the line.

Well that create_workqueue_thread() will basically never fail - it's not a
path we need to be optimising.

And from inspection, cleanup_workqueue_thread() will handle the
non-existent thread quite happily.

Re: [PATCH] Fix deadlock in __create_workqueue

[Andrew Morton]

Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:
>
> On Fri, Apr 30, 2004 at 07:19:01PM -0700, Andrew Morton wrote:
> > Yes, the logic in worker_thread() is a bit dorky, but I
> > don't believe that there is a race in there.
> 
> worker_thread examines kthread_should_stop() while its state
> is TASK_RUNNING, after which it sets its state to TASK_INTERRUPTIBLE.
> If kthread_stop were to come after kthread_should_stop and before
> worker_thread has set its state to TASK_INTERRUPTIBLE (which is possible
> because of a CPU going dead), wouldn't kthread_stop block forever? 
> Note that in case of CPU going dead, it is possible that a worker
> thread bound to the dead cpu continues executing on a different cpu 
> before it is killed in CPU_DEAD processing.

yup, sorry, I misread the code.  Like this?


--- 25/kernel/workqueue.c~worker_thread-race-fix	Mon May  3 13:02:25 2004
+++ 25-akpm/kernel/workqueue.c	Mon May  3 13:07:34 2004
@@ -201,19 +201,20 @@ static int worker_thread(void *__cwq)
 	siginitset(&sa.sa.sa_mask, sigmask(SIGCHLD));
 	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
 
+	set_current_state(TASK_INTERRUPTIBLE);
 	while (!kthread_should_stop()) {
-		set_task_state(current, TASK_INTERRUPTIBLE);
-
 		add_wait_queue(&cwq->more_work, &wait);
 		if (list_empty(&cwq->worklist))
 			schedule();
 		else
-			set_task_state(current, TASK_RUNNING);
+			__set_current_state(TASK_RUNNING);
 		remove_wait_queue(&cwq->more_work, &wait);
 
 		if (!list_empty(&cwq->worklist))
 			run_workqueue(cwq);
+		set_current_state(TASK_INTERRUPTIBLE);
 	}
+	__set_current_state(TASK_RUNNING);
 	return 0;
 }
 

_

Re: [PATCH] Fix deadlock in __create_workqueue

[Srivatsa Vaddagiri]

On Mon, May 03, 2004 at 12:25:20PM -0700, Andrew Morton wrote:
> Well that create_workqueue_thread() will basically never fail - it's not a
> path we need to be optimising.

Even if thread creation normally never fails, we still check for its
return code and have some error recovery code! In that case, 
I dont understand the point behind continuing the loop once thread
destruction fails for some CPU. Lets say on a 128 CPU machine, if
thread creation fails for the 1st CPU (because of say ENOMEM?), then
why continue trying to create threads for the rest of 126 CPUs and
_then_ destroy? Why not just break at the first occurence of failure 
and cleanup then and there?


 


-- 


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

Re: [PATCH] Fix deadlock in __create_workqueue

[Srivatsa Vaddagiri]

On Mon, May 03, 2004 at 01:08:29PM -0700, Andrew Morton wrote:
> yup, sorry, I misread the code.  Like this?

Yes, looks fine to me!

> 
> --- 25/kernel/workqueue.c~worker_thread-race-fix	Mon May  3 13:02:25 2004
> +++ 25-akpm/kernel/workqueue.c	Mon May  3 13:07:34 2004
> @@ -201,19 +201,20 @@ static int worker_thread(void *__cwq)
>  	siginitset(&sa.sa.sa_mask, sigmask(SIGCHLD));
>  	do_sigaction(SIGCHLD, &sa, (struct k_sigaction *)0);
>  
> +	set_current_state(TASK_INTERRUPTIBLE);
>  	while (!kthread_should_stop()) {
> -		set_task_state(current, TASK_INTERRUPTIBLE);
> -
>  		add_wait_queue(&cwq->more_work, &wait);
>  		if (list_empty(&cwq->worklist))
>  			schedule();
>  		else
> -			set_task_state(current, TASK_RUNNING);
> +			__set_current_state(TASK_RUNNING);
>  		remove_wait_queue(&cwq->more_work, &wait);
>  
>  		if (!list_empty(&cwq->worklist))
>  			run_workqueue(cwq);
> +		set_current_state(TASK_INTERRUPTIBLE);
>  	}
> +	__set_current_state(TASK_RUNNING);
>  	return 0;
>  }

-- 


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

Re: [PATCH] Fix deadlock in __create_workqueue

[Andrew Morton]

Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:
>
> On Mon, May 03, 2004 at 12:25:20PM -0700, Andrew Morton wrote:
> > Well that create_workqueue_thread() will basically never fail - it's not a
> > path we need to be optimising.
> 
> Even if thread creation normally never fails, we still check for its
> return code and have some error recovery code! In that case, 
> I dont understand the point behind continuing the loop once thread
> destruction fails for some CPU. Lets say on a 128 CPU machine, if
> thread creation fails for the 1st CPU (because of say ENOMEM?), then
> why continue trying to create threads for the rest of 126 CPUs and
> _then_ destroy? Why not just break at the first occurence of failure 
> and cleanup then and there?
> 

Because there's less code to get wrong.

In this situation, correctness, clarity and code size are the only things
we care about.

Re: [PATCH] Fix deadlock in __create_workqueue

[Rusty Russell]

On Mon, 2004-05-03 at 22:23, Srivatsa Vaddagiri wrote:
> On Fri, Apr 30, 2004 at 07:27:12PM -0700, Andrew Morton wrote:
> > Can we not simply do:
> > 
> > 
> > diff -puN kernel/workqueue.c~a kernel/workqueue.c
> > --- 25/kernel/workqueue.c~a	2004-04-30 19:26:32.003303600 -0700
> > +++ 25-akpm/kernel/workqueue.c	2004-04-30 19:26:44.492404968 -0700
> > @@ -334,6 +334,7 @@ struct workqueue_struct *__create_workqu
> >  				destroy = 1;
> >  		}
> >  	}
> > +	unlock_cpu_hotplug();
> >  
> >  	/*
> >  	 * Was there any error during startup? If yes then clean up:
> > @@ -342,7 +343,6 @@ struct workqueue_struct *__create_workqu
> >  		destroy_workqueue(wq);
> >  		wq = NULL;
> >  	}
> > -	unlock_cpu_hotplug();
> >  	return wq;
> >  }
> 
> I didn't do this because I introduced a break at the first instance
> when create_workqueue_thread failed. Breaking out of the loop
> like that appeared to be more efficient rather than going back and
> trying to create threads for rest of the online cpus, because most
> likely thread creation will fail for other cpus also and anyway
> the workqueue will be destroyed down the line.

The logic was not the way I would have done it, but it *is* neater.  I
prefer the akpm fix I think.

Rusty.
-- 
Anyone who quotes me in their signature is an idiot -- Rusty Russell