Re: [PATCH 3/7] workqueue: rename rebind_workers() toassociate_cpu_pool()

[Tejun Heo]

On Thu, Apr 04, 2013 at 10:05:34AM +0800, Lai Jiangshan wrote:
> merge the code of clearing POOL_DISASSOCIATED to rebind_workers(), and
> rename rebind_workers() to associate_cpu_pool().
> 
> It merges high related code together and simplify
> workqueue_cpu_up_callback().
> 
> Signed-off-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
> ---
>  kernel/workqueue.c |   21 ++++++++++-----------
>  1 files changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 66a9d71..b4369de 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -2270,7 +2270,7 @@ recheck:
>  	 * worker or that someone else has already assumed the manager
>  	 * role.  This is where @worker starts participating in concurrency
>  	 * management if applicable and concurrency management is restored
> -	 * after being rebound.  See rebind_workers() for details.
> +	 * after being rebound.  See associate_cpu_pool() for details.
>  	 */
>  	worker_clr_flags(worker, WORKER_PREP | WORKER_REBOUND);
>  
> @@ -4431,12 +4431,13 @@ static void wq_unbind_fn(struct work_struct *work)
>  }
>  
>  /**
> - * rebind_workers - rebind all workers of a pool to the associated CPU
> + * associate_cpu_pool - rebind all workers of a pool to the associated CPU
>   * @pool: pool of interest
>   *
> - * @pool->cpu is coming online.  Rebind all workers to the CPU.
> + * @pool->cpu is coming online.  Rebind all workers to the CPU and
> + * set the pool associated
>   */
> -static void rebind_workers(struct worker_pool *pool)
> +static void associate_cpu_pool(struct worker_pool *pool)
>  {
>  	struct worker *worker;
>  	int wi;
> @@ -4451,8 +4452,9 @@ static void rebind_workers(struct worker_pool *pool)
>  	 * from CPU_ONLINE, the following shouldn't fail.
>  	 */
>  	for_each_pool_worker(worker, wi, pool)
> -		WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task,
> -						  pool->attrs->cpumask) < 0);
> +		if (WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task,
> +				pool->attrs->cpumask) < 0))
> +			return;
>  
>  	spin_lock_irq(&pool->lock);
>  
> @@ -4491,6 +4493,7 @@ static void rebind_workers(struct worker_pool *pool)
>  		ACCESS_ONCE(worker->flags) = worker_flags;
>  	}
>  
> +	pool->flags &= ~POOL_DISASSOCIATED;

So, now we're clearing DISASSOCIATED after rebinding workers instead
of before.  Is that safe?  Even if so, why no mention of that in the
patch description?  Changes like this are functional changes which can
cause subtle issues and *should* be explained and justified in the
patch description.  Please put more effort in explaining what's going
on.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/