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/
