From: Lai Jiangshan <jiangshanlai@gmail.com>
To: Peter Zijlstra <peterz@infradead.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>,
Lai Jiangshan <laijs@linux.alibaba.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:00:40 +0800 [thread overview]
Message-ID: <CAJhGHyDZMGtHztzZSfBjXzhZfqo07HUTXXDxV3JSyM0+vOwqdA@mail.gmail.com> (raw)
In-Reply-To: <X/7VQ8pF5h/K+Cj1@hirez.programming.kicks-ass.net>
On Wed, Jan 13, 2021 at 7:11 PM Peter Zijlstra <peterz@infradead.org> 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.
Oh, I forgot set_cpus_allowed_ptr() is NOP when combined with
kthread_bind_mask()(create_worker()).
So the problem becomes "how to suppress the warning of online&!active in
__set_cpus_allowed_ptr()" for late *attached unbound rescuer* workers
during hotplug.
>
> Let me consider that a bit more while I try and make sense of that splat
> Paul reported.
>
> ---
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index ec0771e4a3fb..fe05308dc472 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -1844,15 +1844,19 @@ static struct worker *alloc_worker(int node)
> * cpu-[un]hotplugs.
> */
> static void worker_attach_to_pool(struct worker *worker,
> - struct worker_pool *pool)
> + struct worker_pool *pool,
> + bool set_affinity)
> {
> 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 (set_affinity) {
> + /*
> + * 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);
> + }
>
> /*
> * The wq_pool_attach_mutex ensures %POOL_DISASSOCIATED remains
> @@ -1944,7 +1948,7 @@ static struct worker *create_worker(struct worker_pool *pool)
> kthread_bind_mask(worker->task, pool->attrs->cpumask);
>
> /* successful, attach the worker to the pool */
> - worker_attach_to_pool(worker, pool);
> + worker_attach_to_pool(worker, pool, false);
>
> /* start the newly created worker */
> raw_spin_lock_irq(&pool->lock);
> @@ -2509,7 +2513,11 @@ static int rescuer_thread(void *__rescuer)
>
> raw_spin_unlock_irq(&wq_mayday_lock);
>
> - worker_attach_to_pool(rescuer, pool);
> + /*
> + * XXX can go splat when running during hot-un-plug and
> + * the pool affinity is wobbly.
> + */
> + worker_attach_to_pool(rescuer, pool, true);
>
> raw_spin_lock_irq(&pool->lock);
>
next prev parent reply other threads:[~2021-01-13 12:01 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 [this message]
2021-01-13 12:57 ` Lai Jiangshan
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=CAJhGHyDZMGtHztzZSfBjXzhZfqo07HUTXXDxV3JSyM0+vOwqdA@mail.gmail.com \
--to=jiangshanlai@gmail.com \
--cc=axboe@kernel.dk \
--cc=cai@redhat.com \
--cc=decui@microsoft.com \
--cc=laijs@linux.alibaba.com \
--cc=linux-kernel@vger.kernel.org \
--cc=paulmck@kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
--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.