* [PATCH] kvm: get rid of srcu idx
@ 2010-10-24 17:16 Michael S. Tsirkin
2010-10-25 13:52 ` Marcelo Tosatti
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2010-10-24 17:16 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm, Avi Kivity
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 <mst@redhat.com>
---
Lightly tested.
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 311f6da..edb9dfa 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -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;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index c13cc48..60eb9db 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: get rid of srcu idx
2010-10-25 13:52 ` Marcelo Tosatti
@ 2010-10-25 13:20 ` Michael S. Tsirkin
2010-10-25 20:13 ` Marcelo Tosatti
0 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2010-10-25 13:20 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: kvm, Avi Kivity
On Mon, Oct 25, 2010 at 11:52:29AM -0200, Marcelo Tosatti wrote:
> On Sun, Oct 24, 2010 at 07:16:10PM +0200, Michael S. Tsirkin wrote:
> > 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 <mst@redhat.com>
> >
> > ---
>
> 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.
--
MST
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: get rid of srcu idx
2010-10-24 17:16 [PATCH] kvm: get rid of srcu idx Michael S. Tsirkin
@ 2010-10-25 13:52 ` Marcelo Tosatti
2010-10-25 13:20 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Marcelo Tosatti @ 2010-10-25 13:52 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, Avi Kivity
On Sun, Oct 24, 2010 at 07:16:10PM +0200, Michael S. Tsirkin wrote:
> 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 <mst@redhat.com>
>
> ---
Looks good to me.
Not needed for -stable, i don't see a problem with the current
code?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: get rid of srcu idx
2010-10-25 13:20 ` Michael S. Tsirkin
@ 2010-10-25 20:13 ` Marcelo Tosatti
2010-10-26 6:58 ` Gleb Natapov
0 siblings, 1 reply; 9+ messages in thread
From: Marcelo Tosatti @ 2010-10-25 20:13 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, Avi Kivity
On Mon, Oct 25, 2010 at 03:20:59PM +0200, Michael S. Tsirkin wrote:
> On Mon, Oct 25, 2010 at 11:52:29AM -0200, Marcelo Tosatti wrote:
> > On Sun, Oct 24, 2010 at 07:16:10PM +0200, Michael S. Tsirkin wrote:
> > > 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 <mst@redhat.com>
> > >
> > > ---
> >
> > 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: get rid of srcu idx
2010-10-25 20:13 ` Marcelo Tosatti
@ 2010-10-26 6:58 ` Gleb Natapov
2010-10-26 7:57 ` Avi Kivity
0 siblings, 1 reply; 9+ messages in thread
From: Gleb Natapov @ 2010-10-26 6:58 UTC (permalink / raw)
To: Marcelo Tosatti; +Cc: Michael S. Tsirkin, kvm, Avi Kivity
On Mon, Oct 25, 2010 at 06:13:56PM -0200, Marcelo Tosatti wrote:
> On Mon, Oct 25, 2010 at 03:20:59PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 25, 2010 at 11:52:29AM -0200, Marcelo Tosatti wrote:
> > > On Sun, Oct 24, 2010 at 07:16:10PM +0200, Michael S. Tsirkin wrote:
> > > > 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 <mst@redhat.com>
> > > >
> > > > ---
> > >
> > > 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.
>
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: get rid of srcu idx
2010-10-26 6:58 ` Gleb Natapov
@ 2010-10-26 7:57 ` Avi Kivity
2010-10-26 8:01 ` Gleb Natapov
0 siblings, 1 reply; 9+ messages in thread
From: Avi Kivity @ 2010-10-26 7:57 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Marcelo Tosatti, Michael S. Tsirkin, kvm
On 10/26/2010 08:58 AM, Gleb Natapov wrote:
> >
> > 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.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: get rid of srcu idx
2010-10-26 7:57 ` Avi Kivity
@ 2010-10-26 8:01 ` Gleb Natapov
2010-10-26 8:06 ` Avi Kivity
0 siblings, 1 reply; 9+ messages in thread
From: Gleb Natapov @ 2010-10-26 8:01 UTC (permalink / raw)
To: Avi Kivity; +Cc: Marcelo Tosatti, Michael S. Tsirkin, kvm
On Tue, Oct 26, 2010 at 09:57:08AM +0200, Avi Kivity wrote:
> On 10/26/2010 08:58 AM, Gleb Natapov wrote:
> >>
> >> 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.
>
But shouldn't we disallow recursive srcu lock tacking?
--
Gleb.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: get rid of srcu idx
2010-10-26 8:01 ` Gleb Natapov
@ 2010-10-26 8:06 ` Avi Kivity
2010-10-26 8:53 ` Michael S. Tsirkin
0 siblings, 1 reply; 9+ messages in thread
From: Avi Kivity @ 2010-10-26 8:06 UTC (permalink / raw)
To: Gleb Natapov; +Cc: Marcelo Tosatti, Michael S. Tsirkin, kvm
On 10/26/2010 10:01 AM, Gleb Natapov wrote:
> On Tue, Oct 26, 2010 at 09:57:08AM +0200, Avi Kivity wrote:
> > On 10/26/2010 08:58 AM, Gleb Natapov wrote:
> > >>
> > >> 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.
> >
> But shouldn't we disallow recursive srcu lock tacking?
With different srcu_idx, nested (or overlapping) srcu is legal.
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] kvm: get rid of srcu idx
2010-10-26 8:06 ` Avi Kivity
@ 2010-10-26 8:53 ` Michael S. Tsirkin
0 siblings, 0 replies; 9+ messages in thread
From: Michael S. Tsirkin @ 2010-10-26 8:53 UTC (permalink / raw)
To: Avi Kivity; +Cc: Gleb Natapov, Marcelo Tosatti, kvm
On Tue, Oct 26, 2010 at 10:06:41AM +0200, Avi Kivity wrote:
> On 10/26/2010 10:01 AM, Gleb Natapov wrote:
> >On Tue, Oct 26, 2010 at 09:57:08AM +0200, Avi Kivity wrote:
> >> On 10/26/2010 08:58 AM, Gleb Natapov wrote:
> >> >>
> >> >> 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.
> >>
> >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.
> --
> error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-10-26 9:00 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-24 17:16 [PATCH] kvm: get rid of srcu idx Michael S. Tsirkin
2010-10-25 13:52 ` Marcelo Tosatti
2010-10-25 13:20 ` Michael S. Tsirkin
2010-10-25 20:13 ` Marcelo Tosatti
2010-10-26 6:58 ` Gleb Natapov
2010-10-26 7:57 ` Avi Kivity
2010-10-26 8:01 ` Gleb Natapov
2010-10-26 8:06 ` Avi Kivity
2010-10-26 8:53 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.