Re: [PATCH 1/3] workqueue: add wq_unbound_online_cpumask

[Tejun Heo]

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