Using the tlb flush util function where applicable

PATCH In review Liang Chen 4 participants 7 messages Sep 12, 2014 – Sep 17, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Liang Chen

author

2014-09-12 21:06 UTC #0 lore raw

PATCH 1/1 Using the tlb flush util function where applicable

Using kvm_mmu_flush_tlb as the other places to make sure vcpu
stat is incremented

Signed-off-by Liang Chen <[email protected]>

---
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/kvm/vmx.c +1 -1

index bfe11cf..439682e 100644
@@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
 		unsigned long sysenter_esp;
 
-		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
+		kvm_mmu_flush_tlb(vcpu);
 		local_irq_disable();
 		crash_disable_local_vmclear(cpu);

Signature · 1 line

1.9.1

Radim Krčmář

2014-09-15 19:33 UTC #1 lore raw

2014-09-12 17:06-0400, Liang Chen:

Quoted 5 lines

Using kvm_mmu_flush_tlb as the other places to make sure vcpu
stat is incremented
Signed-off-by Liang Chen <[email protected]>
---

Good catch.

Quoted 8 lines

 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/x86/kvm/vmx.c +0 -0

index bfe11cf..439682e 100644

arch/x86/kvm/vmx.c static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) · line 1816

Quoted 8 lines

arch/x86/kvm/vmx.c +1 -1

@@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
 		unsigned long sysenter_esp;
 
-		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
+		kvm_mmu_flush_tlb(vcpu);
 		local_irq_disable();
 		crash_disable_local_vmclear(cpu);

And to hijack this thread ...
I noticed three other deficits in stat.tlb_flush, patch below.

Do you prefer the current behavior?

--- 8< ---
KVM: x86: count actual tlb flushes

- we count KVM_REQ_TLB_FLUSH requests, not actual flushes (KVM can have multiple requests for one flush)
- flushes from kvm_flush_remote_tlbs aren't counted
- it's easy to make a direct request by mistake

Solve these by postponing the counting to kvm_check_request().

Signed-off-by Radim Krčmář <[email protected]>

---
(And what about a possible followup patch that replaces
kvm_mmu_flush_tlb() with kvm_make_request() again? It would free the namespace a bit and we could call something similarly named from vcpu_enter_guest() to do the job.)

arch/x86/kvm/mmu.c +0 -1

index 9314678..b41fd97 100644
@@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
 
 void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
 {
-	++vcpu->stat.tlb_flush;
 	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 }
 EXPORT_SYMBOL_GPL(kvm_mmu_flush_tlb);

arch/x86/kvm/x86.c +3 -1

index 916e895..0f0ad08 100644
@@ -6041,8 +6041,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		}
 		if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu))
 			kvm_mmu_sync_roots(vcpu);
-		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
+		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
+			++vcpu->stat.tlb_flush;
 			kvm_x86_ops->tlb_flush(vcpu);
+		}
 		if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
 			vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
 			r = 0;

Liang Chen

author

2014-09-15 22:49 UTC #2 lore raw

Quoted 27 lines · Radim Krčmář

2014-09-12 17:06-0400, Liang Chen:
Quoted 5 lines
Using kvm_mmu_flush_tlb as the other places to make sure vcpu
stat is incremented
Signed-off-by Liang Chen <[email protected]>
---
Good catch.
Quoted 8 lines
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kvm/vmx.c +0 -0
index bfe11cf..439682e 100644
arch/x86/kvm/vmx.c static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) · line 1816
Quoted 8 lines
arch/x86/kvm/vmx.c +1 -1
@@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
 		unsigned long sysenter_esp;
 
-		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
+		kvm_mmu_flush_tlb(vcpu);
 		local_irq_disable();
 		crash_disable_local_vmclear(cpu);
 
And to hijack this thread ...
I noticed three other deficits in stat.tlb_flush, patch below.

Quoted 1 line

Do you prefer the current behavior?

Yeah, you are right. I think the patch below gives a more thorough solution to make sure the counter really shows the number of tlb flushes - not the time kvm_mmu_flush_tlb is called. Thanks!

Quoted 8 lines

--- 8< ---
KVM: x86: count actual tlb flushes
- we count KVM_REQ_TLB_FLUSH requests, not actual flushes (KVM can have multiple requests for one flush)
- flushes from kvm_flush_remote_tlbs aren't counted
- it's easy to make a direct request by mistake

Quoted 8 lines

Solve these by postponing the counting to kvm_check_request().
Signed-off-by Radim Krčmář <[email protected]>
---
(And what about a possible followup patch that replaces
kvm_mmu_flush_tlb() with kvm_make_request() again? It would free the namespace a bit and we could call something similarly named from vcpu_enter_guest() to do the job.)

That seems good. I can take the labor to make the followup patch ;)

Thanks,
Liang Chen

Quoted 28 lines

arch/x86/kvm/mmu.c +0 -1

index 9314678..b41fd97 100644
@@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
 
 void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
 {
-	++vcpu->stat.tlb_flush;
 	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 }
 EXPORT_SYMBOL_GPL(kvm_mmu_flush_tlb);

arch/x86/kvm/x86.c +3 -1

index 916e895..0f0ad08 100644
@@ -6041,8 +6041,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		}
 		if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu))
 			kvm_mmu_sync_roots(vcpu);
-		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
+		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
+			++vcpu->stat.tlb_flush;
 			kvm_x86_ops->tlb_flush(vcpu);
+		}
 		if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
 			vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
 			r = 0;

