This patch adds the necessary Hyper-V specific code to allow
PV qspinlock work on Hyper-V.
In wait callback function, read HV_X64_MSR_GUEST_IDLE MSR
to trigger the guest's transition to the idle power state
which can be exited by an IPI even if IF flag is disabled.
Beside that, make HVCALL_NOTIFY_LONG_SPIN_WAIT hypervall to
notify the hypervisor that the calling virtual processor is
attempting to acquire a resource that is potentially held
by another virtual processor within the same partition. This
scheduling hint improves the scalability of partitions with
more than one virtual processor.
In kick callback function, just send platform IPI to make
waiting vcpu exit idle state.
In vcpu_is_preempted callback function, return false directly
because Hyper-V does not provide such interface so far.
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Yi Sun (3):
X86/Hyper-V: Add Guest IDLE MSR support
locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
hv: add description for hv_nopvspin
Documentation/admin-guide/kernel-parameters.txt | 5 ++
arch/x86/hyperv/Makefile | 2 +-
arch/x86/hyperv/hv_spinlock.c | 99 +++++++++++++++++++++++++
arch/x86/include/asm/hyperv-tlfs.h | 5 ++
arch/x86/include/asm/mshyperv.h | 3 +
arch/x86/kernel/smpboot.c | 2 +
6 files changed, 115 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/hyperv/hv_spinlock.c
--
1.9.1
Follow PV spinlock mechanism to implement the callback functions
to allow the CPU idling and kicking operations on Hyper-V.
HVCALL_NOTIFY_LONG_SPIN_WAIT is a hypercall used by a guest OS to
notify the hypervisor that the calling virtual processor is attempting
to acquire a resource that is potentially held by another virtual
processor within the same partition. This scheduling hint improves the
scalability of partitions with more than one virtual processor.
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Signed-off-by: Yi Sun <[email protected]>
---
arch/x86/hyperv/Makefile | 2 +-
arch/x86/hyperv/hv_spinlock.c | 99 +++++++++++++++++++++++++++++++++++++++++
arch/x86/include/asm/mshyperv.h | 3 ++
arch/x86/kernel/smpboot.c | 2 +
4 files changed, 105 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/hyperv/hv_spinlock.c
diff --git a/arch/x86/hyperv/Makefile b/arch/x86/hyperv/Makefile
index b21ee65..5b6937c 100644
--- a/arch/x86/hyperv/Makefile
+++ b/arch/x86/hyperv/Makefile
@@ -1,2 +1,2 @@
obj-y := hv_init.o mmu.o nested.o
-obj-$(CONFIG_X86_64) += hv_apic.o
+obj-$(CONFIG_X86_64) += hv_apic.o hv_spinlock.o
diff --git a/arch/x86/hyperv/hv_spinlock.c b/arch/x86/hyperv/hv_spinlock.c
new file mode 100644
index 0000000..134c89f
--- /dev/null
+++ b/arch/x86/hyperv/hv_spinlock.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Hyper-V specific spinlock code.
+ *
+ * Copyright (C) 2018, Intel, Inc.
+ *
+ * Author : Yi Sun <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
+ * NON INFRINGEMENT. See the GNU General Public License for more
+ * details.
+ *
+ */
+
+#include <linux/kernel_stat.h>
+#include <linux/spinlock.h>
+#include <linux/debugfs.h>
+#include <linux/log2.h>
+#include <linux/gfp.h>
+
+#include <asm/mshyperv.h>
+#include <asm/hyperv-tlfs.h>
+#include <asm/paravirt.h>
+#include <asm/qspinlock.h>
+#include <asm/apic.h>
+
+static bool hv_pvspin = true;
+static u32 spin_wait_info = 0;
+
+static void hv_notify_long_spin_wait(void)
+{
+ u64 input = spin_wait_info | 0x00000000ffffffff;
+
+ spin_wait_info++;
+ hv_do_fast_hypercall8(HVCALL_NOTIFY_LONG_SPIN_WAIT, input);
+}
+
+static void hv_qlock_kick(int cpu)
+{
+ spin_wait_info--;
+ apic->send_IPI(cpu, X86_PLATFORM_IPI_VECTOR);
+}
+
+/*
+ * Halt the current CPU & release it back to the host
+ */
+static void hv_qlock_wait(u8 *byte, u8 val)
+{
+ unsigned long msr_val;
+
+ if (READ_ONCE(*byte) != val)
+ return;
+
+ hv_notify_long_spin_wait();
+
+ rdmsrl(HV_X64_MSR_GUEST_IDLE, msr_val);
+}
+
+/*
+ * Hyper-V does not support this so far.
+ */
+bool hv_vcpu_is_preempted(int vcpu)
+{
+ return false;
+}
+PV_CALLEE_SAVE_REGS_THUNK(hv_vcpu_is_preempted);
+
+void __init hv_init_spinlocks(void)
+{
+ if (!hv_pvspin ||
+ !apic ||
+ !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
+ !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
+ pr_warn("hv: PV spinlocks disabled\n");
+ return;
+ }
+ pr_info("hv: PV spinlocks enabled\n");
+
+ __pv_init_lock_hash();
+ pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
+ pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
+ pv_lock_ops.wait = hv_qlock_wait;
+ pv_lock_ops.kick = hv_qlock_kick;
+ pv_lock_ops.vcpu_is_preempted = PV_CALLEE_SAVE(hv_vcpu_is_preempted);
+}
+
+static __init int hv_parse_nopvspin(char *arg)
+{
+ hv_pvspin = false;
+ return 0;
+}
+early_param("hv_nopvspin", hv_parse_nopvspin);
diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
index f377044..ac36ea9 100644
--- a/arch/x86/include/asm/mshyperv.h
+++ b/arch/x86/include/asm/mshyperv.h
@@ -355,6 +355,8 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,
static inline void hv_apic_init(void) {}
#endif
+void __init hv_init_spinlocks(void);
+
#else /* CONFIG_HYPERV */
static inline void hyperv_init(void) {}
static inline bool hv_is_hyperv_initialized(void) { return false; }
@@ -368,6 +370,7 @@ static inline struct hv_vp_assist_page *hv_get_vp_assist_page(unsigned int cpu)
return NULL;
}
static inline int hyperv_flush_guest_mapping(u64 as) { return -1; }
+void __init hv_init_spinlocks(void) {}
#endif /* CONFIG_HYPERV */
#ifdef CONFIG_HYPERV_TSCPAGE
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index f02ecaf..8bf08ba 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -81,6 +81,7 @@
#include <asm/cpu_device_id.h>
#include <asm/spec-ctrl.h>
#include <asm/hw_irq.h>
+#include <asm/mshyperv.h>
/* representing HT siblings of each logical CPU */
DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_map);
@@ -1335,6 +1336,7 @@ void __init native_smp_prepare_boot_cpu(void)
/* already set me in cpu_online_mask in boot_cpu_init() */
cpumask_set_cpu(me, cpu_callout_mask);
cpu_set_state_online(me);
+ hv_init_spinlocks();
}
void __init calculate_max_logical_packages(void)
--
1.9.1
hv_nopvspin is a command line parameter used to
disable Hyper-V PV spinlock.
Cc: Jonathan Corbet <[email protected]>
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Signed-off-by: Yi Sun <[email protected]>
---
Documentation/admin-guide/kernel-parameters.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 64a3bf5..fc761b5 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1385,6 +1385,11 @@
hvc_iucv_allow= [S390] Comma-separated list of z/VM user IDs.
If specified, z/VM IUCV HVC accepts connections
from listed z/VM user IDs only.
+
+ hv_nopvspin [X86,HYPER_V]
+ Disables the ticketlock slowpath using HYPER-V PV
+ optimizations.
+
keep_bootcon [KNL]
Do not unregister boot console at start. This is only
useful for debugging when something happens in the window
--
1.9.1
Hyper-V may expose a HV_X64_MSR_GUEST_IDLE MSR. We can read
HYPERV_CPUID_FEATURES eax to check it. Read this MSR can
trigger the guest's transition to the idle power state which
can be exited by an IPI even if IF flag is disabled.
Cc: "K. Y. Srinivasan" <[email protected]>
Cc: Haiyang Zhang <[email protected]>
Cc: Stephen Hemminger <[email protected]>
Signed-off-by: Yi Sun <[email protected]>
---
arch/x86/include/asm/hyperv-tlfs.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/arch/x86/include/asm/hyperv-tlfs.h b/arch/x86/include/asm/hyperv-tlfs.h
index e977b6b..2a2fa17 100644
--- a/arch/x86/include/asm/hyperv-tlfs.h
+++ b/arch/x86/include/asm/hyperv-tlfs.h
@@ -38,6 +38,8 @@
#define HV_MSR_TIME_REF_COUNT_AVAILABLE (1 << 1)
/* Partition reference TSC MSR is available */
#define HV_MSR_REFERENCE_TSC_AVAILABLE (1 << 9)
+/* Partition Guest IDLE MSR is available */
+#define HV_X64_MSR_GUEST_IDLE_AVAILABLE (1 << 10)
/* A partition's reference time stamp counter (TSC) page */
#define HV_X64_MSR_REFERENCE_TSC 0x40000021
@@ -246,6 +248,9 @@
#define HV_X64_MSR_STIMER3_CONFIG 0x400000B6
#define HV_X64_MSR_STIMER3_COUNT 0x400000B7
+/* Hyper-V guest idle MSR */
+#define HV_X64_MSR_GUEST_IDLE 0x400000F0
+
/* Hyper-V guest crash notification MSR's */
#define HV_X64_MSR_CRASH_P0 0x40000100
#define HV_X64_MSR_CRASH_P1 0x40000101
--
1.9.1
On Thu, 13 Sep 2018, Yi Sun wrote:
> +#include <asm/mshyperv.h>
>
> /* representing HT siblings of each logical CPU */
> DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_map);
> @@ -1335,6 +1336,7 @@ void __init native_smp_prepare_boot_cpu(void)
> /* already set me in cpu_online_mask in boot_cpu_init() */
> cpumask_set_cpu(me, cpu_callout_mask);
> cpu_set_state_online(me);
> + hv_init_spinlocks();
No. We have smp_ops.smp_prepare_boot_cpu for that.
Thanks,
tglx
On Thu, 13 Sep 2018, Yi Sun wrote:
> +++ b/arch/x86/hyperv/hv_spinlock.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * Hyper-V specific spinlock code.
> + *
> + * Copyright (C) 2018, Intel, Inc.
> + *
> + * Author : Yi Sun <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> + * NON INFRINGEMENT. See the GNU General Public License for more
> + * details.
Please remove the boilerplate. The SPDX identifier is enough. If in doubt
talk to Intel legal.
> +static bool hv_pvspin = true;
__initdata please.
> +static u32 spin_wait_info = 0;
No need for 0 initialization.
> +
> +static void hv_notify_long_spin_wait(void)
> +{
> + u64 input = spin_wait_info | 0x00000000ffffffff;
What? The result is always 0x00000000ffffffff .....
> + spin_wait_info++;
> + hv_do_fast_hypercall8(HVCALL_NOTIFY_LONG_SPIN_WAIT, input);
> +}
> +
> +static void hv_qlock_kick(int cpu)
> +{
> + spin_wait_info--;
Looking at the above this is completely pointless and I have no idea what
that variable is supposed to do.
> + apic->send_IPI(cpu, X86_PLATFORM_IPI_VECTOR);
> +}
> +
> +/*
> + * Halt the current CPU & release it back to the host
> + */
> +static void hv_qlock_wait(u8 *byte, u8 val)
> +{
> + unsigned long msr_val;
> +
> + if (READ_ONCE(*byte) != val)
> + return;
> +
> + hv_notify_long_spin_wait();
> +
> + rdmsrl(HV_X64_MSR_GUEST_IDLE, msr_val);
Magic rdmsrl(). That wants a comment what this is for.
> +/*
> + * Hyper-V does not support this so far.
> + */
> +bool hv_vcpu_is_preempted(int vcpu)
static ?
> +{
> + return false;
> +}
> +PV_CALLEE_SAVE_REGS_THUNK(hv_vcpu_is_preempted);
> +
> +void __init hv_init_spinlocks(void)
> +{
> + if (!hv_pvspin ||
> + !apic ||
> + !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
> + !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
> + pr_warn("hv: PV spinlocks disabled\n");
Why does this need to be pr_warn? This is purely informational. Also please
use pr_fmt instead of the 'hv:' prefix.
> +static __init int hv_parse_nopvspin(char *arg)
> +{
> + hv_pvspin = false;
> + return 0;
> +}
> +early_param("hv_nopvspin", hv_parse_nopvspin);
That lacks Documentation of the command line parameter. Wants to be in the
same patch.
Thanks,
tglx
Hi Yi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/x86/core]
[also build test ERROR on v4.19-rc3 next-20180913]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yi-Sun/Enable-PV-qspinlock-for-Hyper-V/20180913-220827
config: x86_64-fedora-25 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All errors (new ones prefixed by >>):
arch/x86/hyperv/hv_spinlock.c: In function 'hv_init_spinlocks':
>> arch/x86/hyperv/hv_spinlock.c:86:2: error: implicit declaration of function '__pv_init_lock_hash'; did you mean 'spin_lock_bh'? [-Werror=implicit-function-declaration]
__pv_init_lock_hash();
^~~~~~~~~~~~~~~~~~~
spin_lock_bh
>> arch/x86/hyperv/hv_spinlock.c:87:42: error: '__pv_queued_spin_lock_slowpath' undeclared (first use in this function); did you mean 'queued_spin_lock_slowpath'?
pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
queued_spin_lock_slowpath
arch/x86/hyperv/hv_spinlock.c:87:42: note: each undeclared identifier is reported only once for each function it appears in
In file included from arch/x86/include/asm/msr.h:246:0,
from arch/x86/include/asm/processor.h:21,
from arch/x86/include/asm/cpufeature.h:5,
from arch/x86/include/asm/thread_info.h:53,
from include/linux/thread_info.h:38,
from arch/x86/include/asm/preempt.h:7,
from include/linux/preempt.h:81,
from include/linux/smp.h:60,
from include/linux/kernel_stat.h:5,
from arch/x86/hyperv/hv_spinlock.c:22:
arch/x86/include/asm/paravirt.h:775:35: error: '__raw_callee_save___pv_queued_spin_unlock' undeclared (first use in this function); did you mean '__raw_callee_save_hv_vcpu_is_preempted'?
((struct paravirt_callee_save) { __raw_callee_save_##func })
^
arch/x86/hyperv/hv_spinlock.c:88:35: note: in expansion of macro 'PV_CALLEE_SAVE'
pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +86 arch/x86/hyperv/hv_spinlock.c
74
75 void __init hv_init_spinlocks(void)
76 {
77 if (!hv_pvspin ||
78 !apic ||
79 !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
80 !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
81 pr_warn("hv: PV spinlocks disabled\n");
82 return;
83 }
84 pr_info("hv: PV spinlocks enabled\n");
85
> 86 __pv_init_lock_hash();
> 87 pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
88 pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
89 pv_lock_ops.wait = hv_qlock_wait;
90 pv_lock_ops.kick = hv_qlock_kick;
91 pv_lock_ops.vcpu_is_preempted = PV_CALLEE_SAVE(hv_vcpu_is_preempted);
92 }
93
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Yi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on tip/x86/core]
[also build test ERROR on v4.19-rc3 next-20180913]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Yi-Sun/Enable-PV-qspinlock-for-Hyper-V/20180913-220827
config: x86_64-fedora-25 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
arch/x86//hyperv/hv_spinlock.c: In function 'hv_init_spinlocks':
arch/x86//hyperv/hv_spinlock.c:86:2: error: implicit declaration of function '__pv_init_lock_hash'; did you mean 'spin_lock_bh'? [-Werror=implicit-function-declaration]
__pv_init_lock_hash();
^~~~~~~~~~~~~~~~~~~
spin_lock_bh
arch/x86//hyperv/hv_spinlock.c:87:42: error: '__pv_queued_spin_lock_slowpath' undeclared (first use in this function); did you mean 'queued_spin_lock_slowpath'?
pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
queued_spin_lock_slowpath
arch/x86//hyperv/hv_spinlock.c:87:42: note: each undeclared identifier is reported only once for each function it appears in
In file included from arch/x86/include/asm/msr.h:246:0,
from arch/x86/include/asm/processor.h:21,
from arch/x86/include/asm/cpufeature.h:5,
from arch/x86/include/asm/thread_info.h:53,
from include/linux/thread_info.h:38,
from arch/x86/include/asm/preempt.h:7,
from include/linux/preempt.h:81,
from include/linux/smp.h:60,
from include/linux/kernel_stat.h:5,
from arch/x86//hyperv/hv_spinlock.c:22:
>> arch/x86/include/asm/paravirt.h:775:35: error: '__raw_callee_save___pv_queued_spin_unlock' undeclared (first use in this function); did you mean '__raw_callee_save_hv_vcpu_is_preempted'?
((struct paravirt_callee_save) { __raw_callee_save_##func })
^
>> arch/x86//hyperv/hv_spinlock.c:88:35: note: in expansion of macro 'PV_CALLEE_SAVE'
pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
arch/x86/hyperv/hv_spinlock.c: In function 'hv_init_spinlocks':
arch/x86/hyperv/hv_spinlock.c:86:2: error: implicit declaration of function '__pv_init_lock_hash'; did you mean 'spin_lock_bh'? [-Werror=implicit-function-declaration]
__pv_init_lock_hash();
^~~~~~~~~~~~~~~~~~~
spin_lock_bh
arch/x86/hyperv/hv_spinlock.c:87:42: error: '__pv_queued_spin_lock_slowpath' undeclared (first use in this function); did you mean 'queued_spin_lock_slowpath'?
pv_lock_ops.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
queued_spin_lock_slowpath
arch/x86/hyperv/hv_spinlock.c:87:42: note: each undeclared identifier is reported only once for each function it appears in
In file included from arch/x86/include/asm/msr.h:246:0,
from arch/x86/include/asm/processor.h:21,
from arch/x86/include/asm/cpufeature.h:5,
from arch/x86/include/asm/thread_info.h:53,
from include/linux/thread_info.h:38,
from arch/x86/include/asm/preempt.h:7,
from include/linux/preempt.h:81,
from include/linux/smp.h:60,
from include/linux/kernel_stat.h:5,
from arch/x86/hyperv/hv_spinlock.c:22:
>> arch/x86/include/asm/paravirt.h:775:35: error: '__raw_callee_save___pv_queued_spin_unlock' undeclared (first use in this function); did you mean '__raw_callee_save_hv_vcpu_is_preempted'?
((struct paravirt_callee_save) { __raw_callee_save_##func })
^
arch/x86/hyperv/hv_spinlock.c:88:35: note: in expansion of macro 'PV_CALLEE_SAVE'
pv_lock_ops.queued_spin_unlock = PV_CALLEE_SAVE(__pv_queued_spin_unlock);
^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +775 arch/x86/include/asm/paravirt.h
2e47d3e6 include/asm-x86/paravirt.h Glauber de Oliveira Costa 2008-01-30 744
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 745 /*
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 746 * Generate a thunk around a function which saves all caller-save
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 747 * registers except for the return value. This allows C functions to
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 748 * be called from assembler code where fewer than normal registers are
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 749 * available. It may also help code generation around calls from C
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 750 * code if the common case doesn't use many registers.
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 751 *
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 752 * When a callee is wrapped in a thunk, the caller can assume that all
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 753 * arg regs and all scratch registers are preserved across the
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 754 * call. The return value in rax/eax will not be saved, even for void
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 755 * functions.
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 756 */
87b240cb arch/x86/include/asm/paravirt.h Josh Poimboeuf 2016-01-21 757 #define PV_THUNK_NAME(func) "__raw_callee_save_" #func
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 758 #define PV_CALLEE_SAVE_REGS_THUNK(func) \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 759 extern typeof(func) __raw_callee_save_##func; \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 760 \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 761 asm(".pushsection .text;" \
87b240cb arch/x86/include/asm/paravirt.h Josh Poimboeuf 2016-01-21 762 ".globl " PV_THUNK_NAME(func) ";" \
87b240cb arch/x86/include/asm/paravirt.h Josh Poimboeuf 2016-01-21 763 ".type " PV_THUNK_NAME(func) ", @function;" \
87b240cb arch/x86/include/asm/paravirt.h Josh Poimboeuf 2016-01-21 764 PV_THUNK_NAME(func) ":" \
87b240cb arch/x86/include/asm/paravirt.h Josh Poimboeuf 2016-01-21 765 FRAME_BEGIN \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 766 PV_SAVE_ALL_CALLER_REGS \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 767 "call " #func ";" \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 768 PV_RESTORE_ALL_CALLER_REGS \
87b240cb arch/x86/include/asm/paravirt.h Josh Poimboeuf 2016-01-21 769 FRAME_END \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 770 "ret;" \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 771 ".popsection")
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 772
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 773 /* Get a reference to a callee-save function */
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 774 #define PV_CALLEE_SAVE(func) \
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 @775 ((struct paravirt_callee_save) { __raw_callee_save_##func })
ecb93d1c arch/x86/include/asm/paravirt.h Jeremy Fitzhardinge 2009-01-28 776
:::::: The code at line 775 was first introduced by commit
:::::: ecb93d1ccd0aac63f03be2db3cac3fa974716f4c x86/paravirt: add register-saving thunks to reduce caller register pressure
:::::: TO: Jeremy Fitzhardinge <[email protected]>
:::::: CC: H. Peter Anvin <[email protected]>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 18-09-13 13:06:13, Thomas Gleixner wrote:
> On Thu, 13 Sep 2018, Yi Sun wrote:
> > +#include <asm/mshyperv.h>
> >
> > /* representing HT siblings of each logical CPU */
> > DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_map);
> > @@ -1335,6 +1336,7 @@ void __init native_smp_prepare_boot_cpu(void)
> > /* already set me in cpu_online_mask in boot_cpu_init() */
> > cpumask_set_cpu(me, cpu_callout_mask);
> > cpu_set_state_online(me);
> > + hv_init_spinlocks();
>
> No. We have smp_ops.smp_prepare_boot_cpu for that.
>
Got it, will change it. Thanks!
> Thanks,
>
> tglx
On 18-09-13 13:24:07, Thomas Gleixner wrote:
> On Thu, 13 Sep 2018, Yi Sun wrote:
> > +++ b/arch/x86/hyperv/hv_spinlock.c
> > @@ -0,0 +1,99 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Hyper-V specific spinlock code.
> > + *
> > + * Copyright (C) 2018, Intel, Inc.
> > + *
> > + * Author : Yi Sun <[email protected]>
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License version 2 as published
> > + * by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful, but
> > + * WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE, GOOD TITLE or
> > + * NON INFRINGEMENT. See the GNU General Public License for more
> > + * details.
>
> Please remove the boilerplate. The SPDX identifier is enough. If in doubt
> talk to Intel legal.
>
I will check this. Thanks!
> > +static bool hv_pvspin = true;
>
> __initdata please.
>
> > +static u32 spin_wait_info = 0;
>
> No need for 0 initialization.
>
Will modify them.
> > +
> > +static void hv_notify_long_spin_wait(void)
> > +{
> > + u64 input = spin_wait_info | 0x00000000ffffffff;
>
> What? The result is always 0x00000000ffffffff .....
>
Oh, sorry for such error.
> > + spin_wait_info++;
> > + hv_do_fast_hypercall8(HVCALL_NOTIFY_LONG_SPIN_WAIT, input);
> > +}
> > +
> > +static void hv_qlock_kick(int cpu)
> > +{
> > + spin_wait_info--;
>
> Looking at the above this is completely pointless and I have no idea what
> that variable is supposed to do.
>
Per Microsoft Hypervisor Top Level Functional Specification, the input
parameter of HVCALL_NOTIFY_LONG_SPIN_WAIT is defined as below:
SpinwaitInfo – Specifies the accumulated count the guest was spinning.
So, it is increased when guest is spinning and reduced when spinlock is
released.
I may add comments to explain it.
> > + apic->send_IPI(cpu, X86_PLATFORM_IPI_VECTOR);
> > +}
> > +
> > +/*
> > + * Halt the current CPU & release it back to the host
> > + */
> > +static void hv_qlock_wait(u8 *byte, u8 val)
> > +{
> > + unsigned long msr_val;
> > +
> > + if (READ_ONCE(*byte) != val)
> > + return;
> > +
> > + hv_notify_long_spin_wait();
> > +
> > + rdmsrl(HV_X64_MSR_GUEST_IDLE, msr_val);
>
> Magic rdmsrl(). That wants a comment what this is for.
>
Per above spec, reading HV_X64_MSR_GUEST_IDLE can make guest idle.
I will add comment here.
> > +/*
> > + * Hyper-V does not support this so far.
> > + */
> > +bool hv_vcpu_is_preempted(int vcpu)
>
> static ?
>
PV_CALLEE_SAVE_REGS_THUNK definition is below.
#define PV_CALLEE_SAVE_REGS_THUNK(func) \
extern typeof(func) __raw_callee_save_##func; \
......
So, the hv_vcpu_is_preempted cannot be declared as 'static'. Otherwise,
the make fails with below info.
arch/x86/hyperv/hv_spinlock.o: In function `__raw_callee_save_hv_vcpu_is_preempted':
hv_spinlock.c:(.text+0xd): undefined reference to `hv_vcpu_is_preempted'
> > +{
> > + return false;
> > +}
> > +PV_CALLEE_SAVE_REGS_THUNK(hv_vcpu_is_preempted);
> > +
> > +void __init hv_init_spinlocks(void)
> > +{
> > + if (!hv_pvspin ||
> > + !apic ||
> > + !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
> > + !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
> > + pr_warn("hv: PV spinlocks disabled\n");
>
> Why does this need to be pr_warn? This is purely informational. Also please
> use pr_fmt instead of the 'hv:' prefix.
>
Got it. Thanks!
> > +static __init int hv_parse_nopvspin(char *arg)
> > +{
> > + hv_pvspin = false;
> > + return 0;
> > +}
> > +early_param("hv_nopvspin", hv_parse_nopvspin);
>
> That lacks Documentation of the command line parameter. Wants to be in the
> same patch.
>
Will merge patch 3 into 2. Thanks!
> Thanks,
>
> tglx
On Fri, 14 Sep 2018, Yi Sun wrote:
> > > +static void hv_notify_long_spin_wait(void)
> > > +{
> > > + u64 input = spin_wait_info | 0x00000000ffffffff;
> >
> > What? The result is always 0x00000000ffffffff .....
> >
> Oh, sorry for such error.
>
> > > + spin_wait_info++;
> > > + hv_do_fast_hypercall8(HVCALL_NOTIFY_LONG_SPIN_WAIT, input);
> > > +}
> > > +
> > > +static void hv_qlock_kick(int cpu)
> > > +{
> > > + spin_wait_info--;
> >
> > Looking at the above this is completely pointless and I have no idea what
> > that variable is supposed to do.
> >
> Per Microsoft Hypervisor Top Level Functional Specification, the input
> parameter of HVCALL_NOTIFY_LONG_SPIN_WAIT is defined as below:
>
> SpinwaitInfo – Specifies the accumulated count the guest was spinning.
>
> So, it is increased when guest is spinning and reduced when spinlock is
> released.
But that's a global variable, so it might be incremented and decremented by
several CPUs at once. I don't have the spec and have no time to study it
either, but global does not make any sense to me. The spin wait info comes
from a single guest CPU and the wakeup is targeted at that guest CPU as
well. So why global? It might be defined that way, but then you really want
to explain it proper.
Thanks,
tglx
From Yi Sun Sent: Thursday, September 13, 2018 2:13 AM
> This patch adds the necessary Hyper-V specific code to allow
> PV qspinlock work on Hyper-V.
>
Have you done any performance measurements with this
new code, so that we know whether there is any improvement,
or even potentially any degradation in some circumstances?
Michael
On 18-09-14 12:42:33, Thomas Gleixner wrote:
> On Fri, 14 Sep 2018, Yi Sun wrote:
> > > > +static void hv_notify_long_spin_wait(void)
> > > > +{
> > > > + u64 input = spin_wait_info | 0x00000000ffffffff;
> > >
> > > What? The result is always 0x00000000ffffffff .....
> > >
> > Oh, sorry for such error.
> >
> > > > + spin_wait_info++;
> > > > + hv_do_fast_hypercall8(HVCALL_NOTIFY_LONG_SPIN_WAIT, input);
> > > > +}
> > > > +
> > > > +static void hv_qlock_kick(int cpu)
> > > > +{
> > > > + spin_wait_info--;
> > >
> > > Looking at the above this is completely pointless and I have no idea what
> > > that variable is supposed to do.
> > >
> > Per Microsoft Hypervisor Top Level Functional Specification, the input
> > parameter of HVCALL_NOTIFY_LONG_SPIN_WAIT is defined as below:
> >
> > SpinwaitInfo – Specifies the accumulated count the guest was spinning.
> >
> > So, it is increased when guest is spinning and reduced when spinlock is
> > released.
>
> But that's a global variable, so it might be incremented and decremented by
> several CPUs at once. I don't have the spec and have no time to study it
> either, but global does not make any sense to me. The spin wait info comes
> from a single guest CPU and the wakeup is targeted at that guest CPU as
> well. So why global? It might be defined that way, but then you really want
> to explain it proper.
>
> Thanks,
>
> tglx
>
Let me check this more. Will reply to you later. Thanks!
BRs,
Yi Sun
On 18-09-15 23:43:12, Michael Kelley (EOSG) wrote:
> >From Yi Sun Sent: Thursday, September 13, 2018 2:13 AM
>
> > This patch adds the necessary Hyper-V specific code to allow
> > PV qspinlock work on Hyper-V.
> >
>
> Have you done any performance measurements with this
> new code, so that we know whether there is any improvement,
> or even potentially any degradation in some circumstances?
>
> Michael
>
I executed reaim in Guest (20 vcpus, 30G memory). Reaim is one of
the performance test methods used by qspinlock patch set. Results
are below. With hv_spinlock enabled, performance is better.
hv_spinlock disabled:
$ src/reaim -c data/reaim.config -f data/workfile.compute -i 16 -e 256
Num Parent Child Child Jobs per Jobs/min/ Std_dev Std_dev JTI
Forked Time SysTime UTime Minute Child Time Percent
1 1.16 0.05 1.08 5224.14 5224.14 0.00 0.00 100
17 1.40 0.75 19.22 73585.71 4328.57 0.05 4.03 95
33 2.17 1.29 36.71 92156.68 2792.63 0.12 6.13 93
49 2.84 1.59 50.50 104556.34 2133.80 0.17 6.68 93
65 3.71 2.17 68.11 106172.51 1633.42 0.24 7.14 92
81 4.63 2.87 84.00 106017.28 1308.86 0.35 8.31 91
97 5.55 3.40 101.11 105913.51 1091.89 0.32 6.27 93
113 6.28 3.90 117.70 109041.40 964.97 0.38 6.51 93
129 7.20 4.44 134.00 108575.00 841.67 0.62 9.31 90
145 8.02 4.98 150.90 109563.59 755.61 0.61 8.18 91
178 9.75 6.46 184.96 110633.85 621.54 0.62 6.78 93
211 11.91 7.65 226.10 107360.20 508.82 0.88 7.96 92
244 13.30 8.28 253.82 111175.94 455.64 0.80 6.44 93
Max Jobs per Minute 111175.94
hv_spinlock enabled:
Num Parent Child Child Jobs per Jobs/min/ Std_dev Std_dev JTI
Forked Time SysTime UTime Minute Child Time Percent
1 1.07 0.04 1.00 5663.55 5663.55 0.00 0.00 100
17 1.21 0.58 17.63 85140.50 5008.26 0.03 3.05 96
33 1.88 1.18 34.22 106372.34 3223.40 0.09 5.27 94
49 3.04 1.83 56.27 97677.63 1993.42 0.20 7.35 92
65 3.66 2.26 68.64 107622.95 1655.74 0.26 7.86 92
81 4.50 2.85 84.45 109080.00 1346.67 0.25 6.03 93
97 5.33 3.29 100.55 110285.18 1136.96 0.36 7.43 92
113 6.20 3.87 117.76 110448.39 977.42 0.39 6.87 93
129 6.96 4.42 132.93 112318.97 870.69 0.45 6.92 93
145 7.86 5.01 151.02 111793.89 770.99 0.50 6.76 93
179 9.71 6.16 185.62 111713.70 624.10 0.72 7.92 92
213 11.70 7.43 224.77 110323.08 517.95 0.73 6.69 93
247 13.45 8.48 259.19 111287.73 450.56 0.73 5.78 94
Max Jobs per Minute 112318.97
Hi,
On 18-09-17 20:54:52, Yi Sun wrote:
> On 18-09-14 12:42:33, Thomas Gleixner wrote:
> > On Fri, 14 Sep 2018, Yi Sun wrote:
> > > > > +static void hv_notify_long_spin_wait(void)
> > > > > +{
> > > > > + u64 input = spin_wait_info | 0x00000000ffffffff;
> > > >
> > > > What? The result is always 0x00000000ffffffff .....
> > > >
> > > Oh, sorry for such error.
> > >
> > > > > + spin_wait_info++;
> > > > > + hv_do_fast_hypercall8(HVCALL_NOTIFY_LONG_SPIN_WAIT, input);
> > > > > +}
> > > > > +
> > > > > +static void hv_qlock_kick(int cpu)
> > > > > +{
> > > > > + spin_wait_info--;
> > > >
> > > > Looking at the above this is completely pointless and I have no idea what
> > > > that variable is supposed to do.
> > > >
> > > Per Microsoft Hypervisor Top Level Functional Specification, the input
> > > parameter of HVCALL_NOTIFY_LONG_SPIN_WAIT is defined as below:
> > >
> > > SpinwaitInfo – Specifies the accumulated count the guest was spinning.
> > >
> > > So, it is increased when guest is spinning and reduced when spinlock is
> > > released.
> >
> > But that's a global variable, so it might be incremented and decremented by
> > several CPUs at once. I don't have the spec and have no time to study it
> > either, but global does not make any sense to me. The spin wait info comes
> > from a single guest CPU and the wakeup is targeted at that guest CPU as
> > well. So why global? It might be defined that way, but then you really want
> > to explain it proper.
> >
> > Thanks,
> >
> > tglx
> >
> Let me check this more. Will reply to you later. Thanks!
>
> BRs,
> Yi Sun
I have got the details from Microsoft.
Notify_Long_Spin_Wait hypercall should be called after a specific times
spinning which can be got through CPUID. When hypervisor receives this,
it will try to ensure that all vcpus are scheduled.
Notify_Long_Spin_Wait is a standalone feature which should be split out
as a new patch set.
So I would like split it into a new patch set and submit it after PV
Hyper-V Spinlock patch set merged.