Re: [PATCH 14/31] workqueue: replace POOL_MANAGING_WORKERS flag with worker_pool->manager_mutex

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

On 02/03/13 11:24, Tejun Heo wrote:
> POOL_MANAGING_WORKERS is used to synchronize the manager role.
> Synchronizing among workers doesn't need blocking and that's why it's
> implemented as a flag.
> 
> It got converted to a mutex a while back to add blocking wait from CPU
> hotplug path - 6037315269 ("workqueue: use mutex for global_cwq
> manager exclusion").  Later it turned out that synchronization among
> workers and cpu hotplug need to be done separately.  Eventually,
> POOL_MANAGING_WORKERS is restored and workqueue->manager_mutex got
> morphed into workqueue->assoc_mutex - 552a37e936 ("workqueue: restore
> POOL_MANAGING_WORKERS") and b2eb83d123 ("workqueue: rename
> manager_mutex to assoc_mutex").
> 
> Now, we're gonna need to be able to lock out managers from
> destroy_workqueue() to support multiple unbound pools with custom
> attributes making it again necessary to be able to block on the
> manager role.  This patch replaces POOL_MANAGING_WORKERS with
> worker_pool->manager_mutex.
> 
> This patch doesn't introduce any behavior changes.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  kernel/workqueue.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 2645218..68b3443 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -64,7 +64,6 @@ enum {
>  	 * create_worker() is in progress.
>  	 */
>  	POOL_MANAGE_WORKERS	= 1 << 0,	/* need to manage workers */
> -	POOL_MANAGING_WORKERS   = 1 << 1,       /* managing workers */
>  	POOL_DISASSOCIATED	= 1 << 2,	/* cpu can't serve workers */
>  	POOL_FREEZING		= 1 << 3,	/* freeze in progress */
>  
> @@ -145,6 +144,7 @@ struct worker_pool {
>  	DECLARE_HASHTABLE(busy_hash, BUSY_WORKER_HASH_ORDER);
>  						/* L: hash of busy workers */
>  
> +	struct mutex		manager_mutex;	/* the holder is the manager */
>  	struct mutex		assoc_mutex;	/* protect POOL_DISASSOCIATED */
>  	struct ida		worker_ida;	/* L: for worker IDs */
>  
> @@ -702,7 +702,7 @@ static bool need_to_manage_workers(struct worker_pool *pool)
>  /* Do we have too many workers and should some go away? */
>  static bool too_many_workers(struct worker_pool *pool)
>  {
> -	bool managing = pool->flags & POOL_MANAGING_WORKERS;
> +	bool managing = mutex_is_locked(&pool->manager_mutex);
>  	int nr_idle = pool->nr_idle + managing; /* manager is considered idle */
>  	int nr_busy = pool->nr_workers - nr_idle;
>  
> @@ -2027,15 +2027,13 @@ static bool manage_workers(struct worker *worker)
>  	struct worker_pool *pool = worker->pool;
>  	bool ret = false;
>  
> -	if (pool->flags & POOL_MANAGING_WORKERS)
> +	if (!mutex_trylock(&pool->manager_mutex))
>  		return ret;
>  
> -	pool->flags |= POOL_MANAGING_WORKERS;


if mutex_trylock(&pool->manager_mutex) fails, it does not mean
the pool is managing workers. (although current code does).
so I recommend to keep POOL_MANAGING_WORKERS.

I suggest that you reuse assoc_mutex for your purpose(later patches).
(and rename assoc_mutex back to manager_mutex)


> -
>  	/*
>  	 * To simplify both worker management and CPU hotplug, hold off
>  	 * management while hotplug is in progress.  CPU hotplug path can't
> -	 * grab %POOL_MANAGING_WORKERS to achieve this because that can
> +	 * grab @pool->manager_mutex to achieve this because that can
>  	 * lead to idle worker depletion (all become busy thinking someone
>  	 * else is managing) which in turn can result in deadlock under
>  	 * extreme circumstances.  Use @pool->assoc_mutex to synchronize
> @@ -2075,8 +2073,8 @@ static bool manage_workers(struct worker *worker)
>  	ret |= maybe_destroy_workers(pool);
>  	ret |= maybe_create_worker(pool);
>  
> -	pool->flags &= ~POOL_MANAGING_WORKERS;
>  	mutex_unlock(&pool->assoc_mutex);
> +	mutex_unlock(&pool->manager_mutex);
>  	return ret;
>  }
>  
> @@ -3805,6 +3803,7 @@ static int __init init_workqueues(void)
>  			setup_timer(&pool->mayday_timer, pool_mayday_timeout,
>  				    (unsigned long)pool);
>  
> +			mutex_init(&pool->manager_mutex);
>  			mutex_init(&pool->assoc_mutex);
>  			ida_init(&pool->worker_ida);
>