[PATCH 0/3] Add a unified parameter "nopvspin"

[PATCH 0/3] Add a unified parameter "nopvspin"

[Zhenzhong Duan]

There are cases folks want to disable spinlock optimization for
debug/test purpose. Xen and hyperv already have parameters "xen_nopvspin"
and "hv_nopvspin" to support that, but kvm doesn't.

The first patch adds that feature to KVM guest with "nopvspin".
I also changed print code in first patch, I'm not sure if I should
pick that part into a separate patch, just let me know if it's
preferred.

For compatibility reason original parameters "xen_nopvspin" and
"hv_nopvspin" are retained and marked obsolete.


Thanks
Zhenzhong

[PATCH 1/3] KVM: X86: Add "nopvspin" parameter to disable PV spinlocks

[Zhenzhong Duan]

There are cases where a guest tries to switch spinlocks to bare metal
behavior (e.g. by setting "xen_nopvspin" on XEN platform and
"hv_nopvspin" on HYPER_V).

That feature is missed on KVM, add a new parameter "nopvspin" to disable
PV spinlocks for KVM guest.

This new parameter is also intended to replace "xen_nopvspin" and
"hv_nopvspin" in the future.

The global variable pvspin isn't defined as __initdata as it's used at
runtime by XEN guest.

Refactor the print stuff with pr_* which is preferred.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Radim Krcmar <rkrcmar@redhat.com>
Cc: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: Wanpeng Li <wanpengli@tencent.com>
Cc: Jim Mattson <jmattson@google.com>
Cc: Joerg Roedel <joro@8bytes.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
---
 Documentation/admin-guide/kernel-parameters.txt |  4 ++++
 arch/x86/include/asm/qspinlock.h                |  1 +
 arch/x86/kernel/kvm.c                           | 27 ++++++++++++++++---------
 kernel/locking/qspinlock.c                      |  7 +++++++
 4 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index c7ac2f3..4b956d8 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5330,6 +5330,10 @@
 			as generic guest with no PV drivers. Currently support
 			XEN HVM, KVM, HYPER_V and VMWARE guest.
 
+	nopvspin	[X86,KVM] Disables the qspinlock slow path
+			using PV optimizations which allow the hypervisor to
+			'idle' the guest on lock contention.
+
 	xirc2ps_cs=	[NET,PCMCIA]
 			Format:
 			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
index 444d6fd..34a4484 100644
--- a/arch/x86/include/asm/qspinlock.h
+++ b/arch/x86/include/asm/qspinlock.h
@@ -32,6 +32,7 @@ static __always_inline u32 queued_fetch_set_pending_acquire(struct qspinlock *lo
 extern void __pv_init_lock_hash(void);
 extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
 extern void __raw_callee_save___pv_queued_spin_unlock(struct qspinlock *lock);
+extern bool pvspin;
 
 #define	queued_spin_unlock queued_spin_unlock
 /**
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index e820568..7b8cf0d 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -7,6 +7,8 @@
  *   Authors: Anthony Liguori <aliguori@us.ibm.com>
  */
 
+#define pr_fmt(fmt) "KVM: " fmt
+
 #include <linux/context_tracking.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
@@ -286,7 +288,7 @@ static void kvm_register_steal_time(void)
 		return;
 
 	wrmsrl(MSR_KVM_STEAL_TIME, (slow_virt_to_phys(st) | KVM_MSR_ENABLED));
-	pr_info("kvm-stealtime: cpu %d, msr %llx\n",
+	pr_info("stealtime: cpu %d, msr %llx\n",
 		cpu, (unsigned long long) slow_virt_to_phys(st));
 }
 
@@ -321,7 +323,7 @@ static void kvm_guest_cpu_init(void)
 
 		wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
 		__this_cpu_write(apf_reason.enabled, 1);
-		printk(KERN_INFO"KVM setup async PF for cpu %d\n",
+		pr_info("setup async PF for cpu %d\n",
 		       smp_processor_id());
 	}
 
@@ -347,7 +349,7 @@ static void kvm_pv_disable_apf(void)
 	wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
 	__this_cpu_write(apf_reason.enabled, 0);
 
-	printk(KERN_INFO"Unregister pv shared memory for cpu %d\n",
+	pr_info("Unregister pv shared memory for cpu %d\n",
 	       smp_processor_id());
 }
 
@@ -509,7 +511,7 @@ static void kvm_setup_pv_ipi(void)
 {
 	apic->send_IPI_mask = kvm_send_ipi_mask;
 	apic->send_IPI_mask_allbutself = kvm_send_ipi_mask_allbutself;
-	pr_info("KVM setup pv IPIs\n");
+	pr_info("setup pv IPIs\n");
 }
 
 static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
@@ -639,11 +641,11 @@ static void __init kvm_guest_init(void)
 	    !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
 	    kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
 		smp_ops.send_call_func_ipi = kvm_smp_send_call_func_ipi;
-		pr_info("KVM setup pv sched yield\n");
+		pr_info("setup pv sched yield\n");
 	}
 	if (cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/kvm:online",
 				      kvm_cpu_online, kvm_cpu_down_prepare) < 0)