Paolo Bonzini

maintainer

2014-09-16 08:56 UTC #3 lore raw

Il 16/09/2014 00:49, Liang Chen ha scritto:

Quoted 6 lines

Quoted 5 lines
Quoted 5 lines
---
(And what about a possible followup patch that replaces
kvm_mmu_flush_tlb() with kvm_make_request() again? It would free the namespace a bit and we could call something similarly named from vcpu_enter_guest() to do the job.)
That seems good. I can take the labor to make the followup patch ;)

Ok, so I'll not apply your first patch.

Thanks!

Paolo

Wanpeng Li

2014-09-17 00:13 UTC #4 lore raw

In reply to Radim Krčmář

Hi Radim,

Quoted 29 lines · Radim Krčmář

2014-09-12 17:06-0400, Liang Chen:
Quoted 5 lines
Using kvm_mmu_flush_tlb as the other places to make sure vcpu
stat is incremented
Signed-off-by Liang Chen <[email protected]>
---
Good catch.
Quoted 8 lines
 arch/x86/kvm/vmx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kvm/vmx.c +0 -0
index bfe11cf..439682e 100644
arch/x86/kvm/vmx.c static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu) · line 1816
Quoted 8 lines
arch/x86/kvm/vmx.c +1 -1
@@ -1810,7 +1810,7 @@ static void vmx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 		struct desc_ptr *gdt = &__get_cpu_var(host_gdt);
 		unsigned long sysenter_esp;
 
-		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
+		kvm_mmu_flush_tlb(vcpu);
 		local_irq_disable();
 		crash_disable_local_vmclear(cpu);
 
And to hijack this thread ...
I noticed three other deficits in stat.tlb_flush, patch below.

Quoted 6 lines

Do you prefer the current behavior?
--- 8< ---
KVM: x86: count actual tlb flushes
- we count KVM_REQ_TLB_FLUSH requests, not actual flushes

So there maybe multiple requests accumulated at the point of kvm_check_request, if your patch account these accumulations correctly?

Regards,
Wanpeng Li

Quoted 45 lines

(KVM can have multiple requests for one flush)
- flushes from kvm_flush_remote_tlbs aren't counted
- it's easy to make a direct request by mistake
Solve these by postponing the counting to kvm_check_request().
Signed-off-by Radim Krčmář <[email protected]>
---
(And what about a possible followup patch that replaces kvm_mmu_flush_tlb() with kvm_make_request() again? It would free the namespace a bit and we could call something similarly named from vcpu_enter_guest() to do the job.)
arch/x86/kvm/mmu.c +0 -1
index 9314678..b41fd97 100644
@@ -3452,7 +3452,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
 
 void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
 {
-	++vcpu->stat.tlb_flush;
 	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 }
 EXPORT_SYMBOL_GPL(kvm_mmu_flush_tlb);
arch/x86/kvm/x86.c +3 -2
index 916e895..0f0ad08 100644
@@ -6041,8 +6041,10 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
 		}
 		if (kvm_check_request(KVM_REQ_MMU_SYNC, vcpu))
 			kvm_mmu_sync_roots(vcpu);
-		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu))
+		if (kvm_check_request(KVM_REQ_TLB_FLUSH, vcpu)) {
+			++vcpu->stat.tlb_flush;
 			kvm_x86_ops->tlb_flush(vcpu);
+		}
 		if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
 			vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
 			r = 0;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Radim Krčmář

2014-09-17 10:45 UTC #5 lore raw

2014-09-17 08:15+0800, Wanpeng Li:

Quoted 11 lines

Hi Radim,
Quoted 6 lines · Radim Krčmář
Do you prefer the current behavior?
--- 8< ---
KVM: x86: count actual tlb flushes
- we count KVM_REQ_TLB_FLUSH requests, not actual flushes
So there maybe multiple requests accumulated at the point of kvm_check_request, if your patch account these accumulations correctly?

It will ignore request accumulations and count it as one TLB flush, we have to decide what is correct (the value is just statistics)

a) count local KVM_REQ_TLB_FLUSH requests
b) count all TLB flushes
c) both (a) and (b)

I was thinking that when you look at /sys/kernel/debug/kvm/tlb_flushes, you are interested in the number of TLB flushes that VMs did, not requests, so you won't have to add remote_tlb_flushes multiplied by maximal vcpu count and guess their amount from this upper bound.

And that we don't even care about requests, so (c) is just complication.

---
I tried to get an idea about the number of coalesced requests and added a counter called tlb_flush_real, making option (c).
After a night of reading virtio-rng (different experiment) on 1 VCPU VM:

 # cat /sys/kernel/debug/kvm/{tlb_flush{,_real},remote_tlb_flush}
 5927
 5206
 0

Paolo Bonzini

maintainer

2014-09-17 11:06 UTC #6 lore raw

Il 17/09/2014 12:45, Radim Krčmář ha scritto:

Quoted 8 lines

a) count local KVM_REQ_TLB_FLUSH requests
b) count all TLB flushes
c) both (a) and (b)
I was thinking that when you look at /sys/kernel/debug/kvm/tlb_flushes, you are interested in the number of TLB flushes that VMs did, not requests, so you won't have to add remote_tlb_flushes multiplied by maximal vcpu count and guess their amount from this upper bound.

I agree. The difference isn't too big, but if two requests are coalesced they will only cause a hit once.

Paolo

Quoted 1 line

And that we don't even care about requests, so (c) is just complication.