| Message ID | 20240529090132.59434-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/irq: fixes for CPU hot{,un}plug | expand |
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); >
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.
On 29.05.2024 17:20, Roger Pau Monné wrote: > 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. I'm not convinced of looking at it this way. To me the guarantee is merely that no CPU operation is taking place _elsewhere_. As indicated, imo the local CPU should be well aware of what context it's actually in, and hence what is (or is not) appropriate to do at a particular point in time. I guess what I'm missing is an example of a concrete code path where things presently go wrong. Jan
On Wed, May 29, 2024 at 05:31:02PM +0200, Jan Beulich wrote: > On 29.05.2024 17:20, Roger Pau Monné wrote: > > 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. > > I'm not convinced of looking at it this way. To me the guarantee is > merely that no CPU operation is taking place _elsewhere_. As indicated, > imo the local CPU should be well aware of what context it's actually in, > and hence what is (or is not) appropriate to do at a particular point in > time. > > I guess what I'm missing is an example of a concrete code path where > things presently go wrong. See the specific example in patch 3/9 with time_calibration() and it's usage of send_IPI_mask() when called from a CPU executing in cpu_up() context. Thanks, Roger.
diff --git a/xen/common/cpu.c b/xen/common/cpu.c index 8709db4d2957..6173220e771b 100644 --- a/xen/common/cpu.c +++ b/xen/common/cpu.c @@ -68,6 +68,11 @@ void cpu_hotplug_done(void) write_unlock(&cpu_add_remove_lock); } +bool cpu_map_locked(void) +{ + return rw_is_locked(&cpu_add_remove_lock); +} + static NOTIFIER_HEAD(cpu_chain); void __init register_cpu_notifier(struct notifier_block *nb) diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c index 398cfd507c10..7face75648e8 100644 --- 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. + */ + if ( system_state >= SYS_STATE_active && !cpu_map_locked() ) + { + 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);
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. Replace the get_cpu_maps() call with a suitable unreachable assert. Further changes will modify the conditions under which get_cpu_maps() returns success and without the adjustment proposed here the usage of stop_machine_run() in cpu_down() would then return an error. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/common/cpu.c | 5 +++++ xen/common/stop_machine.c | 15 ++++++++------- xen/include/xen/cpu.h | 2 ++ 3 files changed, 15 insertions(+), 7 deletions(-)