[GIT PULL 0/1] KVM: s390: bugfix for kvm/master (4.2)

[GIT PULL 0/1] KVM: s390: bugfix for kvm/master (4.2)

[Christian Borntraeger]

Paolo,

The following changes since commit 0da029ed7ee5fdf49a2a0e14160c3e9999be9292:

  KVM: x86: rename quirk constants to KVM_X86_QUIRK_* (2015-07-23 08:24:42 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvms390/linux.git  tags/kvm-s390-master-20150730

for you to fetch changes up to 586b7ccdb7143b6a9b975d2c6ad52b6ca5c162b9:

  KVM: s390: Fix hang VCPU hang/loop regression (2015-07-30 13:11:13 +0200)

----------------------------------------------------------------
KVM: s390: bugfix for kvm/master (4.2)

Here is a bugfix for a regression that was introduced after 4.1
with the commit commit 785dbef407d8 ("KVM: s390: optimize round
trip time in request handling"). After lots of cpu hotplugs in the
guest (online/offline) sometimes a guest CPU did loop within host
KVM code. Reason was that PROG_REQUEST was set in the sie control
block, but no request was pending. This made commit 785dbef407d8
the suspect and changing that area to always reset PROG_REQUEST
did indeed fix the problem.

Special thanks to David Hildenbrand, who helped understanding the
exact sequence that led to the problem.

----------------------------------------------------------------
Christian Borntraeger (1):
      KVM: s390: Fix hang VCPU hang/loop regression

 arch/s390/kvm/kvm-s390.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

[GIT PULL 1/1] KVM: s390: Fix hang VCPU hang/loop regression

[Christian Borntraeger]

commit 785dbef407d8 ("KVM: s390: optimize round trip time in request
handling") introduced a regression. This regression was seen with
CPU hotplug in the guest and switching between 1 or 2 CPUs. This will
set/reset the IBS control via synced request.

Whenever we make a synced request, we first set the vcpu->requests
bit and then block the vcpu. The handler, on the other hand, unblocks
itself, processes vcpu->requests (by clearing them) and unblocks itself
once again.

Now, if the requester sleeps between setting of vcpu->requests and
blocking, the handler will clear the vcpu->requests bit and try to
unblock itself (although no bit is set). When the requester wakes up,
it blocks the VCPU and we have a blocked VCPU without requests.

Solution is to always unset the block bit.

Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
Reviewed-by: David Hildenbrand <dahi@linux.vnet.ibm.com>
Fixes: 785dbef407d8 ("KVM: s390: optimize round trip time in request handling")
---
 arch/s390/kvm/kvm-s390.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 2078f92..f32f843 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1742,10 +1742,10 @@ static bool ibs_enabled(struct kvm_vcpu *vcpu)
 
 static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
 {
-	if (!vcpu->requests)
-		return 0;
 retry:
 	kvm_s390_vcpu_request_handled(vcpu);
+	if (!vcpu->requests)
+		return 0;
 	/*
 	 * We use MMU_RELOAD just to re-arm the ipte notifier for the
 	 * guest prefix page. gmap_ipte_notify will wait on the ptl lock.
-- 
2.3.0

Re: [GIT PULL 1/1] KVM: s390: Fix hang VCPU hang/loop regression

[Paolo Bonzini]

On 30/07/2015 13:22, Christian Borntraeger wrote:
>  static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
>  {
> -	if (!vcpu->requests)
> -		return 0;
>  retry:
>  	kvm_s390_vcpu_request_handled(vcpu);
> +	if (!vcpu->requests)
> +		return 0;
>  	/*

Should kvm_s390_vcpu_request_handled(vcpu); go before the retry label?

It shouldn't be too common to have two requests, and you're doing an 
extra atomic operation to clear PROG_REQUEST.

Or in fact, if you do this:

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 05e99b8ef465..a22a13fefb42 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1080,7 +1080,7 @@ static inline void kvm_make_request(int req, struct kvm_vcpu *vcpu)
 
 static inline bool kvm_check_request(int req, struct kvm_vcpu *vcpu)
 {
-	if (test_bit(req, &vcpu->requests)) {
+	if (vcpu->requests && test_bit(req, &vcpu->requests)) {
 		clear_bit(req, &vcpu->requests);
 		return true;
 	} else {
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index f32f843a3631..08e24dfc60d3 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -1742,10 +1742,7 @@ static bool ibs_enabled(struct kvm_vcpu *vcpu)
 
 static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
 {
-retry:
 	kvm_s390_vcpu_request_handled(vcpu);
-	if (!vcpu->requests)
-		return 0;
 	/*
 	 * We use MMU_RELOAD just to re-arm the ipte notifier for the
 	 * guest prefix page. gmap_ipte_notify will wait on the ptl lock.
@@ -1760,12 +1757,10 @@ retry:
 				      PAGE_SIZE * 2);
 		if (rc)
 			return rc;
-		goto retry;
 	}
 
 	if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
 		vcpu->arch.sie_block->ihcpu = 0xffff;
-		goto retry;
 	}
 
 	if (kvm_check_request(KVM_REQ_ENABLE_IBS, vcpu)) {
@@ -1774,7 +1769,6 @@ retry:
 			atomic_set_mask(CPUSTAT_IBS,
 					&vcpu->arch.sie_block->cpuflags);
 		}
-		goto retry;
 	}
 
 	if (kvm_check_request(KVM_REQ_DISABLE_IBS, vcpu)) {
@@ -1783,11 +1777,11 @@ retry:
 			atomic_clear_mask(CPUSTAT_IBS,
 					  &vcpu->arch.sie_block->cpuflags);
 		}
-		goto retry;
 	}
 
 	/* nothing to do, just clear the request */
-	clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
+	if (vcpu->requests)
+		clear_bit(KVM_REQ_UNHALT, &vcpu->requests);
 
 	return 0;
 }

... the compiler should be able to thread all the jumps together.  The
resulting code should be the same or better assuming that two requests
are rare.

This of course should be for 4.3---I've pulled your branch into kvm/master.

Paolo

Re: [GIT PULL 1/1] KVM: s390: Fix hang VCPU hang/loop regression

[Christian Borntraeger]

Am 30.07.2015 um 13:38 schrieb Paolo Bonzini:
> 
> 
> On 30/07/2015 13:22, Christian Borntraeger wrote:
>>  static int kvm_s390_handle_requests(struct kvm_vcpu *vcpu)
>>  {
>> -	if (!vcpu->requests)
>> -		return 0;
>>  retry:
>>  	kvm_s390_vcpu_request_handled(vcpu);
>> +	if (!vcpu->requests)
>> +		return 0;
>>  	/*
> 
> Should kvm_s390_vcpu_request_handled(vcpu); go before the retry label?
> 
> It shouldn't be too common to have two requests, and you're doing an 
> extra atomic operation to clear PROG_REQUEST.

Handling a request is slow path anyway and my last optimization was not
considering all side effects - so give me some time to comsume your comment 
to make it better this time....

[...]
> This of course should be for 4.3

yes :-)


> ---I've pulled your branch into kvm/master.