-		pr_err("kvm_guest: Failed to install cpu hotplug callbacks\n");
+		pr_err("failed to install cpu hotplug callbacks\n");
 #else
 	sev_map_percpu_data();
 	kvm_guest_cpu_init();
@@ -746,7 +748,7 @@ static __init int kvm_setup_pv_tlb_flush(void)
 			zalloc_cpumask_var_node(per_cpu_ptr(&__pv_tlb_mask, cpu),
 				GFP_KERNEL, cpu_to_node(cpu));
 		}
-		pr_info("KVM setup pv remote TLB flush\n");
+		pr_info("setup pv remote TLB flush\n");
 	}
 
 	return 0;
@@ -842,6 +844,13 @@ void __init kvm_spinlock_init(void)
 	if (num_possible_cpus() == 1)
 		return;
 
+	if (!pvspin) {
+		pr_info("PV spinlocks disabled\n");
+		static_branch_disable(&virt_spin_lock_key);
+		return;
+	}
+	pr_info("PV spinlocks enabled\n");
+
 	__pv_init_lock_hash();
 	pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
 	pv_ops.lock.queued_spin_unlock =
@@ -872,8 +881,8 @@ static void kvm_enable_host_haltpoll(void *i)
 void arch_haltpoll_enable(unsigned int cpu)
 {
 	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL)) {
-		pr_err_once("kvm: host does not support poll control\n");
-		pr_err_once("kvm: host upgrade recommended\n");
+		pr_err_once("host does not support poll control\n");
+		pr_err_once("host upgrade recommended\n");
 		return;
 	}
 
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index 2473f10..945b510 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -580,4 +580,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
 #include "qspinlock_paravirt.h"
 #include "qspinlock.c"
 
+bool pvspin = true;
+static __init int parse_nopvspin(char *arg)
+{
+	pvspin = false;
+	return 0;
+}
+early_param("nopvspin", parse_nopvspin);
 #endif
-- 
1.8.3.1

Re: [PATCH 1/3] KVM: X86: Add "nopvspin" parameter to disable PV spinlocks

[Vitaly Kuznetsov]

Zhenzhong Duan <zhenzhong.duan@oracle.com> writes:

> There are cases where a guest tries to switch spinlocks to bare metal
> behavior (e.g. by setting "xen_nopvspin" on XEN platform and
> "hv_nopvspin" on HYPER_V).
>
> That feature is missed on KVM, add a new parameter "nopvspin" to disable
> PV spinlocks for KVM guest.
>
> This new parameter is also intended to replace "xen_nopvspin" and
> "hv_nopvspin" in the future.

Any reason to not do it right now? We will probably need to have compat
code to support xen_nopvspin/hv_nopvspin too but emit a 'is deprecated'
warning.

>
> The global variable pvspin isn't defined as __initdata as it's used at
> runtime by XEN guest.
>
> Refactor the print stuff with pr_* which is preferred.

Please do it in a separate patch.

