Enable HvNotifyLongSpinWait for Hyper-V
Subsystems (3)
cover Enable HvNotifyLongSpinWait for Hyper-V
The HvNotifyLongSpinWait hypercall is 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 Virtual Machine. This scheduling hint improves the scalability of VMs with more than one virtual processor on Hyper-V.
Per MSFT TLFS, the retry number is sent to hypervisor only when the retry number exceeds the recommended number. If recommended number is 0xFFFFFFFF, never retry.
The recommended number if got from EBX of Implementation Recommendations MSR (0x40000004).
This patch set bases on latest tip branch because below patch set has not been merged into main branch.
[PATCH v4 0/2] Enable PV qspinlock for Hyper-V
Yi Sun (2):
x86/hyperv: get number of spinlock retry on Hyper-V x86/hyperv: call HVCALL_NOTIFY_LONG_SPIN_WAIT
arch/x86/hyperv/hv_spinlock.c | 18 ++++++++++++++++++ arch/x86/include/asm/mshyperv.h | 4 ++++ arch/x86/kernel/cpu/mshyperv.c | 1 + kernel/locking/qspinlock_paravirt.h | 10 ++++++++++ 4 files changed, 33 insertions(+)
Signature · 1 line
1.9.1
PATCH 1/2 x86/hyperv: get spinlock retry number on Hyper-V
EBX of Implementation Recommendations MSR (0x40000004) indicates recommended number of attempts to retry a spinlock failure before notifying the hypervisor about the failures.
0xFFFFFFFF indicates never to retry.
--- arch/x86/include/asm/mshyperv.h | 3 +++ arch/x86/kernel/cpu/mshyperv.c | 1 + 2 files changed, 4 insertions(+)
arch/x86/include/asm/mshyperv.h +3 -0
@@ -11,10 +11,13 @@#define VP_INVAL U32_MAX +#define HYPERV_SPINLOCK_RETRY_NEVER U32_MAX +struct ms_hyperv_info {u32 features;u32 misc_features;u32 hints; +u32 num_spin_retry;u32 nested_features;u32 max_vp_index;u32 max_lp_index;
arch/x86/kernel/cpu/mshyperv.c +1 -0
@@ -222,6 +222,7 @@ static void __init ms_hyperv_init_platform(void) ms_hyperv.features =cpuid_eax (HYPERV_CPUID_FEATURES ); ms_hyperv.misc_features =cpuid_edx (HYPERV_CPUID_FEATURES ); ms_hyperv.hints =cpuid_eax (HYPERV_CPUID_ENLIGHTMENT_INFO ); + ms_hyperv.num_spin_retry =cpuid_ebx (HYPERV_CPUID_ENLIGHTMENT_INFO );pr_info ("Hyper-V: features 0x%x, hints 0x%x\n" , ms_hyperv.features, ms_hyperv.hints);
Signature · 1 line
1.9.1
PATCH 2/2 x86/hyperv: make HvNotifyLongSpinWait hypercall
In reply to Yi SunThe HvNotifyLongSpinWait hypercall (HVCALL_NOTIFY_LONG_SPIN_WAIT) is 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 Virtual Machine. This scheduling hint improves the scalability of VMs with more than one virtual processor on Hyper-V.
Per MSFT TLFS, the retry number (SpinWaitInfo) is sent to hypervisor only when the retry number exceeds the recommended number. If recommended number is 0xFFFFFFFF, never retry.
--- arch/x86/hyperv/hv_spinlock.c | 18 ++++++++++++++++++ arch/x86/include/asm/mshyperv.h | 1 + kernel/locking/qspinlock_paravirt.h | 10 ++++++++++ 3 files changed, 29 insertions(+)
arch/x86/hyperv/hv_spinlock.c +18 -0
@@ -18,6 +18,24 @@static bool __initdata hv_pvspin = true; +bool hv_notify_long_spin_wait (int retry_num) +{ +/* +* Per MSFT TLFS, the SpinWaitInfo is sent to hypervisor only when +* the retry number exceeds the recommended number. +* +* If recommended number is 0xFFFFFFFF, never retry. +*/ +if (ms_hyperv.num_spin_retry ==HYPERV_SPINLOCK_RETRY_NEVER ) +return false; + +if ((0 == retry_num % ms_hyperv.num_spin_retry) && retry_num) +hv_do_fast_hypercall8 (HVCALL_NOTIFY_LONG_SPIN_WAIT , + retry_num); + +return true; +} +static void hv_qlock_kick (int cpu) { apic->send_IPI (cpu,X86_PLATFORM_IPI_VECTOR );
arch/x86/include/asm/mshyperv.h +1 -0
@@ -356,6 +356,7 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,void hv_apic_init (void );void __init hv_init_spinlocks (void );bool hv_vcpu_is_preempted (int vcpu); +bool hv_notify_long_spin_wait (int retry_num);#else static inline void hv_apic_init (void ) {}#endif
kernel/locking/qspinlock_paravirt.h +10 -0
@@ -7,6 +7,8 @@#include <linux/bootmem.h> #include <linux/debug_locks.h> +#include <asm/mshyperv.h> +/* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them.
@@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) wait_early = true;break ; } +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif cpu_relax (); }
@@ -433,6 +439,10 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)for (loop =SPIN_THRESHOLD ; loop; loop--) {if (trylock_clear_pending (lock)) goto gotlock; +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif cpu_relax (); }clear_pending (lock);
Signature · 1 line
1.9.1
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 79 lines · Yi Sun
The HvNotifyLongSpinWait hypercall (
HVCALL_NOTIFY_LONG_SPIN_WAIT) is 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 Virtual Machine. This scheduling hint improves the scalability of VMs with more than one virtual processor on Hyper-V.Per MSFT TLFS, the retry number (SpinWaitInfo) is sent to hypervisor only when the retry number exceeds the recommended number. If recommended number is
0xFFFFFFFF, never retry.Signed-off-by Yi Sun <[email protected]>Cc "K. Y. Srinivasan" <[email protected]>Cc Haiyang Zhang <[email protected]>Cc Stephen Hemminger <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Michael Kelley (EOSG) <[email protected]>Cc Juergen Gross <[email protected]>--- arch/x86/hyperv/hv_spinlock.c | 18 ++++++++++++++++++ arch/x86/include/asm/mshyperv.h | 1 + kernel/locking/qspinlock_paravirt.h | 10 ++++++++++ 3 files changed, 29 insertions(+)arch/x86/hyperv/hv_spinlock.c +18
-0@@ -18,6 +18,24 @@static bool __initdata hv_pvspin = true; +bool hv_notify_long_spin_wait (int retry_num) +{ +/* +* Per MSFT TLFS, the SpinWaitInfo is sent to hypervisor only when +* the retry number exceeds the recommended number. +* +* If recommended number is 0xFFFFFFFF, never retry. +*/ +if (ms_hyperv.num_spin_retry ==HYPERV_SPINLOCK_RETRY_NEVER ) +return false; + +if ((0 == retry_num % ms_hyperv.num_spin_retry) && retry_num) +hv_do_fast_hypercall8 (HVCALL_NOTIFY_LONG_SPIN_WAIT , + retry_num); + +return true; +} +static void hv_qlock_kick (int cpu) { apic->send_IPI (cpu,X86_PLATFORM_IPI_VECTOR );arch/x86/include/asm/mshyperv.h +1
-0@@ -356,6 +356,7 @@ static inline int cpumask_to_vpset(struct hv_vpset *vpset,void hv_apic_init (void );void __init hv_init_spinlocks (void );bool hv_vcpu_is_preempted (int vcpu); +bool hv_notify_long_spin_wait (int retry_num);#else static inline void hv_apic_init (void ) {}#endif kernel/locking/qspinlock_paravirt.h +2
-0@@ -7,6 +7,8 @@#include <linux/bootmem.h> #include <linux/debug_locks.h> +#include <asm/mshyperv.h> +/* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them.
kernel/locking/qspinlock_paravirt.h static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) · line 313
Quoted 7 lines
kernel/locking/qspinlock_paravirt.h +4
-0@@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) wait_early = true;break ; } +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif
I don't like that. Why should a KVM or Xen guest call into a hyperv specific function?
Can't you move this to existing hyperv specific paravirt hooks?
Juergen
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 1 line · Juergen Gross
On 19/10/2018 15:13, Yi Sun wrote:
[...]
Quoted 25 lines
Quoted 14 lines
kernel/locking/qspinlock_paravirt.h +2
-0@@ -7,6 +7,8 @@#include <linux/bootmem.h> #include <linux/debug_locks.h> +#include <asm/mshyperv.h> +/* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them. kernel/locking/qspinlock_paravirt.h static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) · line 313
Quoted 7 lines
kernel/locking/qspinlock_paravirt.h +4
-0@@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) wait_early = true;break ; } +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif I don't like that. Why should a KVM or Xen guest call into a hyperv specific function?
Quoted 2 lines
Can't you move this to existing hyperv specific paravirt hooks?
hv_notify_long_spin_wait() must be called in this loop but it seems there is no appropriate existing paravirt hook here. So, can I add one more hook in pv_lock_ops to do this notification?
Quoted 2 lines
Juergen
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 30 lines · Yi Sun
Quoted 1 line · Juergen Gross
On 19/10/2018 15:13, Yi Sun wrote:
[...]
Quoted 25 lines
Quoted 14 lines
kernel/locking/qspinlock_paravirt.h +2
-0@@ -7,6 +7,8 @@#include <linux/bootmem.h> #include <linux/debug_locks.h> +#include <asm/mshyperv.h> +/* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them. kernel/locking/qspinlock_paravirt.h static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) · line 313
Quoted 7 lines
kernel/locking/qspinlock_paravirt.h +4
-0@@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) wait_early = true;break ; } +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif I don't like that. Why should a KVM or Xen guest call into a hyperv specific function?
Quoted 5 lines
Quoted 2 lines
Can't you move this to existing hyperv specific paravirt hooks?
hv_notify_long_spin_wait()must be called in this loop but it seems there is no appropriate existing paravirt hook here. So, can I add one more hook inpv_lock_opsto do this notification?
vcpu_is_preempted() is already part of this loop. And this is a paravirt hook. Can't you make use of that? This might require adding another parameter to this hook, but I'd prefer that over another pv-spinlock hook.
Adding some more locking maintainers and Waiman to the Cc: list.
Juergen
Waiman Long
reviewerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 39 lines · Juergen Gross
Quoted 28 lines · Yi Sun
Quoted 1 line · Juergen Gross
On 19/10/2018 15:13, Yi Sun wrote:
[...]
Quoted 24 lines
Quoted 14 lines
kernel/locking/qspinlock_paravirt.h +2
-0@@ -7,6 +7,8 @@#include <linux/bootmem.h> #include <linux/debug_locks.h> +#include <asm/mshyperv.h> +/* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them. kernel/locking/qspinlock_paravirt.h static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) · line 313
Quoted 7 lines
kernel/locking/qspinlock_paravirt.h +4
-0@@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) wait_early = true;break ; } +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif I don't like that. Why should a KVM or Xen guest call into a hyperv specific function?
Quoted 5 lines
Quoted 2 lines
Can't you move this to existing hyperv specific paravirt hooks?
hv_notify_long_spin_wait()must be called in this loop but it seems there is no appropriate existing paravirt hook here. So, can I add one more hook inpv_lock_opsto do this notification?
vcpu_is_preempted()is already part of this loop. And this is a paravirt hook. Can't you make use of that? This might require adding another parameter to this hook, but I'd prefer that over another pv-spinlock hook.
Quoted 5 lines
Adding some more locking maintainers and Waiman to the Cc: list.
Juergen
I agree with Juergen on that. I would suggest rename the vcpu_is_preempted hook into a more generic vcpu_stop_spinning, perhaps, so different hypervisors can act on the information accordingly. Adding an extra parameter is fine.
Cheers,
Longman
Peter Zijlstra
maintainerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Firstly, who come a patch that is grubbing around in kernel/locking/ has an x86/hyperv subject and isn't Cc'ed to the locking maintainers?
Quoted 14 lines · Waiman Long
Quoted 13 lines · Juergen Gross
Quoted 12 lines · Yi Sun
Quoted 1 line · Juergen Gross
On 19/10/2018 15:13, Yi Sun wrote:
[...]
Quoted 8 lines
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h +0
-0@@ -7,6 +7,8 @@#include <linux/bootmem.h> #include <linux/debug_locks.h>
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
kernel/locking/qspinlock_paravirt.h +2
-0+#include <asm/mshyperv.h> +/* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them. Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) · line 313
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h +4
-0@@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) wait_early = true;break ; } +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif
Secondly; how come you thought that was acceptable in any way shape or form?
Quoted 9 lines
Quoted 4 lines
vcpu_is_preempted()is already part of this loop. And this is a paravirt hook. Can't you make use of that? This might require adding another parameter to this hook, but I'd prefer that over another pv-spinlock hook.I agree with Juergen on that. I would suggest rename the
vcpu_is_preemptedhook into a more genericvcpu_stop_spinning, perhaps, so different hypervisors can act on the information accordingly. Adding an extra parameter is fine.
No; no extra parameters. vcpu_is_preempted() is a simple and intuitive interface. Why would we want to make it complicated?
Waiman Long
reviewerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 38 lines · Peter Zijlstra
Firstly, who come a patch that is grubbing around in
kernel/locking/has an x86/hyperv subject and isn't Cc'ed to the locking maintainers?Quoted 14 lines · Waiman Long
Quoted 13 lines · Juergen Gross
Quoted 12 lines · Yi Sun
Quoted 1 line · Juergen Gross
On 19/10/2018 15:13, Yi Sun wrote:
[...]
Quoted 8 lines
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h +0
-0@@ -7,6 +7,8 @@#include <linux/bootmem.h> #include <linux/debug_locks.h> Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
kernel/locking/qspinlock_paravirt.h +2
-0+#include <asm/mshyperv.h> +/* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them. Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) · line 313
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h +4
-0@@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) wait_early = true;break ; } +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif Secondly; how come you thought that was acceptable in any way shape or form?
Quoted 4 lines
Quoted 4 lines
vcpu_is_preempted()is already part of this loop. And this is a paravirt hook. Can't you make use of that? This might require adding another parameter to this hook, but I'd prefer that over another pv-spinlock hook.
Quoted 6 lines
Quoted 4 lines
I agree with Juergen on that. I would suggest rename the
vcpu_is_preemptedhook into a more genericvcpu_stop_spinning, perhaps, so different hypervisors can act on the information accordingly. Adding an extra parameter is fine.No; no extra parameters.
vcpu_is_preempted()is a simple and intuitive interface. Why would we want to make it complicated?
Hyperv seems to do it in a somewhat different way by looking at the spin counter and decide if it should continue. I don't know why they do that and what advantage it has.
The current patch is definitely not OK. A revised patch that makes use of an existing paravirt hook will be more acceptable. Again, I would like to see some performance figure and why they do it this way to see if it is worthwhile to change the existing interface.
Cheers,
Longman
Peter Zijlstra
maintainerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 7 lines · Waiman Long
Quoted 6 lines
Quoted 4 lines
I agree with Juergen on that. I would suggest rename the
vcpu_is_preemptedhook into a more genericvcpu_stop_spinning, perhaps, so different hypervisors can act on the information accordingly. Adding an extra parameter is fine.No; no extra parameters.
vcpu_is_preempted()is a simple and intuitive interface. Why would we want to make it complicated?
Quoted 8 lines
Hyperv seems to do it in a somewhat different way by looking at the spin counter and decide if it should continue. I don't know why they do that and what advantage it has.
The current patch is definitely not OK. A revised patch that makes use of an existing paravirt hook will be more acceptable. Again, I would like to see some performance figure and why they do it this way to see if it is worthwhile to change the existing interface.
Note that there are vcpu_is_preempted() users that are not in a spin-loop.
Waiman Long
reviewerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 12 lines · Peter Zijlstra
Quoted 10 lines · Waiman Long
Quoted 6 lines
Quoted 4 lines
I agree with Juergen on that. I would suggest rename the
vcpu_is_preemptedhook into a more genericvcpu_stop_spinning, perhaps, so different hypervisors can act on the information accordingly. Adding an extra parameter is fine.No; no extra parameters.
vcpu_is_preempted()is a simple and intuitive interface. Why would we want to make it complicated?Hyperv seems to do it in a somewhat different way by looking at the spin counter and decide if it should continue. I don't know why they do that and what advantage it has.
Quoted 6 lines
Quoted 4 lines
The current patch is definitely not OK. A revised patch that makes use of an existing paravirt hook will be more acceptable. Again, I would like to see some performance figure and why they do it this way to see if it is worthwhile to change the existing interface.
Note that there are
vcpu_is_preempted()users that are not in a spin-loop.
You are right. I forgot about that. In that case, someone has to prove that using an alternative way to stop spinning really has a performance advantage compared to what we already have today.
Cheers,
Longman
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
In reply to Peter ZijlstraQuoted 4 lines · Peter Zijlstra
Firstly, who come a patch that is grubbing around in
kernel/locking/has an x86/hyperv subject and isn't Cc'ed to the locking maintainers?
I am sorry. That is my fault to forget to add locking maintainers.
Quoted 15 lines · Waiman Long
Quoted 14 lines · Juergen Gross
Quoted 13 lines · Yi Sun
Quoted 12 lines · Juergen Gross
Quoted 1 line · Yi Sun
[...]
Quoted 8 lines
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h +0
-0@@ -7,6 +7,8 @@#include <linux/bootmem.h> #include <linux/debug_locks.h>
Quoted 17 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
kernel/locking/qspinlock_paravirt.h +2
-0+#include <asm/mshyperv.h> +/* * Implement paravirt qspinlocks; the general idea is to halt the vcpus instead * of spinning them. Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) · line 313
Quoted 8 lines
kernel/locking/qspinlock_paravirt.h +4
-0@@ -305,6 +307,10 @@ static void pv_wait_node(struct mcs_spinlock *node, struct mcs_spinlock *prev) wait_early = true;break ; } +#if defined(CONFIG_X86_64 ) && defined(CONFIG_PARAVIRT_SPINLOCKS ) &&IS_ENABLED (CONFIG_HYPERV ) +if (!hv_notify_long_spin_wait (SPIN_THRESHOLD - loop)) +break ; +#endif Secondly; how come you thought that was acceptable in any way shape or form?
Sorry for that. Will try another way.
Quoted 12 lines
Quoted 9 lines
Quoted 4 lines
vcpu_is_preempted()is already part of this loop. And this is a paravirt hook. Can't you make use of that? This might require adding another parameter to this hook, but I'd prefer that over another pv-spinlock hook.I agree with Juergen on that. I would suggest rename the
vcpu_is_preemptedhook into a more genericvcpu_stop_spinning, perhaps, so different hypervisors can act on the information accordingly. Adding an extra parameter is fine.No; no extra parameters.
vcpu_is_preempted()is a simple and intuitive interface. Why would we want to make it complicated?
Peter Zijlstra
maintainerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 11 lines · Yi Sun
Quoted 8 lines · Peter Zijlstra
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+#if defined(CONFIG_X86_64) && defined(CONFIG_PARAVIRT_SPINLOCKS) && IS_ENABLED(CONFIG_HYPERV) + if (!hv_notify_long_spin_wait(SPIN_THRESHOLD - loop)) + break; +#endifSecondly; how come you thought that was acceptable in any way shape or form?
Sorry for that. Will try another way.
Can you try and explain why vcpu_is_preempted() doesn't work for you?
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Yi Sun
Quoted 8 lines · Peter Zijlstra
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+#if defined(CONFIG_X86_64) && defined(CONFIG_PARAVIRT_SPINLOCKS) && IS_ENABLED(CONFIG_HYPERV) + if (!hv_notify_long_spin_wait(SPIN_THRESHOLD - loop)) + break; +#endifSecondly; how come you thought that was acceptable in any way shape or form?
Quoted 3 lines
Quoted 1 line
Sorry for that. Will try another way.
Can you try and explain why
vcpu_is_preempted()doesn't work for you?
I thought HvSpinWaitInfo is used to notify hypervisor the spin number which is different with vcpu_is_preempted. So I did not consider vcpu_is_preempted.
But HvSpinWaitInfo is a quite simple function and could be combined with vcpu_is_preempted together. So I think it is OK to use vcpu_is_preempted to make codes clean. I will have a try.
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 16 lines · Yi Sun
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Yi Sun
Quoted 8 lines · Peter Zijlstra
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+#if defined(CONFIG_X86_64) && defined(CONFIG_PARAVIRT_SPINLOCKS) && IS_ENABLED(CONFIG_HYPERV) + if (!hv_notify_long_spin_wait(SPIN_THRESHOLD - loop)) + break; +#endifSecondly; how come you thought that was acceptable in any way shape or form?
Quoted 3 lines
Quoted 1 line
Sorry for that. Will try another way.
Can you try and explain why
vcpu_is_preempted()doesn't work for you?
Quoted 7 lines
I thought HvSpinWaitInfo is used to notify hypervisor the spin number which is different with
vcpu_is_preempted. So I did not considervcpu_is_preempted.But HvSpinWaitInfo is a quite simple function and could be combined with
vcpu_is_preemptedtogether. So I think it is OK to usevcpu_is_preemptedto make codes clean. I will have a try.
After checking codes, there is one issue to call vcpu_is_preempted. There are two spin loops in qspinlock_paravirt.h. One loop in 'pv_wait_node' calls vcpu_is_preempted. But another loop in 'pv_wait_head_or_lock' does not call vcpu_is_preempted. It also does not call any other ops of 'pv_lock_ops' in the loop. So I am afraid we have to add one more ops in 'pv_lock_ops' to do this.
Peter Zijlstra
maintainerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 13 lines · Yi Sun
Quoted 11 lines · Yi Sun
Quoted 1 line · Peter Zijlstra
Can you try and explain why
vcpu_is_preempted()doesn't work for you?I thought HvSpinWaitInfo is used to notify hypervisor the spin number which is different with
vcpu_is_preempted. So I did not considervcpu_is_preempted.But HvSpinWaitInfo is a quite simple function and could be combined with
vcpu_is_preemptedtogether. So I think it is OK to usevcpu_is_preemptedto make codes clean. I will have a try.
Quoted 6 lines
After checking codes, there is one issue to call
vcpu_is_preempted. There are two spin loops inqspinlock_paravirt.h. One loop in 'pv_wait_node' callsvcpu_is_preempted. But another loop in 'pv_wait_head_or_lock' does not callvcpu_is_preempted. It also does not call any other ops of 'pv_lock_ops' in the loop. So I am afraid we have to add one more ops in 'pv_lock_ops' to do this.
Why? Would not something like the below cure that? Waiman, can you have a look at this; I always forget how that paravirt crud works.
--- kernel/locking/qspinlock.c | 5 +++-- kernel/locking/qspinlock_paravirt.h | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-)
kernel/locking/qspinlock.c +3 -2
@@ -286,7 +286,8 @@ static __always_inline void __pv_wait_node(struct mcs_spinlock *node, static __always_inline void __pv_kick_node(struct qspinlock *lock,struct mcs_spinlock *node) { } static __always_inline u32 __pv_wait_head_or_lock(struct qspinlock *lock, -struct mcs_spinlock *node) +struct mcs_spinlock *node, +struct mcs_spinlock *prev) {return 0 ; }#define pv_enabled () false
@@ -500,7 +501,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)* If PV isn't active, 0 will be returned instead. * */ -if ((val =pv_wait_head_or_lock (lock, node))) +if ((val =pv_wait_head_or_lock (lock, node, prev))) goto locked; val =atomic_cond_read_acquire (&lock->val, !(VAL & _Q_LOCKED_PENDING_MASK));
kernel/locking/qspinlock_paravirt.h +3 -2
@@ -399,9 +399,10 @@ static void pv_kick_node(struct qspinlock *lock, struct mcs_spinlock *node)* The current value of the lock will be returned for additional processing. */ static u32 -pv_wait_head_or_lock (struct qspinlock *lock,struct mcs_spinlock *node) +pv_wait_head_or_lock (struct qspinlock *lock,struct mcs_spinlock *node,struct mcs_spinlock *prev) {struct pv_node *pn = (struct pv_node *)node; +struct pv_node *pp = (struct pv_node *)prev;struct qspinlock **lp =NULL ;int waitcnt =0 ;int loop;
@@ -430,7 +431,7 @@ pv_wait_head_or_lock(struct qspinlock *lock, struct mcs_spinlock *node)* disable lock stealing before attempting to acquire the lock. */ set_pending (lock); -for (loop =SPIN_THRESHOLD ; loop; loop--) { +for (loop =SPIN_THRESHOLD ; loop && !vcpu_is_preempted (prev->cpu); loop--) {if (trylock_clear_pending (lock)) goto gotlock;cpu_relax ();
Waiman Long
reviewerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Yi Sun
Quoted 9 lines · Yi Sun
Quoted 1 line · Peter Zijlstra
Can you try and explain why
vcpu_is_preempted()doesn't work for you?I thought HvSpinWaitInfo is used to notify hypervisor the spin number which is different with
vcpu_is_preempted. So I did not considervcpu_is_preempted.But HvSpinWaitInfo is a quite simple function and could be combined with
vcpu_is_preemptedtogether. So I think it is OK to usevcpu_is_preemptedto make codes clean. I will have a try.
Quoted 8 lines
Quoted 6 lines
After checking codes, there is one issue to call
vcpu_is_preempted. There are two spin loops inqspinlock_paravirt.h. One loop in 'pv_wait_node' callsvcpu_is_preempted. But another loop in 'pv_wait_head_or_lock' does not callvcpu_is_preempted. It also does not call any other ops of 'pv_lock_ops' in the loop. So I am afraid we have to add one more ops in 'pv_lock_ops' to do this.Why? Would not something like the below cure that? Waiman, can you have a look at this; I always forget how that paravirt crud works.
There are two major reasons why the vcpu_is_preempt() test isn't done at pv_wait_head_or_lock(). First of all, we may not have a valid prev pointer after all if it is the first one to enter the queue while the lock is busy. Secondly, because of lock stealing, the cpu number pointed by a valid prev pointer may not be the actual cpu that is currently holding the lock. Another minor reason is that we want to minimize the lock transfer latency and so don't want to sleep too early while waiting at the queue head.
Cheers,
Longman
Peter Zijlstra
maintainerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 21 lines · Waiman Long
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Yi Sun
Quoted 9 lines · Yi Sun
Quoted 1 line · Peter Zijlstra
Can you try and explain why
vcpu_is_preempted()doesn't work for you?I thought HvSpinWaitInfo is used to notify hypervisor the spin number which is different with
vcpu_is_preempted. So I did not considervcpu_is_preempted.But HvSpinWaitInfo is a quite simple function and could be combined with
vcpu_is_preemptedtogether. So I think it is OK to usevcpu_is_preemptedto make codes clean. I will have a try.Quoted 8 lines
Quoted 6 lines
After checking codes, there is one issue to call
vcpu_is_preempted. There are two spin loops inqspinlock_paravirt.h. One loop in 'pv_wait_node' callsvcpu_is_preempted. But another loop in 'pv_wait_head_or_lock' does not callvcpu_is_preempted. It also does not call any other ops of 'pv_lock_ops' in the loop. So I am afraid we have to add one more ops in 'pv_lock_ops' to do this.Why? Would not something like the below cure that? Waiman, can you have a look at this; I always forget how that paravirt crud works.
Quoted 8 lines
There are two major reasons why the
vcpu_is_preempt()test isn't done atpv_wait_head_or_lock(). First of all, we may not have a valid prev pointer after all if it is the first one to enter the queue while the lock is busy. Secondly, because of lock stealing, the cpu number pointed by a valid prev pointer may not be the actual cpu that is currently holding the lock. Another minor reason is that we want to minimize the lock transfer latency and so don't want to sleep too early while waiting at the queue head.
So Yi, are you actually seeing a problem? If so, can you give details?
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 31 lines · Peter Zijlstra
Quoted 21 lines · Waiman Long
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Yi Sun
Quoted 9 lines · Yi Sun
Quoted 1 line · Peter Zijlstra
Can you try and explain why
vcpu_is_preempted()doesn't work for you?I thought HvSpinWaitInfo is used to notify hypervisor the spin number which is different with
vcpu_is_preempted. So I did not considervcpu_is_preempted.But HvSpinWaitInfo is a quite simple function and could be combined with
vcpu_is_preemptedtogether. So I think it is OK to usevcpu_is_preemptedto make codes clean. I will have a try.Quoted 8 lines
Quoted 6 lines
After checking codes, there is one issue to call
vcpu_is_preempted. There are two spin loops inqspinlock_paravirt.h. One loop in 'pv_wait_node' callsvcpu_is_preempted. But another loop in 'pv_wait_head_or_lock' does not callvcpu_is_preempted. It also does not call any other ops of 'pv_lock_ops' in the loop. So I am afraid we have to add one more ops in 'pv_lock_ops' to do this.Why? Would not something like the below cure that? Waiman, can you have a look at this; I always forget how that paravirt crud works.
Quoted 8 lines
There are two major reasons why the
vcpu_is_preempt()test isn't done atpv_wait_head_or_lock(). First of all, we may not have a valid prev pointer after all if it is the first one to enter the queue while the lock is busy. Secondly, because of lock stealing, the cpu number pointed by a valid prev pointer may not be the actual cpu that is currently holding the lock. Another minor reason is that we want to minimize the lock transfer latency and so don't want to sleep too early while waiting at the queue head.
Quoted 1 line
So Yi, are you actually seeing a problem? If so, can you give details?
Where does the patch come from? I cannot find it through google.
Per Waiman's comment, it seems not suitable to call vcpu_is_preempted() in pv_wait_head_or_lock(). So, we cannot make HvSpinWaitInfo notification through vcpu_is_preempted() for such case. Based on that, I suggest to add one more callback function in pv_lock_ops.
BTW, which performance test do you suggest? I am trying to test it by AIM7.
Peter Zijlstra
maintainerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 5 lines · Yi Sun
Quoted 1 line · Peter Zijlstra
So Yi, are you actually seeing a problem? If so, can you give details?
Where does the patch come from? I cannot find it through google.
What patch!? The one I posted:
https://lkml.kernel.org/r/[email protected]
I made that up real quick.
Quoted 4 lines
Per Waiman's comment, it seems not suitable to call
vcpu_is_preempted()inpv_wait_head_or_lock(). So, we cannot make HvSpinWaitInfo notification throughvcpu_is_preempted()for such case. Based on that, I suggest to add one more callback function inpv_lock_ops.
We're not doing anything until you can show a problem. If it ain't broken, don't fix it etc..
Quoted 2 lines
BTW, which performance test do you suggest? I am trying to test it by AIM7.
Waiman?
Waiman Long
reviewerRe: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
In reply to Yi SunQuoted 30 lines · Yi Sun
Quoted 29 lines · Peter Zijlstra
Quoted 28 lines · Waiman Long
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Yi Sun
Quoted 9 lines · Yi Sun
Quoted 1 line · Peter Zijlstra
Can you try and explain why
vcpu_is_preempted()doesn't work for you?I thought HvSpinWaitInfo is used to notify hypervisor the spin number which is different with
vcpu_is_preempted. So I did not considervcpu_is_preempted.But HvSpinWaitInfo is a quite simple function and could be combined with
vcpu_is_preemptedtogether. So I think it is OK to usevcpu_is_preemptedto make codes clean. I will have a try.Quoted 8 lines
Quoted 6 lines
After checking codes, there is one issue to call
vcpu_is_preempted. There are two spin loops inqspinlock_paravirt.h. One loop in 'pv_wait_node' callsvcpu_is_preempted. But another loop in 'pv_wait_head_or_lock' does not callvcpu_is_preempted. It also does not call any other ops of 'pv_lock_ops' in the loop. So I am afraid we have to add one more ops in 'pv_lock_ops' to do this.Why? Would not something like the below cure that? Waiman, can you have a look at this; I always forget how that paravirt crud works.
There are two major reasons why the
vcpu_is_preempt()test isn't done atpv_wait_head_or_lock(). First of all, we may not have a valid prev pointer after all if it is the first one to enter the queue while the lock is busy. Secondly, because of lock stealing, the cpu number pointed by a valid prev pointer may not be the actual cpu that is currently holding the lock. Another minor reason is that we want to minimize the lock transfer latency and so don't want to sleep too early while waiting at the queue head.
Quoted 7 lines
Quoted 1 line
So Yi, are you actually seeing a problem? If so, can you give details?
Where does the patch come from? I cannot find it through google.
Per Waiman's comment, it seems not suitable to call
vcpu_is_preempted()inpv_wait_head_or_lock(). So, we cannot make HvSpinWaitInfo notification throughvcpu_is_preempted()for such case. Based on that, I suggest to add one more callback function inpv_lock_ops.
I am hesitant to add any additional check at the spinning loop in pv_wait_head_or_lock() especially one that is a hypercall or a callback that will take time to execute. The testing that I had done in the past indicated that it would slow down locking performance especially if the VM wasn't overcommitted at all.
Any additional slack in pv_wait_node() can be mitigated by the lock stealing that can happen. Slack in pv_wait_head_or_lock(), on the other hand, will certainly increase the lock transfer latency and impact performance. So you need performance data to show that it is worthwhile to do so.
As for performance test, the kernel has a builtin locktorture test if you configured it in. So show us the performance data with and without the patch.
Cheers,
Longman
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Quoted 45 lines · Waiman Long
Quoted 30 lines · Yi Sun
Quoted 29 lines · Peter Zijlstra
Quoted 28 lines · Waiman Long
Quoted 11 lines · Peter Zijlstra
Quoted 10 lines · Yi Sun
Quoted 9 lines · Yi Sun
Quoted 1 line · Peter Zijlstra
Can you try and explain why
vcpu_is_preempted()doesn't work for you?I thought HvSpinWaitInfo is used to notify hypervisor the spin number which is different with
vcpu_is_preempted. So I did not considervcpu_is_preempted.But HvSpinWaitInfo is a quite simple function and could be combined with
vcpu_is_preemptedtogether. So I think it is OK to usevcpu_is_preemptedto make codes clean. I will have a try.Quoted 8 lines
Quoted 6 lines
After checking codes, there is one issue to call
vcpu_is_preempted. There are two spin loops inqspinlock_paravirt.h. One loop in 'pv_wait_node' callsvcpu_is_preempted. But another loop in 'pv_wait_head_or_lock' does not callvcpu_is_preempted. It also does not call any other ops of 'pv_lock_ops' in the loop. So I am afraid we have to add one more ops in 'pv_lock_ops' to do this.Why? Would not something like the below cure that? Waiman, can you have a look at this; I always forget how that paravirt crud works.
There are two major reasons why the
vcpu_is_preempt()test isn't done atpv_wait_head_or_lock(). First of all, we may not have a valid prev pointer after all if it is the first one to enter the queue while the lock is busy. Secondly, because of lock stealing, the cpu number pointed by a valid prev pointer may not be the actual cpu that is currently holding the lock. Another minor reason is that we want to minimize the lock transfer latency and so don't want to sleep too early while waiting at the queue head.Quoted 7 lines
Quoted 1 line
So Yi, are you actually seeing a problem? If so, can you give details?
Where does the patch come from? I cannot find it through google.
Per Waiman's comment, it seems not suitable to call
vcpu_is_preempted()inpv_wait_head_or_lock(). So, we cannot make HvSpinWaitInfo notification throughvcpu_is_preempted()for such case. Based on that, I suggest to add one more callback function inpv_lock_ops.I am hesitant to add any additional check at the spinning loop in
pv_wait_head_or_lock()especially one that is a hypercall or a callback that will take time to execute. The testing that I had done in the past indicated that it would slow down locking performance especially if the VM wasn't overcommitted at all.
Quoted 6 lines
Any additional slack in
pv_wait_node()can be mitigated by the lock stealing that can happen. Slack inpv_wait_head_or_lock(), on the other hand, will certainly increase the lock transfer latency and impact performance. So you need performance data to show that it is worthwhile to do so.
Ok, I will make performance test to show if it is worthwhile to call SpinWaitInfo in pv_wait_head_or_lock.
Quoted 3 lines
As for performance test, the kernel has a builtin locktorture test if you configured it in. So show us the performance data with and without the patch.
Thank you! I will make performance test for whole patch.
Quoted 3 lines
Cheers,
Longman
RE: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
In reply to Yi SunFrom: Yi Sun <[email protected]> Sent: Friday, October 19, 2018 6:14 AM
Quoted 11 lines
The HvNotifyLongSpinWait hypercall (
HVCALL_NOTIFY_LONG_SPIN_WAIT) is 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 Virtual Machine. This scheduling hint improves the scalability of VMs with more than one virtual processor on Hyper-V.Per MSFT TLFS, the retry number (SpinWaitInfo) is sent to hypervisor only when the retry number exceeds the recommended number. If recommended number is
0xFFFFFFFF, never retry.
The HvNotifyLongSpinWait hypercall should be understood to be advisory only. As you noted, it is a scheduling hint to the hypervisor that some virtual CPU in the VM holds a spin lock. Even though Linux knows which vCPU holds the spin lock, the hypercall does not provide a way to give that information to Hyper-V. The hypercall always returns immediately.
The "retry number" is a bit mis-named in the Hyper-V Top Level Functional Spec (TLFS). It is essentially a threshold value. Hyper-V is saying "don't bother to advise me about the spin lock until you have done a certain number of spins." This threshold prevents over-notifying Hyper-V such that the notification becomes somewhat
meaningless. It's not immediately clear to me why the hypercall passes
that value as an input, but maybe it lets the Hyper-V scheduler prioritize among vCPUs based on how many times they have spun for a lock. I think we were told that current Hyper-V implementations ignore this input value anyway.
I believe the description of the sentinel value 0xFFFFFFFF in the Hyper-V TLFS is incorrect. Because it is the max possible threshold value, that value in the EBX register just means to not ever bother to
notify. The description should be "0xFFFFFFFF indicates never to notify."
The value does *not* indicate anything about retrying to obtain the spin lock.
Quoted 3 lines
static bool
__initdatahv_pvspin= true;+bool
hv_notify_long_spin_wait(int retry_num)
retry_num should probably be declared as unsigned int. You don't want it to be treated as a negative number if the high order bit is set.
arch/x86/hyperv/hv_spinlock.c line 32
Quoted 11 lines
arch/x86/hyperv/hv_spinlock.c +11
-0+{ +/* +* Per MSFT TLFS, the SpinWaitInfo is sent to hypervisor only when +* the retry number exceeds the recommended number. +* +* If recommended number is 0xFFFFFFFF, never retry. +*/ +if (ms_hyperv.num_spin_retry ==HYPERV_SPINLOCK_RETRY_NEVER ) +return false; + +if ((0 == retry_num % ms_hyperv.num_spin_retry) && retry_num)
I don't know if the "%" function is right here. Your implementation will
notify Hyper-V on every multiple of num_spin_retry. The alternative is to
notify once when the threshold is exceeded, and never again for this particular attempt to obtain a spin lock. We should check with the Hyper-V team for which approach they expect to be used.
arch/x86/hyperv/hv_spinlock.c line 34
Quoted 2 lines
arch/x86/hyperv/hv_spinlock.c +2
-0+hv_do_fast_hypercall8 (HVCALL_NOTIFY_LONG_SPIN_WAIT , + retry_num);
The Hyper-V TLFS seems to be inconsistent on whether the input parameter
is 32-bits or 64-bits. In one place it is typed as UINT64, but in another place
it is shown as only 4 bytes. Need to clear this up with the Hyper-V team as well.
Review
Quoted 2 lines
(quoted diff) +2
-0+ + return true;
I don't see a need for this function to return true vs. false. Any calling code
should not change its behavior based on num_spin_retry. This function will
either notify Hyper-V or not notify Hyper-V, depending on whether the number of attempts to obtain the spinlock meets the threshold. But calling code will do the same thing regardless of whether such a notification is made.
Michael
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
Hi, Michael,
Thanks a lot for the review and comments! Let us sync with Hyper-V team to confirm these suspicious points.
BRs,
Sun Yi
Quoted 31 lines · Michael Kelley
From: Yi Sun <[email protected]> Sent: Friday, October 19, 2018 6:14 AM
Quoted 11 lines
The HvNotifyLongSpinWait hypercall (
HVCALL_NOTIFY_LONG_SPIN_WAIT) is 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 Virtual Machine. This scheduling hint improves the scalability of VMs with more than one virtual processor on Hyper-V.Per MSFT TLFS, the retry number (SpinWaitInfo) is sent to hypervisor only when the retry number exceeds the recommended number. If recommended number is
0xFFFFFFFF, never retry.The HvNotifyLongSpinWait hypercall should be understood to be advisory only. As you noted, it is a scheduling hint to the hypervisor that some virtual CPU in the VM holds a spin lock. Even though Linux knows which vCPU holds the spin lock, the hypercall does not provide a way to give that information to Hyper-V. The hypercall always returns immediately.
The "retry number" is a bit mis-named in the Hyper-V Top Level Functional Spec (TLFS). It is essentially a threshold value. Hyper-V is saying "don't bother to advise me about the spin lock until you have done a certain number of spins." This threshold prevents over-notifying Hyper-V such that the notification becomes somewhat
meaningless. It's not immediately clear to me why the hypercall passes
that value as an input, but maybe it lets the Hyper-V scheduler prioritize among vCPUs based on how many times they have spun for a lock. I think we were told that current Hyper-V implementations ignore this input value anyway.
Quoted 7 lines
I believe the description of the sentinel value
0xFFFFFFFFin the Hyper-V TLFS is incorrect. Because it is the max possible threshold value, that value in the EBX register just means to not ever bother to
notify. The description should be "0xFFFFFFFFindicates never to notify."
The value does *not* indicate anything about retrying to obtain the spin lock.
I will send mail to Hyper-V team to clarify these.
Quoted 8 lines
Quoted 3 lines
static bool
__initdatahv_pvspin= true;+bool
hv_notify_long_spin_wait(int retry_num)
retry_numshould probably be declared as unsigned int. You don't want it to be treated as a negative number if the high order bit is set.
Yes, I should declare it as 'unsigned int'. Thanks!
Quoted 36 lines
arch/x86/hyperv/hv_spinlock.c line 32
Quoted 11 lines
arch/x86/hyperv/hv_spinlock.c +11
-0+{ +/* +* Per MSFT TLFS, the SpinWaitInfo is sent to hypervisor only when +* the retry number exceeds the recommended number. +* +* If recommended number is 0xFFFFFFFF, never retry. +*/ +if (ms_hyperv.num_spin_retry ==HYPERV_SPINLOCK_RETRY_NEVER ) +return false; + +if ((0 == retry_num % ms_hyperv.num_spin_retry) && retry_num)I don't know if the "%" function is right here. Your implementation will
notify Hyper-V on every multiple ofnum_spin_retry. The alternative is to
notify once when the threshold is exceeded, and never again for this particular attempt to obtain a spin lock. We should check with the Hyper-V team for which approach they expect to be used.arch/x86/hyperv/hv_spinlock.c line 34
Quoted 2 lines
arch/x86/hyperv/hv_spinlock.c +2
-0+hv_do_fast_hypercall8 (HVCALL_NOTIFY_LONG_SPIN_WAIT , + retry_num);The Hyper-V TLFS seems to be inconsistent on whether the input parameter
is 32-bits or 64-bits. In one place it is typed as UINT64, but in another place
it is shown as only 4 bytes. Need to clear this up with the Hyper-V team as well.Review
Quoted 2 lines
(quoted diff) +2
-0+ + return true;I don't see a need for this function to return true vs. false. Any calling code
should not change its behavior based onnum_spin_retry. This function will
either notify Hyper-V or not notify Hyper-V, depending on whether the number of attempts to obtain the spinlock meets the threshold. But calling code will do the same thing regardless of whether such a notification is made.Michael
Re: [PATCH v1 2/2] x86/hyperv: make HvNotifyLongSpinWait hypercall
In reply to Michael KelleyAfter syncing with Hyper-V team, we have got answers as below.
Quoted 31 lines · Michael Kelley
From: Yi Sun <[email protected]> Sent: Friday, October 19, 2018 6:14 AM
Quoted 11 lines
The HvNotifyLongSpinWait hypercall (
HVCALL_NOTIFY_LONG_SPIN_WAIT) is 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 Virtual Machine. This scheduling hint improves the scalability of VMs with more than one virtual processor on Hyper-V.Per MSFT TLFS, the retry number (SpinWaitInfo) is sent to hypervisor only when the retry number exceeds the recommended number. If recommended number is
0xFFFFFFFF, never retry.The HvNotifyLongSpinWait hypercall should be understood to be advisory only. As you noted, it is a scheduling hint to the hypervisor that some virtual CPU in the VM holds a spin lock. Even though Linux knows which vCPU holds the spin lock, the hypercall does not provide a way to give that information to Hyper-V. The hypercall always returns immediately.
The "retry number" is a bit mis-named in the Hyper-V Top Level Functional Spec (TLFS). It is essentially a threshold value. Hyper-V is saying "don't bother to advise me about the spin lock until you have done a certain number of spins." This threshold prevents over-notifying Hyper-V such that the notification becomes somewhat
meaningless. It's not immediately clear to me why the hypercall passes
that value as an input, but maybe it lets the Hyper-V scheduler prioritize among vCPUs based on how many times they have spun for a lock. I think we were told that current Hyper-V implementations ignore this input value anyway.
Quoted 7 lines
I believe the description of the sentinel value
0xFFFFFFFFin the Hyper-V TLFS is incorrect. Because it is the max possible threshold value, that value in the EBX register just means to not ever bother to
notify. The description should be "0xFFFFFFFFindicates never to notify."
The value does *not* indicate anything about retrying to obtain the spin lock.
You are right. 0xFFFFFFFF only indicates never to notify. We should not break the spin loop.
Quoted 8 lines
Quoted 3 lines
static bool
__initdatahv_pvspin= true;+bool
hv_notify_long_spin_wait(int retry_num)
retry_numshould probably be declared as unsigned int. You don't want it to be treated as a negative number if the high order bit is set.
Yes, thanks!
Quoted 9 lines
Quoted 9 lines
arch/x86/hyperv/hv_spinlock.c +9
-0+{ +/* +* Per MSFT TLFS, the SpinWaitInfo is sent to hypervisor only when +* the retry number exceeds the recommended number. +* +* If recommended number is 0xFFFFFFFF, never retry. +*/ +if (ms_hyperv.num_spin_retry ==HYPERV_SPINLOCK_RETRY_NEVER ) +return false;
Quoted 9 lines
arch/x86/hyperv/hv_spinlock.c line 32
Quoted 2 lines
arch/x86/hyperv/hv_spinlock.c +2
-0+ +if ((0 == retry_num % ms_hyperv.num_spin_retry) && retry_num)I don't know if the "%" function is right here. Your implementation will
notify Hyper-V on every multiple ofnum_spin_retry. The alternative is to
notify once when the threshold is exceeded, and never again for this particular attempt to obtain a spin lock. We should check with the Hyper-V team for which approach they expect to be used.
We should send the notification on every multiple of the recommended number.
Quoted 8 lines
arch/x86/hyperv/hv_spinlock.c line 34
Quoted 2 lines
arch/x86/hyperv/hv_spinlock.c +2
-0+hv_do_fast_hypercall8 (HVCALL_NOTIFY_LONG_SPIN_WAIT , + retry_num);The Hyper-V TLFS seems to be inconsistent on whether the input parameter
is 32-bits or 64-bits. In one place it is typed as UINT64, but in another place
it is shown as only 4 bytes. Need to clear this up with the Hyper-V team as well.
It is 32-bits.
Quoted 9 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + return true;I don't see a need for this function to return true vs. false. Any calling code
should not change its behavior based onnum_spin_retry. This function will
either notify Hyper-V or not notify Hyper-V, depending on whether the number of attempts to obtain the spinlock meets the threshold. But calling code will do the same thing regardless of whether such a notification is made.
You are right. I will change it to 'void'.
Quoted 1 line
Michael