Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753942AbbEKNER (ORCPT ); Mon, 11 May 2015 09:04:17 -0400 Received: from mga01.intel.com ([192.55.52.88]:53370 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751816AbbEKNEP (ORCPT ); Mon, 11 May 2015 09:04:15 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,407,1427785200"; d="scan'208";a="723983533" Message-ID: <5550A7DA.90800@linux.intel.com> Date: Mon, 11 May 2015 21:00:10 +0800 From: Xiao Guangrong User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Paolo Bonzini CC: gleb@kernel.org, mtosatti@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/9] KVM: MMU: introduce slot_handle_level() and its helper References: <1430389490-24602-1-git-send-email-guangrong.xiao@linux.intel.com> <1430389490-24602-3-git-send-email-guangrong.xiao@linux.intel.com> <554B54D2.5020403@redhat.com> In-Reply-To: <554B54D2.5020403@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3439 Lines: 114 On 05/07/2015 08:04 PM, Paolo Bonzini wrote: > > > On 30/04/2015 12:24, guangrong.xiao@linux.intel.com wrote: >> From: Xiao Guangrong >> >> There are several places walking all rmaps for the memslot so that >> introduce common functions to cleanup the code >> >> Signed-off-by: Xiao Guangrong >> --- >> arch/x86/kvm/mmu.c | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 63 insertions(+) >> >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c >> index ea3e3e4..75a3459 100644 >> --- a/arch/x86/kvm/mmu.c >> +++ b/arch/x86/kvm/mmu.c >> @@ -4410,6 +4410,69 @@ void kvm_mmu_setup(struct kvm_vcpu *vcpu) >> init_kvm_mmu(vcpu); >> } >> >> +/* The return value indicates if tlb flush on all vcpus is needed. */ >> +typedef bool (*slot_level_handler) (struct kvm *kvm, unsigned long *rmap); >> + >> +/* The caller should hold mmu-lock before calling this function. */ >> +static bool >> +slot_handle_level(struct kvm *kvm, struct kvm_memory_slot *memslot, >> + slot_level_handler fn, int min_level, int max_level, >> + bool lock_flush_tlb) > > Why not introduce for_each_slot_rmap first, instead of introducing one > implementation first and then switching to another? It's a small > change to reorder the patches like that. Yes, it's better, will do it in v2. > I think we should have three > iterator macros: > > #define for_each_rmap_spte(rmap, iter, spte) > > #define for_each_slot_rmap(slot, min_level, max_level, iter, rmapp) > > #define for_each_slot_rmap_range(slot, iter, min_level, max_level, \ > start_gfn, end_gfn, iter, rmapp) > > where the last two take care of initializing the walker/iterator in the > first part of the "for". Okay, i agree. > > This way, this function would be introduced immediately as this very > readable code: > > struct slot_rmap_iterator iter; > unsigned long *rmapp; > bool flush = false; > > for_each_slot_rmap(memslot, min_level, max_level, &iter, rmapp) { > if (*rmapp) > flush |= fn(kvm, rmapp); > > if (need_resched() || spin_needbreak(&kvm->mmu_lock)) { > if (flush && lock_flush_tlb) { > kvm_flush_remote_tlbs(kvm); > flush = false; > } > cond_resched_lock(&kvm->mmu_lock); > } > } > > /* > * What about adding this here: then callers that pass > * lock_flush_tlb == true need not care about the return > * value! > */ > if (flush && lock_flush_tlb) { > kvm_flush_remote_tlbs(kvm); > flush = false; > } > > return flush; Good idea. > > In addition, some of these functions need to be marked always_inline I > think; either slot_handle_level/slot_handle_*_level, or the > iterators/walkers. Can you collect kvm.ko size for both cases? After applying patch 1 ~ 5: no inline: $ size arch/x86/kvm/kvm.ko text data bss dec hex filename 366406 51535 473 418414 6626e arch/x86/kvm/kvm.ko inline: $ size arch/x86/kvm/kvm.ko text data bss dec hex filename 366638 51535 473 418646 66356 arch/x86/kvm/kvm.ko Since there are static functions i prefer allowing GCC automatically optimizes the code to marking always-inline. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/