>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Radim Krcmar <rkrcmar@redhat.com>
> Cc: Sean Christopherson <sean.j.christopherson@intel.com>
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Wanpeng Li <wanpengli@tencent.com>
> Cc: Jim Mattson <jmattson@google.com>
> Cc: Joerg Roedel <joro@8bytes.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  4 ++++
>  arch/x86/include/asm/qspinlock.h                |  1 +
>  arch/x86/kernel/kvm.c                           | 27 ++++++++++++++++---------
>  kernel/locking/qspinlock.c                      |  7 +++++++
>  4 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index c7ac2f3..4b956d8 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -5330,6 +5330,10 @@
>  			as generic guest with no PV drivers. Currently support
>  			XEN HVM, KVM, HYPER_V and VMWARE guest.
>  
> +	nopvspin	[X86,KVM] Disables the qspinlock slow path
> +			using PV optimizations which allow the hypervisor to
> +			'idle' the guest on lock contention.
> +
>  	xirc2ps_cs=	[NET,PCMCIA]
>  			Format:
>  			<irq>,<irq_mask>,<io>,<full_duplex>,<do_sound>,<lockup_hack>[,<irq2>[,<irq3>[,<irq4>]]]
> diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h
> index 444d6fd..34a4484 100644
> --- a/arch/x86/include/asm/qspinlock.h
> +++ b/arch/x86/include/asm/qspinlock.h
> @@ -32,6 +32,7 @@ static __always_inline u32 queued_fetch_set_pending_acquire(struct qspinlock *lo
>  extern void __pv_init_lock_hash(void);
>  extern void __pv_queued_spin_lock_slowpath(struct qspinlock *lock, u32 val);
>  extern void __raw_callee_save___pv_queued_spin_unlock(struct qspinlock *lock);
> +extern bool pvspin;
>  
>  #define	queued_spin_unlock queued_spin_unlock
>  /**
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index e820568..7b8cf0d 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -7,6 +7,8 @@
>   *   Authors: Anthony Liguori <aliguori@us.ibm.com>
>   */
>  
> +#define pr_fmt(fmt) "KVM: " fmt
> +
>  #include <linux/context_tracking.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> @@ -286,7 +288,7 @@ static void kvm_register_steal_time(void)
>  		return;
>  
>  	wrmsrl(MSR_KVM_STEAL_TIME, (slow_virt_to_phys(st) | KVM_MSR_ENABLED));
> -	pr_info("kvm-stealtime: cpu %d, msr %llx\n",
> +	pr_info("stealtime: cpu %d, msr %llx\n",
>  		cpu, (unsigned long long) slow_virt_to_phys(st));
>  }
>  
> @@ -321,7 +323,7 @@ static void kvm_guest_cpu_init(void)
>  
>  		wrmsrl(MSR_KVM_ASYNC_PF_EN, pa);
>  		__this_cpu_write(apf_reason.enabled, 1);
> -		printk(KERN_INFO"KVM setup async PF for cpu %d\n",
> +		pr_info("setup async PF for cpu %d\n",
>  		       smp_processor_id());
>  	}
>  
> @@ -347,7 +349,7 @@ static void kvm_pv_disable_apf(void)
>  	wrmsrl(MSR_KVM_ASYNC_PF_EN, 0);
>  	__this_cpu_write(apf_reason.enabled, 0);
>  
> -	printk(KERN_INFO"Unregister pv shared memory for cpu %d\n",
> +	pr_info("Unregister pv shared memory for cpu %d\n",
>  	       smp_processor_id());
>  }
>  
> @@ -509,7 +511,7 @@ static void kvm_setup_pv_ipi(void)
>  {
>  	apic->send_IPI_mask = kvm_send_ipi_mask;
>  	apic->send_IPI_mask_allbutself = kvm_send_ipi_mask_allbutself;
> -	pr_info("KVM setup pv IPIs\n");
> +	pr_info("setup pv IPIs\n");
>  }
>  
>  static void kvm_smp_send_call_func_ipi(const struct cpumask *mask)
> @@ -639,11 +641,11 @@ static void __init kvm_guest_init(void)
>  	    !kvm_para_has_hint(KVM_HINTS_REALTIME) &&
>  	    kvm_para_has_feature(KVM_FEATURE_STEAL_TIME)) {
>  		smp_ops.send_call_func_ipi = kvm_smp_send_call_func_ipi;
> -		pr_info("KVM setup pv sched yield\n");
> +		pr_info("setup pv sched yield\n");
>  	}
>  	if (cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN, "x86/kvm:online",
>  				      kvm_cpu_online, kvm_cpu_down_prepare) < 0)
> -		pr_err("kvm_guest: Failed to install cpu hotplug callbacks\n");
> +		pr_err("failed to install cpu hotplug callbacks\n");
>  #else
>  	sev_map_percpu_data();
>  	kvm_guest_cpu_init();
> @@ -746,7 +748,7 @@ static __init int kvm_setup_pv_tlb_flush(void)
>  			zalloc_cpumask_var_node(per_cpu_ptr(&__pv_tlb_mask, cpu),
>  				GFP_KERNEL, cpu_to_node(cpu));
>  		}
> -		pr_info("KVM setup pv remote TLB flush\n");
> +		pr_info("setup pv remote TLB flush\n");
>  	}
>  
>  	return 0;
> @@ -842,6 +844,13 @@ void __init kvm_spinlock_init(void)
>  	if (num_possible_cpus() == 1)
>  		return;
>  
> +	if (!pvspin) {
> +		pr_info("PV spinlocks disabled\n");
> +		static_branch_disable(&virt_spin_lock_key);
> +		return;
> +	}
> +	pr_info("PV spinlocks enabled\n");
> +
>  	__pv_init_lock_hash();
>  	pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
>  	pv_ops.lock.queued_spin_unlock =
> @@ -872,8 +881,8 @@ static void kvm_enable_host_haltpoll(void *i)
>  void arch_haltpoll_enable(unsigned int cpu)
>  {
>  	if (!kvm_para_has_feature(KVM_FEATURE_POLL_CONTROL)) {
> -		pr_err_once("kvm: host does not support poll control\n");
> -		pr_err_once("kvm: host upgrade recommended\n");
> +		pr_err_once("host does not support poll control\n");
> +		pr_err_once("host upgrade recommended\n");
>  		return;
>  	}
>  
> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
> index 2473f10..945b510 100644
> --- a/kernel/locking/qspinlock.c
> +++ b/kernel/locking/qspinlock.c
> @@ -580,4 +580,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>  #include "qspinlock_paravirt.h"
>  #include "qspinlock.c"
>  
> +bool pvspin = true;
> +static __init int parse_nopvspin(char *arg)
> +{
> +	pvspin = false;
> +	return 0;
> +}
> +early_param("nopvspin", parse_nopvspin);
>  #endif

-- 
Vitaly

Re: [PATCH 1/3] KVM: X86: Add "nopvspin" parameter to disable PV spinlocks

[Zhenzhong Duan]

On 2019/9/30 23:41, Vitaly Kuznetsov wrote:
> Zhenzhong Duan<zhenzhong.duan@oracle.com>  writes:
>
>> There are cases where a guest tries to switch spinlocks to bare metal
>> behavior (e.g. by setting "xen_nopvspin" on XEN platform and
>> "hv_nopvspin" on HYPER_V).
>>
>> That feature is missed on KVM, add a new parameter "nopvspin" to disable
>> PV spinlocks for KVM guest.
>>
>> This new parameter is also intended to replace "xen_nopvspin" and
>> "hv_nopvspin" in the future.
> Any reason to not do it right now? We will probably need to have compat
> code to support xen_nopvspin/hv_nopvspin too but emit a 'is deprecated'
> warning.

Sorry the description isn't clear, I'll fix it.

I did the compat work in the other two patches.
[PATCH 2/3] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"
[PATCH 3/3] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"

>
>> The global variable pvspin isn't defined as __initdata as it's used at
>> runtime by XEN guest.
>>
>> Refactor the print stuff with pr_* which is preferred.
> Please do it in a separate patch.

Ok, I'll do that in v2. Thanks for review.

Zhenzhong

Re: [PATCH 1/3] KVM: X86: Add "nopvspin" parameter to disable PV spinlocks

[Vitaly Kuznetsov]

Zhenzhong Duan <zhenzhong.duan@oracle.com> writes:

> On 2019/9/30 23:41, Vitaly Kuznetsov wrote:
>> Zhenzhong Duan<zhenzhong.duan@oracle.com>  writes:
>>
>>> There are cases where a guest tries to switch spinlocks to bare metal
>>> behavior (e.g. by setting "xen_nopvspin" on XEN platform and
>>> "hv_nopvspin" on HYPER_V).
>>>
>>> That feature is missed on KVM, add a new parameter "nopvspin" to disable
>>> PV spinlocks for KVM guest.
>>>
>>> This new parameter is also intended to replace "xen_nopvspin" and
>>> "hv_nopvspin" in the future.
>> Any reason to not do it right now? We will probably need to have compat
>> code to support xen_nopvspin/hv_nopvspin too but emit a 'is deprecated'
>> warning.
>
> Sorry the description isn't clear, I'll fix it.
>
> I did the compat work in the other two patches.
> [PATCH 2/3] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"
> [PATCH 3/3] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"
>

For some reason I got CCed only on the first one and moreover, I don't
see e.g PATCH3 on 'linux-hyperv' mailing list.

>>
>>> The global variable pvspin isn't defined as __initdata as it's used at
>>> runtime by XEN guest.
>>>
>>> Refactor the print stuff with pr_* which is preferred.
>> Please do it in a separate patch.
>
> Ok, I'll do that in v2. Thanks for review.

Thanks!

-- 
Vitaly

Re: [PATCH 1/3] KVM: X86: Add "nopvspin" parameter to disable PV spinlocks

[Zhenzhong Duan]

On 2019/10/1 16:39, Vitaly Kuznetsov wrote:
> Zhenzhong Duan<zhenzhong.duan@oracle.com>  writes:
>
>> On 2019/9/30 23:41, Vitaly Kuznetsov wrote:
>>> Zhenzhong Duan<zhenzhong.duan@oracle.com>   writes:
>>>
>>>> There are cases where a guest tries to switch spinlocks to bare metal
>>>> behavior (e.g. by setting "xen_nopvspin" on XEN platform and
>>>> "hv_nopvspin" on HYPER_V).
>>>>
>>>> That feature is missed on KVM, add a new parameter "nopvspin" to disable
>>>> PV spinlocks for KVM guest.
>>>>
>>>> This new parameter is also intended to replace "xen_nopvspin" and
>>>> "hv_nopvspin" in the future.
>>> Any reason to not do it right now? We will probably need to have compat
>>> code to support xen_nopvspin/hv_nopvspin too but emit a 'is deprecated'
>>> warning.
>> Sorry the description isn't clear, I'll fix it.
>>
>> I did the compat work in the other two patches.
>> [PATCH 2/3] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"
>> [PATCH 3/3] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"
>>
> For some reason I got CCed only on the first one and moreover,

The three patches have different maintainers/reviewers by get_maintainer.pl, I added
"Cc: maintainers/reviewers" to each patch then git-sendemail picked them automaticly.
I meaned to not disturb maintainers with the field they aren't in charge of. It looks
I'm wrong.

So what's the correct way dealing with this? Should I send the whole patchset to all
the maintainers/reviewers related to all the patches?

> I don't see e.g PATCH3 on 'linux-hyperv' mailing list.

Thanks for point out, I'll add it.

Zhenzhong

Re: [PATCH 1/3] KVM: X86: Add "nopvspin" parameter to disable PV spinlocks

[Sean Christopherson]

On Tue, Oct 01, 2019 at 05:47:00PM +0800, Zhenzhong Duan wrote:
> 
> On 2019/10/1 16:39, Vitaly Kuznetsov wrote:
> >Zhenzhong Duan<zhenzhong.duan@oracle.com>  writes:
> >
> >>On 2019/9/30 23:41, Vitaly Kuznetsov wrote:
> >>>Zhenzhong Duan<zhenzhong.duan@oracle.com>   writes:
> >>>
> >>>>There are cases where a guest tries to switch spinlocks to bare metal
> >>>>behavior (e.g. by setting "xen_nopvspin" on XEN platform and
> >>>>"hv_nopvspin" on HYPER_V).
> >>>>
> >>>>That feature is missed on KVM, add a new parameter "nopvspin" to disable
> >>>>PV spinlocks for KVM guest.
> >>>>
> >>>>This new parameter is also intended to replace "xen_nopvspin" and
> >>>>"hv_nopvspin" in the future.
> >>>Any reason to not do it right now? We will probably need to have compat
> >>>code to support xen_nopvspin/hv_nopvspin too but emit a 'is deprecated'
> >>>warning.
> >>Sorry the description isn't clear, I'll fix it.
> >>
> >>I did the compat work in the other two patches.
> >>[PATCH 2/3] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"
> >>[PATCH 3/3] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"
> >>
> >For some reason I got CCed only on the first one and moreover,
> 
> The three patches have different maintainers/reviewers by get_maintainer.pl, I added
> "Cc: maintainers/reviewers" to each patch then git-sendemail picked them automaticly.
> I meaned to not disturb maintainers with the field they aren't in charge of. It looks
> I'm wrong.
> 
> So what's the correct way dealing with this? Should I send the whole patchset to all
> the maintainers/reviewers related to all the patches?

There's no one right answer to that question, folks have different
preferences.  My general rule of thumb is to cc everyone on all patches
unless the series is obnoxiously large *and* isolated to a specific part
of the kernel.  The idea being that people are more likely to be annoyed
if they can't find all patches in a relatively small series (this case)
than they are about receiving a mail or two that they don't care about.

At a minimum I would cc everyone involved on the cover letter, and cc the
relevant mailing lists on all patches.  Sending everyone the cover letter
provides people a quick overview of the patches they didn't receive, as
well as a starting point if they want to find those patches.  Cc'ing the
mailing list(s) can make it even easier to find the patches.  The cover
letter is also a good place to explain why you didn't cc everyone on all
patches (or vice versa).

Also, the cover letter should have the shortlog and overall diffstats.
'git format-patch --cover-letter' will do the work for you.

Re: [PATCH 1/3] KVM: X86: Add "nopvspin" parameter to disable PV spinlocks

[Zhenzhong Duan]

On 2019/10/3 0:47, Sean Christopherson wrote:
> On Tue, Oct 01, 2019 at 05:47:00PM +0800, Zhenzhong Duan wrote:
>> On 2019/10/1 16:39, Vitaly Kuznetsov wrote:
>>> Zhenzhong Duan<zhenzhong.duan@oracle.com>  writes:
>>>
>>>> On 2019/9/30 23:41, Vitaly Kuznetsov wrote:
>>>>> Zhenzhong Duan<zhenzhong.duan@oracle.com>   writes:
>>>>>
>>>>>> There are cases where a guest tries to switch spinlocks to bare metal
>>>>>> behavior (e.g. by setting "xen_nopvspin" on XEN platform and
>>>>>> "hv_nopvspin" on HYPER_V).
>>>>>>
>>>>>> That feature is missed on KVM, add a new parameter "nopvspin" to disable
>>>>>> PV spinlocks for KVM guest.
>>>>>>
>>>>>> This new parameter is also intended to replace "xen_nopvspin" and
>>>>>> "hv_nopvspin" in the future.
>>>>> Any reason to not do it right now? We will probably need to have compat
>>>>> code to support xen_nopvspin/hv_nopvspin too but emit a 'is deprecated'
>>>>> warning.
>>>> Sorry the description isn't clear, I'll fix it.
>>>>
>>>> I did the compat work in the other two patches.
>>>> [PATCH 2/3] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"
>>>> [PATCH 3/3] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"
>>>>
>>> For some reason I got CCed only on the first one and moreover,
>> The three patches have different maintainers/reviewers by get_maintainer.pl, I added
>> "Cc: maintainers/reviewers" to each patch then git-sendemail picked them automaticly.
>> I meaned to not disturb maintainers with the field they aren't in charge of. It looks
>> I'm wrong.
>>
>> So what's the correct way dealing with this? Should I send the whole patchset to all
>> the maintainers/reviewers related to all the patches?
> There's no one right answer to that question, folks have different
> preferences.  My general rule of thumb is to cc everyone on all patches
> unless the series is obnoxiously large *and* isolated to a specific part
> of the kernel.  The idea being that people are more likely to be annoyed
> if they can't find all patches in a relatively small series (this case)
> than they are about receiving a mail or two that they don't care about.
>
> At a minimum I would cc everyone involved on the cover letter, and cc the
> relevant mailing lists on all patches.  Sending everyone the cover letter
> provides people a quick overview of the patches they didn't receive, as
> well as a starting point if they want to find those patches.  Cc'ing the
> mailing list(s) can make it even easier to find the patches.  The cover
> letter is also a good place to explain why you didn't cc everyone on all
> patches (or vice versa).
>
> Also, the cover letter should have the shortlog and overall diffstats.
> 'git format-patch --cover-letter' will do the work for you.

Thanks for your detailed reply, I's clear to me what to do now.

Zhenzhong

[PATCH 2/3] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"

[Zhenzhong Duan]

Fix stale description of "xen_nopvspin" as we use qspinlock now.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 Documentation/admin-guide/kernel-parameters.txt |  7 ++++---
 arch/x86/xen/spinlock.c                         | 13 +++++++------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 4b956d8..1f0a62f 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5303,8 +5303,9 @@
 			never -- do not unplug even if version check succeeds
 
 	xen_nopvspin	[X86,XEN]
-			Disables the ticketlock slowpath using Xen PV
-			optimizations.
+			Disables the qspinlock slowpath using Xen PV optimizations.
+			This parameter is obsoleted by "nopvspin" parameter, which
+			has equivalent effect for XEN platform.
 
 	xen_nopv	[X86]
 			Disables the PV optimizations forcing the HVM guest to
@@ -5330,7 +5331,7 @@
 			as generic guest with no PV drivers. Currently support
 			XEN HVM, KVM, HYPER_V and VMWARE guest.
 
-	nopvspin	[X86,KVM] Disables the qspinlock slow path
+	nopvspin	[X86,XEN,KVM] Disables the qspinlock slow path
 			using PV optimizations which allow the hypervisor to
 			'idle' the guest on lock contention.
 
diff --git a/arch/x86/xen/spinlock.c b/arch/x86/xen/spinlock.c
index 6deb490..092a53f 100644
--- a/arch/x86/xen/spinlock.c
+++ b/arch/x86/xen/spinlock.c
@@ -18,7 +18,6 @@
 static DEFINE_PER_CPU(int, lock_kicker_irq) = -1;
 static DEFINE_PER_CPU(char *, irq_name);
 static DEFINE_PER_CPU(atomic_t, xen_qlock_wait_nest);
-static bool xen_pvspin = true;
 
 static void xen_qlock_kick(int cpu)
 {
@@ -68,7 +67,7 @@ void xen_init_lock_cpu(int cpu)
 	int irq;
 	char *name;
 
-	if (!xen_pvspin)
+	if (!pvspin)
 		return;
 
 	WARN(per_cpu(lock_kicker_irq, cpu) >= 0, "spinlock on CPU%d exists on IRQ%d!\n",
@@ -93,7 +92,7 @@ void xen_init_lock_cpu(int cpu)
 
 void xen_uninit_lock_cpu(int cpu)
 {
-	if (!xen_pvspin)
+	if (!pvspin)
 		return;
 
 	unbind_from_irqhandler(per_cpu(lock_kicker_irq, cpu), NULL);
@@ -117,9 +116,9 @@ void __init xen_init_spinlocks(void)
 
 	/*  Don't need to use pvqspinlock code if there is only 1 vCPU. */
 	if (num_possible_cpus() == 1)
-		xen_pvspin = false;
+		pvspin = false;
 
-	if (!xen_pvspin) {
+	if (!pvspin) {
 		printk(KERN_DEBUG "xen: PV spinlocks disabled\n");
 		static_branch_disable(&virt_spin_lock_key);
 		return;
@@ -137,7 +136,9 @@ void __init xen_init_spinlocks(void)
 
 static __init int xen_parse_nopvspin(char *arg)
 {
-	xen_pvspin = false;
+	pr_notice("\"xen_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
+	if (xen_domain())
+		pvspin = false;
 	return 0;
 }
 early_param("xen_nopvspin", xen_parse_nopvspin);
-- 
1.8.3.1

Re: [PATCH 2/3] xen: Mark "xen_nopvspin" parameter obsolete and map it to "nopvspin"

[Jürgen Groß]

On 29.09.19 14:21, Zhenzhong Duan wrote:
> Fix stale description of "xen_nopvspin" as we use qspinlock now.
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Stefano Stabellini <sstabellini@kernel.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

[PATCH 3/3] x86/hyperv: Mark "hv_nopvspin" parameter obsolete and map it to "nopvspin"

[Zhenzhong Duan]

Included asm/hypervisor.h in order to reference x86_hyper_type.

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
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>
Cc: Sasha Levin <sashal@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
---
 Documentation/admin-guide/kernel-parameters.txt | 6 +++++-
 arch/x86/hyperv/hv_spinlock.c                   | 9 +++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 1f0a62f..43f922c 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1436,6 +1436,10 @@
 	hv_nopvspin	[X86,HYPER_V] Disables the paravirt spinlock optimizations
 				      which allow the hypervisor to 'idle' the
 				      guest on lock contention.
+				      This parameter is obsoleted by "nopvspin"
+				      parameter, which has equivalent effect for
+				      HYPER_V platform.
+
 
 	keep_bootcon	[KNL]
 			Do not unregister boot console at start. This is only
@@ -5331,7 +5335,7 @@
 			as generic guest with no PV drivers. Currently support
 			XEN HVM, KVM, HYPER_V and VMWARE guest.
 
-	nopvspin	[X86,XEN,KVM] Disables the qspinlock slow path
+	nopvspin	[X86,XEN,KVM,HYPER_V] Disables the qspinlock slow path
 			using PV optimizations which allow the hypervisor to
 			'idle' the guest on lock contention.
 
diff --git a/arch/x86/hyperv/hv_spinlock.c b/arch/x86/hyperv/hv_spinlock.c
index 07f21a0..00c2902 100644
--- a/arch/x86/hyperv/hv_spinlock.c
+++ b/arch/x86/hyperv/hv_spinlock.c
@@ -12,12 +12,11 @@
 
 #include <linux/spinlock.h>
 
+#include <asm/hypervisor.h>
 #include <asm/mshyperv.h>
 #include <asm/paravirt.h>
 #include <asm/apic.h>
 
-static bool __initdata hv_pvspin = true;
-
 static void hv_qlock_kick(int cpu)
 {
 	apic->send_IPI(cpu, X86_PLATFORM_IPI_VECTOR);
@@ -64,7 +63,7 @@ __visible bool hv_vcpu_is_preempted(int vcpu)
 
 void __init hv_init_spinlocks(void)
 {
-	if (!hv_pvspin || !apic ||
+	if (!pvspin || !apic ||
 	    !(ms_hyperv.hints & HV_X64_CLUSTER_IPI_RECOMMENDED) ||
 	    !(ms_hyperv.features & HV_X64_MSR_GUEST_IDLE_AVAILABLE)) {
 		pr_info("PV spinlocks disabled\n");
@@ -82,7 +81,9 @@ void __init hv_init_spinlocks(void)
 
 static __init int hv_parse_nopvspin(char *arg)
 {
-	hv_pvspin = false;
+	pr_notice("\"hv_nopvspin\" is deprecated, please use \"nopvspin\" instead\n");
+	if (x86_hyper_type != X86_HYPER_MS_HYPERV)
+		pvspin = false;
 	return 0;
 }
 early_param("hv_nopvspin", hv_parse_nopvspin);
-- 
1.8.3.1