| Message ID | 20120924071612.GE20907@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/24/2012 09:16 AM, Gleb Natapov wrote: > On Mon, Sep 24, 2012 at 03:24:47PM +0900, Takuya Yoshikawa wrote: >> This is an RFC since I have not done any comparison with the approach >> using for_each_set_bit() which can be seen in Avi's work. >> >> Takuya >> --- >> >> We did a simple test to see which requests we would get at the same time >> in vcpu_enter_guest() and got the following numbers: >> >> |...........|...............|........|...............|.| >> (N) (E) (S) (ES) others >> 22.3% 30.7% 16.0% 29.5% 1.4% >> >> (N) : Nothing >> (E) : Only KVM_REQ_EVENT >> (S) : Only KVM_REQ_STEAL_UPDATE >> (ES): Only KVM_REQ_EVENT and KVM_REQ_STEAL_UPDATE >> >> * Note that the exact numbers can change for other guests. >> > Yes, for guests that do not enable steal time KVM_REQ_STEAL_UPDATE > should be never set, but currently it is. The patch (not tested) should > fix this. > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 901ad00..01572f5 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1544,6 +1544,8 @@ static void accumulate_steal_time(struct kvm_vcpu *vcpu) > delta = current->sched_info.run_delay - vcpu->arch.st.last_steal; > vcpu->arch.st.last_steal = current->sched_info.run_delay; > vcpu->arch.st.accum_steal = delta; > + > + kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); > } > > static void record_steal_time(struct kvm_vcpu *vcpu) > @@ -1673,8 +1675,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > accumulate_steal_time(vcpu); > preempt_enable(); > > - kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); > - > break; > case MSR_KVM_PV_EOI_EN: > if (kvm_lapic_enable_pv_eoi(vcpu, data)) > @@ -2336,7 +2336,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > } > > accumulate_steal_time(vcpu); > - kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); > } > Even better would be not to account heavyweight exits as steal time. But we'd need a sched_exit preempt notifier for that.
On Mon, 24 Sep 2012 09:16:12 +0200 Gleb Natapov <gleb@redhat.com> wrote: > Yes, for guests that do not enable steal time KVM_REQ_STEAL_UPDATE > should be never set, but currently it is. The patch (not tested) should > fix this. Nice! Takuya > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 901ad00..01572f5 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1544,6 +1544,8 @@ static void accumulate_steal_time(struct kvm_vcpu *vcpu) > delta = current->sched_info.run_delay - vcpu->arch.st.last_steal; > vcpu->arch.st.last_steal = current->sched_info.run_delay; > vcpu->arch.st.accum_steal = delta; > + > + kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); > } > > static void record_steal_time(struct kvm_vcpu *vcpu) > @@ -1673,8 +1675,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > accumulate_steal_time(vcpu); > preempt_enable(); > > - kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); > - > break; > case MSR_KVM_PV_EOI_EN: > if (kvm_lapic_enable_pv_eoi(vcpu, data)) > @@ -2336,7 +2336,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) > } > > accumulate_steal_time(vcpu); > - kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); > } > > void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu) > -- > Gleb. > -- > 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 Mon, 24 Sep 2012 09:16:12 +0200 Gleb Natapov <gleb@redhat.com> wrote: > Yes, for guests that do not enable steal time KVM_REQ_STEAL_UPDATE > should be never set, but currently it is. The patch (not tested) should > fix this. Thinking a bit more about KVM_REQ_STEAL_UPDATE... > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 901ad00..01572f5 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1544,6 +1544,8 @@ static void accumulate_steal_time(struct kvm_vcpu *vcpu) > delta = current->sched_info.run_delay - vcpu->arch.st.last_steal; > vcpu->arch.st.last_steal = current->sched_info.run_delay; > vcpu->arch.st.accum_steal = delta; > + > + kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); > } Even without this flag, we know when we should record the steal time: that's when vcpu->arch.st.accum_steal != 0. > > static void record_steal_time(struct kvm_vcpu *vcpu) So we can live without KVM_REQ_STEAL_UPDATE and make record_steal_time() do the job when vcpu->arch.st.accum_steal != 0 so that it can be called unconditionally in vcpu_enter_guest(). Why didn't we do that? The only reason I can think of is we wanted to eliminate that check for the zero-requests case. But does such a micro-optimization really help us? Thanks, Takuya -- 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/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 901ad00..01572f5 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1544,6 +1544,8 @@ static void accumulate_steal_time(struct kvm_vcpu *vcpu) delta = current->sched_info.run_delay - vcpu->arch.st.last_steal; vcpu->arch.st.last_steal = current->sched_info.run_delay; vcpu->arch.st.accum_steal = delta; + + kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); } static void record_steal_time(struct kvm_vcpu *vcpu) @@ -1673,8 +1675,6 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) accumulate_steal_time(vcpu); preempt_enable(); - kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); - break; case MSR_KVM_PV_EOI_EN: if (kvm_lapic_enable_pv_eoi(vcpu, data)) @@ -2336,7 +2336,6 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu) } accumulate_steal_time(vcpu); - kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu); } void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)