Skip to contentSkip to conversation

KVM: x86: count actual tlb flushes

PATCH Superseded Liang Chen 4 participants 7 messages Sep 17, 2014 – Sep 18, 2014

Subsystems (3)
x86 KVM KVM x86

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Liang Chen

author
#0 lore raw

PATCH v2 1/1 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(), and refactor the code to use kvm_make_request again.

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

---
Changes since v2:

* Instead of calling kvm_mmu_flush_tlb everywhere to make sure the stat is always incremented, postponing the counting to kvm_check_request.

(The idea comes from Radim. Much of the work is indeed done by him and is included in this patch, otherwise I couldn't start working on the followup work as I promised early. As I'm new to kvm development, please let me know if I am doing wrong here.)

 arch/x86/include/asm/kvm_host.h |  1 -
 arch/x86/kvm/mmu.c              | 17 +++++------------
 arch/x86/kvm/vmx.c              |  2 +-
 arch/x86/kvm/x86.c              | 10 ++++++++--
 4 files changed, 14 insertions(+), 16 deletions(-)
arch/x86/include/asm/kvm_host.h +0 -1
index 7c492ed..77ade89 100644
@@ -917,7 +917,6 @@ void kvm_inject_nmi(struct kvm_vcpu *vcpu);
 
 int fx_init(struct kvm_vcpu *vcpu);
 
-void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu);
 void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 		       const u8 *new, int bytes);
 int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn);
arch/x86/kvm/mmu.c +5 -12
index 9314678..acc2d0c5 100644
@@ -1749,7 +1749,7 @@ static int __kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp,
 		return 1;
 	}
 
-	kvm_mmu_flush_tlb(vcpu);
+	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 	return 0;
 }
 

@@ -1802,7 +1802,7 @@ static void kvm_sync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
 
 	kvm_mmu_commit_zap_page(vcpu->kvm, &invalid_list);
 	if (flush)
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 }
 
 struct mmu_page_path {

@@ -2536,7 +2536,7 @@ static void mmu_set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 	      true, host_writable)) {
 		if (write_fault)
 			*emulate = 1;
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 	}
 
 	if (unlikely(is_mmio_spte(*sptep) && emulate))

@@ -3450,13 +3450,6 @@ static void nonpaging_init_context(struct kvm_vcpu *vcpu,
 	context->nx = false;
 }
 
-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);
-
 void kvm_mmu_new_cr3(struct kvm_vcpu *vcpu)
 {
 	mmu_free_roots(vcpu);

@@ -3962,7 +3955,7 @@ static void mmu_pte_write_flush_tlb(struct kvm_vcpu *vcpu, bool zap_page,
 	if (remote_flush)
 		kvm_flush_remote_tlbs(vcpu->kvm);
 	else if (local_flush)
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 }
 
 static u64 mmu_pte_write_fetch_gpte(struct kvm_vcpu *vcpu, gpa_t *gpa,

@@ -4223,7 +4216,7 @@ EXPORT_SYMBOL_GPL(kvm_mmu_page_fault);
 void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva)
 {
 	vcpu->arch.mmu.invlpg(vcpu, gva);
-	kvm_mmu_flush_tlb(vcpu);
+	kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 	++vcpu->stat.invlpg;
 }
 EXPORT_SYMBOL_GPL(kvm_mmu_invlpg);
arch/x86/kvm/vmx.c +1 -1
index bfe11cf..bb0a7ab 100644
@@ -6617,7 +6617,7 @@ static int handle_invept(struct kvm_vcpu *vcpu)
 	switch (type) {
 	case VMX_EPT_EXTENT_GLOBAL:
 		kvm_mmu_sync_roots(vcpu);
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 		nested_vmx_succeed(vcpu);
 		break;
 	default:
arch/x86/kvm/x86.c +8 -2
index 8f1e22d..f9fca52f 100644
@@ -726,7 +726,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3)
 {
 	if (cr3 == kvm_read_cr3(vcpu) && !pdptrs_changed(vcpu)) {
 		kvm_mmu_sync_roots(vcpu);
-		kvm_mmu_flush_tlb(vcpu);
+		kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
 		return 0;
 	}
 

@@ -5989,6 +5989,12 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu)
 	kvm_apic_update_tmr(vcpu, tmr);
 }
 
