LKML Archive on lore.kernel.org
 help / Atom feed
* [PATCH v1 0/3] Enable PV qspinlock for Hyper-V
@ 2018-09-13  9:13 Yi Sun
  2018-09-13  9:13 ` [PATCH v1 1/3] X86/Hyper-V: Add Guest IDLE MSR support Yi Sun
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Yi Sun @ 2018-09-13  9:13 UTC (permalink / raw)
  To: linux-kernel
  Cc: x86, chao.p.peng, chao.gao, isaku.yamahata, michael.h.kelly,
	tianyu.lan, Yi Sun, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Jonathan Corbet

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v1 1/3] X86/Hyper-V: Add Guest IDLE MSR support
  2018-09-13  9:13 [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Yi Sun
@ 2018-09-13  9:13 ` Yi Sun
  2018-09-13  9:13 ` [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V Yi Sun
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 15+ messages in thread
From: Yi Sun @ 2018-09-13  9:13 UTC (permalink / raw)
  To: linux-kernel
  Cc: x86, chao.p.peng, chao.gao, isaku.yamahata, michael.h.kelly,
	tianyu.lan, Yi Sun, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-13  9:13 [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Yi Sun
  2018-09-13  9:13 ` [PATCH v1 1/3] X86/Hyper-V: Add Guest IDLE MSR support Yi Sun
