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

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

On Tue, Jun 11, 2024 at 09:42:39AM +0200, Jan Beulich wrote:
> 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.

Right.  What about replacing the paragraph with:

"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 corner case makes using get_cpu_maps() alone
not enough to prevent using the shorthand in CPU hotplug regions."

I think the rest is of the commit message is not controversial.

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

I've assumed the wording in the commet that says: "no CPU hotplug or
unplug operations are taking place" would already cover the addition
of the !cpu_in_hotplug_context() check.

Thanks, Roger.