Re: [PATCH 3/3 V8] workqueue: Allow modifying low level unbound workqueue cpumask

[Tejun Heo <tj@kernel.org>]

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