Fix deadlock in __create_workqueue

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.

Yes, the logic in worker_thread() is a bit dorky, but I don't believe that there is a race in there.

Quoted 2 lines · Andrew Morton

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?

kernel/workqueue.c renamed from kernel/workqueue.c~worker_thread-race-fix +4 -3

@@ -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;
 }

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

kernel/workqueue.c renamed from kernel/workqueue.c~a +1 -0

@@ -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:

Quoted 12 lines · Andrew Morton

Can we not simply do:

diff -puN kernel/workqueue.c~a kernel/workqueue.c

kernel/workqueue.c renamed from kernel/workqueue.c~a +1 -0

@@ -334,6 +334,7 @@ struct workqueue_struct *__create_workqu
 				destroy = 1;
 		}
 	}
+	unlock_cpu_hotplug();
 

kernel/workqueue.c struct workqueue_struct *__create_workqu · line 347

Quoted 9 lines

kernel/workqueue.c renamed from kernel/workqueue.c~a +0 -1

 	/*
 	 * 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.

Quoted 2 lines · Andrew Morton

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?

Quoted 12 lines · Andrew Morton

Can we not simply do:

diff -puN kernel/workqueue.c~a kernel/workqueue.c

kernel/workqueue.c renamed from kernel/workqueue.c~a +1 -0

@@ -334,6 +334,7 @@ struct workqueue_struct *__create_workqu
 				destroy = 1;
 		}
 	}
+	unlock_cpu_hotplug();
 

kernel/workqueue.c struct workqueue_struct *__create_workqu · line 347

Quoted 9 lines

kernel/workqueue.c renamed from kernel/workqueue.c~a +0 -1

 	/*
 	 * 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.