Re: [PATCH RFC 2/2 V2] workqueue: use dedicated creater kthread for all pools

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

On 07/29/2014 11:04 PM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Jul 29, 2014 at 05:16:07PM +0800, Lai Jiangshan wrote:
> 
> First of all, the patch is too big.  This is a rather pervasive
> change.  Please split it up if at all possible.
> 
>> +/* Start the mayday timer and the creater when needed */
>> +static inline void start_creater_work(struct worker_pool *pool)
>> +{
>> +	if (pool->nr_idle || pool->creating || list_empty(&pool->worklist))
>> +		return;
> 
> pool->creating is an optimization around queue_kthread_work(), right?
> So that you don't have to grab the lock every time a work item is
> queued.  Please explain things like that explicitly.  Also, the
> condition itself needs explanation.  This is what guarantees that the
> queue is not stalled after all.
> 
> Hmmm... list_empty() is unnecessary when called from the queueing
> path.  Do we want to move that out of this function?
> 
>>  	/* we own @work, set data and link */
>>  	set_work_pwq(work, pwq, extra_flags);
>>  	list_add_tail(&work->entry, head);
>> +	start_creater_work(pool);
> 
> creator is spelled with an 'o' not 'e'.  Also, it'd be better if the
> name reflects that this is a kthread_work not a workqueue one.
> 
>> +static void create_worker(struct kthread_work *work)
>>  {
> ...
>>  fail:
>>  	if (id >= 0)
>>  		ida_simple_remove(&pool->worker_ida, id);
>>  	kfree(worker);
>> -	return NULL;
>> +
>> +	/* cool down before next create_worker() */
>> +	schedule_timeout_interruptible(CREATE_COOLDOWN);
>> +	del_timer_sync(&pool->mayday_timer);
>> +
>> +	spin_lock_irq(&pool->lock);
>> +	pool->creating = false;
>> +	start_creater_work(pool);
>> +	spin_unlock_irq(&pool->lock);
> 
> Why?  Just sleep and retry?  What's the point of requeueing?

Accepted your comments except this one which may need to discuss
for an additional round.  Requeueing passes the retry to the
kthread_worker and gives a change to the other pools which are also
creating worker.

This patch will be deferred until 3.19 due to some unbound patches
are ready soon.

Thanks!!
Lai

> 
>> -/**
>>   * process_one_work - process single work
>>   * @worker: self
>>   * @work: work to process
>> @@ -1991,6 +1905,7 @@ __acquires(&pool->lock)
>>  	work_color = get_work_color(work);
>>  
>>  	list_del_init(&work->entry);
>> +	start_creater_work(pool);
> 
> Should this be combined with wake_up_worker()?
> 
> Thanks.
>