[PATCH/RFC 0/3] KVM: yield heuristic improvements

[PATCH/RFC 0/3] KVM: yield heuristic improvements

[Christian Borntraeger]

Paolo, Gleb,

here are 3 patches that improve the s390 guest performance on contended
systems. Basic idea is to also allow yielding to CPUs which still sleep
but are runnable. The patch set should be no-ops for !s390.

Michael Mueller (3):
  kvm: Introduction of kvm_arch_vcpu_dont_yield()
  s390/kvm: Platform specific kvm_arch_vcpu_dont_yield
  s390/kvm: Set preempted flag during vcpu wakeup and interrupt delivery

 arch/s390/kvm/Kconfig     | 1 +
 arch/s390/kvm/interrupt.c | 3 +++
 arch/s390/kvm/kvm-s390.c  | 7 +++++++
 include/linux/kvm_host.h  | 9 +++++++++
 virt/kvm/Kconfig          | 3 +++
 virt/kvm/kvm_main.c       | 2 +-
 6 files changed, 24 insertions(+), 1 deletion(-)

-- 
1.8.4.2

[PATCH/RFC 1/3] kvm: Introduction of kvm_arch_vcpu_dont_yield()

[Christian Borntraeger]

From: Michael Mueller <mimu@linux.vnet.ibm.com>

The function kvm_arch_vcpu_dont_yield() allows the platform dependant
extension of the heuristic being used to identify unsuitable virtual cpus
during undirected yield processing.

The default implementation used the existing waitqueue check.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 include/linux/kvm_host.h | 9 +++++++++
 virt/kvm/Kconfig         | 3 +++
 virt/kvm/kvm_main.c      | 2 +-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f5937b8..3e979a2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -686,6 +686,15 @@ static inline wait_queue_head_t *kvm_arch_vcpu_wq(struct kvm_vcpu *vcpu)
 #endif
 }
 
+#ifndef CONFIG_HAVE_KVM_ARCH_VCPU_DONT_YIELD
+static inline bool kvm_arch_vcpu_dont_yield(struct kvm_vcpu *vcpu)
+{
+	return waitqueue_active(kvm_arch_vcpu_wq(vcpu));
+}
+#else
+bool kvm_arch_vcpu_dont_yield(struct kvm_vcpu *vcpu);
+#endif
+
 int kvm_arch_init_vm(struct kvm *kvm, unsigned long type);
 void kvm_arch_destroy_vm(struct kvm *kvm);
 void kvm_arch_sync_events(struct kvm *kvm);
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index 13f2d19..e1e46a8 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -34,3 +34,6 @@ config HAVE_KVM_CPU_RELAX_INTERCEPT
 
 config KVM_VFIO
        bool
+
+config HAVE_KVM_ARCH_VCPU_DONT_YIELD
+       bool
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a9e999a..6829e6f 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -1804,7 +1804,7 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me)
 				continue;
 			if (vcpu == me)
 				continue;
-			if (waitqueue_active(&vcpu->wq))
+			if (kvm_arch_vcpu_dont_yield(vcpu))
 				continue;
 			if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
 				continue;
-- 
1.8.4.2

[PATCH/RFC 2/3] s390/kvm: Platform specific kvm_arch_vcpu_dont_yield

[Christian Borntraeger]

From: Michael Mueller <mimu@linux.vnet.ibm.com>

Commit "s390/kvm: Use common waitqueue" caused a performance regression
on s390. It turned out that a yield candidate was missed by just a simple
test on its non-empty waitqueue. If an interrupt is outstanding, the candidate
might be suitable. kvm_arch_vcpu_dont_yield is extended by a test that
additionally tests for not yet delivered interrupts.

Significant performance measurement work and code analysis to solve
this issue was provided by Mao Chuan Li and his team in Beijing.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/Kconfig    | 1 +
 arch/s390/kvm/kvm-s390.c | 7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
index c8bacbc..e44adef 100644
--- a/arch/s390/kvm/Kconfig
+++ b/arch/s390/kvm/Kconfig
@@ -25,6 +25,7 @@ config KVM
 	select HAVE_KVM_EVENTFD
 	select KVM_ASYNC_PF
 	select KVM_ASYNC_PF_SYNC
