[RFC PATCH] Use vAPIC when doing IPI for PVHVM guests.

[RFC PATCH] Use vAPIC when doing IPI for PVHVM guests.

[Konrad Rzeszutek Wilk]

Hey,

I was running some tools in which we would heavily do rescheduling
of events - and realized to my surprise that the event channels (and
the hypercall) would slow things down. If I used the vAPIC with its
IPI support (so no VMEXIT) I got much much better performance.

Now this is an RFC because:
 1). I hadn't verified from the xentrace  how much less VMEXITS we get.
    But I remember Boris's patches and they gave at least 10%.
    I think this will get the same performance or even better.

 2). I don't know what to do with migration. That is if the guest
    migrates to older hardware it needs to recheck this I presume?

 3). Should this be enabled by default? I did get better performance
   but that was synthetic.

Thoughts?


 Documentation/kernel-parameters.txt | 15 +++++------
 arch/x86/xen/enlighten.c            | 50 +++++++++++++++++++++++++++++++++----
 arch/x86/xen/smp.c                  | 22 ++++++++++++++--
 arch/x86/xen/spinlock.c             | 18 ++++---------
 arch/x86/xen/xen-ops.h              |  4 +++
 5 files changed, 82 insertions(+), 27 deletions(-)

Konrad Rzeszutek Wilk (2):
      xen/apic: Use vAPIC for IPI if the hardware supports it.
      xen_nopv: Combine a bunch of the PV features that can be disabled

[RFC PATCH v1 1/2] xen/apic: Use vAPIC for IPI if the hardware supports it.

[Konrad Rzeszutek Wilk]

Instead of using the event channels.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 arch/x86/xen/smp.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/smp.c b/arch/x86/xen/smp.c
index 3f4ebf0..d4f4560 100644
--- a/arch/x86/xen/smp.c
+++ b/arch/x86/xen/smp.c
@@ -793,15 +793,33 @@ static int xen_hvm_cpu_up(unsigned int cpu, struct task_struct *tidle)
 	return rc;
 }
 
+static bool __init xen_use_vapic(void)
+{
+	uint32_t eax = cpuid_eax(xen_cpuid_base() + 4);
+
+	if (((eax & XEN_HVM_CPUID_X2APIC_VIRT) && x2apic_mode) ||
+	    ((eax & XEN_HVM_CPUID_APIC_ACCESS_VIRT) && cpu_has_apic))
+		return true;
+
+	return false;
+}
 void __init xen_hvm_smp_init(void)
 {
+	bool vapic;
+
 	if (!xen_have_vector_callback)
 		return;
+
+	vapic = xen_use_vapic();
 	smp_ops.smp_prepare_cpus = xen_hvm_smp_prepare_cpus;
-	smp_ops.smp_send_reschedule = xen_smp_send_reschedule;
 	smp_ops.cpu_up = xen_hvm_cpu_up;
 	smp_ops.cpu_die = xen_cpu_die;
+	smp_ops.smp_prepare_boot_cpu = xen_smp_prepare_boot_cpu;
+
+	printk("Using %s IPI mode\n", vapic ? "vAPIC" : "event channel");
+	if (vapic)
+		return;
+	smp_ops.smp_send_reschedule = xen_smp_send_reschedule;
 	smp_ops.send_call_func_ipi = xen_smp_send_call_function_ipi;
 	smp_ops.send_call_func_single_ipi = xen_smp_send_call_function_single_ipi;
-	smp_ops.smp_prepare_boot_cpu = xen_smp_prepare_boot_cpu;
 }
-- 
2.4.3

Re: [RFC PATCH v1 1/2] xen/apic: Use vAPIC for IPI if the hardware supports it.

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1382 bytes --]

Hi Konrad,

