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

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-06-12 at 10:56 +0200, Jan Beulich wrote:
> On 12.06.2024 10:09, Roger Pau Monné wrote:
> > 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."
> 
> Thanks.
> 
> > I think the rest is of the commit message is not controversial.
> 
> Indeed.
> 
> > > > --- 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.
> 
> Hmm, yes, you're right. Just needs a release-ack then to go in (with
> the
> description adjustment).
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

~ Oleksii