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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 03/11/2023 2:45 pm, Roger Pau Monne wrote:
> The current implementation of x2APIC requires to either use Cluster Logical or

I'd suggest starting with "Xen's current ..." to make it clear that this
is our logic, not a property of x2APIC itself.

> 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

s/less/fewer/

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

96 looks suspiciously like an Intel number.  In nothing else, you ought
to say which CPU is it, because microarchitecture matters.  By any
chance can we try this on one of the Bergamos, to give us a datapoint at
512?

As far as the stats go, it would be helpful to give a number for Cluster
mode too, because if it's different from Mixed mode (in this test), then
it shows an error in our reasoning.

> Make the newly introduced mode the default one.
>
> Note the printing of the APIC addressing mode done in connect_bsp_APIC() has
> been removed, as with the newly introduced mixed mode this would require more
> fine grained printing, or else would be incorrect.  The addressing mode can
> already be derived from the APIC driver in use, which is printed by different
> helpers.
>
> Suggested-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thankyou for doing this.  It's far less invasive than I was fearing.

One real bug (which Gitlab will block on), one other suspected bug that
probably nothing will notice. Minor notes otherwise.

> diff --git a/CHANGELOG.md b/CHANGELOG.md
> index b184dde8b15f..80deba5d2550 100644
> --- a/CHANGELOG.md
> +++ b/CHANGELOG.md
> @@ -9,6 +9,8 @@ The format is based on [Keep a 
> Changelog](https://keepachangelog.com/en/1.0.0/)
>  ### Changed
>  
>  ### Added
> + - On x86 introduce a new x2APIC driver that uses Cluster Logical addressing
> +   mode for IPIs and Physical addressing mode for external interrupts.

Could I request that you copy the structure of 4.18 post-reformat, in
the hope that I don't have to repeat that patch for 4.19.

Something like

 - On x86:
   - Introduce a new ...

> diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
> index eac77573bd75..cd9286f295e5 100644
> --- 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"
> +choice
> +     prompt "x2APIC Destination mode"

"x2APIC Driver default" is going to be more meaningful to a non-expert
reading this menu entry, IMO.

> diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
> index 707deef98c27..875952adc42d 100644
> --- a/xen/arch/x86/genapic/x2apic.c
> +++ b/xen/arch/x86/genapic/x2apic.c
> @@ -180,6 +180,34 @@ 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 interrupts.  Such mode has the benefits of not
> + * sharing the vector space with all CPUs on the cluster, while still 
> allowing
> + * 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),

Newline here please.

> +    /*
> +     * The following fields are exclusively used by external interrupts and
> +     * hence are set to use Physical destination mode handlers.
> +     */
> +    .int_delivery_mode = dest_Fixed,
> +    .int_dest_mode = 0 /* physical delivery */,
> +    .vector_allocation_cpumask = vector_allocation_cpumask_phys,
> +    .cpu_mask_to_apicid = cpu_mask_to_apicid_phys,

And here.

> +    /*
> +     * The following fields are exclusively used by IPIs and hence are set to
> +     * use Cluster Logical destination mode handlers.  Note that 
> init_apic_ldr
> +     * is not used by IPIs, but the per-CPU fields it initializes are only 
> used
> +     * by the IPI hooks.
> +     */
> +    .init_apic_ldr = init_apic_ldr_x2apic_cluster,
> +    .send_IPI_mask = send_IPI_mask_x2apic_cluster,
> +    .send_IPI_self = send_IPI_self_x2apic

Trailing comma please.

> +};
> +
>  static int cf_check update_clusterinfo(
>      struct notifier_block *nfb, unsigned long action, void *hcpu)
>  {
> @@ -220,38 +248,56 @@ static struct notifier_block x2apic_cpu_nfb = {
>  static int8_t __initdata x2apic_phys = -1;
>  boolean_param("x2apic_phys", x2apic_phys);
>  
> +enum {
> +   unset, physical, cluster, mixed
> +} static __initdata x2apic_mode = unset;
> +
> +static int __init parse_x2apic_mode(const char *s)

cf_check

> +{
> +    if ( !cmdline_strcmp(s, "physical") )
> +        x2apic_mode = physical;
> +    else if ( !cmdline_strcmp(s, "cluster") )
> +        x2apic_mode = cluster;
> +    else if ( !cmdline_strcmp(s, "mixed") )
> +        x2apic_mode = mixed;
> +    else
> +        return EINVAL;

-EINVAL ?

> +
> +    return 0;
> +}
> +custom_param("x2apic-mode", parse_x2apic_mode);
> +
>  const struct genapic *__init apic_x2apic_probe(void)
>  {
> -    if ( x2apic_phys < 0 )
> +    /* x2apic-mode option has preference over x2apic_phys. */
> +    if ( x2apic_phys >= 0 && x2apic_mode == unset )
> +        x2apic_mode = x2apic_phys ? physical : cluster;

I know this is just a rearrangement, but IMO it's clearer to follow as:

/* Honour the legacy cmdline setting if it's the only one provided. */
if (  x2apic_mode == unset && x2apic_phys >= 0 )
    ...

There are too many x2apic's in the first comment to read what's going on
at a glance.

~Andrew