On 2021/1/13 19:10, Peter Zijlstra wrote:
On Tue, Jan 12, 2021 at 11:38:12PM +0800, Lai Jiangshan wrote:
But the hard problem is "how to suppress the warning of
online&!active in __set_cpus_allowed_ptr()" for late spawned
unbound workers during hotplug.
I cannot see create_worker() go bad like that.
The thing is, it uses:
kthread_bind_mask(, pool->attr->cpumask)
worker_attach_to_pool()
set_cpus_allowed_ptr(, pool->attr->cpumask)
which means set_cpus_allowed_ptr() must be a NOP, because the affinity
is already set by kthread_bind_mask(). Further, the first wakeup of that
worker will then hit:
select_task_rq()
is_cpu_allowed()
is_per_cpu_kthread() -- false
select_fallback_rq()
So normally that really isn't a problem. I can only see a tiny hole
there, where someone changes the cpumask between kthread_bind_mask() and
set_cpus_allowed_ptr(). AFAICT that can be fixed in two ways:
- add wq_pool_mutex around things in create_worker(), or
- move the set_cpus_allowed_ptr() out of worker_attach_to_pool() and
into rescuer_thread().
Which then brings us to rescuer_thread... If we manage to trigger the
rescuer during hotplug, then yes, I think that can go wobbly.
How about the following idea (not complied, not tested).
It does not call set_cpus_allowed_ptr() for just created workers.
It does not change cpumask for rescuer except when it is per cpu pool.
The only problem is that, unbound rescue worker doesn't comply with
wq_unbound_cpumask nor wq->unbound_attrs->cpumask. Another 50 Lines
of code can make it complied, but I don't want to type it in email
and complicated the idea.
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 9880b6c0e272..df2082283c1e 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1849,10 +1849,30 @@ 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.
+ * If we called from create_worker(), we don't need to call
+ * set_cpus_allowed_ptr() since we just kthread_bind_mask() it.
+ *
+ * The only other path gets us here is rescuer_thread().
+ *
+ * When !(pool->flags & POOL_DISASSOCIATED), it is per-cpu pool
+ * and we should rebind the rescuer worker to the target CPU.
+ *
+ * When it is a rescuer worker attaching to unbound pool, we keep
+ * the affinity for rescuer worker to be cpu_possible_mask.
+ *
+ * Note: unbound rescue worker doesn't comply with wq_unbound_cpumask
+ * nor wq->unbound_attrs->cpumask. The optimal choice is to keep
+ * the affinity for rescuer worker to be
+ * wq_unbound_cpumask & wq->unbound_attrs->cpumask
+ * but there is no reliable way to set it back via
+ * set_cpus_allowed_ptr() when its affinity is changed by scheduler
+ * due to CPU hotplug, so we just use cpu_possible_mask for resuer.
+ *
+ * set_cpus_allowed_ptr() will not fail since
+ * !(pool->flags & POOL_DISASSOCIATED)
*/
- set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask);
+ if (worker->rescue_wq && !(pool->flags & POOL_DISASSOCIATED))
+ WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task,
pool->attrs->cpumask) < 0);
/*
* The wq_pool_attach_mutex ensures %POOL_DISASSOCIATED remains
@@ -5043,7 +5063,8 @@ static void restore_unbound_workers_cpumask(struct
worker_pool *pool, int cpu)
/* 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);
+ if (!worker->rescue_wq)
+ WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, &cpumask)
< 0);
}
int workqueue_prepare_cpu(unsigned int cpu)
