Re: [PATCH 04/10] workqueue: destroy worker directly in the idle timeout handler

[Lai Jiangshan <laijs@cn.fujitsu.com>]

On 05/05/2014 10:36 PM, Tejun Heo wrote:
> On Sun, Apr 27, 2014 at 12:08:59PM +0800, Lai Jiangshan wrote:
>> Since kthread_stop() is removed from destroy_worker(),
>> destroy_worker() doesn't need to sleep.
>> Since "unbind the worker" is moved out from destroy_worker(),
>> destroy_worker() doesn't require manager_mutex.
>>
>> So destroy_worker() can be directly called in the idle timeout
>> handler, it helps us remove POOL_MANAGE_WORKERS and
>> maybe_destroy_worker() and simplify the manage_workers()
>>
>> After POOL_MANAGE_WORKERS is removed, worker_thread() doesn't
>> need to test whether it needs to manage after processed works.
>> So we can remove this test branch.
> 
> Ah, so, you can take out workers directly from idle timer.  Yeah,
> that's nice.  I'm not a big fan of the wait_queue usage in the
> previous patch tho.  Can we use a completion instead?
> 
> Thanks.
> 

1) complete() can't be called inside attach_mutex due to the worker
   shouldn't access to the pool after complete().
2) put_unbound_pool() may called from get_unbound_pool(), we need to add
   an additional check and avoid the wait_for_completion() if so.

+static void worker_detach_from_pool(struct worker *worker, struct worker_pool *pool)
+{
+	bool is_last;
+
+	mutex_lock(&pool->bind_mutex);
+	list_del(&worker->bind_entry);
+	is_last = list_empty(&worker->bind_entry);
+	mutex_unlock(&pool->bind_mutex);
+
+	/* need some comments here */
+	if (is_last)
+		complete(&pool->workers_detached);
+}


@@ -3588,6 +3587,7 @@ static void put_unbound_pool(struct worker_pool *pool)
 	mutex_lock(&pool->manager_mutex);
 	spin_lock_irq(&pool->lock);
 	
+	need_to_wait = pool->nr_workers != 0; /* it may be called from get_unbound_pool() */
 	while ((worker = first_worker(pool)))
 		destroy_worker(worker);
 	WARN_ON(pool->nr_workers || pool->nr_idle);
@@ -3596,6 +3596,8 @@ static void put_unbound_pool(struct worker_pool *pool)
 	mutex_unlock(&pool->manager_mutex);
 	mutex_unlock(&pool->manager_arb);
 
+	if (need_to_wait)
+		wait_for_completion(&pool->workers_detached);

 	/* shut down the timers */
 	del_timer_sync(&pool->idle_timer);
 	del_timer_sync(&pool->mayday_timer);


So I think wait_queue is more grace.

Thanks,
Lai