workqueue: destroy_worker() should destroy idle workers only

We used to have the CPU online failure path where a worker is created
and then destroyed without being started. A worker was created for
the CPU coming online and if the online operation failed the created worker
was shut down without being started.  But this behavior was changed.
The first worker is created and started at the same time for the CPU coming
online.

It means that we had already ensured in the code that destroy_worker()
destroys only idle workers and we don't want to allow it to destroy
any non-idle worker in the future. Otherwise, it may be buggy and it
may be extremely hard to check. We should force destroy_worker() to
destroy only idle workers explicitly.

Since destroy_worker() destroys only idle workers, this patch does not
change any functionality. We just need to update the comments and the
sanity check code.

In the sanity check code, we will refuse to destroy the worker
if !(worker->flags & WORKER_IDLE).

If the worker entered idle which means it is already started,
so we remove the check of "worker->flags & WORKER_STARTED",
after this removal, WORKER_STARTED is totally unneeded,
so we remove WORKER_STARTED too.

In the comments for create_worker(), "Create a new worker which is bound..."
is changed to "... which is attached..." due to we change the name of this
behavior to attaching.

tj: Minor description / comment updates.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Tejun Heo <tj@kernel.org>

1 files changed, 8 insertions, 11 deletions

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 910d963f6b76..189d79be8091 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -73,7 +73,6 @@ enum {
         POOL_FREEZING           = 1 << 3,       /* freeze in progress */
 
         /* worker flags */
-        WORKER_STARTED          = 1 << 0,       /* started */
         WORKER_DIE              = 1 << 1,       /* die die die */
         WORKER_IDLE             = 1 << 2,       /* is idle */
         WORKER_PREP             = 1 << 3,       /* preparing to run works */
@@ -1692,9 +1691,8 @@ static struct worker *alloc_worker(void)
  * create_worker - create a new workqueue worker
  * @pool: pool the new worker will belong to
  *
- * Create a new worker which is bound to @pool.  The returned worker
- * can be started by calling start_worker() or destroyed using
- * destroy_worker().
+ * Create a new worker which is attached to @pool.  The new worker must be
+ * started by start_worker().
  *
  * CONTEXT:
  * Might sleep.  Does GFP_KERNEL allocations.
@@ -1778,7 +1776,6 @@ fail:
  */
 static void start_worker(struct worker *worker)
 {
-        worker->flags |= WORKER_STARTED;
         worker->pool->nr_workers++;
         worker_enter_idle(worker);
         wake_up_process(worker->task);
@@ -1814,7 +1811,8 @@ static int create_and_start_worker(struct worker_pool *pool)
  * destroy_worker - destroy a workqueue worker
  * @worker: worker to be destroyed
  *
- * Destroy @worker and adjust @pool stats accordingly.
+ * Destroy @worker and adjust @pool stats accordingly.  The worker should
+ * be idle.
  *
  * CONTEXT:
  * spin_lock_irq(pool->lock) which is released and regrabbed.
@@ -1828,13 +1826,12 @@ static void destroy_worker(struct worker *worker)
 
         /* sanity check frenzy */
         if (WARN_ON(worker->current_work) ||
-            WARN_ON(!list_empty(&worker->scheduled)))
+            WARN_ON(!list_empty(&worker->scheduled)) ||
+            WARN_ON(!(worker->flags & WORKER_IDLE)))
                 return;
 
-        if (worker->flags & WORKER_STARTED)
-                pool->nr_workers--;
-        if (worker->flags & WORKER_IDLE)
-                pool->nr_idle--;
+        pool->nr_workers--;
+        pool->nr_idle--;
 
         /*
          * Once WORKER_DIE is set, the kworker may destroy itself at any