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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 29, 2024 at 03:04:13PM +0200, Jan Beulich wrote:
> 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.

This is mostly a pre-req for the next change that switches
get_cpu_maps() to return false if the current CPU is holding the CPU
maps lock in write mode.

IF we don't want to go this route we need a way to signal
send_IPI_mask() when a CPU hot{,un}plug operation is taking place,
because get_cpu_maps() enough is not suitable.

Overall I don't like the corner case where get_cpu_maps() returns true
if a CPU hot{,un}plug operation is taking place in the current CPU
context.  The guarantee of get_cpu_maps() is that no CPU hot{,un}plug
operations can be in progress if it returns true.

Thanks, Roger.