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

[Peter Zijlstra]

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.

> >
> > 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'.

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?