| Message ID | 1410978955-19003-1-git-send-email-liangchen.linux@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/18/2014 02:35 AM, Liang Chen wrote: > - 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. > 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. ] -- 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
Il 18/09/2014 07:05, Xiao Guangrong ha scritto: > On 09/18/2014 02:35 AM, Liang Chen wrote: >> - 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. > >> 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: > [ 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 -- 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 09/18/2014 08:47 AM, Paolo Bonzini wrote: > Il 18/09/2014 07:05, Xiao Guangrong ha scritto: >> On 09/18/2014 02:35 AM, Liang Chen wrote: >>> - 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. >> >>> 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: > >> [ 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 -- 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
2014-09-17 14:35-0400, 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(), > and refactor the code to use kvm_make_request again. > > Signed-off-by: Liang Chen <liangchen.linux@gmail.com> > --- > 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.) 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 linux-kernel@vger.kernel.org. (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?) -- 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
2014-09-18 14:47+0200, Paolo Bonzini: > Il 18/09/2014 07:05, Xiao Guangrong ha scritto: > > On 09/18/2014 02:35 AM, Liang Chen wrote: > >> - 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. > > > >> 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. 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) > However, I agree with this > suggestion: > > > [ 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. -- 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 09/18/2014 10:00 AM, Radim Kr?má? wrote: > 2014-09-17 14:35-0400, 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(), >> and refactor the code to use kvm_make_request again. >> >> Signed-off-by: Liang Chen <liangchen.linux@gmail.com> >> --- >> 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.) > 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 linux-kernel@vger.kernel.org. > (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?) 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 -- 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/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 7c492ed..77ade89 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -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); diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 9314678..acc2d0c5 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -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); diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c index bfe11cf..bb0a7ab 100644 --- a/arch/x86/kvm/vmx.c +++ b/arch/x86/kvm/vmx.c @@ -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: diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 8f1e22d..f9fca52f 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -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;
- 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 <liangchen.linux@gmail.com> --- 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(-)