+	select HAVE_KVM_ARCH_VCPU_DONT_YIELD
 	---help---
 	  Support hosting paravirtualized guest machines using the SIE
 	  virtualization capability on the mainframe. This should work
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index a5da2cc..1a33e1e 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1231,6 +1231,13 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
 	return VM_FAULT_SIGBUS;
 }
 
+#ifdef CONFIG_HAVE_KVM_ARCH_VCPU_DONT_YIELD
+bool kvm_arch_vcpu_dont_yield(struct kvm_vcpu *vcpu)
+{
+	return waitqueue_active(&vcpu->wq) && !kvm_cpu_has_interrupt(vcpu);
+}
+#endif
+
 void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
 			   struct kvm_memory_slot *dont)
 {
-- 
1.8.4.2

Re: [PATCH/RFC 2/3] s390/kvm: Platform specific kvm_arch_vcpu_dont_yield

[Paolo Bonzini]

Il 11/02/2014 12:45, Christian Borntraeger ha scritto:
> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>
> Commit "s390/kvm: Use common waitqueue" caused a performance regression
> on s390. It turned out that a yield candidate was missed by just a simple
> test on its non-empty waitqueue. If an interrupt is outstanding, the candidate
> might be suitable. kvm_arch_vcpu_dont_yield is extended by a test that
> additionally tests for not yet delivered interrupts.
>
> Significant performance measurement work and code analysis to solve
> this issue was provided by Mao Chuan Li and his team in Beijing.
>
> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  arch/s390/kvm/Kconfig    | 1 +
>  arch/s390/kvm/kvm-s390.c | 7 +++++++
>  2 files changed, 8 insertions(+)
>
> diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
> index c8bacbc..e44adef 100644
> --- a/arch/s390/kvm/Kconfig
> +++ b/arch/s390/kvm/Kconfig
> @@ -25,6 +25,7 @@ config KVM
>  	select HAVE_KVM_EVENTFD
>  	select KVM_ASYNC_PF
>  	select KVM_ASYNC_PF_SYNC
> +	select HAVE_KVM_ARCH_VCPU_DONT_YIELD
>  	---help---
>  	  Support hosting paravirtualized guest machines using the SIE
>  	  virtualization capability on the mainframe. This should work
> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
> index a5da2cc..1a33e1e 100644
> --- a/arch/s390/kvm/kvm-s390.c
> +++ b/arch/s390/kvm/kvm-s390.c
> @@ -1231,6 +1231,13 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
>  	return VM_FAULT_SIGBUS;
>  }
>
> +#ifdef CONFIG_HAVE_KVM_ARCH_VCPU_DONT_YIELD
> +bool kvm_arch_vcpu_dont_yield(struct kvm_vcpu *vcpu)
> +{
> +	return waitqueue_active(&vcpu->wq) && !kvm_cpu_has_interrupt(vcpu);
> +}
> +#endif

I wonder if just using "&& !kvm_arch_vcpu_runnable(vcpu)" in 
kvm_vcpu_on_spin would be better.

Right now, you do not need it in s390 because kvm_vcpu_block is not used 
either.  But you could simply define it to kvm_cpu_has_interrupt(vcpu) 
instead.

Paolo

