From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paolo Bonzini Subject: Re: [PATCH v2] KVM: x86: count actual tlb flushes Date: Thu, 18 Sep 2014 14:47:55 +0200 Message-ID: <541AD47B.2010102@redhat.com> References: <1410978955-19003-1-git-send-email-liangchen.linux@gmail.com> <541A681B.1020503@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: kvm@vger.kernel.org, rkrcmar@redhat.com To: Xiao Guangrong , Liang Chen Return-path: Received: from mx1.redhat.com ([209.132.183.28]:46849 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753456AbaIRMsC (ORCPT ); Thu, 18 Sep 2014 08:48:02 -0400 In-Reply-To: <541A681B.1020503@linux.vnet.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: 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