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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Oct 30, 2023 at 03:32:56PM +0100, Jan Beulich wrote:
> 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:

Thanks.

Do we still want to keep the pure cluster mode?

We do need to keep the pure Physical mode in case the FADT flags force
us into using it, but there's no flag to force the usage of Logical
destination mode only.

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

Yes, indeed.

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

In this case we should replace existing instances of ---help---
otherwise I copy from one of those and forgot to adjust.

Kconfig is usually done by copy&paste (at least for me), and hence
having proper format everywhere will make that less error prone.

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

Ah, yes, got confused.

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

Yes, all fields except for send_IPI_{mask,self} only apply to external
interrupts, not IPIs. And init_apic_ldr is kind of weird because it
just inits the data related fields in order to do the cluster
delivery, but doesn't clobber any data used by physical mode anyway.

Thanks, Roger.