linux-kernel - Re: [PATCH v2 5/6] workqueue: use system_highpri_wq for highpri workers in rebind

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:	Tue, 14 Aug 2012 11:29:11 -0700
From:	Tejun Heo <tj@...nel.org>
To:	Joonsoo Kim <js1304@...il.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/6] workqueue: use system_highpri_wq for highpri
 workers in rebind_workers()

On Wed, Aug 15, 2012 at 03:10:15AM +0900, Joonsoo Kim wrote:
> In rebind_workers(), we do inserting a work to rebind to cpu for busy workers.
> Currently, in this case, we use only system_wq. This makes a possible
> error situation as there is mismatch between cwq->pool and worker->pool.
> 
> To prevent this, we should use system_highpri_wq for highpri worker
> to match theses. This implements it.
> 
> Signed-off-by: Joonsoo Kim <js1304@...il.com>
> 
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 470b0eb..4c5733c1 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1738,6 +1738,7 @@ retry:
>  	/* rebind busy workers */
>  	for_each_busy_worker(worker, i, pos, gcwq) {
>  		struct work_struct *rebind_work = &worker->rebind_work;
> +		struct workqueue_struct *wq;
>  
>  		/* morph UNBOUND to REBIND */
>  		worker->flags &= ~WORKER_UNBOUND;
> @@ -1749,9 +1750,12 @@ retry:
>  
>  		/* wq doesn't matter, use the default one */
>  		debug_work_activate(rebind_work);
> -		insert_work(get_cwq(gcwq->cpu, system_wq), rebind_work,
> -			    worker->scheduled.next,
> -			    work_color_to_flags(WORK_NO_COLOR));
> +		wq = worker_pool_pri(worker->pool) ? system_highpri_wq :
> +								system_wq;
> +		insert_work(
> +			get_cwq(gcwq->cpu, wq),
> +			rebind_work, worker->scheduled.next,
> +			work_color_to_flags(WORK_NO_COLOR));
				
Umm... this indentation is ugly.  Please follow the indentation of the
surrounding code.  If ?: gets too long, using if/else might be better.
Please also comment why the above is necessary.  The comment above
says "wq doesn't matter" and then we're choosing workqueue, which is
confusing.

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/