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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 30 Jul 2014 08:32:51 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	Tejun Heo <tj@...nel.org>
CC:	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC 2/2 V2] workqueue: use dedicated creater kthread for
 all pools

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.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/