Re: [PATCH] x86/x2apic: introduce a mixed physical/cluster mode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.10.2023 15:51, Roger Pau Monne wrote:
> The current implementation of x2APIC requires to either use Cluster Logical or
> Physical mode for all interrupts.  However the selection of Physical vs 
> Logical
> is not done at APIC setup, an APIC can be addressed both in Physical or 
> Logical
> destination modes concurrently.
> 
> Introduce a new x2APIC mode called mixed, which uses Logical Cluster mode for
> IPIs, and Physical mode for external interrupts, thus attempting to use the
> best method for each interrupt type.
> 
> Using Physical mode for external interrupts allows more vectors to be used, 
> and
> interrupt balancing to be more accurate.
> 
> Using Logical Cluster mode for IPIs allows less accesses to the ICR register
> when sending those, as multiple CPUs can be targeted with a single ICR 
> register
> write.
> 
> A simple test calling flush_tlb_all() 10000 times in a tight loop on a 96 CPU
> box gives the following average figures:
> 
> Physical mode: 26617931ns
> Mixed mode:    23865337ns
> 
> So ~10% improvement versus plain Physical mode.

Nice.

>  Note that Xen uses Cluster
> mode by default, and hence is already using the fastest way for IPI delivery 
> at
> the cost of reducing the amount of vectors available system-wide.
> 
> Make the newly introduced mode the default one.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> Do we want to keep a full Logical Cluster mode available?  I don't see a 
> reason
> to target external interrupts in Logical Cluster mode, but maybe there's
> something I'm missing.
> 
> It's not clear to me whether the ACPI FADT flags are meant to apply only to
> external interrupt delivery mode, or also to IPI delivery.  If
> ACPI_FADT_APIC_PHYSICAL is only meant to apply to external interrupts and not
> IPIs then we could possibly get rid of physical mode IPI delivery.
> 
> Still need to put this under XenServer extensive testing, but wanted to get
> some feedback before in case approach greatly changes.

Looks quite okay, just a couple of minor remarks:

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2802,6 +2802,14 @@ the watchdog.
>  
>  Permit use of x2apic setup for SMP environments.
>  
> +### x2apic-mode (x86)
> +> `= physical | cluster | mixed`
> +
> +> Default: `physical` if **FADT** mandates physical mode, `mixed` otherwise.
> +
> +In the case that x2apic is in use, this option switches between modes to
> +address APICs in the system as interrupt destinations.
> +
>  ### x2apic_phys (x86)
>  > `= <boolean>`
>  
> @@ -2812,6 +2820,9 @@ In the case that x2apic is in use, this option switches 
> between physical and
>  clustered mode.  The default, given no hint from the **FADT**, is cluster
>  mode.
>  
> +**WARNING: `x2apic_phys` is deprecated and superseded by `x2apic-mode`.
> +The later takes precedence if both are set.**

s/later/latter/ ?

This may further want a CHANGELOG.md entry.

> --- a/xen/arch/x86/Kconfig
> +++ b/xen/arch/x86/Kconfig
> @@ -228,11 +228,18 @@ config XEN_ALIGN_2M
>  
>  endchoice
>  
> -config X2APIC_PHYSICAL
> -     bool "x2APIC Physical Destination mode"
> -     help
> -       Use x2APIC Physical Destination mode by default when available.
> +choice
> +     prompt "x2APIC Destination mode"
> +     default X2APIC_MIXED
> +     ---help---

No new ---help--- please (also below); it ought to be just help going forward.

> +       Select APIC addressing when x2APIC is enabled.
>  
> +       The default mode is mixed which should provide the best aspects
> +       of both physical and cluster modes.
> +
> +config X2APIC_PHYSICAL
> +       tristate "Physical Destination mode"
> +     ---help---

Something's odd with indentation here. But first of all - why tristate? We
don't have modules in Xen.

> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -180,6 +180,25 @@ static const struct genapic __initconstrel 
> apic_x2apic_cluster = {
>      .send_IPI_self = send_IPI_self_x2apic
>  };
>  
> +/*
> + * Mixed x2APIC mode: use physical for external (device) interrupts, and
> + * cluster for inter processor interrupt.  Such mode has the benefits of not
> + * sharing the vector space with all CPUs on the cluster, while still allows
> + * IPIs to be more efficiently delivered by not having to perform an ICR
> + * write for each target CPU.
> + */
> +static const struct genapic __initconstrel apic_x2apic_mixed = {
> +    APIC_INIT("x2apic_mixed", NULL),
> +    /* NB: int_{delivery,dest}_mode are only used by non-IPI callers. */
> +    .int_delivery_mode = dest_Fixed,
> +    .int_dest_mode = 0 /* physical delivery */,
> +    .init_apic_ldr = init_apic_ldr_x2apic_cluster,
> +    .vector_allocation_cpumask = vector_allocation_cpumask_phys,
> +    .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,

You have a non-IPI-only comment further up, but that - if in fact 
applicable here - would need to extend to these two hook functions as
well.

Jan