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

[Lai Jiangshan]

On 04/28/2015 11:49 AM, Tejun Heo wrote:
> Hello,
> 
> On Tue, Apr 28, 2015 at 10:24:31AM +0800, Lai Jiangshan wrote:
>>>> 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.
>>
>> We need to save the user original configured attrs.
>> When any time wq_unbound_cpumask is changed, we should use
>> the user original configured attrs (cpumask) to re-calculate
>> the pwqs and avoid losing any information.
> 
> Sure, we can do that for new_attrs and then mask tmp_attrs further w/
> wq_unbound_cpumask, no?
> 
> Thanks.
> 

We need to pass new_attrs to wq_calc_node_cpumask().

If new_attrs (the first argument of wq_calc_node_cpumask()) is not masked
with wq_unbound_cpumask when passed in, wq_calc_node_cpumask()
will be much complicated (I tried coding it yesterday).

Quote:
static bool wq_calc_node_cpumask(const struct workqueue_attrs *attrs, int node,
				 int cpu_going_down, cpumask_t *cpumask)
{
	if (!wq_numa_enabled || attrs->no_numa)
		goto use_dfl;

	/* does @node have any online CPUs @attrs wants? */
	cpumask_and(cpumask, cpumask_of_node(node), attrs->cpumask); [1]
	if (cpu_going_down >= 0)
		cpumask_clear_cpu(cpu_going_down, cpumask);

	if (cpumask_empty(cpumask))
		goto use_dfl;

	/* yeap, return possible CPUs in @node that @attrs wants */
	cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]); [2]
	return !cpumask_equal(cpumask, attrs->cpumask); [3]

use_dfl:
	cpumask_copy(cpumask, attrs->cpumask); [4]
	return false;
}


If @attrs is not masked with wq_unbound_cpumask when passed in, the code
needs add two maskings (with wq_unbound_cpumask) at [1] and [2].

And the code requests to get the cpumask of the default pwq at [3]&[4],
thus the code need to (re-)calculate the default pwq's attrs here and
doubles the code. (this calculation is already done before this function).

It will make all things simple and avoid complicating the wq_calc_node_cpumask(),
if wq_calc_node_cpumask() is kept unchanged but accepts only the default pwq's
attrs as its first argument.

The call-site in wq_update_unbound_numa() is changed in V8 to meet this requirement.

@@ -3705,11 +3714,11 @@ static void wq_update_unbound_numa(struct workqueue_struct *wq, int cpu,
 
 	/*
 	 * Let's determine what needs to be done.  If the target cpumask is
-	 * different from wq's, we need to compare it to @pwq's and create
-	 * a new one if they don't match.  If the target cpumask equals
-	 * wq's, the default pwq should be used.
+	 * different from the default pwq's, we need to compare it to @pwq's
+	 * and create a new one if they don't match.  If the target cpumask
+	 * equals the default pwq's, the default pwq should be used.
 	 */
-	if (wq_calc_node_cpumask(wq->unbound_attrs, node, cpu_off, cpumask)) {
+	if (wq_calc_node_cpumask(wq->dfl_pwq->pool->attrs, node, cpu_off, cpumask)) {
 		if (cpumask_equal(cpumask, pwq->pool->attrs->cpumask))
 			goto out_unlock;
 	} else {


This requirement is not a new requirement.  In the code before this patch,
the argument @attrs for wq_calc_node_cpumask() is expected to be the default
pwq's attrs which happens to be wq->unbound_attrs all the time.

In the code after this patch, the argument @attrs for wq_calc_node_cpumask()
is still expected to be the default pwq's attrs which may not be
wq->unbound_attrs.

So the requirement is not new and wq_calc_node_cpumask() is untouched,
but the comment for wq_calc_node_cpumask() needs to be updated which
I should have done, forgive me.

Thanks,
Lai.



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