[PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Oscar Salvador]

This is just a resend of Waiman Long's patch.
I could not find why it was not merged to upstream, so I thought
to give it another chance.
What follows is what Waiman Long wrote.

Xen has an kernel command line argument "xen_nopvspin" to disable
paravirtual spinlocks. This patch adds a similar "kvm_nopvspin"
argument to disable paravirtual spinlocks for KVM. This can be useful
for testing as well as allowing administrators to choose unfair lock
for their KVM guests if they want to.

Signed-off-by: Waiman Long <longman@redhat.com>
Signed-off-by: Oscar Salvador <osalvador@suse.de>
---
 Documentation/admin-guide/kernel-parameters.txt |  6 +++++-
 arch/x86/kernel/kvm.c                           | 14 +++++++++++++-
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index d9c171ce4190..56c6e3acdf8e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1899,6 +1899,10 @@
 			feature (tagged TLBs) on capable Intel chips.
 			Default is 1 (enabled)
 
+	kvm_nopvspin	[X86,KVM]
+			Disables the paravirtualized spinlock slowpath
+			optimizations for KVM.
+
 	l2cr=		[PPC]
 
 	l3cr=		[PPC]
@@ -4533,7 +4537,7 @@
 			never -- do not unplug even if version check succeeds
 
 	xen_nopvspin	[X86,XEN]
-			Disables the ticketlock slowpath using Xen PV
+			Disables the spinlock slowpath using Xen PV
 			optimizations.
 
 	xen_nopv	[X86]
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index d04e30e3c0ff..51addf874fc1 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -568,6 +568,18 @@ static void kvm_kick_cpu(int cpu)
 	kvm_hypercall2(KVM_HC_KICK_CPU, flags, apicid);
 }
 
+static bool kvm_pvspin = true;
+
+/*
+ * Allow disabling of PV spinlock in kernel command line
+ */
+static __init int kvm_parse_nopvspin(char *arg)
+{
+	kvm_pvspin = false;
+	return 0;
+}
+early_param("kvm_nopvspin", kvm_parse_nopvspin);
+
 #include <asm/qspinlock.h>
 
 static void kvm_wait(u8 *ptr, u8 val)
@@ -633,7 +645,7 @@ asm(
  */
 void __init kvm_spinlock_init(void)
 {
-	if (!kvm_para_available())
+	if (!kvm_para_available() || !kvm_pvspin)
 		return;
 	/* Does host kernel support KVM_FEATURE_PV_UNHALT? */
 	if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT))
-- 
2.13.5

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Peter Zijlstra]

On Mon, Sep 04, 2017 at 04:28:36PM +0200, Oscar Salvador wrote:
> This is just a resend of Waiman Long's patch.
> I could not find why it was not merged to upstream, so I thought
> to give it another chance.
> What follows is what Waiman Long wrote.
> 
> Xen has an kernel command line argument "xen_nopvspin" to disable
> paravirtual spinlocks. This patch adds a similar "kvm_nopvspin"
> argument to disable paravirtual spinlocks for KVM. This can be useful
> for testing as well as allowing administrators to choose unfair lock
> for their KVM guests if they want to.

For testing its trivial to hack your kernel and I don't feel this is
something an Admin can make reasonable decisions about.

So why? In general less knobs is better.

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Waiman Long]

On 09/04/2017 10:40 AM, Peter Zijlstra wrote:
> On Mon, Sep 04, 2017 at 04:28:36PM +0200, Oscar Salvador wrote:
>> This is just a resend of Waiman Long's patch.
>> I could not find why it was not merged to upstream, so I thought
>> to give it another chance.
>> What follows is what Waiman Long wrote.
>>
>> Xen has an kernel command line argument "xen_nopvspin" to disable
>> paravirtual spinlocks. This patch adds a similar "kvm_nopvspin"
>> argument to disable paravirtual spinlocks for KVM. This can be useful
>> for testing as well as allowing administrators to choose unfair lock
>> for their KVM guests if they want to.
> For testing its trivial to hack your kernel and I don't feel this is
> something an Admin can make reasonable decisions about.

I almost forgot about this patch that I sent quite a while ago. I was
sending this patch out mainly to maintain consistency between KVM and
Xen. This patch is not that important to me, and that is why I didn't
push it further.

Cheers,
Longman
 

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Davidlohr Bueso]

