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

[Lai Jiangshan]

On 04/28/2015 06:16 PM, Lai Jiangshan wrote:
> 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?

Hello, TJ,

I didn't accept your this comments in V9 patch.

I had explained it in other long email (embedded here).
I will leave for several days, so I sent V9 patch with an
unsettled comment.

Thanks,
Lai

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

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