+static void kvm_mmu_flush_tlb(struct kvm_vcpu *vcpu)
+{
+	++vcpu->stat.tlb_flush;
+	kvm_x86_ops->tlb_flush(vcpu);
+}
+
 /*
  * Returns 1 to let __vcpu_run() continue the guest execution loop without
  * exiting to the userspace.  Otherwise, the value will be returned to the

@@ -6018,7 +6024,7 @@ 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))
-			kvm_x86_ops->tlb_flush(vcpu);
+			kvm_mmu_flush_tlb(vcpu);
 		if (kvm_check_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu)) {
 			vcpu->run->exit_reason = KVM_EXIT_TPR_ACCESS;
 			r = 0;
Signature · 1 line
1.9.1
#1 lore raw
Quoted 6 lines · Liang Chen

- 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(),

It's good.

Quoted 1 line

and refactor the code to use kvm_make_request again.

Why this refactor is needed? It's really a bad idea using raw-bit-set instead of meaningful name.

[ Btw, maybe kvm_mmu_flush_local_tlb is a better name than kvm_mmu_flush_tlb() in the current code. ]

maintainer
#2 lore raw

Il 18/09/2014 07:05, Xiao Guangrong ha scritto:

Quoted 6 lines · Liang Chen
Quoted 5 lines

- 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
Quoted 1 line

Solve these by postponing the counting to kvm_check_request(),

It's good.

Quoted 1 line

and refactor the code to use kvm_make_request again.

Why this refactor is needed? It's really a bad idea using raw-bit-set instead of meaningful name.

set_bit is worse than kvm_make_request, but adding a one-line wrapper around kvm_make_request is not particularly useful.

We have the following requests:

- used multiple times:

        kvm_make_request(KVM_REQ_APF_HALT, vcpu);
        kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_EVENT, vcpu);
        kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
        kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
        kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);

- used once:

        kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
        kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
        kvm_make_request(KVM_REQ_NMI, vcpu);
        kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
        kvm_make_request(KVM_REQ_PMI, pmc->vcpu);
        kvm_make_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu);

So I'm pretty much ambivalent about that. However, I agree with this suggestion:

Quoted 2 lines

[ Btw, maybe kvm_mmu_flush_local_tlb is a better name than kvm_mmu_flush_tlb() in the current code. ]

Or even better, call it kvm_vcpu_flush_tlb since it is all within x86.c/vmx.c/svm.c and the MMU is not involved at all.

Paolo

author
#3 lore raw
Quoted 40 lines · Paolo Bonzini

Il 18/09/2014 07:05, Xiao Guangrong ha scritto:

Quoted 6 lines · Liang Chen
Quoted 5 lines

- 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 6 lines
Quoted 1 line

Solve these by postponing the counting to kvm_check_request(),

It's good.

Quoted 1 line

and refactor the code to use kvm_make_request again.

Why this refactor is needed? It's really a bad idea using raw-bit-set instead of meaningful name.

set_bit is worse than kvm_make_request, but adding a one-line wrapper around kvm_make_request is not particularly useful.

We have the following requests:

- used multiple times:

        kvm_make_request(KVM_REQ_APF_HALT, vcpu);
        kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_EVENT, vcpu);
        kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
        kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
        kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);

- used once:

        kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
        kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
        kvm_make_request(KVM_REQ_NMI, vcpu);
        kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
        kvm_make_request(KVM_REQ_PMI, pmc->vcpu);
        kvm_make_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu);

So I'm pretty much ambivalent about that. However, I agree with this suggestion:

Quoted 6 lines
Quoted 2 lines

[ Btw, maybe kvm_mmu_flush_local_tlb is a better name than kvm_mmu_flush_tlb() in the current code. ]

Or even better, call it kvm_vcpu_flush_tlb since it is all within x86.c/vmx.c/svm.c and the MMU is not involved at all.

Paolo

Thanks for the quick feedback! I will incorporate the name change into the next iteration of the patch.

Thanks!
Liang Chen

#4 lore raw
In reply to Paolo Bonzini

2014-09-18 14:47+0200, Paolo Bonzini:

Quoted 40 lines

Il 18/09/2014 07:05, Xiao Guangrong ha scritto:

Quoted 6 lines · Liang Chen
Quoted 5 lines

- 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
Quoted 1 line

Solve these by postponing the counting to kvm_check_request(),

It's good.

Quoted 1 line

and refactor the code to use kvm_make_request again.

Why this refactor is needed? It's really a bad idea using raw-bit-set instead of meaningful name.

set_bit is worse than kvm_make_request, but adding a one-line wrapper around kvm_make_request is not particularly useful.

We have the following requests:

- used multiple times:

        kvm_make_request(KVM_REQ_APF_HALT, vcpu);
        kvm_make_request(KVM_REQ_CLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_EVENT, vcpu);
        kvm_make_request(KVM_REQ_GLOBAL_CLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
        kvm_make_request(KVM_REQ_STEAL_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_TLB_FLUSH, vcpu);
        kvm_make_request(KVM_REQ_TRIPLE_FAULT, vcpu);

- used once:

        kvm_make_request(KVM_REQ_DEACTIVATE_FPU, vcpu);
        kvm_make_request(KVM_REQ_MASTERCLOCK_UPDATE, vcpu);
        kvm_make_request(KVM_REQ_MMU_SYNC, vcpu);
        kvm_make_request(KVM_REQ_NMI, vcpu);
        kvm_make_request(KVM_REQ_PENDING_TIMER, vcpu);
        kvm_make_request(KVM_REQ_PMI, pmc->vcpu);
        kvm_make_request(KVM_REQ_REPORT_TPR_ACCESS, vcpu);
Quoted 1 line

So I'm pretty much ambivalent about that.

I love abstractions, but consistency is above that, so I'd rather use kvm_make_requests unless we modified all of them ... which is a lot of work for the gain. (make_request is just clunky)

Quoted 7 lines

However, I agree with this > suggestion:

Quoted 2 lines

[ Btw, maybe kvm_mmu_flush_local_tlb is a better name than kvm_mmu_flush_tlb() in the current code. ]

Or even better, call it kvm_vcpu_flush_tlb since it is all within x86.c/vmx.c/svm.c and the MMU is not involved at all.

Good ideas.

#5 lore raw
In reply to Liang Chen

2014-09-17 14:35-0400, Liang Chen:

Quoted 12 lines

- 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(), and refactor the code to use kvm_make_request again.

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

---
Changes since v2:

Quoted 8 lines

* Instead of calling kvm_mmu_flush_tlb everywhere to make sure the stat is always incremented, postponing the counting to kvm_check_request.

(The idea comes from Radim. Much of the work is indeed done by him and is included in this patch, otherwise I couldn't start working on the followup work as I promised early. As I'm new to kvm development, please let me know if I am doing wrong here.)

I found (shame on me) Documentation/development-process/ when looking how to help and it looks really good.
(If you read it, the rest of my mail will be obsolete :)

You usually want to Cc [email protected].
(I've heard that someone actually reads it directly and it is a good
archive otherwise. It allows people to git blame your code and find the discussion in their preferred mail reader.)

The hard part about posting a patch is splitting it ...
You want to separate logical changes to make the code maintainable:
For this patch, I would create at least two-part series (cover letter!)

- change the meaning of tlb_flush
- refactor code

And see if it would make sense to split the refactoring further or if it breaks when only a first part of the whole series is applied.

It's not a problem if your code depends on unmerged patches, you can include someone else's code in the series as long as it isn't modified. (Which probably is better than just mentioning that your code depends on
some other patches from the list, but I'm not applying it ... Paolo?)

author
#6 lore raw
Quoted 37 lines · Radim Krčmář

2014-09-17 14:35-0400, Liang Chen:

Quoted 12 lines

- 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(), and refactor the code to use kvm_make_request again.

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

---
Changes since v2:

Quoted 8 lines

* Instead of calling kvm_mmu_flush_tlb everywhere to make sure the stat is always incremented, postponing the counting to kvm_check_request.

(The idea comes from Radim. Much of the work is indeed done by him and is included in this patch, otherwise I couldn't start working on the followup work as I promised early. As I'm new to kvm development, please let me know if I am doing wrong here.)

I found (shame on me) Documentation/development-process/ when looking how to help and it looks really good.
(If you read it, the rest of my mail will be obsolete :)

You usually want to Cc [email protected].
(I've heard that someone actually reads it directly and it is a good
archive otherwise. It allows people to git blame your code and find the discussion in their preferred mail reader.)

The hard part about posting a patch is splitting it ...
You want to separate logical changes to make the code maintainable:
For this patch, I would create at least two-part series (cover letter!)

- change the meaning of tlb_flush
- refactor code

Quoted 7 lines

And see if it would make sense to split the refactoring further or if it breaks when only a first part of the whole series is applied.

It's not a problem if your code depends on unmerged patches, you can include someone else's code in the series as long as it isn't modified. (Which probably is better than just mentioning that your code depends on
some other patches from the list, but I'm not applying it ... Paolo?)

Thank you very much for the help! Creating a patch series and including your patch intact as the first one sound to be the best ;)

Thanks,
Liang