Re: [PATCH 07/31] workqueue: restructure pool / pool_workqueue iterations in freeze/thaw functions

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

On 02/03/13 11:23, Tejun Heo wrote:
> The three freeze/thaw related functions - freeze_workqueues_begin(),
> freeze_workqueues_busy() and thaw_workqueues() - need to iterate
> through all pool_workqueues of all freezable workqueues.  They did it
> by first iterating pools and then visiting all pwqs (pool_workqueues)
> of all workqueues and process it if its pwq->pool matches the current
> pool.  This is rather backwards and done this way partly because
> workqueue didn't have fitting iteration helpers and partly to avoid
> the number of lock operations on pool->lock.
> 
> Workqueue now has fitting iterators and the locking operation overhead
> isn't anything to worry about - those locks are unlikely to be
> contended and the same CPU visiting the same set of locks multiple
> times isn't expensive.
> 
> Restructure the three functions such that the flow better matches the
> logical steps and pwq iteration is done using for_each_pwq() inside
> workqueue iteration.
> 
> * freeze_workqueues_begin(): Setting of FREEZING is moved into a
>   separate for_each_pool() iteration.  pwq iteration for clearing
>   max_active is updated as described above.
> 
> * freeze_workqueues_busy(): pwq iteration updated as described above.
> 
> * thaw_workqueues(): The single for_each_wq_cpu() iteration is broken
>   into three discrete steps - clearing FREEZING, restoring max_active,
>   and kicking workers.  The first and last steps use for_each_pool()
>   and the second step uses pwq iteration described above.
> 
> This makes the code easier to understand and removes the use of
> for_each_wq_cpu() for walking pwqs, which can't support multiple
> unbound pwqs which will be needed to implement unbound workqueues with
> custom attributes.
> 
> This patch doesn't introduce any visible behavior changes.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  kernel/workqueue.c | 87 ++++++++++++++++++++++++++++--------------------------
>  1 file changed, 45 insertions(+), 42 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 869dbcc..9f195aa 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3598,6 +3598,8 @@ EXPORT_SYMBOL_GPL(work_on_cpu);
>  void freeze_workqueues_begin(void)
>  {
>  	struct worker_pool *pool;
> +	struct workqueue_struct *wq;
> +	struct pool_workqueue *pwq;
>  	int id;
>  
>  	spin_lock_irq(&workqueue_lock);
> @@ -3605,23 +3607,24 @@ void freeze_workqueues_begin(void)
>  	WARN_ON_ONCE(workqueue_freezing);
>  	workqueue_freezing = true;
>  
> +	/* set FREEZING */
>  	for_each_pool(pool, id) {
> -		struct workqueue_struct *wq;
> -
>  		spin_lock(&pool->lock);
> -
>  		WARN_ON_ONCE(pool->flags & POOL_FREEZING);
>  		pool->flags |= POOL_FREEZING;
> +		spin_unlock(&pool->lock);
> +	}
>  
> -		list_for_each_entry(wq, &workqueues, list) {
> -			struct pool_workqueue *pwq = get_pwq(pool->cpu, wq);
> +	/* suppress further executions by setting max_active to zero */
> +	list_for_each_entry(wq, &workqueues, list) {
> +		if (!(wq->flags & WQ_FREEZABLE))
> +			continue;
>  
> -			if (pwq && pwq->pool == pool &&
> -			    (wq->flags & WQ_FREEZABLE))
> -				pwq->max_active = 0;
> +		for_each_pwq(pwq, wq) {
> +			spin_lock(&pwq->pool->lock);
> +			pwq->max_active = 0;
> +			spin_unlock(&pwq->pool->lock);
>  		}
> -
> -		spin_unlock(&pool->lock);
>  	}
>  
>  	spin_unlock_irq(&workqueue_lock);
> @@ -3642,25 +3645,22 @@ void freeze_workqueues_begin(void)
>   */
>  bool freeze_workqueues_busy(void)
>  {
> -	unsigned int cpu;
>  	bool busy = false;
> +	struct workqueue_struct *wq;
> +	struct pool_workqueue *pwq;
>  
>  	spin_lock_irq(&workqueue_lock);
>  
>  	WARN_ON_ONCE(!workqueue_freezing);
>  
> -	for_each_wq_cpu(cpu) {
> -		struct workqueue_struct *wq;
> +	list_for_each_entry(wq, &workqueues, list) {
> +		if (!(wq->flags & WQ_FREEZABLE))
> +			continue;
>  		/*
>  		 * nr_active is monotonically decreasing.  It's safe
>  		 * to peek without lock.
>  		 */
> -		list_for_each_entry(wq, &workqueues, list) {
> -			struct pool_workqueue *pwq = get_pwq(cpu, wq);
> -
> -			if (!pwq || !(wq->flags & WQ_FREEZABLE))
> -				continue;
> -
> +		for_each_pwq(pwq, wq) {
>  			WARN_ON_ONCE(pwq->nr_active < 0);
>  			if (pwq->nr_active) {
>  				busy = true;
> @@ -3684,40 +3684,43 @@ out_unlock:
>   */
>  void thaw_workqueues(void)
>  {
> -	unsigned int cpu;
> +	struct workqueue_struct *wq;
> +	struct pool_workqueue *pwq;
> +	struct worker_pool *pool;
> +	int id;
>  
>  	spin_lock_irq(&workqueue_lock);
>  
>  	if (!workqueue_freezing)
>  		goto out_unlock;
>  
> -	for_each_wq_cpu(cpu) {
> -		struct worker_pool *pool;
> -		struct workqueue_struct *wq;
> -
> -		for_each_std_worker_pool(pool, cpu) {
> -			spin_lock(&pool->lock);
> -
> -			WARN_ON_ONCE(!(pool->flags & POOL_FREEZING));
> -			pool->flags &= ~POOL_FREEZING;
> -
> -			list_for_each_entry(wq, &workqueues, list) {
> -				struct pool_workqueue *pwq = get_pwq(cpu, wq);
> -
> -				if (!pwq || pwq->pool != pool ||
> -				    !(wq->flags & WQ_FREEZABLE))
> -					continue;
> -
> -				/* restore max_active and repopulate worklist */
> -				pwq_set_max_active(pwq, wq->saved_max_active);
> -			}
> +	/* clear FREEZING */
> +	for_each_pool(pool, id) {
> +		spin_lock(&pool->lock);
> +		WARN_ON_ONCE(!(pool->flags & POOL_FREEZING));
> +		pool->flags &= ~POOL_FREEZING;
> +		spin_unlock(&pool->lock);
> +	}


I think it would be better if we move this code to ...

>  
> -			wake_up_worker(pool);
> +	/* restore max_active and repopulate worklist */
> +	list_for_each_entry(wq, &workqueues, list) {
> +		if (!(wq->flags & WQ_FREEZABLE))
> +			continue;
>  
> -			spin_unlock(&pool->lock);
> +		for_each_pwq(pwq, wq) {
> +			spin_lock(&pwq->pool->lock);
> +			pwq_set_max_active(pwq, wq->saved_max_active);
> +			spin_unlock(&pwq->pool->lock);
>  		}
>  	}
>  
> +	/* kick workers */
> +	for_each_pool(pool, id) {
> +		spin_lock(&pool->lock);
> +		wake_up_worker(pool);
> +		spin_unlock(&pool->lock);
> +	}


... to here.

clear FREEZING and then kick.


> +
>  	workqueue_freezing = false;
>  out_unlock:
>  	spin_unlock_irq(&workqueue_lock);