From: Lai Jiangshan <laijs@linux.alibaba.com>
To: Peter Zijlstra <peterz@infradead.org>,
Lai Jiangshan <jiangshanlai@gmail.com>, Tejun Heo <tj@kernel.org>
Cc: Valentin Schneider <valentin.schneider@arm.com>,
Thomas Gleixner <tglx@linutronix.de>,
LKML <linux-kernel@vger.kernel.org>, Qian Cai <cai@redhat.com>,
Vincent Donnefort <vincent.donnefort@arm.com>,
Dexuan Cui <decui@microsoft.com>,
Paul McKenney <paulmck@kernel.org>,
Vincent Guittot <vincent.guittot@linaro.org>,
Steven Rostedt <rostedt@goodmis.org>,
Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH -tip V3 0/8] workqueue: break affinity initiatively
Date: Wed, 13 Jan 2021 20:57:56 +0800 [thread overview]
Message-ID: <7e92d3b2-2323-f608-1090-e2c91aa612ce@linux.alibaba.com> (raw)
In-Reply-To: <X/7VQ8pF5h/K+Cj1@hirez.programming.kicks-ass.net>
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)
next prev parent reply other threads:[~2021-01-13 13:03 UTC|newest]
Thread overview: 43+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-26 2:51 [PATCH -tip V3 0/8] workqueue: break affinity initiatively Lai Jiangshan
2020-12-26 2:51 ` [PATCH -tip V3 1/8] workqueue: use cpu_possible_mask instead of cpu_active_mask to break affinity Lai Jiangshan
2020-12-26 2:51 ` [PATCH -tip V3 2/8] workqueue: Manually break affinity on pool detachment Lai Jiangshan
2020-12-26 2:51 ` [PATCH -tip V3 3/8] workqueue: introduce wq_online_cpumask Lai Jiangshan
2021-01-04 13:56 ` Peter Zijlstra
2021-01-05 2:41 ` Lai Jiangshan
2021-01-05 2:53 ` Lai Jiangshan
2021-01-05 8:23 ` Lai Jiangshan
2021-01-05 13:17 ` Peter Zijlstra
2021-01-05 14:37 ` Lai Jiangshan
2021-01-05 14:40 ` Lai Jiangshan
2021-01-05 16:24 ` Peter Zijlstra
2020-12-26 2:51 ` [PATCH -tip V3 4/8] workqueue: use wq_online_cpumask in restore_unbound_workers_cpumask() Lai Jiangshan
2020-12-26 2:51 ` [PATCH -tip V3 5/8] workqueue: Manually break affinity on hotplug for unbound pool Lai Jiangshan
[not found] ` <20201226101631.5448-1-hdanton@sina.com>
2020-12-27 14:04 ` Lai Jiangshan
2020-12-26 2:51 ` [PATCH -tip V3 6/8] workqueue: reorganize workqueue_online_cpu() Lai Jiangshan
2020-12-26 2:51 ` [PATCH -tip V3 7/8] workqueue: reorganize workqueue_offline_cpu() unbind_workers() Lai Jiangshan
2020-12-26 2:51 ` [PATCH -tip V3 8/8] workqueue: Fix affinity of kworkers when attaching into pool Lai Jiangshan
[not found] ` <20201229100639.2086-1-hdanton@sina.com>
2020-12-29 10:13 ` Lai Jiangshan
2021-01-08 11:46 ` [PATCH -tip V3 0/8] workqueue: break affinity initiatively Peter Zijlstra
2021-01-11 10:07 ` Thomas Gleixner
2021-01-11 11:01 ` Peter Zijlstra
2021-01-11 15:00 ` Paul E. McKenney
2021-01-11 17:16 ` Peter Zijlstra
2021-01-11 18:09 ` Paul E. McKenney
2021-01-11 21:50 ` Paul E. McKenney
2021-01-12 17:14 ` Paul E. McKenney
2021-01-12 23:53 ` Paul E. McKenney
2021-01-15 9:11 ` Peter Zijlstra
2021-01-15 13:04 ` Peter Zijlstra
2021-01-16 6:00 ` Lai Jiangshan
2021-01-11 19:21 ` Valentin Schneider
2021-01-11 20:23 ` Peter Zijlstra
2021-01-11 22:47 ` Valentin Schneider
2021-01-12 4:33 ` Lai Jiangshan
2021-01-12 14:53 ` Peter Zijlstra
2021-01-12 15:38 ` Lai Jiangshan
2021-01-13 11:10 ` Peter Zijlstra
2021-01-13 12:00 ` Lai Jiangshan
2021-01-13 12:57 ` Lai Jiangshan [this message]
2021-01-12 17:52 ` Valentin Schneider
2021-01-12 14:57 ` Jens Axboe
2021-01-12 15:51 ` Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7e92d3b2-2323-f608-1090-e2c91aa612ce@linux.alibaba.com \
--to=laijs@linux.alibaba.com \
--cc=axboe@kernel.dk \
--cc=cai@redhat.com \
--cc=decui@microsoft.com \
--cc=jiangshanlai@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--cc=tj@kernel.org \
--cc=valentin.schneider@arm.com \
--cc=vincent.donnefort@arm.com \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.