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