Re: [PATCH -tip V3 3/8] workqueue: introduce wq_online_cpumask

[Lai Jiangshan]

On Tue, Jan 5, 2021 at 9:17 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Tue, Jan 05, 2021 at 04:23:44PM +0800, Lai Jiangshan wrote:
> > On Tue, Jan 5, 2021 at 10:41 AM Lai Jiangshan <jiangshanlai@xxxxxxxxx> wrote:
> > > On Mon, Jan 4, 2021 at 9:56 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > > > On Sat, Dec 26, 2020 at 10:51:11AM +0800, Lai Jiangshan wrote:
> > > > > From: Lai Jiangshan <laijs@xxxxxxxxxxxxxxxxx>
> > > > >
> > > > > wq_online_cpumask is the cached result of cpu_online_mask with the
> > > > > going-down cpu cleared.
> > > >
> > > > You can't use cpu_active_mask ?
> > >
> > > When a cpu is going out:
> > > (cpu_active_mask is not protected by workqueue mutexs.)
>
> But it is protected by the hotplug lock, which is really all you need
> afaict.
>
> If the worker thread gets spawned before workqueue_offline_cpu(), said
> function will observe it and adjust the mask, if it gets spawned after
> it, it must observe a 'reduced' cpu_active_mask.

Making the workqueue set workers' cpumask correctly is easy.
The hard part is how to suppress the warning.

It is true that said function will observe it and adjust the mask,
but the warning is already issued.

>
> > >
> > > create_worker() for unbound pool  |  cpu offlining
> > > check cpu_active_mask             |
> > check wq_online_cpumask
> > >                                   |  remove bit from cpu_active_mask
> > >                                   |  no cpu in pool->attrs->cpumask is active
> > > set pool->attrs->cpumask to worker|
> > > and hit the warning
> >                                     |  remove bit from wq_online_cpumask
> >
> > Even with the help of wq_online_cpumask, the patchset can't silence
> > the warning in __set_cpus_allowed_ptr() in this case.  It is indeed
> > hard to suppress the warning for unbound pools.  Maybe we need something
> > like this (outmost callback of CPUHP_AP_WORKQUEUE_UNBOUND_ONLINE,
> > so that workqueue can do preparation when offlining before AP_ACTIVE):
> >
> > diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> > index 0042ef362511..ac2103deb20b 100644
> > --- a/include/linux/cpuhotplug.h
> > +++ b/include/linux/cpuhotplug.h
> > @@ -20,6 +20,9 @@
> >   *               |                               ^
> >   *               v                               |
> >   *              AP_ACTIVE                      AP_ACTIVE
> > + *               |                               ^
> > + *               v                               |
> > + *              ONLINE                         ONLINE
> >   */
> >
> >  enum cpuhp_state {
> > @@ -194,6 +197,7 @@ enum cpuhp_state {
> >         CPUHP_AP_X86_HPET_ONLINE,
> >         CPUHP_AP_X86_KVM_CLK_ONLINE,
> >         CPUHP_AP_ACTIVE,
> > +       CPUHP_AP_WORKQUEUE_UNBOUND_ONLINE,
> >         CPUHP_ONLINE,
> >  };
> >
>
> That's waay to late, by then userspace is long running and expecting
> things to 'just-work'.

I don't like this way either, I just list three ways I can think of.
I prefer the way that __set_cpus_allowed_ptr() doesn't warn
for kworkers.

>
> But afaict, things will mostly work for you when you use cpu_active_mask
> on cpu-down and cpu_online_mask on cpu-up.
>
> But I think I see the problem, it is spawning a new worker after
> workqueue_online_cpu() but before sched_cpu_activate(), right? That
> wants to have the wider mask set.
>
> To solve that, the spawning of workers thing needs to know where we are
> in the hotplug process, and it can track that using
> workqueue_{on,off}line_cpu(). If it happens after offline, it needs to
> use cpu_active_mask, if it happens after online cpu_online_mask is your
> guy.
>
> Does that make sense?

There are six stages we need to know when spawning a worker:

stageA ap_deactive stageB workqueue_offline stageC
stageD workqueue_online stageE ap_active stageF

I don't think create_worker()/worker_attach_to_pool() can know where
it is in the hotplug process unless it uses get_online_cpus() so that
it knows it is not in the hotplug process.  There is no way to maintain
needed information since there are no workqueue callbacks in the proper
stages in the hotplug process.

Again, making the workqueue set workers' cpumask correctly is easy.
But we can't distinguish stageA&B or stageE&F to suppress the warning
in __set_cpus_allowed_ptr() for new unbound workers when pool->attr->cpumask
has only one cpu online&!active since there is no way to keep
cpu_active_mask stable except get_online_cpus().