On Mon, 04 Sep 2017, Peter Zijlstra wrote:

>For testing its trivial to hack your kernel and I don't feel this is
>something an Admin can make reasonable decisions about.
>
>So why? In general less knobs is better.

+1.

Also, note how b8fa70b51aa (xen, pvticketlocks: Add xen_nopvspin parameter
to disable xen pv ticketlocks) has no justification as to why its wanted
in the first place. The only thing I could find was from 15a3eac0784
(xen/spinlock: Document the xen_nopvspin parameter):

"Useful for diagnosing issues and comparing benchmarks in over-commit CPU scenarios."

So I vote for no additional knobs, specially for such core code.

Thanks,
Davidlohr

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Juergen Gross]

On 05/09/17 00:21, Davidlohr Bueso wrote:
> On Mon, 04 Sep 2017, Peter Zijlstra wrote:
> 
>> For testing its trivial to hack your kernel and I don't feel this is
>> something an Admin can make reasonable decisions about.
>>
>> So why? In general less knobs is better.
> 
> +1.
> 
> Also, note how b8fa70b51aa (xen, pvticketlocks: Add xen_nopvspin parameter
> to disable xen pv ticketlocks) has no justification as to why its wanted
> in the first place. The only thing I could find was from 15a3eac0784
> (xen/spinlock: Document the xen_nopvspin parameter):
> 
> "Useful for diagnosing issues and comparing benchmarks in over-commit
> CPU scenarios."

Hmm, I think I should clarify the Xen knob, as I was the one requesting
it:

In my previous employment we had a configuration where dom0 ran
exclusively on a dedicated set of physical cpus. We experienced
scalability problems when doing I/O performance tests: with a decent
number of dom0 cpus we achieved throughput of 700 MB/s with only 20%
cpu load in dom0. A higher dom0 cpu count let the throughput drop to
about 150 MB/s and cpu load was up to 100%. Reason was the additional
load due to hypervisor interactions on a high frequency lock.

So in special configurations at least for Xen the knob is useful for
production environment.


Juergen

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Oscar Salvador]

On 09/05/2017 08:28 AM, Juergen Gross wrote:
> On 05/09/17 00:21, Davidlohr Bueso wrote:
>> On Mon, 04 Sep 2017, Peter Zijlstra wrote:
>>
>>> For testing its trivial to hack your kernel and I don't feel this is
>>> something an Admin can make reasonable decisions about.
>>>
>>> So why? In general less knobs is better.
>> +1.
>>
>> Also, note how b8fa70b51aa (xen, pvticketlocks: Add xen_nopvspin parameter
>> to disable xen pv ticketlocks) has no justification as to why its wanted
>> in the first place. The only thing I could find was from 15a3eac0784
>> (xen/spinlock: Document the xen_nopvspin parameter):
>>
>> "Useful for diagnosing issues and comparing benchmarks in over-commit
>> CPU scenarios."
> Hmm, I think I should clarify the Xen knob, as I was the one requesting
> it:
>
> In my previous employment we had a configuration where dom0 ran
> exclusively on a dedicated set of physical cpus. We experienced
> scalability problems when doing I/O performance tests: with a decent
> number of dom0 cpus we achieved throughput of 700 MB/s with only 20%
> cpu load in dom0. A higher dom0 cpu count let the throughput drop to
> about 150 MB/s and cpu load was up to 100%. Reason was the additional
> load due to hypervisor interactions on a high frequency lock.
>
> So in special configurations at least for Xen the knob is useful for
> production environment.

It may be that the original patch was just to keep consistency between 
Xen and KVM, and also only for testing purposes.
But we find a case when a customer of ours is running some workloads 
with 1<->1 mapping between physical cores and virtual cores, and we 
realized that with the pv spinlocks disabled there is a 4-5% of 
performance gain.

A perf analysis showed that the application was very lock intensive with 
a lot of time spent in __raw_callee_save___pv_queued_spin_unlock.

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Peter Zijlstra]

On Tue, Sep 05, 2017 at 08:57:16AM +0200, Oscar Salvador wrote:
> It may be that the original patch was just to keep consistency between Xen
> and KVM, and also only for testing purposes.
> But we find a case when a customer of ours is running some workloads with
> 1<->1 mapping between physical cores and virtual cores, and we realized that
> with the pv spinlocks disabled there is a 4-5% of performance gain.

