linux-kernel - Re: [PATCH 3/7] workqueue: rename rebind_workers() to associate_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 4 Apr 2013 07:15:23 -0700
From:	Tejun Heo <tj@...nel.org>
To:	Lai Jiangshan <laijs@...fujitsu.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/7] workqueue: rename rebind_workers() to
 associate_cpu_pool()

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@...fujitsu.com>
> ---
>  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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/