[PATCH] arm/arm64: KVM: Propertly account for guest CPU time

2015-05-28 Thread Christoffer Dall
Until now we have been calling kvm_guest_exit after re-enabling
interrupts when we come back from the guest, but this has the
unfortunate effect that CPU time accounting done in the context of timer
interrupts doesn't properly notice that the time since the last tick was
spent in the guest.

Inspired by the comment in the x86 code, simply move the
kvm_guest_exit() call below the local_irq_enable() call and change
__kvm_guest_exit() to kvm_guest_exit(), because we are now calling this
function with interrupts enabled.  Note that AFAIU we don't need an
explicit barrier like x86 because the arm/arm64 implementation of
local_irq_(en/dis)able has an implicit barrier.

At the same time, move the trace_kvm_exit() call outside of the atomic
section, since there is no reason for us to do that with interrupts
disabled.

Signed-off-by: Christoffer Dall christoffer.d...@linaro.org
---
This patch is based on kvm/queue, because it has the kvm_guest_enter/exit
rework recently posted by Christian Borntraeger.  I hope I got the logic
of this wrong, there were 2 slightly worrying facts about this:

First, we now enable and disable and enable interrupts on each exit
path, but I couldn't see any performance overhead on hackbench - yes the
only benchmark we care abotu.

Second, looking at the power and mips code, they seem to also call
kvm_guest_exit() before enabling interrupts, so I don't understand how
guest CPU time accounting works on those architectures.

 arch/arm/kvm/arm.c | 16 
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index e41cb11..bd0e463 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -559,8 +559,10 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
kvm_run *run)
ret = kvm_call_hyp(__kvm_vcpu_run, vcpu);
 
vcpu-mode = OUTSIDE_GUEST_MODE;
-   __kvm_guest_exit();
-   trace_kvm_exit(kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
+   /*
+* Back from guest
+*/
+
/*
 * We may have taken a host interrupt in HYP mode (ie
 * while executing the guest). This interrupt is still
@@ -574,8 +576,14 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
kvm_run *run)
local_irq_enable();
 
/*
-* Back from guest
-*/
+* We do local_irq_enable() before calling kvm_guest_exit so
+* that the cputime accounting done in the context of timer
+* interrupts properly accounts time spent in the guest as
+* guest time.
+*/
+   kvm_guest_exit();
+   trace_kvm_exit(kvm_vcpu_trap_get_class(vcpu), *vcpu_pc(vcpu));
+
 
kvm_timer_sync_hwstate(vcpu);
kvm_vgic_sync_hwstate(vcpu);
-- 
2.1.2.330.g565301e.dirty

--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] arm/arm64: KVM: Propertly account for guest CPU time

2015-05-28 Thread Christoffer Dall
On Thu, May 28, 2015 at 02:49:09PM +0200, Christoffer Dall wrote:
 Until now we have been calling kvm_guest_exit after re-enabling
 interrupts when we come back from the guest, but this has the
 unfortunate effect that CPU time accounting done in the context of timer
 interrupts doesn't properly notice that the time since the last tick was
 spent in the guest.
 
 Inspired by the comment in the x86 code, simply move the
 kvm_guest_exit() call below the local_irq_enable() call and change
 __kvm_guest_exit() to kvm_guest_exit(), because we are now calling this
 function with interrupts enabled.  Note that AFAIU we don't need an
 explicit barrier like x86 because the arm/arm64 implementation of
 local_irq_(en/dis)able has an implicit barrier.
 
 At the same time, move the trace_kvm_exit() call outside of the atomic
 section, since there is no reason for us to do that with interrupts
 disabled.
 
 Signed-off-by: Christoffer Dall christoffer.d...@linaro.org
 ---
 This patch is based on kvm/queue, because it has the kvm_guest_enter/exit
 rework recently posted by Christian Borntraeger.  I hope I got the logic
 of this wrong, there were 2 slightly worrying facts about this:

Of course this should have been:

I hope I got the logic of this *right*, but there...

Damn it!
-Christoffer
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html