Re: [PATCH] workqueue: keep unbound rescuer's cpumask to be default cpumask

2021-01-19 Thread Tejun Heo
On Sat, Jan 16, 2021 at 02:57:53PM +0800, Lai Jiangshan wrote:
> From: Lai Jiangshan 
> 
> When we attach a rescuer to a pool, we will set the rescuer's cpumask
> to the pool's cpumask.  If there is hotplug ongoing, it may cause
> the rescuer running on the dying CPU and cause bug or it may cause
> warning of setting online&!active cpumask.
> 
> So we have to find a reliable way to set cpumask when attaching
> rescuer.
> 
> When the pool is percpu pool, we have easy way to reliably
> set cpumask with the help of %POOL_DISASSOCIATED.
> 
> But when it is unbound rescuer, the problem becomes harder, because
> we can't neither use get_online_cpus() here nor test cpu_active_mask
> without synchronization.
> 
> Atfer investigation, and noticing the unbound nature of the unbound
> rescuer, we decide to make it use the wq's default pwq's cpumask so
> that we don't need to set the rescuer's cpumask when attaching.
> 
> To implement it, we have to set unbound rescuer's cpumask to the
> default pwq's cpumask when creation and maintain it when hotplug
> or the default pwq is changed.

Yeah, this approach makes sense to me. It doesn't look like all problems are
resolved but for the rescuer behavior part, please feel free to add

Acked-by: Tejun Heo 

Thanks.

-- 
tejun


[PATCH] workqueue: keep unbound rescuer's cpumask to be default cpumask

2021-01-15 Thread Lai Jiangshan
From: Lai Jiangshan 

When we attach a rescuer to a pool, we will set the rescuer's cpumask
to the pool's cpumask.  If there is hotplug ongoing, it may cause
the rescuer running on the dying CPU and cause bug or it may cause
warning of setting online&!active cpumask.

So we have to find a reliable way to set cpumask when attaching
rescuer.

When the pool is percpu pool, we have easy way to reliably
set cpumask with the help of %POOL_DISASSOCIATED.

But when it is unbound rescuer, the problem becomes harder, because
we can't neither use get_online_cpus() here nor test cpu_active_mask
without synchronization.

Atfer investigation, and noticing the unbound nature of the unbound
rescuer, we decide to make it use the wq's default pwq's cpumask so
that we don't need to set the rescuer's cpumask when attaching.

To implement it, we have to set unbound rescuer's cpumask to the
default pwq's cpumask when creation and maintain it when hotplug
or the default pwq is changed.

Signed-off-by: Lai Jiangshan 
---
NOTE:
this patch is designed to be a complement of Peter's
patchset https://lore.kernel.org/lkml/20210112144344.850850...@infradead.org/
where it has a problem dealing with rescuer.

 kernel/workqueue.c | 101 +
 1 file changed, 92 insertions(+), 9 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9880b6c0e272..901abab945d4 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1849,16 +1849,37 @@ static void worker_attach_to_pool(struct worker *worker,
mutex_lock(&wq_pool_attach_mutex);
 
/*
-* set_cpus_allowed_ptr() will fail if the cpumask doesn't have any
-* online CPUs.  It'll be re-applied when any of the CPUs come up.
-*/
-   set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
-
-   /*
+* If we called from create_worker(), we don't need to call
+* set_cpus_allowed_ptr() since we just call kthread_bind_mask()
+* on it with proper cpumask.
+*
+* The only other path gets us here is rescuer_thread().  And we
+* only call set_cpus_allowed_ptr() to rescuer for percpu pool.
+*
+* For bound percpu pool, the task's mask is set to the pool's
+* mask.  For unbound percpu pool whose cpu is offline, the task's
+* mask is set to cpu_possible_mask instead of sticking to the
+* previous served percpu pool's mask.
+*
+* Unboud wq's rescuer's cpumask is kept as the same as the wq's
+* default pwq's cpumask and maintained when cpu hotplug and
+* the update of the wq's default pwq.
+*
+* set_cpus_allowed_ptr() will not fail here since the pool's CPU
+* is online for !POOL_DISASSOCIATED and set_cpus_allowed_ptr() can
+* not fail on cpu_possible_mask.
+*
 * The wq_pool_attach_mutex ensures %POOL_DISASSOCIATED remains
 * stable across this function.  See the comments above the flag
 * definition for details.
 */
+   if (worker->rescue_wq && pool->cpu >= 0) {
+   if (!(pool->flags & POOL_DISASSOCIATED))
+   WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, 
pool->attrs->cpumask) < 0);
+   else
+   WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, 
cpu_possible_mask) < 0);
+   }
+
if (pool->flags & POOL_DISASSOCIATED)
worker->flags |= WORKER_UNBOUND;
 
@@ -4003,6 +4024,25 @@ static void apply_wqattrs_commit(struct 
apply_wqattrs_ctx *ctx)
link_pwq(ctx->dfl_pwq);
swap(ctx->wq->dfl_pwq, ctx->dfl_pwq);
 
+   if (ctx->wq->rescuer) {
+   struct task_struct *task = ctx->wq->rescuer->task;
+   struct cpumask *dfl_mask = ctx->dfl_pwq->pool->attrs->cpumask;
+
+   /*
+* set the rescuer's cpumask to default pwq's cpumask.
+*
+* There might be no CPUs online in the default pwq's
+* cpumask and set_cpus_allowed_ptr() will fail on this
+* cpumask, so we use cpu_possible_mask instead and
+* restore_unbound_rescuer_cpumask() will restore for us
+* when the CPUs are back.
+*/
+   if (cpumask_intersects(dfl_mask, cpu_online_mask))
+   WARN_ON_ONCE(set_cpus_allowed_ptr(task, dfl_mask) < 0);
+   else
+   WARN_ON_ONCE(set_cpus_allowed_ptr(task, 
cpu_possible_mask) < 0);
+   }
+
mutex_unlock(&ctx->wq->mutex);
 }
 
@@ -4241,7 +4281,14 @@ static int init_rescuer(struct workqueue_struct *wq)
}
 
wq->rescuer = rescuer;
-   kthread_bind_mask(rescuer->task, cpu_possible_mask);
+   if (wq->flags & WQ_UNBOUND) {
+   /* grab wq->mutex for accessing wq->dfl_pwq */
+   mutex_lock(&wq->mutex);
+   kthread_bind_mask(rescuer->task, 
wq->dfl_p