Messages in this thread | | | Date | Mon, 27 Apr 2015 12:07:56 -0400 | From | Tejun Heo <> | Subject | Re: [PATCH 3/3 V8] workqueue: Allow modifying low level unbound workqueue cpumask |
| |
Hello, Lai.
Overall, it looks good, just a couple more nits.
On Mon, Apr 27, 2015 at 05:58:40PM +0800, Lai Jiangshan wrote: > The oreder-workquue is ignore from the low level unbound workqueue
Ordered workqueues are ignored
> cpumask, it will be handled in near future. > > All the (default & per-nodes') pwqs are mandatorily controlled by default & per-node > the low level cpumask. If the user configured cpumask doesn't overlap > with the low level cpumask, the low level cpumask will be used for the > wq instead. > > The default wq_unbound_cpumask is still cpu_possible_mask due to the workqueue > subsystem doesn't know what is the best default value for the runtime, the > system manager or other subsystem which knows the sufficient information should set > it when needed.
Please re-flow the paragraph. Also, ultimately, we want this to consider isolcpus, right?
> --- a/include/linux/workqueue.h > +++ b/include/linux/workqueue.h > @@ -424,6 +424,7 @@ struct workqueue_attrs *alloc_workqueue_attrs(gfp_t gfp_mask); > void free_workqueue_attrs(struct workqueue_attrs *attrs); > int apply_workqueue_attrs(struct workqueue_struct *wq, > const struct workqueue_attrs *attrs); > +int workqueue_set_unbound_cpumask(cpumask_var_t cpumask);
Why is this a public function?
> --- a/kernel/workqueue.c > +++ b/kernel/workqueue.c > @@ -3548,13 +3549,18 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, > * If something goes wrong during CPU up/down, we'll fall back to > * the default pwq covering whole @attrs->cpumask. Always create > * it even if we don't use it immediately. > + * > + * If the user configured cpumask doesn't overlap with the > + * wq_unbound_cpumask, we fallback to the wq_unbound_cpumask. > */ > + if (unlikely(cpumask_empty(new_attrs->cpumask))) > + cpumask_copy(new_attrs->cpumask, wq_unbound_cpumask);
Please see below.
> ctx->dfl_pwq = alloc_unbound_pwq(wq, new_attrs); > if (!ctx->dfl_pwq) > goto out_free; > > for_each_node(node) { > - if (wq_calc_node_cpumask(attrs, node, -1, tmp_attrs->cpumask)) { > + if (wq_calc_node_cpumask(new_attrs, node, -1, tmp_attrs->cpumask)) { > ctx->pwq_tbl[node] = alloc_unbound_pwq(wq, tmp_attrs); > if (!ctx->pwq_tbl[node]) > goto out_free; > @@ -3564,7 +3570,10 @@ apply_wqattrs_prepare(struct workqueue_struct *wq, > } > } > > + /* save the user configured attrs */ > + cpumask_and(new_attrs->cpumask, attrs->cpumask, cpu_possible_mask);
Wouldn't this make a lot more sense above when copying @attrs into @new_attrs? The comment there even says "make a copy of @attrs and sanitize it". Copy to @new_attrs, mask with wq_unbound_cpumask and fall back to wq_unbound_cpumask if empty.
> +static int workqueue_apply_unbound_cpumask(void) > +{ ... > + list_for_each_entry_safe(ctx, n, &ctxs, list) {
Is the following list_del() necessary? The list is never used again, right?
> + list_del(&ctx->list); > + if (!ret) > + apply_wqattrs_commit(ctx); > + apply_wqattrs_cleanup(ctx); > + } > + > + return ret; > +} ... > +int workqueue_set_unbound_cpumask(cpumask_var_t cpumask) > +{ ... > +} > +EXPORT_SYMBOL_GPL(workqueue_set_unbound_cpumask);
Again, why is this exported? Who's the expected user?
Thanks.
-- tejun
| |