There are very definite downsides to using a test-and-set spinlock.

A much better option would be one that forces the use of native
qspinlock in the 1:1 case. That means you have to fail both pv_enabled()
and virt_spin_lock().

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Peter Zijlstra]

On Tue, Sep 05, 2017 at 08:28:10AM +0200, Juergen Gross wrote:
> On 05/09/17 00:21, Davidlohr Bueso wrote:
> > On Mon, 04 Sep 2017, Peter Zijlstra wrote:
> > 
> >> For testing its trivial to hack your kernel and I don't feel this is
> >> something an Admin can make reasonable decisions about.
> >>
> >> So why? In general less knobs is better.
> > 
> > +1.
> > 
> > Also, note how b8fa70b51aa (xen, pvticketlocks: Add xen_nopvspin parameter
> > to disable xen pv ticketlocks) has no justification as to why its wanted
> > in the first place. The only thing I could find was from 15a3eac0784
> > (xen/spinlock: Document the xen_nopvspin parameter):
> > 
> > "Useful for diagnosing issues and comparing benchmarks in over-commit
> > CPU scenarios."
> 
> Hmm, I think I should clarify the Xen knob, as I was the one requesting
> it:
> 
> In my previous employment we had a configuration where dom0 ran
> exclusively on a dedicated set of physical cpus. We experienced
> scalability problems when doing I/O performance tests: with a decent
> number of dom0 cpus we achieved throughput of 700 MB/s with only 20%
> cpu load in dom0. A higher dom0 cpu count let the throughput drop to
> about 150 MB/s and cpu load was up to 100%. Reason was the additional
> load due to hypervisor interactions on a high frequency lock.
> 
> So in special configurations at least for Xen the knob is useful for
> production environment.

So the problem with qspinlock is that it will revert to a classic
test-and-set spinlock if you don't do paravirt but are running a HV.

And test-and-set is unfair and has all kinds of ugly starvation cases,
esp on slightly bigger hardware.

So if we'd want to cater to the 1:1 virt case, we'll need to come up
with something else. _IF_ it is an issue of course.

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Juergen Gross]

On 05/09/17 08:58, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 08:28:10AM +0200, Juergen Gross wrote:
>> On 05/09/17 00:21, Davidlohr Bueso wrote:
>>> On Mon, 04 Sep 2017, Peter Zijlstra wrote:
>>>
>>>> For testing its trivial to hack your kernel and I don't feel this is
>>>> something an Admin can make reasonable decisions about.
>>>>
>>>> So why? In general less knobs is better.
>>>
>>> +1.
>>>
>>> Also, note how b8fa70b51aa (xen, pvticketlocks: Add xen_nopvspin parameter
>>> to disable xen pv ticketlocks) has no justification as to why its wanted
>>> in the first place. The only thing I could find was from 15a3eac0784
>>> (xen/spinlock: Document the xen_nopvspin parameter):
>>>
>>> "Useful for diagnosing issues and comparing benchmarks in over-commit
>>> CPU scenarios."
>>
>> Hmm, I think I should clarify the Xen knob, as I was the one requesting
>> it:
>>
>> In my previous employment we had a configuration where dom0 ran
>> exclusively on a dedicated set of physical cpus. We experienced
>> scalability problems when doing I/O performance tests: with a decent
>> number of dom0 cpus we achieved throughput of 700 MB/s with only 20%
>> cpu load in dom0. A higher dom0 cpu count let the throughput drop to
>> about 150 MB/s and cpu load was up to 100%. Reason was the additional
>> load due to hypervisor interactions on a high frequency lock.
>>
>> So in special configurations at least for Xen the knob is useful for
>> production environment.
> 
> So the problem with qspinlock is that it will revert to a classic
> test-and-set spinlock if you don't do paravirt but are running a HV.

In the Xen case we just use the bare metal settings when xen_nopvspin
has been specified. So paravirt, but without modifying any pv_lock_ops
functions.


Juergen

> 
> And test-and-set is unfair and has all kinds of ugly starvation cases,
> esp on slightly bigger hardware.
> 
> So if we'd want to cater to the 1:1 virt case, we'll need to come up
> with something else. _IF_ it is an issue of course.
> 

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Peter Zijlstra]

