Hello, Michael.
It would have been better to continue debugging in the prev thread.
This still seems incorrect for the same reason as before.
On Tue, Jun 06, 2017 at 09:09:40AM -0500, Michael Bringmann wrote:
> On NUMA systems with dynamic processors, the content of the cpumask
> may change over time. As new processors are added via DLPAR operations,
> workqueues are created for them. Depending upon the order in which CPUs
> are added/removed, we may run into problems with the content of the
> cpumask used by the workqueues. This patch deals with situations where
> the online cpumask for a node is a proper superset of possible cpumask
> for the node. It also deals with edge cases where the order in which
> CPUs are removed/added from the online cpumask may leave the set for a
> node empty, and require execution by CPUs on another node.
>
> In these and other cases, the patch attempts to ensure that a valid,
> usable cpumask is used to set up newly created pools for workqueues.
>
> Signed-off-by: Tejun Heo <t...@kernel.org> & Michael Bringmann
> <m...@linux.vnet.ibm.com>
Heh, you can't add sob's for other people. For partial attributions,
you can just note in the description.
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index c74bf39..460de61 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3366,6 +3366,9 @@ static struct worker_pool *get_unbound_pool(const
> struct workqueue_attrs *attrs)
> copy_workqueue_attrs(pool->attrs, attrs);
> pool->node = target_node;
>
> + if (!cpumask_weight(pool->attrs->cpumask))
> + cpumask_copy(pool->attrs->cpumask,
> cpumask_of(smp_processor_id()));
So, this is still wrong.
> /*
> * no_numa isn't a worker_pool attribute, always clear it. See
> * 'struct workqueue_attrs' comments for detail.
> @@ -3559,13 +3562,13 @@ static struct pool_workqueue
> *alloc_unbound_pwq(struct workqueue_struct *wq,
> * stable.
> *
> * Return: %true if the resulting @cpumask is different from @attrs->cpumask,
> - * %false if equal.
> + * %false if equal. On %false return, the content of @cpumask is undefined.
> */
> 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;
> + return false;
>
> /* does @node have any online CPUs @attrs wants? */
> cpumask_and(cpumask, cpumask_of_node(node), attrs->cpumask);
> @@ -3573,15 +3576,13 @@ static bool wq_calc_node_cpumask(const struct
> workqueue_attrs *attrs, int node,
> cpumask_clear_cpu(cpu_going_down, cpumask);
>
> if (cpumask_empty(cpumask))
> - goto use_dfl;
> + return false;
>
> /* yeap, return possible CPUs in @node that @attrs wants */
> 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;
> + return !cpumask_empty(cpumask) &&
> + !cpumask_equal(cpumask, attrs->cpumask);
And this part doesn't really change that.
CPUs going offline or online shouldn't change their relation to
wq_numa_possible_cpumask. I wonder whether the arch code is changing
CPU id <-> NUMA node mapping on CPU on/offlining. x86 used to do that
too and got recently modified. Can you see whether that's the case?
Thanks.
--
tejun