[auto build test ERROR on v4.3-rc4 -- if it's inappropriate base, please ignore]

config: x86_64-randconfig-x016-201540 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   arch/x86/xen/smp.c: In function 'xen_use_vapic':
>> arch/x86/xen/smp.c:800:14: error: 'XEN_HVM_CPUID_X2APIC_VIRT' undeclared (first use in this function)
     if (((eax & XEN_HVM_CPUID_X2APIC_VIRT) && x2apic_mode) ||
                 ^
   arch/x86/xen/smp.c:800:14: note: each undeclared identifier is reported only once for each function it appears in
>> arch/x86/xen/smp.c:801:14: error: 'XEN_HVM_CPUID_APIC_ACCESS_VIRT' undeclared (first use in this function)
         ((eax & XEN_HVM_CPUID_APIC_ACCESS_VIRT) && cpu_has_apic))
                 ^

vim +/XEN_HVM_CPUID_X2APIC_VIRT +800 arch/x86/xen/smp.c

   794	}
   795	
   796	static bool __init xen_use_vapic(void)
   797	{
   798		uint32_t eax = cpuid_eax(xen_cpuid_base() + 4);
   799	
 > 800		if (((eax & XEN_HVM_CPUID_X2APIC_VIRT) && x2apic_mode) ||
 > 801		    ((eax & XEN_HVM_CPUID_APIC_ACCESS_VIRT) && cpu_has_apic))
   802			return true;
   803	
   804		return false;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25228 bytes --]

[RFC PATCH v1 2/2] xen_nopv: Combine a bunch of the PV features that can be disabled

[Konrad Rzeszutek Wilk]

under one parameter. Removes the xen_nopvspin parameter and
makes it part of the xen_nopv.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 Documentation/kernel-parameters.txt | 15 +++++------
 arch/x86/xen/enlighten.c            | 50 +++++++++++++++++++++++++++++++++----
 arch/x86/xen/spinlock.c             | 18 ++++---------
 arch/x86/xen/xen-ops.h              |  4 +++
 4 files changed, 62 insertions(+), 25 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 22a4b68..73cd745 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -4125,13 +4125,14 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 				the unplug protocol
 			never -- do not unplug even if version check succeeds
 
-	xen_nopvspin	[X86,XEN]
-			Disables the ticketlock slowpath using Xen PV
-			optimizations.
-
-	xen_nopv	[X86]
-			Disables the PV optimizations forcing the HVM guest to
-			run as generic HVM guest with no PV drivers.
+	xen_nopv=	[X86,XEN]
+			Disables various (or all) PV optimizations forcing the
+			HVM (or PV) guest to run without them.
+			Format: [off0,][off]
+			all -- every PV feature on HVM.
+			spin -- Disables the ticketlock slowpath using Xen PV
+                                optimizations (PV and HVM).
+			ipi - Disable PV event channel IPI (on HVM).
 
 	xirc2ps_cs=	[NET,PCMCIA]
 			Format:
diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
index 30d12af..c644d2c 100644
--- a/arch/x86/xen/enlighten.c
+++ b/arch/x86/xen/enlighten.c
@@ -1829,17 +1829,57 @@ static void __init xen_hvm_guest_init(void)
 }
 #endif
 
