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

Lai Jiangshan
2014-05-07T13:38:42Z

On Wed, May 7, 2014 at 9:12 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Lai.
>
> On Wed, May 07, 2014 at 03:10:20PM +0800, Lai Jiangshan wrote:
>> 1) complete() can't be called inside attach_mutex due to the worker
>> shouldn't access to the pool after complete().
>
> Sure, complete it after releasing the lock. Shutdown can't complete
> before the completion gets completed, right?
>
>> 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.

Do you accept if I remove put_unbound_pool() from get_unbound_pool()
and use several freeing code instead?

>>
>> +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);
>
> Shouldn't it be able to wait whenever it's about to destroy non-empty
> pool?
>
> DECLARE_COMPLETION_ONSTACK(completion);
>
> ...
>
> while ((worker = first_worker(pool))) {
> destroy_worker(worker);
> pool->detach_completion = &completion;
> }
>
> ...
> unlock;
>
> if (pool->detach_completion)
> wait_for_completion();

it is more complex than wait_queue.
if put_unbound_pool() is removed out from get_unbound_pool(),
a simple wait_for_completion() is enough.

> ...
>
> And the worker side can simply do,
>
> struct completion *completion;
>
> if (I'm the last worker exiting)
> completion = pool->detach_completion;
> unlock;
>

if(completion)
> complete(completion);
>
> 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/