linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lai Jiangshan <laijs@cn.fujitsu.com>
To: Tejun Heo <tj@kernel.org>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH 02/10 V4] workqueue: fix deadlock in rebind_workers()
Date: Wed, 05 Sep 2012 09:28:38 +0800	[thread overview]
Message-ID: <5046AAC6.7010704@cn.fujitsu.com> (raw)
In-Reply-To: <20120905005409.GB2836@dhcp-172-17-108-109.mtv.corp.google.com>

On 09/05/2012 08:54 AM, Tejun Heo wrote:
> How about something like the following?  This is more consistent with
> the existing code and as the fixes need to go separately through
> for-3.6-fixes, it's best to stay consistent regardless of the end
> result after all the restructuring.  It's not tested yet.  If you
> don't object, I'll split it into two patches, test and route them
> through for-3.6-fixes w/ your Original-patch-by.
> 

I see that this patch's idea is same as mine but reuses
@idle_rebind.cnt and @idle_rebind.done.

I don't think it is consistent to avoid adding new field
and to reuse old field for different purpose


Thanks
Lai


> Thanks.
> ---
>  kernel/workqueue.c |   51 ++++++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 38 insertions(+), 13 deletions(-)
> 
> Index: work/kernel/workqueue.c
> ===================================================================
> --- work.orig/kernel/workqueue.c
> +++ work/kernel/workqueue.c
> @@ -1326,6 +1326,15 @@ static void idle_worker_rebind(struct wo
>  
>  	/* we did our part, wait for rebind_workers() to finish up */
>  	wait_event(gcwq->rebind_hold, !(worker->flags & WORKER_REBIND));
> +
> +	/*
> +	 * rebind_workers() shouldn't finish until all workers passed the
> +	 * above WORKER_REBIND wait.  Tell it when done.
> +	 */
> +	spin_lock_irq(&worker->pool->gcwq->lock);
> +	if (!--worker->idle_rebind->cnt)
> +		complete(&worker->idle_rebind->done);
> +	spin_unlock_irq(&worker->pool->gcwq->lock);
>  }
>  
>  /*
> @@ -1422,19 +1431,7 @@ retry:
>  		goto retry;
>  	}
>  
> -	/*
> -	 * All idle workers are rebound and waiting for %WORKER_REBIND to
> -	 * be cleared inside idle_worker_rebind().  Clear and release.
> -	 * Clearing %WORKER_REBIND from this foreign context is safe
> -	 * because these workers are still guaranteed to be idle.
> -	 */
> -	for_each_worker_pool(pool, gcwq)
> -		list_for_each_entry(worker, &pool->idle_list, entry)
> -			worker->flags &= ~WORKER_REBIND;
> -
> -	wake_up_all(&gcwq->rebind_hold);

don't need to move down.

> -
> -	/* rebind busy workers */
> +	/* all idle workers are rebound, rebind busy workers */
>  	for_each_busy_worker(worker, i, pos, gcwq) {
>  		struct work_struct *rebind_work = &worker->rebind_work;
>  		unsigned long worker_flags = worker->flags;
> @@ -1454,6 +1451,34 @@ retry:
>  			    worker->scheduled.next,
>  			    work_color_to_flags(WORK_NO_COLOR));
>  	}
> +
> +	/*
> +	 * All idle workers are rebound and waiting for %WORKER_REBIND to
> +	 * be cleared inside idle_worker_rebind().  Clear and release.
> +	 * Clearing %WORKER_REBIND from this foreign context is safe
> +	 * because these workers are still guaranteed to be idle.
> +	 *
> +	 * We need to make sure all idle workers passed WORKER_REBIND wait
> +	 * in idle_worker_rebind() before returning; otherwise, workers can
> +	 * get stuck at the wait if hotplug cycle repeats.
> +	 */
> +	idle_rebind.cnt = 1;
> +	INIT_COMPLETION(idle_rebind.done);
> +
> +	for_each_worker_pool(pool, gcwq) {
> +		list_for_each_entry(worker, &pool->idle_list, entry) {
> +			worker->flags &= ~WORKER_REBIND;
> +			idle_rebind.cnt++;
> +		}
> +	}
> +
> +	wake_up_all(&gcwq->rebind_hold);
> +
> +	if (--idle_rebind.cnt) {
> +		spin_unlock_irq(&gcwq->lock);
> +		wait_for_completion(&idle_rebind.done);
> +		spin_lock_irq(&gcwq->lock);
> +	}
>  }
>  
>  static struct worker *alloc_worker(void)
> 


  reply	other threads:[~2012-09-05  1:26 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-01 16:28 [PATCH 00/10 V4] workqueue: fix and cleanup hotplug/rebind_workers() Lai Jiangshan
2012-09-01 16:28 ` [PATCH 01/10 V4] workqueue: ensure the wq_worker_sleeping() see the right flags Lai Jiangshan
2012-09-04 23:39   ` [PATCH] workqueue: UNBOUND -> REBIND morphing in rebind_workers() should be atomic Tejun Heo
2012-09-04 23:58     ` [PATCH -stable] " Tejun Heo
2012-09-16 15:49       ` Ben Hutchings
2012-09-05  1:05     ` [PATCH] " Lai Jiangshan
2012-09-05  1:17       ` Tejun Heo
2012-09-01 16:28 ` [PATCH 02/10 V4] workqueue: fix deadlock in rebind_workers() Lai Jiangshan
2012-09-05  0:54   ` Tejun Heo
2012-09-05  1:28     ` Lai Jiangshan [this message]
2012-09-05  1:33       ` Tejun Heo
2012-09-01 16:28 ` [PATCH 03/10 V4] workqueue: add POOL_MANAGING_WORKERS Lai Jiangshan
2012-09-01 16:28 ` [PATCH 04/10 V4] workqueue: add manage_workers_slowpath() Lai Jiangshan
2012-09-05  1:12   ` Tejun Heo
2012-09-06  1:55     ` Lai Jiangshan
2012-09-01 16:28 ` [PATCH 05/10 V4] workqueue: move rebind_hold to idle_rebind Lai Jiangshan
2012-09-01 16:28 ` [PATCH 06/10 V4] workqueue: simple clear WORKER_REBIND Lai Jiangshan
2012-09-01 16:28 ` [PATCH 07/10 V4] workqueue: move idle_rebind pointer to gcwq Lai Jiangshan
2012-09-01 16:28 ` [PATCH 08/10 V4] workqueue: explicit way to wait for idles workers to finish Lai Jiangshan
2012-09-01 16:28 ` [PATCH 09/10] workqueue: single pass rebind_workers Lai Jiangshan
2012-09-01 16:28 ` [PATCH 10/10 V4] workqueue: merge the role of rebind_hold to idle_done Lai Jiangshan
2012-09-05  1:15   ` Tejun Heo
2012-09-05  1:48     ` Lai Jiangshan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5046AAC6.7010704@cn.fujitsu.com \
    --to=laijs@cn.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=tj@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).