@ 2018-09-13  9:13 ` Yi Sun
  2018-09-13 11:06   ` Thomas Gleixner
                     ` (3 more replies)
  2018-09-13  9:13 ` [PATCH v1 3/3] hv: add description for hv_nopvspin Yi Sun
  2018-09-15 23:43 ` [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Michael Kelley (EOSG)
  3 siblings, 4 replies; 15+ messages in thread
From: Yi Sun @ 2018-09-13  9:13 UTC (permalink / raw)
  To: linux-kernel
  Cc: x86, chao.p.peng, chao.gao, isaku.yamahata, michael.h.kelly,
	tianyu.lan, Yi Sun, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v1 3/3] hv: add description for hv_nopvspin
  2018-09-13  9:13 [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Yi Sun
  2018-09-13  9:13 ` [PATCH v1 1/3] X86/Hyper-V: Add Guest IDLE MSR support Yi Sun
  2018-09-13  9:13 ` [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V Yi Sun
@ 2018-09-13  9:13 ` Yi Sun
  2018-09-15 23:43 ` [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Michael Kelley (EOSG)
  3 siblings, 0 replies; 15+ messages in thread
From: Yi Sun @ 2018-09-13  9:13 UTC (permalink / raw)
  To: linux-kernel
  Cc: x86, chao.p.peng, chao.gao, isaku.yamahata, michael.h.kelly,
	tianyu.lan, Yi Sun, Jonathan Corbet, K. Y. Srinivasan,
	Haiyang Zhang, Stephen Hemminger

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-13  9:13 ` [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V Yi Sun
@ 2018-09-13 11:06   ` Thomas Gleixner
  2018-09-14  8:53     ` Yi Sun
  2018-09-13 11:24   ` Thomas Gleixner
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2018-09-13 11:06 UTC (permalink / raw)
  To: Yi Sun
  Cc: linux-kernel, x86, chao.p.peng, chao.gao, isaku.yamahata,
	michael.h.kelly, tianyu.lan, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-13  9:13 ` [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V Yi Sun
  2018-09-13 11:06   ` Thomas Gleixner
@ 2018-09-13 11:24   ` Thomas Gleixner
  2018-09-14  9:04     ` Yi Sun
  2018-09-13 16:16   ` kbuild test robot
  2018-09-13 16:42   ` kbuild test robot
  3 siblings, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2018-09-13 11:24 UTC (permalink / raw)
  To: Yi Sun
  Cc: linux-kernel, x86, chao.p.peng, chao.gao, isaku.yamahata,
	michael.h.kelly, tianyu.lan, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-13  9:13 ` [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V Yi Sun
  2018-09-13 11:06   ` Thomas Gleixner
  2018-09-13 11:24   ` Thomas Gleixner
@ 2018-09-13 16:16   ` kbuild test robot
  2018-09-13 16:42   ` kbuild test robot
  3 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2018-09-13 16:16 UTC (permalink / raw)
  To: Yi Sun
  Cc: kbuild-all, linux-kernel, x86, chao.p.peng, chao.gao,
	isaku.yamahata, michael.h.kelly, tianyu.lan, Yi Sun,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-13  9:13 ` [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V Yi Sun
                     ` (2 preceding siblings ...)
  2018-09-13 16:16   ` kbuild test robot
@ 2018-09-13 16:42   ` kbuild test robot
  3 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2018-09-13 16:42 UTC (permalink / raw)
  To: Yi Sun
  Cc: kbuild-all, linux-kernel, x86, chao.p.peng, chao.gao,
	isaku.yamahata, michael.h.kelly, tianyu.lan, Yi Sun,
	K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger

[-- 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 --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-13 11:06   ` Thomas Gleixner
@ 2018-09-14  8:53     ` Yi Sun
  0 siblings, 0 replies; 15+ messages in thread
From: Yi Sun @ 2018-09-14  8:53 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, x86, chao.p.peng, chao.gao, isaku.yamahata,
	michael.h.kelly, tianyu.lan, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-13 11:24   ` Thomas Gleixner
@ 2018-09-14  9:04     ` Yi Sun
  2018-09-14 10:42       ` Thomas Gleixner
  0 siblings, 1 reply; 15+ messages in thread
From: Yi Sun @ 2018-09-14  9:04 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, x86, chao.p.peng, chao.gao, isaku.yamahata,
	michael.h.kelly, tianyu.lan, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-14  9:04     ` Yi Sun
@ 2018-09-14 10:42       ` Thomas Gleixner
  2018-09-17 12:54         ` Yi Sun
  0 siblings, 1 reply; 15+ messages in thread
From: Thomas Gleixner @ 2018-09-14 10:42 UTC (permalink / raw)
  To: Yi Sun
  Cc: linux-kernel, x86, chao.p.peng, chao.gao, isaku.yamahata,
	michael.h.kelly, tianyu.lan, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH v1 0/3] Enable PV qspinlock for Hyper-V
  2018-09-13  9:13 [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Yi Sun
                   ` (2 preceding siblings ...)
  2018-09-13  9:13 ` [PATCH v1 3/3] hv: add description for hv_nopvspin Yi Sun
@ 2018-09-15 23:43 ` Michael Kelley (EOSG)
  2018-09-17 14:37   ` Yi Sun
  3 siblings, 1 reply; 15+ messages in thread
From: Michael Kelley (EOSG) @ 2018-09-15 23:43 UTC (permalink / raw)
  To: Yi Sun, linux-kernel
  Cc: x86, chao.p.peng, chao.gao, isaku.yamahata, michael.h.kelly,
	Tianyu Lan, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	Jonathan Corbet

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
 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-14 10:42       ` Thomas Gleixner
@ 2018-09-17 12:54         ` Yi Sun
  2018-09-20  1:56           ` Yi Sun
  0 siblings, 1 reply; 15+ messages in thread
From: Yi Sun @ 2018-09-17 12:54 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, x86, chao.p.peng, chao.gao, isaku.yamahata,
	michael.h.kelly, tianyu.lan, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 0/3] Enable PV qspinlock for Hyper-V
  2018-09-15 23:43 ` [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Michael Kelley (EOSG)
@ 2018-09-17 14:37   ` Yi Sun
  0 siblings, 0 replies; 15+ messages in thread
From: Yi Sun @ 2018-09-17 14:37 UTC (permalink / raw)
  To: Michael Kelley (EOSG)
  Cc: linux-kernel, x86, chao.p.peng, chao.gao, isaku.yamahata,
	michael.h.kelly, Tianyu Lan, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Jonathan Corbet

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V
  2018-09-17 12:54         ` Yi Sun
@ 2018-09-20  1:56           ` Yi Sun
  0 siblings, 0 replies; 15+ messages in thread
From: Yi Sun @ 2018-09-20  1:56 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-kernel, x86, chao.p.peng, chao.gao, isaku.yamahata,
	michael.h.kelly, tianyu.lan, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-13  9:13 [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Yi Sun
2018-09-13  9:13 ` [PATCH v1 1/3] X86/Hyper-V: Add Guest IDLE MSR support Yi Sun
2018-09-13  9:13 ` [PATCH v1 2/3] locking/pvqspinlock, hv: Enable PV qspinlock for Hyper-V Yi Sun
2018-09-13 11:06   ` Thomas Gleixner
2018-09-14  8:53     ` Yi Sun
2018-09-13 11:24   ` Thomas Gleixner
2018-09-14  9:04     ` Yi Sun
2018-09-14 10:42       ` Thomas Gleixner
2018-09-17 12:54         ` Yi Sun
2018-09-20  1:56           ` Yi Sun
2018-09-13 16:16   ` kbuild test robot
2018-09-13 16:42   ` kbuild test robot
2018-09-13  9:13 ` [PATCH v1 3/3] hv: add description for hv_nopvspin Yi Sun
2018-09-15 23:43 ` [PATCH v1 0/3] Enable PV qspinlock for Hyper-V Michael Kelley (EOSG)
2018-09-17 14:37   ` Yi Sun

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox