From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751950AbbDNGGg (ORCPT ); Tue, 14 Apr 2015 02:06:36 -0400 Received: from mail-ig0-f172.google.com ([209.85.213.172]:35297 "EHLO mail-ig0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750992AbbDNGG0 (ORCPT ); Tue, 14 Apr 2015 02:06:26 -0400 MIME-Version: 1.0 In-Reply-To: <20150414052539.GA2559@kernel> References: <1428046825-6905-1-git-send-email-wanpeng.li@linux.intel.com> <20150414052539.GA2559@kernel> Date: Mon, 13 Apr 2015 23:06:25 -0700 Message-ID: Subject: Re: [PATCH v3] kvm: mmu: lazy collapse small sptes into large sptes From: Andres Lagar-Cavilla To: Wanpeng Li Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Paolo Bonzini , Xiao Guangrong , Eric Northup Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 13, 2015 at 10:25 PM, Wanpeng Li wrote: > Hi Andres, > On Fri, Apr 10, 2015 at 11:05:26AM -0700, Andres Lagar-Cavilla wrote: > [...] >>> + if (sp->role.direct && >>> + !kvm_is_reserved_pfn(pfn) && >>> + PageTransCompound(pfn_to_page(pfn))) { >> >>Not your fault, but PageTransCompound is very unhappy naming, as it >>also yields true for PageHuge. Suggestion: document this check covers >>static hugetlbfs, or switch to PageCompound() check. >> >>A slightly bolder approach would be to refactor and reuse the nearly >>identical check done in transparent_hugepage_adjust, instead of >>open-coding here. In essence this code is asking for the same check, >>plus the out-of-band check for static hugepages. > > PageCompound() check still return true for both transparent huge pages > and hugetlbfs pages, !PageHuge(page) && PageTransHuge(page) check can > guarantee to catch the right transparent huge pages just as my old commit > e76d30e20be5fc ("mm/hwpoison: fix test for a transparent huge page"). > I will send a patch to fix this. > Why would you want to "fix" it that way? Aren't static hugepages supported? (PageAnon is an inline check and much cheaper than !PageHuge(), which is an actual function call) Please consider my suggestion about refactoring the similar checks in transparent_hugepage_adjust. Thanks a ton Andres >> >> >>> + drop_spte(kvm, sptep); >>> + sptep = rmap_get_first(*rmapp, &iter); >>> + need_tlb_flush = 1; >>> + } else >>> + sptep = rmap_get_next(&iter); >>> + } >>> + >>> + return need_tlb_flush; >>> +} >>> + >>> +void kvm_mmu_zap_collapsible_sptes(struct kvm *kvm, >>> + struct kvm_memory_slot *memslot) >>> +{ >>> + bool flush = false; >>> + unsigned long *rmapp; >>> + unsigned long last_index, index; >>> + gfn_t gfn_start, gfn_end; >>> + >>> + spin_lock(&kvm->mmu_lock); >>> + >>> + gfn_start = memslot->base_gfn; >>> + gfn_end = memslot->base_gfn + memslot->npages - 1; >>> + >>> + if (gfn_start >= gfn_end) >>> + goto out; >> >>I don't understand the value of this check here. Are we looking for a >>broken memslot? Shouldn't this be a BUG_ON? Is this the place to care >>about these things? npages is capped to KVM_MEM_MAX_NR_PAGES, i.e. >>2^31. A 64 bit overflow would be caused by a gigantic gfn_start which >>would be trouble in many other ways. >> >>All this to say: please remove the above 5 lines and make code simpler. > > I will send a patch to cleanup it. Thanks for your review. :) > > Regards, > Wanpeng Li > -- Andres Lagar-Cavilla | Google Kernel Team | andreslc@google.com