kvm: get rid of srcu idx

PATCH Superseded Michael S. Tsirkin 4 participants 9 messages Oct 24, 2010 – Oct 26, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Michael S. Tsirkin

author

2010-10-24 17:22 UTC #0 lore raw

PATCH 1/1 kvm: get rid of srcu idx

srcu_idx is easy to misuse as it can not be used
in nested srcu_read_lock calls. Switch to local
index to make correctness easier to verify.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Lightly tested.

arch/x86/kvm/mmu.c +15 ~~-12~~

index 311f6da..edb9dfa 100644
@@ -4672,7 +4729,7 @@ static void kvm_put_guest_xcr0(struct kvm_vcpu *vcpu)
 	}
 }
 
-static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
+static int vcpu_enter_guest(struct kvm_vcpu *vcpu, int *srcu_idx)
 {
 	int r;
 	bool req_int_win = !irqchip_in_kernel(vcpu->kvm) &&

@@ -4744,7 +4801,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		kvm_lapic_sync_to_vapic(vcpu);
 	}
 
-	srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+	srcu_read_unlock(&vcpu->kvm->srcu, *srcu_idx);
 
 	kvm_guest_enter();

@@ -4787,7 +4844,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 
 	preempt_enable();
 
-	vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+	*srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 
 	/*
 	 * Profile KVM exit RIPs:

@@ -4809,6 +4866,7 @@ out:
 static int __vcpu_run(struct kvm_vcpu *vcpu)
 {
 	int r;
+	int srcu_idx;
 	struct kvm *kvm = vcpu->kvm;
 
 	if (unlikely(vcpu->arch.mp_state == KVM_MP_STATE_SIPI_RECEIVED)) {

@@ -4821,17 +4879,18 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 		vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
 	}
 
-	vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
+	
+	srcu_idx = srcu_read_lock(&kvm->srcu);
 	vapic_enter(vcpu);
 
 	r = 1;
 	while (r > 0) {
 		if (vcpu->arch.mp_state == KVM_MP_STATE_RUNNABLE)
-			r = vcpu_enter_guest(vcpu);
+			r = vcpu_enter_guest(vcpu, &srcu_idx);
 		else {
-			srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
+			srcu_read_unlock(&kvm->srcu, srcu_idx);
 			kvm_vcpu_block(vcpu);
-			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
+			srcu_idx = srcu_read_lock(&kvm->srcu);
 			if (kvm_check_request(KVM_REQ_UNHALT, vcpu))
 			{
 				switch(vcpu->arch.mp_state) {

@@ -4866,13 +4926,13 @@ static int __vcpu_run(struct kvm_vcpu *vcpu)
 			++vcpu->stat.signal_exits;
 		}
 		if (need_resched()) {
-			srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
+			srcu_read_unlock(&kvm->srcu, srcu_idx);
 			kvm_resched(vcpu);
-			vcpu->srcu_idx = srcu_read_lock(&kvm->srcu);
+			srcu_idx = srcu_read_lock(&kvm->srcu);
 		}
 	}
 
-	srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx);
+	srcu_read_unlock(&kvm->srcu, srcu_idx);
 
 	vapic_exit(vcpu);

@@ -4883,6 +4943,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 {
 	int r;
 	sigset_t sigsaved;
+	int srcu_idx;
 
 	if (vcpu->sigset_active)
 		sigprocmask(SIG_SETMASK, &vcpu->sigset, &sigsaved);

@@ -4905,9 +4966,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
 			vcpu->mmio_read_completed = 1;
 			vcpu->mmio_needed = 0;
 		}
-		vcpu->srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
+		srcu_idx = srcu_read_lock(&vcpu->kvm->srcu);
 		r = emulate_instruction(vcpu, 0, 0, EMULTYPE_NO_DECODE);
-		srcu_read_unlock(&vcpu->kvm->srcu, vcpu->srcu_idx);
+		srcu_read_unlock(&vcpu->kvm->srcu, srcu_idx);
 		if (r != EMULATE_DONE) {
 			r = 0;
 			goto out;

include/linux/kvm_host.h +0 -1

index c13cc48..60eb9db 100644
@@ -85,7 +85,6 @@ struct kvm_vcpu {
 	struct kvm_run *run;
 	unsigned long requests;
 	unsigned long guest_debug;
-	int srcu_idx;
 
 	int fpu_active;
 	int guest_fpu_loaded, guest_xcr0_loaded;

Marcelo Tosatti

2010-10-25 12:53 UTC #1 lore raw

Quoted 7 lines · Michael S. Tsirkin

srcu_idx is easy to misuse as it can not be used
in nested srcu_read_lock calls. Switch to local
index to make correctness easier to verify.
Signed-off-by Michael S. Tsirkin <[email protected]>
---

Looks good to me.

Not needed for -stable, i don't see a problem with the current code?

Michael S. Tsirkin

author

2010-10-25 13:27 UTC #2 lore raw

Quoted 5 lines · Marcelo Tosatti

Quoted 4 lines · Michael S. Tsirkin
srcu_idx is easy to misuse as it can not be used
in nested srcu_read_lock calls. Switch to local
index to make correctness easier to verify.

Quoted 8 lines

Quoted 3 lines
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Looks good to me.
Not needed for -stable, i don't see a problem with the current code?

It's just that I can not convince myself current code never nestes read_locks with srcu_idx. I used to see host crashes under migration stress they seem gone now but did not retest - could be just luck.

Signature · 1 line

MST

Marcelo Tosatti

2010-10-25 21:20 UTC #3 lore raw

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Marcelo Tosatti
Quoted 7 lines · Michael S. Tsirkin
srcu_idx is easy to misuse as it can not be used
in nested srcu_read_lock calls. Switch to local
index to make correctness easier to verify.
Signed-off-by Michael S. Tsirkin <[email protected]>
---

Quoted 8 lines

Quoted 4 lines
Looks good to me.
Not needed for -stable, i don't see a problem with the current code?
It's just that I can not convince myself current code never nestes read_locks with srcu_idx. I used to see host crashes under migration stress they seem gone now but did not retest - could be just luck.

Well, you just changed where srcu index is saved. I don't see how it could make a difference in practice.

Gleb Natapov

2010-10-26 06:58 UTC #4 lore raw

Quoted 16 lines · Marcelo Tosatti

Quoted 15 lines · Michael S. Tsirkin
Quoted 13 lines · Marcelo Tosatti
Quoted 7 lines · Michael S. Tsirkin
srcu_idx is easy to misuse as it can not be used
in nested srcu_read_lock calls. Switch to local
index to make correctness easier to verify.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Looks good to me.
Not needed for -stable, i don't see a problem with the current code?

Quoted 7 lines

Quoted 3 lines
It's just that I can not convince myself current code never nestes read_locks with srcu_idx. I used to see host crashes under migration stress they seem gone now but did not retest - could be just luck.
Well, you just changed where srcu index is saved. I don't see how it could make a difference in practice.

If there is nested call to srcu read lock if srcu_idx is stored in vcpu nested call will override previous srcu_idx value and srcu unlock will not be called on it, but it will be called twice on new srcu_idx value. If srcu_idx is saved on stack this will not happen, no?

--
Gleb.

Avi Kivity

2010-10-26 07:57 UTC #5 lore raw

Quoted 8 lines · Gleb Natapov

Quoted 4 lines
Well, you just changed where srcu index is saved. I don't see how it could make a difference in practice.
If there is nested call to srcu read lock if srcu_idx is stored in vcpu nested call will override previous srcu_idx value and srcu unlock will not be called on it, but it will be called twice on new srcu_idx value. If srcu_idx is saved on stack this will not happen, no?

Exactly. The case where srcu_idx is passed as a pointer parameter is still dubious, but we can change that too.

Signature · 1 line

error compiling committee.c: too many arguments to function

Gleb Natapov

2010-10-26 08:01 UTC #6 lore raw

Quoted 5 lines · Avi Kivity

Quoted 4 lines · Gleb Natapov
Quoted 4 lines
Well, you just changed where srcu index is saved. I don't see how it could make a difference in practice.

Quoted 8 lines

Quoted 4 lines
If there is nested call to srcu read lock if srcu_idx is stored in vcpu nested call will override previous srcu_idx value and srcu unlock will not be called on it, but it will be called twice on new srcu_idx value. If srcu_idx is saved on stack this will not happen, no?
Exactly. The case where srcu_idx is passed as a pointer parameter is still dubious, but we can change that too.

But shouldn't we disallow recursive srcu lock tacking?

--
Gleb.

Avi Kivity

2010-10-26 08:06 UTC #7 lore raw

Quoted 11 lines · Gleb Natapov

Quoted 10 lines · Avi Kivity
Quoted 8 lines · Gleb Natapov
Quoted 4 lines
Well, you just changed where srcu index is saved. I don't see how it could make a difference in practice.
If there is nested call to srcu read lock if srcu_idx is stored in vcpu nested call will override previous srcu_idx value and srcu unlock will not be called on it, but it will be called twice on new srcu_idx value. If srcu_idx is saved on stack this will not happen, no?

Quoted 4 lines

Quoted 3 lines
Exactly. The case where srcu_idx is passed as a pointer parameter is still dubious, but we can change that too.
But shouldn't we disallow recursive srcu lock tacking?

With different srcu_idx, nested (or overlapping) srcu is legal.

Signature · 1 line

error compiling committee.c: too many arguments to function

Michael S. Tsirkin

author

2010-10-26 09:00 UTC #8 lore raw

Quoted 12 lines · Avi Kivity

Quoted 11 lines · Gleb Natapov
Quoted 10 lines · Avi Kivity
Quoted 8 lines · Gleb Natapov
Quoted 4 lines
Well, you just changed where srcu index is saved. I don't see how it could make a difference in practice.
If there is nested call to srcu read lock if srcu_idx is stored in vcpu nested call will override previous srcu_idx value and srcu unlock will not be called on it, but it will be called twice on new srcu_idx value. If srcu_idx is saved on stack this will not happen, no?

Quoted 6 lines

Quoted 4 lines
Quoted 3 lines
Exactly. The case where srcu_idx is passed as a pointer parameter is still dubious, but we can change that too.
But shouldn't we disallow recursive srcu lock tacking?
With different srcu_idx, nested (or overlapping) srcu is legal.

Right. If vcpu_enter_guest gets calles under a nested srcu call we'll still get a deadlock, however.

Quoted 2 lines

Signature · 1 line

error compiling committee.c: too many arguments to function