> +
>  void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
>  			   struct kvm_memory_slot *dont)
>  {
>

Re: [PATCH/RFC 2/3] s390/kvm: Platform specific kvm_arch_vcpu_dont_yield

[Christian Borntraeger]

On 13/02/14 23:37, Paolo Bonzini wrote:
> Il 11/02/2014 12:45, Christian Borntraeger ha scritto:
>> From: Michael Mueller <mimu@linux.vnet.ibm.com>
>>
>> Commit "s390/kvm: Use common waitqueue" caused a performance regression
>> on s390. It turned out that a yield candidate was missed by just a simple
>> test on its non-empty waitqueue. If an interrupt is outstanding, the candidate
>> might be suitable. kvm_arch_vcpu_dont_yield is extended by a test that
>> additionally tests for not yet delivered interrupts.
>>
>> Significant performance measurement work and code analysis to solve
>> this issue was provided by Mao Chuan Li and his team in Beijing.
>>
>> Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
>> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  arch/s390/kvm/Kconfig    | 1 +
>>  arch/s390/kvm/kvm-s390.c | 7 +++++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/arch/s390/kvm/Kconfig b/arch/s390/kvm/Kconfig
>> index c8bacbc..e44adef 100644
>> --- a/arch/s390/kvm/Kconfig
>> +++ b/arch/s390/kvm/Kconfig
>> @@ -25,6 +25,7 @@ config KVM
>>      select HAVE_KVM_EVENTFD
>>      select KVM_ASYNC_PF
>>      select KVM_ASYNC_PF_SYNC
>> +    select HAVE_KVM_ARCH_VCPU_DONT_YIELD
>>      ---help---
>>        Support hosting paravirtualized guest machines using the SIE
>>        virtualization capability on the mainframe. This should work
>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
>> index a5da2cc..1a33e1e 100644
>> --- a/arch/s390/kvm/kvm-s390.c
>> +++ b/arch/s390/kvm/kvm-s390.c
>> @@ -1231,6 +1231,13 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf)
>>      return VM_FAULT_SIGBUS;
>>  }
>>
>> +#ifdef CONFIG_HAVE_KVM_ARCH_VCPU_DONT_YIELD
>> +bool kvm_arch_vcpu_dont_yield(struct kvm_vcpu *vcpu)
>> +{
>> +    return waitqueue_active(&vcpu->wq) && !kvm_cpu_has_interrupt(vcpu);
>> +}
>> +#endif
> 
> I wonder if just using "&& !kvm_arch_vcpu_runnable(vcpu)" in kvm_vcpu_on_spin would be better.
> 
> Right now, you do not need it in s390 because kvm_vcpu_block is not used either.  But you could simply define it to kvm_cpu_has_interrupt(vcpu) instead.
> 
> Paolo

We had several variants but in the end we tried to come up with a patch that does not
influence other architectures. Your proposal would certainly be fine for s390,
but what impact does it have on x86, arm, arm64? Will it cause performance regressions?
So I think that the patch as is is probably the safest choice until we have some
data from x86, arm, arm64, no?

Christian

Re: [PATCH/RFC 2/3] s390/kvm: Platform specific kvm_arch_vcpu_dont_yield

[Paolo Bonzini]

Il 13/02/2014 23:54, Christian Borntraeger ha scritto:
> We had several variants but in the end we tried to come up with a patch that does not
> influence other architectures. Your proposal would certainly be fine for s390,
> but what impact does it have on x86, arm, arm64? Will it cause performance regressions?

It may also have the same advantages you got on s390.

> So I think that the patch as is is probably the safest choice until we have some
> data from x86, arm, arm64, no?

No, using an existing API is always better than inventing a new one.

If you post the new patch series, and describe the benchmark you were 
using, we can reproduce it on x86.

Paolo

Re: [PATCH/RFC 2/3] s390/kvm: Platform specific kvm_arch_vcpu_dont_yield

[Christian Borntraeger]

On 14/02/14 00:32, Paolo Bonzini wrote:
> Il 13/02/2014 23:54, Christian Borntraeger ha scritto:
>> We had several variants but in the end we tried to come up with a patch that does not
>> influence other architectures. Your proposal would certainly be fine for s390,
>> but what impact does it have on x86, arm, arm64? Will it cause performance regressions?
> 
> It may also have the same advantages you got on s390.
> 
>> So I think that the patch as is is probably the safest choice until we have some
>> data from x86, arm, arm64, no?
> 
> No, using an existing API is always better than inventing a new one.

OK. 
Michael can you rework the series to simply use 
" if (waitqueue_active(&vcpu->wq) && !kvm_arch_vcpu_runnable(vcpu)" in kvm_vcpu_on_spin

and make kvm_arch_vcpu_runnable  kvm_cpu_has_interrupt(vcpu) in s390 code?
That should be equivalent for s390 with even simpler code.
It might also help x86 and others.


> If you post the new patch series, and describe the benchmark you were using, we can reproduce it on x86.

The benchmark was some workload doing lots of semaphore up/down with hundreds
of processes. Will see if I can come up with a minimal test.

Re: [PATCH/RFC 2/3] s390/kvm: Platform specific kvm_arch_vcpu_dont_yield

[Paolo Bonzini]

