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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan