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

Reply via email to