Re: [PATCH 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING

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

On 04/22/2014 06:20 AM, Tejun Heo wrote:
> On Mon, Apr 21, 2014 at 07:59:20PM +0800, Lai Jiangshan wrote:
>> Only workqueues have freezable or freezing attribution/state, not worker pools.
>> But POOL_FREEZING adds a suspicious state and makes reviewers confused.
>>
>> And it causes freeze_workqueues_begin() and thaw_workqueues() much complicated,
>> they need to travel all the pools besides wqs.
>>
>> Since freezable is workqueue instance's attribution, and freezing
>> is workqueue instance's state, so we introduce __WQ_FREEZING
>> to wq->flags instead and remove POOL_FREEZING.
>>
>> It is different from POOL_FREEZING, POOL_FREEZING is simply set
>> all over the world(all pools), while __WQ_FREEZING is only set for freezable wq.
>> freeze_workqueues_begin()/thaw_workqueues() skip to handle non-freezable wqs
>> and don't touch the non-freezable wqs' flags.
> 
> I was about to apply the patch and have updated the patch description.
> 
>   While freezing takes place globally, its execution is per-workqueue;
>   however, the current implementation makes use of the per-worker_pool
>   POOL_FREEZING flag.  While it's not broken, the flag makes the code
>   more confusing and complicates freeze_workqueues_begin() and
>   thaw_workqueues() by requiring them to walk through all pools.
> 
>   Since freezable is a workqueue's attribute, and freezing is a
>   workqueue's state, let's introduce __WQ_FREEZING to wq->flags instead
>   and remove POOL_FREEZING.
> 
>   It is different from POOL_FREEZING in that __WQ_FREEZING is only set
>   for freezable workqueues while POOL_FREEZING is set globally over all
>   pools.  freeze_workqueues_begin() and thaw_workqueues() now skip
>   non-freezable workqueues.
> 
> But looking at the patch, why do we need __WQ_FREEZING at all?  We
> should be able to test workqueue_freezing in pwq_adjust_max_active(),
> right?  The only requirement there would be that

Testing workqueue_freezing requires wq_pool_mutex held.
Although almost-all pwq_adjust_max_active() are called with wq_pool_mutex held,
except workqueue_set_max_active(). But I hope pwq_adjust_max_active()
don't require the heavy wq_pool_mutex.

> pwq_adjust_max_active() is invoked at least once after
> workqueue_freezing is changed, which is already guaranteed.
> 
> Thanks.
>