Il 14/02/2014 10:55, Christian Borntraeger ha scritto:
> OK.
> Michael can you rework the series to simply use
> " if (waitqueue_active(&vcpu->wq) && !kvm_arch_vcpu_runnable(vcpu)" in kvm_vcpu_on_spin
>
> and make kvm_arch_vcpu_runnable  kvm_cpu_has_interrupt(vcpu) in s390 code?
> That should be equivalent for s390 with even simpler code.
> It might also help x86 and others.

Exactly my point. ;)

Paolo

Re: [PATCH/RFC 2/3] s390/kvm: Platform specific kvm_arch_vcpu_dont_yield

[Michael Mueller]

On Fri, 14 Feb 2014 10:55:31 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 14/02/14 00:32, Paolo Bonzini wrote:
> > Il 13/02/2014 23:54, Christian Borntraeger ha scritto:
> >> We had several variants but in the end we tried to come up with a patch that does not
> >> influence other architectures. Your proposal would certainly be fine for s390,
> >> but what impact does it have on x86, arm, arm64? Will it cause performance regressions?
> > 
> > It may also have the same advantages you got on s390.
> > 
> >> So I think that the patch as is is probably the safest choice until we have some
> >> data from x86, arm, arm64, no?
> > 
> > No, using an existing API is always better than inventing a new one.
> 
> OK. 
> Michael can you rework the series to simply use 
> " if (waitqueue_active(&vcpu->wq) && !kvm_arch_vcpu_runnable(vcpu)" in kvm_vcpu_on_spin
> 
> and make kvm_arch_vcpu_runnable  kvm_cpu_has_interrupt(vcpu) in s390 code?
> That should be equivalent for s390 with even simpler code.
> It might also help x86 and others.

Ok, I will adapt the patch accordingly...

> 
> 
> > If you post the new patch series, and describe the benchmark you were using, we can reproduce
> > it on x86.
> 
> The benchmark was some workload doing lots of semaphore up/down with hundreds
> of processes. Will see if I can come up with a minimal test.
> 
> 
> 
> 

[PATCH/RFC 3/3] s390/kvm: Set preempted flag during vcpu wakeup and interrupt delivery

[Christian Borntraeger]

From: Michael Mueller <mimu@linux.vnet.ibm.com>

Commit "kvm: Record the preemption status of vcpus using preempt notifiers"
caused a performance regression on s390. It turned out that in the case that
if a former sleeping cpu, that was woken up, this cpu is not a yield candidate
since it gave up the cpu voluntarily. To retain this candiate its preempted
flag is set during wakeup and interrupt delivery time.

This and commit "s390/kvm: Platform specific kvm_arch_vcpu_dont_yield" have to
be applied both in order to recover the performance regression.

Significant performance measurement work and code analysis to solve this
issue was provided by Mao Chuan Li and his team in Beijing.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/kvm/interrupt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/s390/kvm/interrupt.c b/arch/s390/kvm/interrupt.c
index 1848080..fff070b 100644
--- a/arch/s390/kvm/interrupt.c
+++ b/arch/s390/kvm/interrupt.c
@@ -505,6 +505,7 @@ enum hrtimer_restart kvm_s390_idle_wakeup(struct hrtimer *timer)
 	struct kvm_vcpu *vcpu;
 
 	vcpu = container_of(timer, struct kvm_vcpu, arch.ckc_timer);
+	vcpu->preempted = true;
 	tasklet_schedule(&vcpu->arch.tasklet);
 
 	return HRTIMER_NORESTART;
@@ -732,6 +733,7 @@ static int __inject_vm(struct kvm *kvm, struct kvm_s390_interrupt_info *inti)
 	atomic_set_mask(CPUSTAT_EXT_INT, li->cpuflags);
 	if (waitqueue_active(li->wq))
 		wake_up_interruptible(li->wq);
+	kvm_get_vcpu(kvm, sigcpu)->preempted = true;
 	spin_unlock_bh(&li->lock);
 unlock_fi:
 	spin_unlock(&fi->lock);
@@ -877,6 +879,7 @@ int kvm_s390_inject_vcpu(struct kvm_vcpu *vcpu,
 	atomic_set_mask(CPUSTAT_EXT_INT, li->cpuflags);
 	if (waitqueue_active(&vcpu->wq))
 		wake_up_interruptible(&vcpu->wq);
+	vcpu->preempted = true;
 	spin_unlock_bh(&li->lock);
 	mutex_unlock(&vcpu->kvm->lock);
 	return 0;
-- 
1.8.4.2