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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, Nov 03, 2023 at 03:10:18PM +0000, Andrew Cooper wrote:
> 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?

Let me see if I can grab the only one that's not broken.

Those figures are from an Intel IceLake IIRC.  Cluster mode is the
default, so this change shouldn't effect the performance of builds
that use the default settings.

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

I will leave the helps as-is.

> > +};
> > +
> >  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

I'm probably confused, but other users of custom_param() do have the
cf_check attribute, see parse_spec_ctrl() for example.

Thanks, Roger.