Hello:
On 06/12/2017 12:32 PM, Tejun Heo wrote:
> Hello,
>
> On Mon, Jun 12, 2017 at 12:10:49PM -0500, Michael Bringmann wrote:
>>> The reason why we're ending up with empty masks is because
>>> wq_calc_node_cpumask() is assuming that the possible node cpumask is
>>> always a superset of online (as it should). We can trigger a fat
>>> warning there if that isn't so and just return false from that
>>> function.
>>
>> What would that look like? I should be able to test it on top of the
>> other changes / corrections.
>
> So, the function looks like the following now.
>
> 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? */
> A: cpumask_and(cpumask, cpumask_of_node(node), attrs->cpumask);
> if (cpu_going_down >= 0)
> cpumask_clear_cpu(cpu_going_down, cpumask);
>
> B: if (cpumask_empty(cpumask))
> goto use_dfl;
>
> /* yeap, return possible CPUs in @node that @attrs wants */
> C: cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]);
> return !cpumask_equal(cpumask, attrs->cpumask);
>
> use_dfl:
> cpumask_copy(cpumask, attrs->cpumask);
> return false;
> }
>
> A is calculating the target cpumask to use using the online map. B
> falls back to dfl mask if the intersection is empty. C calculates the
> eventual mask to use from the intersection of possible mask and what's
> requested. The assumption is that because possible is a superset of
> online, C's result can't be smaller than A.
>
> So, what we can do is if to calculate the possible intersection,
> compare it against the online intersection, and if the latter is
> bigger, trigger a big fat warning and return false there.
So the revisions to the function would look like, correct?
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c74bf39..5d7674e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3564,19 +3564,28 @@ static struct pool_workqueue *alloc_unbound_pwq(struct
workqueue_struct *wq,
static bool wq_calc_node_cpumask(const struct workqueue_attrs *attrs, int node,
int cpu_going_down, cpumask_t *cpumask)
{
+ cpumask_t onl_targ_cm;
+
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);
+ cpumask_and(&onl_targ_cm, cpumask_of_node(node), attrs->cpumask);
if (cpu_going_down >= 0)
- cpumask_clear_cpu(cpu_going_down, cpumask);
+ cpumask_clear_cpu(cpu_going_down, &onl_targ_cm);
- if (cpumask_empty(cpumask))
+ if (cpumask_empty(&onl_targ_cm))
goto use_dfl;
/* yeap, return possible CPUs in @node that @attrs wants */
cpumask_and(cpumask, attrs->cpumask, wq_numa_possible_cpumask[node]);
+
+ if (cpumask_weight(&onl_targ_cm) > cpumask_weight(cpumask)) {
+ printk(KERN_INFO "WARNING: WQ cpumask: onl intersect > "
+ "possible intersect\n");
+ return false;
+ }
+
return !cpumask_equal(cpumask, attrs->cpumask);
use_dfl:
>
>>> I have no idea about the specifics of ppc but at least the code base
>>> we have currently expect all possible cpus and nodes and their
>>> mappings to be established on boot.
>>
>> Hopefully, the new properties will fix the holes in the current
>> implementation
>> with regard to hot-add.
>
> Yeah, that's the only proper fix here.
I incorporated other changes to try to fill in the possible map more accurately,
and with only the above modification to workqueue.c, I ran a hot-add CPU test
to add 8 VPs to a powerpc Shared CPU configuration. It produced a lot of the
warning messages -- a lot more than I was expecting. But it did not crash.
I have attached a compressed copy of the log file, here.
>
> Thanks.
>
Thanks.
--
Michael W. Bringmann
Linux Technology Center
IBM Corporation
Tie-Line 363-5196
External: (512) 286-5196
Cell: (512) 466-0650
[email protected]
boot-run.log.xz
Description: application/xz
