Messages in this thread | | | Date | Mon, 27 Oct 2014 09:15:48 -0400 | From | Tejun Heo <> | 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@cn.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
| |