On Tue, Sep 05, 2017 at 09:35:40AM +0200, Juergen Gross wrote:
> > So the problem with qspinlock is that it will revert to a classic
> > test-and-set spinlock if you don't do paravirt but are running a HV.
> 
> In the Xen case we just use the bare metal settings when xen_nopvspin
> has been specified. So paravirt, but without modifying any pv_lock_ops
> functions.

See arch/x86/include/asm/qspinlock.h:virt_spin_lock(). Unless you clear
X86_FEATURE_HYPERVISOR you get a test-and-set spinlock.

And as the comment there says, this is a fallback for !paravirt enabled
hypervisors to avoid the worst of the lock holder preemption crud.

But this very much does not deal with the 1:1 case nicely.

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Juergen Gross]

On 05/09/17 10:10, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 09:35:40AM +0200, Juergen Gross wrote:
>>> So the problem with qspinlock is that it will revert to a classic
>>> test-and-set spinlock if you don't do paravirt but are running a HV.
>>
>> In the Xen case we just use the bare metal settings when xen_nopvspin
>> has been specified. So paravirt, but without modifying any pv_lock_ops
>> functions.
> 
> See arch/x86/include/asm/qspinlock.h:virt_spin_lock(). Unless you clear
> X86_FEATURE_HYPERVISOR you get a test-and-set spinlock.
> 
> And as the comment there says, this is a fallback for !paravirt enabled
> hypervisors to avoid the worst of the lock holder preemption crud.
> 
> But this very much does not deal with the 1:1 case nicely.
> 

Aah, now I've got it.

So maybe we should add virt_spin_lock() to pv_lock_ops? This way e.g.
xen_nopvspin could tweak just the virt_spin_lock() case by letting it
return false all the time?

In case you agree I can setup a patch...


Juergen

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Peter Zijlstra]

On Tue, Sep 05, 2017 at 10:14:21AM +0200, Juergen Gross wrote:
> On 05/09/17 10:10, Peter Zijlstra wrote:
> > On Tue, Sep 05, 2017 at 09:35:40AM +0200, Juergen Gross wrote:
> >>> So the problem with qspinlock is that it will revert to a classic
> >>> test-and-set spinlock if you don't do paravirt but are running a HV.
> >>
> >> In the Xen case we just use the bare metal settings when xen_nopvspin
> >> has been specified. So paravirt, but without modifying any pv_lock_ops
> >> functions.
> > 
> > See arch/x86/include/asm/qspinlock.h:virt_spin_lock(). Unless you clear
> > X86_FEATURE_HYPERVISOR you get a test-and-set spinlock.
> > 
> > And as the comment there says, this is a fallback for !paravirt enabled
> > hypervisors to avoid the worst of the lock holder preemption crud.
> > 
> > But this very much does not deal with the 1:1 case nicely.
> > 
> 
> Aah, now I've got it.
> 
> So maybe we should add virt_spin_lock() to pv_lock_ops? This way e.g.
> xen_nopvspin could tweak just the virt_spin_lock() case by letting it
> return false all the time?

Hmm, that might work. Could we somehow nop that call when
!X86_FEATURE_HYPERVISOR?, that saves native from having to do the call
and would be a win for everyone.

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Juergen Gross]

On 05/09/17 10:28, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 10:14:21AM +0200, Juergen Gross wrote:
>> On 05/09/17 10:10, Peter Zijlstra wrote:
>>> On Tue, Sep 05, 2017 at 09:35:40AM +0200, Juergen Gross wrote:
>>>>> So the problem with qspinlock is that it will revert to a classic
>>>>> test-and-set spinlock if you don't do paravirt but are running a HV.
>>>>
>>>> In the Xen case we just use the bare metal settings when xen_nopvspin
>>>> has been specified. So paravirt, but without modifying any pv_lock_ops
>>>> functions.
>>>
>>> See arch/x86/include/asm/qspinlock.h:virt_spin_lock(). Unless you clear
>>> X86_FEATURE_HYPERVISOR you get a test-and-set spinlock.
>>>
>>> And as the comment there says, this is a fallback for !paravirt enabled
>>> hypervisors to avoid the worst of the lock holder preemption crud.
>>>
>>> But this very much does not deal with the 1:1 case nicely.
>>>
>>
>> Aah, now I've got it.
>>
>> So maybe we should add virt_spin_lock() to pv_lock_ops? This way e.g.
>> xen_nopvspin could tweak just the virt_spin_lock() case by letting it
>> return false all the time?
> 
> Hmm, that might work. Could we somehow nop that call when
> !X86_FEATURE_HYPERVISOR?, that saves native from having to do the call
> and would be a win for everyone.

