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 <t...@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);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
