Re: [RFC PATCH] workqueue: handle CPU hotplug without updating worker pool's attrs

[Lai Jiangshan]

On Wed, Dec 9, 2020 at 4:34 PM Hillf Danton <hdanton@xxxxxxxx> wrote:
>
> This makes the cpumask intact for worker pools of unbound workqueues
> when CPUs go offline because we need to rebind workers to the original
> cpumask(of the original pool) when they come back, as represented by
> the cpu check in restore_unbound_workers_cpumask().
>
> Note this is now only for comments.
>
> Signed-off-by: Hillf Danton <hdanton@xxxxxxxx>
> ---
>
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -4117,6 +4117,32 @@ static void wq_update_unbound_numa(struc
>         copy_workqueue_attrs(target_attrs, wq->unbound_attrs);
>         pwq = unbound_pwq_by_node(wq, node);
>
> +       cpumask = pwq->pool->attrs->cpumask;
> +       if (!online || !cpumask_test_cpu(cpu, cpumask))
> +               return;
> +       do {
> +               struct worker *worker;
> +               int i;
> +
> +               for_each_cpu_and(i, cpu_online_mask, cpumask) {
> +                       if (i != cpu)
> +                               return;
> +               }
> +
> +               /*
> +                * rebind workers only when the first CPU in
> +                * pool's attrs cpumask comes back because scheduler
> +                * may have unbound them when the last CPU went offline
> +                */
> +               mutex_lock(&wq_pool_attach_mutex);
> +
> +               for_each_pool_worker(worker, pwq->pool)
> +                       set_cpus_allowed_ptr(worker->task, cpumask);
> +

There might be multiple pwqs that share the same pool, this line of
code might update the same pool multiple times.

Please keep the logic in restore_unbound_workers_cpumask().

> +               mutex_unlock(&wq_pool_attach_mutex);
> +               return;
> +       } while (0);
> +
>         /*
>          * Let's determine what needs to be done.  If the target cpumask is
>          * different from the default pwq's, we need to compare it to @pwq's
> @@ -5004,34 +5030,6 @@ static void rebind_workers(struct worker
>         raw_spin_unlock_irq(&pool->lock);
>  }
>
> -/**
> - * restore_unbound_workers_cpumask - restore cpumask of unbound workers
> - * @pool: unbound pool of interest
> - * @cpu: the CPU which is coming up
> - *
> - * An unbound pool may end up with a cpumask which doesn't have any online
> - * CPUs.  When a worker of such pool get scheduled, the scheduler resets
> - * its cpus_allowed.  If @cpu is in @pool's cpumask which didn't have any
> - * online CPU before, cpus_allowed of all its workers should be restored.
> - */
> -static void restore_unbound_workers_cpumask(struct worker_pool *pool, int cpu)
> -{
> -       static cpumask_t cpumask;
> -       struct worker *worker;
> -
> -       lockdep_assert_held(&wq_pool_attach_mutex);
> -
> -       /* is @cpu allowed for @pool? */
> -       if (!cpumask_test_cpu(cpu, pool->attrs->cpumask))
> -               return;
> -
> -       cpumask_and(&cpumask, pool->attrs->cpumask, cpu_online_mask);

Good catch of the problem.
But please fix it where the problem is found (here!)
Like this:

+        /* only restore the cpumask of the pool's workers when @cpu is
+           the first online cpu in @pool's cpumask */
+        if (cpumask_weight(cpumask) > 1)
+                return;

        /* as we're called from CPU_ONLINE, the following shouldn't fail */
        for_each_pool_worker(worker, pool)
-                WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, &cpumask) < 0);
+                WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task,
pool->attrs->cpumask) < 0);

> -
> -       /* as we're called from CPU_ONLINE, the following shouldn't fail */
> -       for_each_pool_worker(worker, pool)
> -               WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, &cpumask) < 0);
> -}
> -
>  int workqueue_prepare_cpu(unsigned int cpu)
>  {
>         struct worker_pool *pool;
> @@ -5058,8 +5056,6 @@ int workqueue_online_cpu(unsigned int cp
>
>                 if (pool->cpu == cpu)
>                         rebind_workers(pool);
> -               else if (pool->cpu < 0)
> -                       restore_unbound_workers_cpumask(pool, cpu);
>
>                 mutex_unlock(&wq_pool_attach_mutex);
>         }