Re: [PATCH] workqueue: allow changing attributions of ordered workqueue

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

On 04/23/2014 08:04 AM, Frederic Weisbecker wrote:
> Hi Lai,
> 
> So actually I'll need to use apply_workqueue_attr() on the next patchset. So
> I'm considering this patch.
> 
> Some comments below:
> 
> On Tue, Apr 15, 2014 at 05:58:08PM +0800, Lai Jiangshan wrote:
>> From 534f1df8a5a03427b0fc382150fbd34e05648a28 Mon Sep 17 00:00:00 2001
>> From: Lai Jiangshan <laijs@cn.fujitsu.com>
>> Date: Tue, 15 Apr 2014 17:52:19 +0800
>> Subject: [PATCH] workqueue: allow changing attributions of ordered workqueue
>>
>> Allow changing ordered workqueue's cpumask
>> Allow changing ordered workqueue's nice value
>> Allow registering ordered workqueue to SYSFS
>>
>> Still disallow changing ordered workqueue's max_active which breaks ordered guarantee
>> Still disallow changing ordered workqueue's no_numa which breaks ordered guarantee
>>
>> Changing attributions will introduce new pwqs in a given workqueue, and
>> old implement doesn't have any solution to handle multi-pwqs on ordered workqueues,
>> so changing attributions for ordered workqueues is disallowed.
>>
>> After I investigated several solutions which try to handle multi-pwqs on ordered workqueues,
>> I found the solution which reuse max_active are the simplest.
>> In this solution, the new introduced pwq's max_active is kept as *ZERO* until it become
>> the oldest pwq.
> 
> I don't see where this zero value is enforced. Do you mean 1? That's the initial value of
> ordered max_active pools.

pwq's max_active is force zero in pwq_adjust_max_active().
If the older pwq is still existed, the newer one's max_active is forced zero.

> 
>> Thus only one (the oldest) pwq is active, and ordered is guarantee.
>>
>> This solution forces ordered on higher level while the non-reentrancy is also
>> forced. so we don't need to queue any work to its previous pool. And we shouldn't
>> do it. we must disallow any work repeatedly requeues itself back-to-back
>> which keeps the oldest pwq stay and make newest(if have) pwq starvation(non-active for ever).
>>
>> Build test only.
>> This patch depends on previous patch:
>> workqueue: add __WQ_FREEZING and remove POOL_FREEZING
>>
>> Frederic:
>> 	You can pick this patch to your updated patchset before
>> 	TJ apply it.
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> ---
>>  kernel/workqueue.c |   66 ++++++++++++++++++++++++++++++---------------------
>>  1 files changed, 39 insertions(+), 27 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index 92c9ada..fadcc4a 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -1350,8 +1350,14 @@ retry:
>>  	 * If @work was previously on a different pool, it might still be
>>  	 * running there, in which case the work needs to be queued on that
>>  	 * pool to guarantee non-reentrancy.
>> +	 *
>> +	 * pwqs are guaranteed active orderly for ordered workqueue, and
>> +	 * it guarantees non-reentrancy for works. So any work doesn't need
>> +	 * to be queued on previous pool. And the works shouldn't be queued
>> +	 * on previous pool, since we need to guarantee the prevous pwq
>> +	 * releasable to avoid work-stavation on the newest pool.
> 
> BTW, who needs this reentrancy guarantee? Is this an implicit guarantee for
> all workqueues that is there for backward compatibility? I've seen some
> patches dealing with that lately but I don't recall the details.
> 

dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1

>>  	 */
>> -	last_pool = get_work_pool(work);
>> +	last_pool = wq->flags & __WQ_ORDERED ? NULL : get_work_pool(work);
> 
> Does it hurt performance to do this old worker recovery? It seems to
> when I look at get_work_pool() and find_worker_executing_pool().
> 
> Perhaps we can generalize this check to all wqs which have only one
> worker?
> 
> Anyway that's just optimizations. Nothing that needs to be done in this
> patch.
> 
[...]
> 
> So I have mixed feelings with this patch given the complication. But it's probably
> better to take that direction.

Any feeling is welcome to share here.

> 
> I just wish we had some way to automatically detect situations where a work
> mistakenly runs through re-entrancy. Because if it ever happens randomly,
> it's going to be a hell to debug.

dbf2576e37da0fcc7aacbfbb9fd5d3de7888a3c1 has forced non-reentrant and
is well reviewed. Any additional automatically detect is also welcome
for debugging. But I don't think it is required for your aim or this patch.

> 
> Thanks.
> 
>> -- 
>> 1.7.4.4
>>
> .
>