Re: [PATCH for-4.19 2/9] xen/cpu: do not get the CPU map in stop_machine_run()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.05.2024 11:01, Roger Pau Monne wrote:
> The current callers of stop_machine_run() outside of init code already have 
> the
> CPU maps locked, and hence there's no reason for stop_machine_run() to attempt
> to lock again.

While purely from a description perspective this is okay, ...

> --- a/xen/common/stop_machine.c
> +++ b/xen/common/stop_machine.c
> @@ -82,9 +82,15 @@ int stop_machine_run(int (*fn)(void *data), void *data, 
> unsigned int cpu)
>      BUG_ON(!local_irq_is_enabled());
>      BUG_ON(!is_idle_vcpu(current));
>  
> -    /* cpu_online_map must not change. */
> -    if ( !get_cpu_maps() )
> +    /*
> +     * cpu_online_map must not change.  The only two callers of
> +     * stop_machine_run() outside of init code already have the CPU map 
> locked.
> +     */

... the "two" here is not unlikely to quickly go stale; who knows what PPC
and RISC-V will have as their code becomes more complete?

I'm also unconvinced that requiring ...

> +    if ( system_state >= SYS_STATE_active && !cpu_map_locked() )

... this for all future (post-init) uses of stop_machine_run() is a good
idea. It is quite a bit more natural, to me at least, for the function to
effect this itself, as is the case prior to your change.

In fact I'm puzzled by Arm's (init-time) use: They use it twice, for errata
workaround and feature enabling. They do that after bringing up secondary
CPUs, but still from start_xen(). How's that going to work for CPUs brought
offline and then back online later on? __cpu_up() isn't __init, so there's
no obvious sign that soft-{off,on}lining isn't possible on Arm. Cc-ing Arm
maintainers.

Jan

> +    {
> +        ASSERT_UNREACHABLE();
>          return -EBUSY;
> +    }
>  
>      nr_cpus = num_online_cpus();
>      if ( cpu_online(this) )
> @@ -92,10 +98,7 @@ int stop_machine_run(int (*fn)(void *data), void *data, 
> unsigned int cpu)
>  
>      /* Must not spin here as the holder will expect us to be descheduled. */
>      if ( !spin_trylock(&stopmachine_lock) )
> -    {
> -        put_cpu_maps();
>          return -EBUSY;
> -    }
>  
>      stopmachine_data.fn = fn;
>      stopmachine_data.fn_data = data;
> @@ -136,8 +139,6 @@ int stop_machine_run(int (*fn)(void *data), void *data, 
> unsigned int cpu)
>  
>      spin_unlock(&stopmachine_lock);
>  
> -    put_cpu_maps();
> -
>      return ret;
>  }
>  
> diff --git a/xen/include/xen/cpu.h b/xen/include/xen/cpu.h
> index e1d4eb59675c..d8c8264c58b0 100644
> --- a/xen/include/xen/cpu.h
> +++ b/xen/include/xen/cpu.h
> @@ -13,6 +13,8 @@ void put_cpu_maps(void);
>  void cpu_hotplug_begin(void);
>  void cpu_hotplug_done(void);
>  
> +bool cpu_map_locked(void);
> +
>  /* Receive notification of CPU hotplug events. */
>  void register_cpu_notifier(struct notifier_block *nb);
>