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

[Lai Jiangshan <laijs@cn.fujitsu.com>]

Hello

> 
>> --- 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?


In V4 patchset, Kevin Hilman had requested the wq_unbound_cpumask
to be "cpumask_complement(wq_unbound_cpumask, tick_nohz_full_mask);"

I replied against it and I suggested that wq_unbound_cpumask can be
re-set after workqueue initialized it.

And Frederic Weisbecker seemed on my side:
"""
If it should be the default on NO_HZ_FULL, maybe we should do this from the
tick nohz code. Some late or fs initcall that will do the workqueue affinity,
timer affinity, etc...
"""

So, we need an API to modify the wq_unbound_cpumask, and I provided
this public function.  Otherwise, the other code can't modify it.

> 
>> --- 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.

It should be:

+	copy_workqueue_attrs(new_attrs, attrs);
+	cpumask_and(new_attrs->cpumask, new_attrs->cpumask, cpu_possible_mask);

> 
>> +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?

It isn't necessary. It was added in V7. I thought it could make
the code more normal.


Thanks
Lai