So in fact we want a "always false" shortcut for bare metal and for any
virtualization environment selecting bare metal behavior.

I'll have a try.


Juergen

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Peter Zijlstra]

On Tue, Sep 05, 2017 at 10:52:06AM +0200, Juergen Gross wrote:
> > Hmm, that might work. Could we somehow nop that call when
> > !X86_FEATURE_HYPERVISOR?, that saves native from having to do the call
> > and would be a win for everyone.
> 
> So in fact we want a "always false" shortcut for bare metal and for any
> virtualization environment selecting bare metal behavior.
> 
> I'll have a try.

Right, so for native I think you can do this:


diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
index 11aaf1eaa0e4..4e9839001291 100644
--- a/arch/x86/kernel/paravirt_patch_64.c
+++ b/arch/x86/kernel/paravirt_patch_64.c
@@ -21,6 +21,7 @@ DEF_NATIVE(, mov64, "mov %rdi, %rax");
 #if defined(CONFIG_PARAVIRT_SPINLOCKS)
 DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
 DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %rax, %rax");
+DEF_NATIVE(pv_lock_ops, virt_spin_lock, "xor %rax, %rax");
 #endif
 
 unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
@@ -77,6 +78,13 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
 				goto patch_site;
 			}
 			goto patch_default;
+		case PARAVIRT_PATCH(pv_lock_ops.virt_spin_lock):
+			if (!this_cpu_has(X86_FEATURE_HYPERVISOR)) {
+				start = start_pv_lock_ops_virt_spin_lock;
+				end   = end_pv_lock_ops_virt_spin_lock;
+				goto patch_side;
+			}
+			goto patch_default;
 #endif
 
 	default:

Re: [PATCH resend] x86,kvm: Add a kernel parameter to disable PV spinlock

[Juergen Gross]

On 05/09/17 11:03, Peter Zijlstra wrote:
> On Tue, Sep 05, 2017 at 10:52:06AM +0200, Juergen Gross wrote:
>>> Hmm, that might work. Could we somehow nop that call when
>>> !X86_FEATURE_HYPERVISOR?, that saves native from having to do the call
>>> and would be a win for everyone.
>>
>> So in fact we want a "always false" shortcut for bare metal and for any
>> virtualization environment selecting bare metal behavior.
>>
>> I'll have a try.
> 
> Right, so for native I think you can do this:
> 
> 
> diff --git a/arch/x86/kernel/paravirt_patch_64.c b/arch/x86/kernel/paravirt_patch_64.c
> index 11aaf1eaa0e4..4e9839001291 100644
> --- a/arch/x86/kernel/paravirt_patch_64.c
> +++ b/arch/x86/kernel/paravirt_patch_64.c
> @@ -21,6 +21,7 @@ DEF_NATIVE(, mov64, "mov %rdi, %rax");
>  #if defined(CONFIG_PARAVIRT_SPINLOCKS)
>  DEF_NATIVE(pv_lock_ops, queued_spin_unlock, "movb $0, (%rdi)");
>  DEF_NATIVE(pv_lock_ops, vcpu_is_preempted, "xor %rax, %rax");
> +DEF_NATIVE(pv_lock_ops, virt_spin_lock, "xor %rax, %rax");
>  #endif
>  
>  unsigned paravirt_patch_ident_32(void *insnbuf, unsigned len)
> @@ -77,6 +78,13 @@ unsigned native_patch(u8 type, u16 clobbers, void *ibuf,
>  				goto patch_site;
>  			}
>  			goto patch_default;
> +		case PARAVIRT_PATCH(pv_lock_ops.virt_spin_lock):
> +			if (!this_cpu_has(X86_FEATURE_HYPERVISOR)) {
> +				start = start_pv_lock_ops_virt_spin_lock;
> +				end   = end_pv_lock_ops_virt_spin_lock;
> +				goto patch_side;
> +			}
> +			goto patch_default;

I'd rather add a test to paravirt_patch_default() similar to the
_paravirt_ident_* cases for catching the Xen (and possibly KVM) cases,
too.

I just need to add _paravirt_zero_[32|64] functions I can use for bare
metal and the nopvspin cases.


Juergen