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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/