linux-kernel - Re: [PATCH 1/3] workqueue: add wq_unbound_online

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]

Message-ID: <20141027131548.GD4436@htj.dyndns.org>
Date:	Mon, 27 Oct 2014 09:15:48 -0400
From:	Tejun Heo <tj@...nel.org>
To:	Lai Jiangshan <laijs@...fujitsu.com>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] workqueue: add wq_unbound_online_cpumask

On Wed, Oct 08, 2014 at 11:53:31AM +0800, Lai Jiangshan wrote:
> Current wq_calc_node_cpumask() is complicated by cpumask_of_node(node) whose
> value need to be revised before using and the "revising" needs @cpu_going_down
> which makes more complicated.
> 
> This patch introduces wq_unbound_online_cpumask which is updated before
> wq_update_unbound_numa() in the cpu-hotplug callbacks and wq_calc_node_cpumask()
> can use it instead of cpumask_of_node(node). Thus wq_calc_node_cpumask()
> becomes much simpler and @cpu_going_down is gone.
> 
> Signed-off-by: Lai Jiangshan <laijs@...fujitsu.com>
> ---
>  kernel/workqueue.c |   42 ++++++++++++++++++++----------------------
>  1 files changed, 20 insertions(+), 22 deletions(-)

"much simpler" seems a bit overblown for 2 LOC reduction.

> +/* PL: online cpumask for all unbound wqs */
> +static struct cpumask wq_unbound_online_cpumask;

And now someone who's reading the code has to wonder "why is wq
maintaining a separate copy of cpumask?" and from the code itself it
isn't clear at all.  I don't necessarily dislike the patch and it does
make the code a bit simpler but at the cost of higher obscurity.
You'll at least need to add more comments explaining why the separate
cpumask is necessary and how it's used.  If there are more
simplifications which can build atop, it'd be fine; otherwise, this is
firmly in the 'meh...' territory.

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/