Re: [PATCH 02/10] workqueue: destroy_worker() should destroy idle workers only

Lai Jiangshan
2014-05-06T15:58:10Z

On Mon, May 5, 2014 at 9:13 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> On Sun, Apr 27, 2014 at 12:08:57PM +0800, Lai Jiangshan wrote:
>> @@ -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 bound to @pool.
>> + * The new worker should be started and enter idle by start_worker().
>
> Hmm... we used to have a path where a worker is created and then
> destroyed without being started. IIRC, it was on the CPU online
> failure path. A worker was created for the CPU coming online and if
> the online operation failed the created worker was shut down without
> being started. Right, we no longer shutdown per-cpu pools on offline
> so this doesn't matter anymore. Might worthwhile to note in the patch
> description tho.
>
>> @@ -1815,6 +1812,7 @@ static int create_and_start_worker(struct worker_pool *pool)
>> * @worker: worker to be destroyed
>> *
>> * Destroy @worker and adjust @pool stats accordingly.
>> + * The worker should be idle(WORKER_IDLE).
>
> Just write "The worker should be idle." Also, in general, can you
> please put a space before opening parenthesis?
>
>> *
>> * CONTEXT:
>> * spin_lock_irq(pool->lock) which is released and regrabbed.
>> @@ -1828,13 +1826,13 @@ 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)) ||
>> + WARN_ON(pool->nr_workers == 1 && !list_empty(&pool->worklist)))
>
> I'm not sure about the pool condition check. It's kinda overreaching
> to check for it from worker destruction.

"pool->nr_workers == 1", it is the last worker of the pool, so I add this check.

I will remove it.

>
>> @@ -3589,6 +3587,7 @@ static void put_unbound_pool(struct worker_pool *pool)
>> mutex_lock(&pool->manager_mutex);
>> spin_lock_irq(&pool->lock);
>>
>> + WARN_ON(pool->nr_workers != pool->nr_idle);
>
> Does this condition detect anything new from the condition below?

It is already ensured that all workers are idle since pool->ref==0
and manager_arb is held.

This additional WARN_ON() does double check.
It tells the reviewers that destroying all workers on idle_list
equals to destroying all workers of the pool.

I can remove this WARN_ON().

>
>> while ((worker = first_worker(pool)))
>> destroy_worker(worker);
>> WARN_ON(pool->nr_workers || pool->nr_idle);
>
> Thanks.
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/