Re: [Xen-devel] [PATCH] x86/smp: use APIC ALLBUT destination shorthand when possible

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.01.2020 16:05, Roger Pau Monné  wrote:
> On Thu, Jan 16, 2020 at 11:44:51AM +0100, Jan Beulich wrote:
>> Thinking about it, what about the period of time between a CPU having
>> got physically hot-added (and hence known at the hardware level) and
>> it actually getting brought up for the first time? IOW - do you
>> perhaps need to exclude use of the shortcut also when disabled_cpus
>> is non-zero?
> 
> Yes, I guess there's no signal to Xen that a new CPU is going to be
> physically hot-added, and hence as long as there are empty slots the
> scenario you describe above is possible.
> 
> I don't think there's also anyway to figure out if a system is capable
> of physical CPU hotplug, so as long as there are disabled_cpus we
> shouldn't use the shortcut (that's kind of a shame, because I think
> there are many systems reporting disabled CPUs in MADT).

We may want to provide a command line control to assert "I'm not going
to".

>>> @@ -30,7 +56,40 @@
>>>  
>>>  void send_IPI_mask(const cpumask_t *mask, int vector)
>>>  {
>>> -    alternative_vcall(genapic.send_IPI_mask, mask, vector);
>>> +    bool cpus_locked = false;
>>> +
>>> +    /*
>>> +     * Prevent any CPU hot{un}plug while sending the IPIs if we are to use
>>> +     * a shorthand, also refuse to use a shorthand if not all CPUs are
>>> +     * online or have been parked.
>>> +     */
>>> +    if ( system_state > SYS_STATE_smp_boot && !cpu_overflow &&
>>> +         /* NB: get_cpu_maps lock requires enabled interrupts. */
>>> +         local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
>>> +         (park_offline_cpus ||
>>> +          cpumask_equal(&cpu_online_map, &cpu_present_map)) )
>>
>> On the first and second pass I thought this contradicts the description.
>> To disambiguate (and assuming I understand it correctly), how about
>> saying something like "This can only be safely used when no CPU hotplug
>> or unplug operations are taking place, there are no offline CPUs (unless
>> those have been onlined and parked) and finally ..."?
> 
> I'm not sure I understand what should come after the finally ...

That was to continue with what you had in your text.

>>> +    else
>>> +    {
>>> +        if ( cpus_locked )
>>> +        {
>>> +            put_cpu_maps();
>>> +            cpus_locked = false;
>>> +        }
>>> +        cpumask_clear(this_cpu(scratch_cpumask));
>>
>> Is there a guarantee that the function won't be called with an
>> empty mask? All backing functions look to gracefully deal with
>> this case, yet ...
>>
>>> +    }
>>> +
>>> +    if ( cpumask_equal(mask, this_cpu(scratch_cpumask)) )> +        
>>> __default_send_IPI_shortcut(APIC_DEST_ALLBUT, vector,
>>> +                                    APIC_DEST_PHYSICAL);
>>
>> ... you'd make this an "all-but" message then. Adding a
>> !cpumask_empty() check would seem a little expensive, so how
>> about you copy cpumask_of(smp_processor_id()) above and add
>> !cpumask_test_cpu(smp_processor_id(), ...) here?
> 
> A cpumask_empty check at the top of the function would be easier to
> parse, but it could incur in more overhead, I've implemented what you
> describe.
> 
>>> +    else
>>> +        alternative_vcall(genapic.send_IPI_mask, mask, vector);
>>
>> Is there no reason at all to also check here whether APIC_DEST_ALL
>> could be used? Oh, I see - the backing functions all exclude the
>> local CPU. I wonder why e.g. flush_area_mask() then clears the
>> CPU off the mask it passes on. And with this behavior the
>> single cpumask_equal() check above is too restrictive - you'll
>> want to check whether mask matches the calculated all-but one or
>> cpu_online_map. I.e. perhaps you want
>>
>>         cpumask_or(this_cpu(scratch_cpumask), mask,
>>                    cpumask_of(smp_processor_id()));
>>
>> and then
>>
>>     if ( cpumask_equal(this_cpu(scratch_cpumask), &cpu_online_map) )
>>
>> ?
> 
> Oh, OK, in which case most of the comments above are moot if we go
> this route. What I have now if I properly parsed your comments is:
> 
>     bool cpus_locked = false;
>     cpumask_t *scratch = this_cpu(scratch_cpumask);
> 
>     /*
>      * This can only be safely used when no CPU hotplug or unplug operations
>      * are taking place, there are no offline CPUs (unless those have been
>      * onlined and parked), there are no disabled CPUs and all possible CPUs 
> in
>      * the system have been accounted for.
>      */
>     if ( system_state > SYS_STATE_smp_boot &&
>          !unaccounted_cpus && !disabled_cpus &&
>          /* NB: get_cpu_maps lock requires enabled interrupts. */
>          local_irq_is_enabled() && (cpus_locked = get_cpu_maps()) &&
>          (park_offline_cpus ||
>           cpumask_equal(&cpu_online_map, &cpu_present_map)) )
>         cpumask_or(scratch, mask, cpumask_of(smp_processor_id()));
>     else
>     {
>         if ( cpus_locked )
>         {
>             put_cpu_maps();
>             cpus_locked = false;
>         }
>         cpumask_clear(scratch);
>     }
> 
>     if ( cpumask_equal(scratch, &cpu_online_map) )
>         send_IPI_shortcut(APIC_DEST_ALLBUT, vector, APIC_DEST_PHYSICAL);
>     else
>         alternative_vcall(genapic.send_IPI_mask, mask, vector);
> 
>     if ( cpus_locked )
>         put_cpu_maps();
> 
> Can assert this matches your expectations? I think it fixes your
> comments about empty masks and a mask containing the current pCPU ID.

Looks like so.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel