[PATCH v1 0/3] Enable PV qspinlock for Hyper-V

[PATCH v1 0/3] Enable PV qspinlock for Hyper-V

[Yi Sun]

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" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Cc: Jonathan Corbet <corbet@lwn.net>

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

[PATCH v1 1/3] X86/Hyper-V: Add Guest IDLE MSR support

[Yi Sun]

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" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 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

[PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[Yi Sun]

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" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 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 <yi.y.sun@linux.intel.com>
+ *
+ * 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

[PATCH v1 3/3] hv: add description for hv_nopvspin

[Yi Sun]

hv_nopvspin is a command line parameter used to
disable Hyper-V PV spinlock.

Cc: Jonathan Corbet <corbet@lwn.net>
Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Cc: Haiyang Zhang <haiyangz@microsoft.com>
Cc: Stephen Hemminger <sthemmin@microsoft.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
---
 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

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[Thomas Gleixner]

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

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[Thomas Gleixner]

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 <yi.y.sun@linux.intel.com>
> + *
> + * 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

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[kbuild test robot]

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

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48556 bytes --]

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[kbuild test robot]

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

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 <jeremy@goop.org>
:::::: CC: H. Peter Anvin <hpa@linux.intel.com>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 48556 bytes --]

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[Yi Sun]

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

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[Yi Sun]

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 <yi.y.sun@linux.intel.com>
> > + *
> > + * 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

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[Thomas Gleixner]

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

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

RE: [PATCH v1 0/3] Enable PV qspinlock for Hyper-V

[Michael Kelley (EOSG)]

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
 

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[Yi Sun]

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

Re: [PATCH v1 0/3] Enable PV qspinlock for Hyper-V

[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

Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V

[Yi Sun]

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.