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

Lai Jiangshan
2014-04-23T02:14:00Z

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