Re: [PATCH] workqueue: add __WQ_FREEZING and remove POOL_FREEZING

[Tejun Heo <tj@kernel.org>]

Hello,

On Tue, Mar 25, 2014 at 05:56:04PM +0800, Lai Jiangshan wrote:
> freezing is nothing related to pools, but POOL_FREEZING adds a connection,
> and causes freeze_workqueues_begin() and thaw_workqueues() complicated.
> 
> Since freezing is workqueue instance attribute, so we introduce __WQ_FREEZING
> to wq->flags instead and remove POOL_FREEZING.
> 
> we set __WQ_FREEZING only when freezable(to simplify pwq_adjust_max_active()),
> make freeze_workqueues_begin() and thaw_workqueues() fast skip non-freezable wq.

Please wrap the description to 80 columns.

> @@ -3730,18 +3726,13 @@ static void pwq_unbound_release_workfn(struct work_struct *work)
>  static void pwq_adjust_max_active(struct pool_workqueue *pwq)
>  {
>  	struct workqueue_struct *wq = pwq->wq;
> -	bool freezable = wq->flags & WQ_FREEZABLE;
>  
> -	/* for @wq->saved_max_active */
> +	/* for @wq->saved_max_active and @wq->flags */
>  	lockdep_assert_held(&wq->mutex);
>  
> -	/* fast exit for non-freezable wqs */
> -	if (!freezable && pwq->max_active == wq->saved_max_active)
> -		return;
> -

Why are we removing the above?  Can't we still test __WQ_FREEZING as
we're holding wq->mutex?  I don't really mind removing the
optimization but the patch description at least has to explain what's
going on.

...
>  	list_for_each_entry(wq, &workqueues, list) {
> +		if (!(wq->flags & WQ_FREEZABLE))
> +			continue;

Ah, okay, you're not calling the function at all if WQ_FREEZABLE is
not set.  I couldn't really understand what you were trying to say in
the patch description.  Can you please try to refine the description
more?  It's better to be verbose and clear than short and difficult to
understand.

Thanks.

-- 
tejun