| Message ID | 1392119132-50182-3-git-send-email-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) > { > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > > > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {