Re: [PATCH -tip V3 0/8] workqueue: break affinity initiatively

[Lai Jiangshan]

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)