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

Powered by blists - more mailing lists