Re: [PATCH v2 1/7] x86/smp: do not use shorthand IPI destinations in CPU hot{,un}plug contexts

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.06.2024 16:20, Roger Pau Monne wrote:
> Due to the current rwlock logic, if the CPU calling get_cpu_maps() does so 
> from
> a cpu_hotplug_{begin,done}() region the function will still return success,
> because a CPU taking the rwlock in read mode after having taken it in write
> mode is allowed.  Such behavior however defeats the purpose of get_cpu_maps(),

I'm not happy to see you still have this claim here. The behavior may (appear
to) defeat the purpose here, but as expressed previously I don't view that as
being a general pattern.

> as it should always return false when called with a CPU hot{,un}plug operation
> is in progress.  Otherwise the logic in send_IPI_mask() is wrong, as it could
> decide to use the shorthand even when a CPU operation is in progress.
> 
> Introduce a new helper to detect whether the current caller is between a
> cpu_hotplug_{begin,done}() region and use it in send_IPI_mask() to restrict
> shorthand usage.
> 
> Fixes: 5500d265a2a8 ('x86/smp: use APIC ALLBUT destination shorthand when 
> possible')
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Changes since v1:
>  - Modify send_IPI_mask() to detect CPU hotplug context.
> ---
>  xen/arch/x86/smp.c       |  2 +-
>  xen/common/cpu.c         |  5 +++++
>  xen/include/xen/cpu.h    | 10 ++++++++++
>  xen/include/xen/rwlock.h |  2 ++
>  4 files changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/smp.c b/xen/arch/x86/smp.c
> index 7443ad20335e..04c6a0572319 100644
> --- a/xen/arch/x86/smp.c
> +++ b/xen/arch/x86/smp.c
> @@ -88,7 +88,7 @@ void send_IPI_mask(const cpumask_t *mask, int vector)
>       * the system have been accounted for.
>       */
>      if ( system_state > SYS_STATE_smp_boot &&
> -         !unaccounted_cpus && !disabled_cpus &&
> +         !unaccounted_cpus && !disabled_cpus && !cpu_in_hotplug_context() &&
>           /* NB: get_cpu_maps lock requires enabled interrupts. */
>           local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
>           (park_offline_cpus ||

Along with changing the condition you also want to update the comment to
reflect the code adjustment.

If we can agree on respective wording in both places, I'd be happy to make
respective adjustments while committing; the code changes themselves are
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan