Modify KVM to update guest time accounting.

[PATCH 4/4] Modify KVM to update guest time accounting.

drivers/kvm/kvm.h void kvm_mmu_unload(struct kvm_vcpu *vcp · line 633

Quoted 10 lines · Ingo Molnar

int kvm_hypercall(struct kvm_vcpu *vcpu, struct kvm_run *run);

drivers/kvm/kvm.h +7 -0

+/*
+ * Compatibility define - PF_VCPU is only available in v2.6.24+ kernels:
+ */
+#ifndef PF_VCPU
+# define PF_VCPU	0
+#endif
+

This bit can go; for the external module I can add it back in external-module-compat.h. No need to pollute mainline with backward compatibility stuff.

This bit can go; for the external module I can add it back in external-module-compat.h. No need to pollute mainline with backward compatibility stuff.

Quoted 2 lines · Avi Kivity

This bit can go; for the external module I can add it back in external-module-compat.h. No need to pollute mainline with backward compatibility stuff.

hm:

static inline void kvm_guest_enter(void)

{
        current->flags |= PF_VCPU;
}

static inline void kvm_guest_exit(void)
{
}

shouldnt PF_VCPU be cleared in kvm_guest_exit()?

Quoted 19 lines · Ingo Molnar

Quoted 4 lines · Avi Kivity

This bit can go; for the external module I can add it back in external-module-compat.h. No need to pollute mainline with backward compatibility stuff.

hm:

static inline void kvm_guest_enter(void)

{
        current->flags |= PF_VCPU;
}

static inline void kvm_guest_exit(void)
{
}

Quoted 2 lines

shouldnt PF_VCPU be cleared in kvm_guest_exit()?

IIRC the accounting code clears it, but yes, it may not have been called at all, so clearing it here is needed.

Quoted 25 lines · Avi Kivity

Quoted 23 lines · Ingo Molnar

Quoted 5 lines · Avi Kivity

This bit can go; for the external module I can add it back in external-module-compat.h. No need to pollute mainline with backward compatibility stuff.

hm:

static inline void kvm_guest_enter(void)

{
        current->flags |= PF_VCPU;
}

static inline void kvm_guest_exit(void)
{
}

shouldnt PF_VCPU be cleared in kvm_guest_exit()?

Quoted 4 lines

IIRC the accounting code clears it, but yes, it may not have been called at all, so clearing it here is needed.

No, It must not be cleared here because we can't enter in the accounting code between kvm_guest_enter(void) and kvm_guest_exit(void).

This is why the accounting code clears it.

We can clear it a bit later, after local_irq_enable() in __vcpu_run(). However we need a nop instruction first because "sti" keeps interrupts disabled for one more instruction.

Am Montag, 15. Oktober 2007 schrieb Avi Kivity:

Quoted 3 lines

We can clear it a bit later, after local_irq_enable() in __vcpu_run(). However we need a nop instruction first because "sti" keeps interrupts disabled for one more instruction.

Ah, I see. The host interrupt behaves different and instead of running the interrupt handler, it exits the vmrun on x86? The interrupt handler will be called some cycles after the sti?
That is different to s390. We can run the guest code for a long time and the host instruction pointer stays on the sie instruction. That means, we can interrupt sie and continue by simply setting the instrution pointer (PSW) back to the sie instruction.

Any idea how to make this proper on all architectures? I will have a look.

Quoted 1 line

Any idea how to make this proper on all architectures? I will have a look.

I think the solution is to have an arch dependent kvm_guest_exit(): empty for x86, clearing the bit for s390.

Am Montag, 15. Oktober 2007 schrieb Laurent Vivier:

Quoted 3 lines

Quoted 1 line

Any idea how to make this proper on all architectures? I will have a look.

I think the solution is to have an arch dependent kvm_guest_exit(): empty for x86, clearing the bit for s390.

I think we can merge your patches, as the userspace interface seems fine. To make the accounting work for virtual cpu accounting found on ppc and s390 we can later add an additional patch that also deals with interruptible guest contexts.
So something like this should work:

Signed-off-by Christian Borntraeger <[email protected]>

Quoted 25 lines · Laurent Vivier

Quoted 23 lines · Avi Kivity

Quoted 21 lines · Ingo Molnar

Quoted 4 lines · Avi Kivity

This bit can go; for the external module I can add it back in external-module-compat.h. No need to pollute mainline with backward compatibility stuff.

hm:

static inline void kvm_guest_enter(void)

{
        current->flags |= PF_VCPU;
}

static inline void kvm_guest_exit(void)
{
}

shouldnt PF_VCPU be cleared in kvm_guest_exit()?

Quoted 10 lines

Quoted 4 lines

IIRC the accounting code clears it, but yes, it may not have been called at all, so clearing it here is needed.

No, It must not be cleared here because we can't enter in the accounting code
between kvm_guest_enter(void) and kvm_guest_exit(void).

Right.

Quoted 2 lines

This is why the accounting code clears it.

But if we didn't get an interrupt in that time?

We can clear it a bit later, after local_irq_enable() in __vcpu_run(). However we need a nop instruction first because "sti" keeps interrupts disabled for one more instruction.

Quoted 5 lines

But if we didn't get an interrupt in that time?

We can clear it a bit later, after local_irq_enable() in __vcpu_run(). However we need a nop instruction first because "sti" keeps interrupts disabled for one more instruction.

IMHO, I think it is better to let kvm_guest_exit() empty (you can remove it, if you want):

1st case:
- unset PF_VCPU in kvm_guest_exit(), all the tick is always for system time. Guest time is always 0.

1st case and half:

- like 1st case but we move kvm_guest_exit() as you propose and the reason of the interrupt is the tick interrupt. The tick is for guest time only. I think the probability is very low.

2nd case:
- don't unset PF_VCPU in kvm_guest_exit(), all the tick is for guest time.

Quoted 17 lines · Laurent Vivier

Quoted 7 lines

But if we didn't get an interrupt in that time?

We can clear it a bit later, after local_irq_enable() in __vcpu_run(). However we need a nop instruction first because "sti" keeps interrupts
disabled for one more instruction.

IMHO, I think it is better to let kvm_guest_exit() empty (you can remove it, if
you want):

1st case:
- unset PF_VCPU in kvm_guest_exit(), all the tick is always for system time.
Guest time is always 0.

Quoted 8 lines

1st case and half:

- like 1st case but we move kvm_guest_exit() as you propose and the reason of
the interrupt is the tick interrupt. The tick is for guest time only. I think
the probability is very low.

If the guest is executing for 10% of the time, the probability is exactly 10%, no?

Quoted 4 lines

2nd case:
- don't unset PF_VCPU in kvm_guest_exit(), all the tick is for guest time.

But then even execution in ->handle_exit() is accounted as guest time, which is wrong.

Quoted 31 lines · Avi Kivity

Quoted 26 lines · Laurent Vivier

Quoted 8 lines

But if we didn't get an interrupt in that time?

We can clear it a bit later, after local_irq_enable() in __vcpu_run(). However we need a nop instruction first because "sti" keeps interrupts
disabled for one more instruction.

IMHO, I think it is better to let kvm_guest_exit() empty (you can remove it, if
you want):

1st case:
- unset PF_VCPU in kvm_guest_exit(), all the tick is always for system time.
Guest time is always 0.

1st case and half:

- like 1st case but we move kvm_guest_exit() as you propose and the reason of
the interrupt is the tick interrupt. The tick is for guest time only. I think
the probability is very low.

Quoted 3 lines

If the guest is executing for 10% of the time, the probability is exactly 10%, no?

I think you know that better than me.

But is there homogeneity in probability ?

I mean, if the guest has a lot I/O, it is interrupted by them and the probability to be interrupted by a tick is lower than the time passed in the VCPU ?

Quoted 6 lines

Quoted 6 lines

2nd case:
- don't unset PF_VCPU in kvm_guest_exit(), all the tick is for guest time.

Quoted 3 lines

But then even execution in ->handle_exit() is accounted as guest time, which is wrong.

System time and User time are wrong too as the tick is accounted to the side where it appears, even if CPU has executed code from the other side in a sub-part of the tick. It's not a good argument.

Quoted 31 lines · Laurent Vivier

Quoted 29 lines · Avi Kivity

Quoted 24 lines · Laurent Vivier

Quoted 7 lines

But if we didn't get an interrupt in that time?

We can clear it a bit later, after local_irq_enable() in __vcpu_run(). However we need a nop instruction first because "sti" keeps interrupts
disabled for one more instruction.

IMHO, I think it is better to let kvm_guest_exit() empty (you can remove it, if
you want):

1st case:
- unset PF_VCPU in kvm_guest_exit(), all the tick is always for system time.
Guest time is always 0.

1st case and half:

- like 1st case but we move kvm_guest_exit() as you propose and the reason of
the interrupt is the tick interrupt. The tick is for guest time only. I think
the probability is very low.

Quoted 8 lines

Quoted 3 lines

If the guest is executing for 10% of the time, the probability is exactly 10%, no?

I think you know that better than me.

But is there homogeneity in probability ?

It's exactly the same issue as with systime and usertime. The interrupt samples the program counter at various points at a fairly low frequency (milliseconds) while syscalls last a few dozens of microseconds. Probability makes it average out correctly in the end.

[Ingo, what about dyntick? suppose you have just one process that calls read() from /dev/zero repeatedly. There'd be very few (or no) interrupts -- what happens to accounting accuracy?]

Quoted 3 lines

I mean, if the guest has a lot I/O, it is interrupted by them and the probability to be interrupted by a tick is lower than the time passed in the VCPU ?

Suppose the time to service the I/O is exactly equal to the amount running in guest mode. Then the probability of the interrupt happening in guest mode is equal to it happening outside guest mode and you'd get 50% guest, 50% system/user, which is what you want.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

2nd case:
- don't unset PF_VCPU in kvm_guest_exit(), all the tick is for guest time.

Quoted 8 lines

Quoted 3 lines

But then even execution in ->handle_exit() is accounted as guest time, which is wrong.

System time and User time are wrong too as the tick is accounted to the side where it appears, even if CPU has executed code from the other side in a sub-part of the tick. It's not a good argument.

It's at least consistent... the same errors for everyone, so it averages out in the end.

clear PF_VCPU in kvm_guest_exit() and move kvm_guest_exit() after local_irq_enable().

clear PF_VCPU in kvm_guest_exit() and move kvm_guest_exit() after local_irq_enable().

According comments from Avi, we can clear PF_VCPU in kvm_guest_exit if we move it after local_irq_enable().

http://lkml.org/lkml/2007/10/15/114

To simplify s390 port, we don't clear it in account_system_time().

http://lkml.org/lkml/2007/10/15/183

Signed-off-by Laurent Vivier <[email protected]>

---
 drivers/kvm/kvm.h      |    1 +
 drivers/kvm/kvm_main.c |    3 ++-
 kernel/sched.c         |    1 -
 3 files changed, 3 insertions(+), 2 deletions(-)

drivers/kvm/kvm.h +0 -0

index e9dbf67..e8b4902 100644

drivers/kvm/kvm.h +0 -0

 static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,

drivers/kvm/kvm_main.c +0 -0

index 87275be..a9db477 100644

drivers/kvm/kvm_main.c +2 -0

+	kvm_guest_exit();
+
 	preempt_enable();
 

/*

Quoted 24 lines · Laurent Vivier

clear PF_VCPU in kvm_guest_exit() and move kvm_guest_exit() after local_irq_enable().

According comments from Avi, we can clear PF_VCPU in kvm_guest_exit if we move it after local_irq_enable().

http://lkml.org/lkml/2007/10/15/114

To simplify s390 port, we don't clear it in account_system_time().

http://lkml.org/lkml/2007/10/15/183

Signed-off-by Laurent Vivier <[email protected]>

---
 drivers/kvm/kvm.h      |    1 +
 drivers/kvm/kvm_main.c |    3 ++-
 kernel/sched.c         |    1 -
 3 files changed, 3 insertions(+), 2 deletions(-)

drivers/kvm/kvm.h +0 -0

index e9dbf67..e8b4902 100644

drivers/kvm/kvm.h static inline void kvm_guest_enter(void) · line 682

Quoted 6 lines

drivers/kvm/kvm.h +1 -0

@@ -677,6 +677,7 @@ static inline void kvm_guest_enter(void)
 
 static inline void kvm_guest_exit(void)
 {
+	current->flags &= ~PF_VCPU;
 }
 

Ingo already added this.

Quoted 7 lines

drivers/kvm/kvm.h +0 -0

 static inline int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gva_t gva,

drivers/kvm/kvm_main.c +0 -0

index 87275be..a9db477 100644

drivers/kvm/kvm_main.c again: · line 2202

Quoted 10 lines

drivers/kvm/kvm_main.c +0 -1

@@ -2194,12 +2194,13 @@ again:
 
 	kvm_x86_ops->run(vcpu, kvm_run);
 
-	kvm_guest_exit();
 	vcpu->guest_mode = 0;
 	local_irq_enable();
 
 	++vcpu->stat.exits;
 
 

Need a barrier() here. Otherwise the compiler may reorder "kvm_guest_exit();" and "++vcpu->stats.exits;", making kvm_guest_exit() right after local_irq_enable(). If it inlines kvm_guest_exit(), and further encodes it in one instruction (both likely) we get:

sti
andl $something, (something_else)

The andl executes in interrupt shadow (that is, interrupts are still disabled) and the timer tick won't see PF_VCPU.

Quoted 4 lines

drivers/kvm/kvm_main.c +2 -0

+	kvm_guest_exit();
+
 	preempt_enable();
 

Quoted 2 lines

/*

Please copy kvm-devel on kvm patches.

ppc and s390 offer the possibility to track process times precisely by looking at cpu timer on every context switch, irq, softirq etc. We can use that infrastructure as well for guest time accounting. We need to account the used time before we change the state. This patch adds a call to account_system_vtime to kvm_guest_enter and kvm_guest exit. If CONFIG_VIRT_CPU_ACCOUNTING is not set, account_system_vtime is defined in hardirq.h as an empty function, which means this patch does not change the behaviour on other platforms.

I compile tested this patch on x86 and function tested the patch on s390.

Avi, please apply.

Avi,

ppc and s390 offer the possibility to track process times precisely by looking at cpu timer on every context switch, irq, softirq etc. We can use that infrastructure as well for guest time accounting. We need to account the used time before we change the state. This patch adds a call to account_system_vtime to kvm_guest_enter and kvm_guest exit. If CONFIG_VIRT_CPU_ACCOUNTING is not set, account_system_vtime is defined in hardirq.h as an empty function, which means this patch does not change the behaviour on other platforms.

I compile tested this patch on x86 and function tested the patch on s390.

Avi, please apply.

Signed-off-by Christian Borntraeger <[email protected]>

---
 drivers/kvm/kvm.h |    3 +++
 1 file changed, 3 insertions(+)

Index: kvm/drivers/kvm/kvm.h
===================================================================

drivers/kvm/kvm.h +1 -0

@@ -7,6 +7,7 @@
  */
 
 #include <linux/types.h>
+#include <linux/hardirq.h>
 #include <linux/list.h>
 #include <linux/mutex.h>
 #include <linux/spinlock.h>

@@ -669,11 +670,13 @@ __init void kvm_arch_init(void);
 
 static inline void kvm_guest_enter(void)

Quoted 8 lines

drivers/kvm/kvm.h +1 -0

@@ -669,11 +670,13 @@ __init void kvm_arch_init(void);
 
 static inline void kvm_guest_enter(void)
 {
+	account_system_vtime(current);
 	current->flags |= PF_VCPU;
 }
 

drivers/kvm/kvm.h __init void kvm_arch_init(void); · line 681

Quoted 5 lines

drivers/kvm/kvm.h +1 -0

 static inline void kvm_guest_exit(void)
 {
+	account_system_vtime(current);
 	current->flags &= ~PF_VCPU;
 }

I don't understand. Should kvm_guest_exit() be calling account_user_vtime() (instead of account_system_vtime())?

According comments from Avi, we can clear PF_VCPU in kvm_guest_exit if we move it after local_irq_enable().

http://lkml.org/lkml/2007/10/15/114

To simplify s390 port, we don't clear it in account_system_time().

http://lkml.org/lkml/2007/10/15/183

Quoted 5 lines · Laurent Vivier

According comments from Avi, we can clear PF_VCPU in kvm_guest_exit if we move it after local_irq_enable().

http://lkml.org/lkml/2007/10/15/114

Quoted 4 lines

To simplify s390 port, we don't clear it in account_system_time().

http://lkml.org/lkml/2007/10/15/183

Applied (the kvm part), and added a fat comment on the barrier. Can you send a signed-off-by: line?

According comments from Avi, we can clear PF_VCPU in kvm_guest_exit if we move it after local_irq_enable().

http://lkml.org/lkml/2007/10/15/114

To simplify s390 port, we don't clear it in account_system_time().

http://lkml.org/lkml/2007/10/15/183

Quoted 3 lines · Laurent Vivier

According comments from Avi, we can clear PF_VCPU in kvm_guest_exit if we move it after local_irq_enable().

Quoted 6 lines

http://lkml.org/lkml/2007/10/15/114

To simplify s390 port, we don't clear it in account_system_time().

http://lkml.org/lkml/2007/10/15/183

thanks - i have picked your patch up.

Quoted 10 lines · Ingo Molnar

Quoted 7 lines · Laurent Vivier

According comments from Avi, we can clear PF_VCPU in kvm_guest_exit if we move it after local_irq_enable().

http://lkml.org/lkml/2007/10/15/114

To simplify s390 port, we don't clear it in account_system_time().

Quoted 6 lines

Quoted 2 lines

http://lkml.org/lkml/2007/10/15/183

thanks - i have picked your patch up.

Note the kvm part is already in my 2.6.24 queue.

Quoted 3 lines

Note the kvm part is already in my 2.6.24 queue.

ok, then please push the whole patch to Linus, the scheduler bits are:

Acked-by Ingo Molnar <[email protected]>

No, It must not be cleared here because we can't enter in the accounting code between kvm_guest_enter(void) and kvm_guest_exit(void).

This is why the accounting code clears it.

I put a kvm_guest_exit() only for symmetry.