Messages in this thread Patch in this message | | | Date | Tue, 28 Apr 2015 18:16:11 +0800 | From | Lai Jiangshan <> | Subject | Re: [PATCH 3/3 V8] workqueue: Allow modifying low level unbound workqueue cpumask |
| |
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.
| |