-static bool xen_nopv = false;
+static unsigned int xen_nopv_feat;
+#define XEN_NOPV_SPIN	1<<1
+#define XEN_NOPV_IPI	1<<2
+#define XEN_NOPV_ALL	(XEN_NOPV_SPIN | XEN_NOPV_IPI)
+
+bool xen_no_pvspin(void)
+{
+	return xen_nopv_feat & XEN_NOPV_SPIN;
+}
+
+bool xen_no_pv(void)
+{
+	return xen_nopv_feat & XEN_NOPV_ALL;
+}
+
+bool xen_no_pvipi(void)
+{
+	return xen_nopv_feat & XEN_NOPV_IPI;
+}
+
 static __init int xen_parse_nopv(char *arg)
 {
-       xen_nopv = true;
-       return 0;
+	char *p, *q;
+	int l;
+
+	for (p = arg; p; p = q) {
+		q = strchr(p, ',');
+		if (q) {
+			l = q - p;
+			q++;
+		} else {
+			l = strlen(p);
+		}
+		if (!strncmp(p, "spin", l))
+			xen_nopv_feat |= XEN_NOPV_SPIN;
+		else if (!strncmp(p, "all", l))
+			xen_nopv_feat = XEN_NOPV_ALL;
+		else if (!strncmp(p, "ipi", l))
+			xen_nopv_feat |= XEN_NOPV_IPI;
+		else
+			printk(KERN_WARNING "unrecognised option '%s' "
+				 "in parameter 'xen_nopv'\n", p);
+	}
+	printk(KERN_DEBUG "xen_nopv_feat = 0x%x\n", xen_nopv_feat);
+	return 0;
 }
 early_param("xen_nopv", xen_parse_nopv);
 
 static uint32_t __init xen_platform(void)
 {
-	if (xen_nopv)
+	if (xen_no_pv())
 		return 0;
 
 	return xen_cpuid_base();
@@ -1847,7 +1887,7 @@ static uint32_t __init xen_platform(void)
 
 bool xen_hvm_need_lapic(void)
 {
-	if (xen_nopv)
+	if (xen_no_pv())
 		return false;
 	if (xen_pv_domain())
 		return false;
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 9e2ba5c..31b91e9 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -19,7 +19,6 @@
 
 static DEFINE_PER_CPU(int, lock_kicker_irq) = -1;
 static DEFINE_PER_CPU(char *, irq_name);
-static bool xen_pvspin = true;
 
 #ifdef CONFIG_QUEUED_SPINLOCKS
 
@@ -277,7 +276,7 @@ void xen_init_lock_cpu(int cpu)
 	int irq;
 	char *name;
 
-	if (!xen_pvspin)
+	if (!xen_no_pvspin())
 		return;
 
 	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
@@ -302,7 +301,7 @@ void xen_init_lock_cpu(int cpu)
 
 void xen_uninit_lock_cpu(int cpu)
 {
-	if (!xen_pvspin)
+	if (!xen_no_pvspin())
 		return;
 
 	unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
@@ -323,7 +322,7 @@ void xen_uninit_lock_cpu(int cpu)
 void __init xen_init_spinlocks(void)
 {
 
-	if (!xen_pvspin) {
+	if (!xen_no_pvspin()) {
 		printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
 		return;
 	}
@@ -348,7 +347,7 @@ void __init xen_init_spinlocks(void)
  */
 static __init int xen_init_spinlocks_jump(void)
 {
-	if (!xen_pvspin)
+	if (!xen_no_pvspin())
 		return 0;
 
 	if (!xen_domain())
@@ -359,13 +358,6 @@ static __init int xen_init_spinlocks_jump(void)
 }
 early_initcall(xen_init_spinlocks_jump);
 
-static __init int xen_parse_nopvspin(char *arg)
-{
-	xen_pvspin = false;
-	return 0;
-}
-early_param("xen_nopvspin", xen_parse_nopvspin);
-
 #if defined(CONFIG_XEN_DEBUG_FS) && !defined(CONFIG_QUEUED_SPINLOCKS)
 
 static struct dentry *d_spin_debug;
@@ -377,7 +369,7 @@ static int __init xen_spinlock_debugfs(void)
 	if (d_xen == NULL)
 		return -ENOMEM;
 
-	if (!xen_pvspin)
+	if (xen_no_pvspin())
 		return 0;
 
 	d_spin_debug = debugfs_create_dir("spinlocks", d_xen);
diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h
index 1399423..3652d38 100644
--- a/arch/x86/xen/xen-ops.h
+++ b/arch/x86/xen/xen-ops.h
@@ -149,4 +149,8 @@ __visible void xen_adjust_exception_frame(void);
 extern int xen_panic_handler_init(void);
 
 void xen_pvh_secondary_vcpu_init(int cpu);
+
+bool xen_no_pvspin(void);
+bool xen_no_pvipi(void);
+bool xen_no_pv(void);
 #endif /* XEN_OPS_H */
-- 
2.4.3

Re: [RFC PATCH v1 2/2] xen_nopv: Combine a bunch of the PV features that can be disabled

[Boris Ostrovsky]

On 10/07/2015 04:21 PM, Konrad Rzeszutek Wilk wrote:
> under one parameter. Removes the xen_nopvspin parameter and
> makes it part of the xen_nopv.
>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>   Documentation/kernel-parameters.txt | 15 +++++------
>   arch/x86/xen/enlighten.c            | 50 +++++++++++++++++++++++++++++++++----
>   arch/x86/xen/spinlock.c             | 18 ++++---------
>   arch/x86/xen/xen-ops.h              |  4 +++
>   4 files changed, 62 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 22a4b68..73cd745 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -4125,13 +4125,14 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>   				the unplug protocol
>   			never -- do not unplug even if version check succeeds
>   
> -	xen_nopvspin	[X86,XEN]
> -			Disables the ticketlock slowpath using Xen PV
> -			optimizations.
> -
> -	xen_nopv	[X86]
> -			Disables the PV optimizations forcing the HVM guest to
> -			run as generic HVM guest with no PV drivers.
> +	xen_nopv=	[X86,XEN]
> +			Disables various (or all) PV optimizations forcing the
> +			HVM (or PV) guest to run without them.
> +			Format: [off0,][off]

{ [spin][,ipi] | all }


> +			all -- every PV feature on HVM.
> +			spin -- Disables the ticketlock slowpath using Xen PV
> +                                optimizations (PV and HVM).
> +			ipi - Disable PV event channel IPI (on HVM).
>   
>   	xirc2ps_cs=	[NET,PCMCIA]
>   			Format:
> diff --git a/arch/x86/xen/enlighten.c b/arch/x86/xen/enlighten.c
> index 30d12af..c644d2c 100644
> --- a/arch/x86/xen/enlighten.c
> +++ b/arch/x86/xen/enlighten.c
> @@ -1829,17 +1829,57 @@ static void __init xen_hvm_guest_init(void)
>   }
>   #endif
>   
> -static bool xen_nopv = false;
> +static unsigned int xen_nopv_feat;
> +#define XEN_NOPV_SPIN	1<<1
> +#define XEN_NOPV_IPI	1<<2
> +#define XEN_NOPV_ALL	(XEN_NOPV_SPIN | XEN_NOPV_IPI)
> +
> +bool xen_no_pvspin(void)
> +{
> +	return xen_nopv_feat & XEN_NOPV_SPIN;
> +}
> +
> +bool xen_no_pv(void)
> +{
> +	return xen_nopv_feat & XEN_NOPV_ALL;
> +}

Should XEN_NOPV_ALL be the 'OR' of the other two? I think it implies 
them but is broader than that. We may decide to have NOPV_IPI and 
NOPV_SPIN but still run a Xen-aware guest (i.e. we want to return 'true' 
from xen_platform()).

(Besides, this will return true if only one of the two bits is set.)

> +
> +bool xen_no_pvipi(void)
> +{
> +	return xen_nopv_feat & XEN_NOPV_IPI;
> +}

I don't see anyone using this.

Also, I think these should be inlines.


-boris

Re: [RFC PATCH] Use vAPIC when doing IPI for PVHVM guests.

[Juergen Gross]

On 10/07/2015 10:21 PM, Konrad Rzeszutek Wilk wrote:
> Hey,
>
> I was running some tools in which we would heavily do rescheduling
> of events - and realized to my surprise that the event channels (and
> the hypercall) would slow things down. If I used the vAPIC with its
> IPI support (so no VMEXIT) I got much much better performance.
>
> Now this is an RFC because:
>   1). I hadn't verified from the xentrace  how much less VMEXITS we get.
>      But I remember Boris's patches and they gave at least 10%.
>      I think this will get the same performance or even better.
>
>   2). I don't know what to do with migration. That is if the guest
>      migrates to older hardware it needs to recheck this I presume?

Same problem applies to many other features. In case you want to
migrate to a machine with less features you'd have to mask those
features in the cpuid data of the domain.

>   3). Should this be enabled by default? I did get better performance
>     but that was synthetic.

Having some benchmark results would help to decide this. :-)

