From: Lai Jiangshan <jiangshanlai@gmail.com>
To: Hillf Danton <hdanton@sina.com>
Cc: LKML <linux-kernel@vger.kernel.org>,
Valentin Schneider <valentin.schneider@arm.com>,
Peter Zijlstra <peterz@infradead.org>, Qian Cai <cai@redhat.com>,
Vincent Donnefort <vincent.donnefort@arm.com>,
Dexuan Cui <decui@microsoft.com>,
Lai Jiangshan <laijs@linux.alibaba.com>,
Tejun Heo <tj@kernel.org>,
Daniel Bristot de Oliveira <bristot@redhat.com>
Subject: Re: [PATCH -tip V3 5/8] workqueue: Manually break affinity on hotplug for unbound pool
Date: Sun, 27 Dec 2020 22:04:03 +0800 [thread overview]
Message-ID: <CAJhGHyDM89Kq_Dop-6c8_6B4K545MHMJDxGggpTmjxu4Wuz7zQ@mail.gmail.com> (raw)
In-Reply-To: <20201226101631.5448-1-hdanton@sina.com>
On Sat, Dec 26, 2020 at 6:16 PM Hillf Danton <hdanton@sina.com> wrote:
>
> Sat, 26 Dec 2020 10:51:13 +0800
> > From: Lai Jiangshan <laijs@linux.alibaba.com>
> >
> > There is possible that a per-node pool/woker's affinity is a single
> > CPU. It can happen when the workqueue user changes the cpumask of the
> > workqueue or when wq_unbound_cpumask is changed by system adim via
> > /sys/devices/virtual/workqueue/cpumask. And pool->attrs->cpumask
> > is workqueue's cpumask & wq_unbound_cpumask & possible_cpumask_of_the_node,
> > which can be a single CPU and makes the pool's workers to be "per cpu
> > kthread".
> >
> > And it can also happen when the cpu is the first online and has been
> > the only online cpu in pool->attrs->cpumask. In this case, the worker
> > task cpumask is single cpu no matter what pool->attrs->cpumask since
> > commit d945b5e9f0e3 ("workqueue: Fix setting affinity of unbound worker
> > threads").
> >
> > And the scheduler won't break affinity on the "per cpu kthread" workers
> > when the CPU is going down, so we have to do it by our own.
> >
> > We do it by reusing existing restore_unbound_workers_cpumask() and rename
> > it to update_unbound_workers_cpumask(). When the number of the online
> > CPU of the pool goes from 1 to 0, we break the affinity initiatively.
> >
> > Note here, we even break the affinity for non-per-cpu-kthread workers,
> > because first, the code path is slow path which is not worth too much to
> > optimize, second, we don't need to rely on the code/conditions when the
> > scheduler forces breaking affinity for us.
> >
> > The way to break affinity is to set the workers' affinity to
> > cpu_possible_mask, so that we preserve the same behavisor when
> > the scheduler breaks affinity for us.
> >
> > Fixes: 06249738a41a ("workqueue: Manually break affinity on hotplug")
> > Acked-by: Tejun Heo <tj@kernel.org>
> > Signed-off-by: Lai Jiangshan <laijs@linux.alibaba.com>
> > ---
> > kernel/workqueue.c | 48 ++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> > index 0a95ae14d46f..79cc87df0cda 100644
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -5019,16 +5019,18 @@ static void rebind_workers(struct worker_pool *pool)
> > }
> >
> > /**
> > - * restore_unbound_workers_cpumask - restore cpumask of unbound workers
> > + * update_unbound_workers_cpumask - update cpumask of unbound workers
> > * @pool: unbound pool of interest
> > - * @cpu: the CPU which is coming up
> > + * @online: whether @cpu is coming up or going down
> > + * @cpu: the CPU which is coming up or going down
> > *
> > * An unbound pool may end up with a cpumask which doesn't have any online
> > - * CPUs. When a worker of such pool get scheduled, the scheduler resets
> > - * its cpus_allowed. If @cpu is in @pool's cpumask which didn't have any
> > - * online CPU before, cpus_allowed of all its workers should be restored.
> > + * CPUs. We have to reset workers' cpus_allowed of such pool. And we
> > + * restore the workers' cpus_allowed when the pool's cpumask has online
> > + * CPU.
> > */
> > -static void restore_unbound_workers_cpumask(struct worker_pool *pool, int cpu)
> > +static void update_unbound_workers_cpumask(struct worker_pool *pool,
> > + bool online, int cpu)
> > {
> > static cpumask_t cpumask;
> > struct worker *worker;
> > @@ -5042,6 +5044,23 @@ static void restore_unbound_workers_cpumask(struct worker_pool *pool, int cpu)
> >
> > cpumask_and(&cpumask, pool->attrs->cpumask, wq_online_cpumask);
> >
> > + if (!online) {
> > + if (cpumask_weight(&cpumask) > 0)
> > + return;
>
> We can apply the weight check also to the online case.
>
> > + /*
> > + * All unbound workers can be possibly "per cpu kthread"
> > + * if this is the only online CPU in pool->attrs->cpumask
> > + * from the last time it has been brought up until now.
> > + * And the scheduler won't break affinity on the "per cpu
> > + * kthread" workers when the CPU is going down, so we have
> > + * to do it by our own.
> > + */
> > + for_each_pool_worker(worker, pool)
> > + WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, cpu_possible_mask) < 0);
> > +
> > + return;
> > + }
> > +
> > /* 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);
>
> What is the reason that pool->attrs->cpumask is not restored if it is
> not a typo, given that restore appears in the change to the above doc?
reason:
d945b5e9f0e3 ("workqueue: Fix setting affinity of unbound worker
threads").
I don't like this change either, but I don't want to touch it
now. I will improve it late by moving handling for unbound wq/pool/worker
to a work item (out of cpu hotplug processing) and so that
we can restore pool->attrs->cpumask to workers.
The reason is also the reason I drop the patch1 of the V2 patch.
Did you see any problem with d945b5e9f0e3 except for that it does not
update the comment and it is not so efficient.
>
> BTW is there a git tree available with this patchset tucked in?
>
> > @@ -5075,7 +5094,7 @@ int workqueue_online_cpu(unsigned int cpu)
> > if (pool->cpu == cpu)
> > rebind_workers(pool);
> > else if (pool->cpu < 0)
> > - restore_unbound_workers_cpumask(pool, cpu);
> > + update_unbound_workers_cpumask(pool, true, cpu);
> >
> > mutex_unlock(&wq_pool_attach_mutex);
> > }
> > @@ -5090,7 +5109,9 @@ int workqueue_online_cpu(unsigned int cpu)
> >
> > int workqueue_offline_cpu(unsigned int cpu)
> > {
> > + struct worker_pool *pool;
> > struct workqueue_struct *wq;
> > + int pi;
> >
> > /* unbinding per-cpu workers should happen on the local CPU */
> > if (WARN_ON(cpu != smp_processor_id()))
> > @@ -5098,9 +5119,20 @@ int workqueue_offline_cpu(unsigned int cpu)
> >
> > unbind_workers(cpu);
> >
> > - /* update NUMA affinity of unbound workqueues */
> > mutex_lock(&wq_pool_mutex);
> > cpumask_clear_cpu(cpu, wq_online_cpumask);
> > +
> > + /* update CPU affinity of workers of unbound pools */
> > + for_each_pool(pool, pi) {
> > + mutex_lock(&wq_pool_attach_mutex);
> > +
> > + if (pool->cpu < 0)
> > + update_unbound_workers_cpumask(pool, false, cpu);
> > +
> > + mutex_unlock(&wq_pool_attach_mutex);
> > + }
> > +
> > + /* update NUMA affinity of unbound workqueues */
> > list_for_each_entry(wq, &workqueues, list)
> > wq_update_unbound_numa(wq, cpu);
> > mutex_unlock(&wq_pool_mutex);
> > --
> > 2.19.1.6.gb485710b
next prev parent reply other threads:[~2020-12-27 14:05 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 [this message]
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
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=CAJhGHyDM89Kq_Dop-6c8_6B4K545MHMJDxGggpTmjxu4Wuz7zQ@mail.gmail.com \
--to=jiangshanlai@gmail.com \
--cc=bristot@redhat.com \
--cc=cai@redhat.com \
--cc=decui@microsoft.com \
--cc=hdanton@sina.com \
--cc=laijs@linux.alibaba.com \
--cc=linux-kernel@vger.kernel.org \
--cc=peterz@infradead.org \
--cc=tj@kernel.org \
--cc=valentin.schneider@arm.com \
--cc=vincent.donnefort@arm.com \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).