I'd be especially interested in checking "no vcpu over-commitment"
and "heavy vcpu over-commitment" scenarios regarding the effect of
the feature.

>
> Thoughts?

I like the idea.


Juergen

Re: [Xen-devel] [RFC PATCH] Use vAPIC when doing IPI for PVHVM guests.

[Andrew Cooper]

On 08/10/15 06:05, Juergen Gross wrote:
> On 10/07/2015 10:21 PM, Konrad Rzeszutek Wilk wrote:
>> Hey,
>>
>> I was running some tools in which we would heavily do rescheduling
>> of events - and realized to my surprise that the event channels (and
>> the hypercall) would slow things down. If I used the vAPIC with its
>> IPI support (so no VMEXIT) I got much much better performance.
>>
>> Now this is an RFC because:
>>   1). I hadn't verified from the xentrace  how much less VMEXITS we get.
>>      But I remember Boris's patches and they gave at least 10%.
>>      I think this will get the same performance or even better.
>>
>>   2). I don't know what to do with migration. That is if the guest
>>      migrates to older hardware it needs to recheck this I presume?
>
> Same problem applies to many other features. In case you want to
> migrate to a machine with less features you'd have to mask those
> features in the cpuid data of the domain.

Those leaves in particular are from the HV set rather than the plain
featureset.  One way or another there will be an APIC to use, but those
features are expected to appear/disappear across migrate to indicate
whether hardware assistance is in use or not.

Therefore, they should be resampled and re-acted-upon in the resume path.

~Andrew