Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753739Ab3EWIJb (ORCPT ); Thu, 23 May 2013 04:09:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59693 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751274Ab3EWIJ2 (ORCPT ); Thu, 23 May 2013 04:09:28 -0400 Date: Thu, 23 May 2013 11:09:22 +0300 From: Gleb Natapov To: Xiao Guangrong Cc: avi.kivity@gmail.com, mtosatti@redhat.com, pbonzini@redhat.com, linux-kernel@vger.kernel.org, kvm@vger.kernel.org Subject: Re: [PATCH v7 09/11] KVM: MMU: introduce kvm_mmu_prepare_zap_obsolete_page Message-ID: <20130523080922.GG26157@redhat.com> References: <1369252560-11611-1-git-send-email-xiaoguangrong@linux.vnet.ibm.com> <1369252560-11611-10-git-send-email-xiaoguangrong@linux.vnet.ibm.com> <20130523055725.GA26157@redhat.com> <519DB372.3080803@linux.vnet.ibm.com> <20130523061818.GC26157@redhat.com> <519DB7D3.7030101@linux.vnet.ibm.com> <20130523073708.GE26157@redhat.com> <519DCA38.30200@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <519DCA38.30200@linux.vnet.ibm.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4853 Lines: 126 On Thu, May 23, 2013 at 03:50:16PM +0800, Xiao Guangrong wrote: > On 05/23/2013 03:37 PM, Gleb Natapov wrote: > > On Thu, May 23, 2013 at 02:31:47PM +0800, Xiao Guangrong wrote: > >> On 05/23/2013 02:18 PM, Gleb Natapov wrote: > >>> On Thu, May 23, 2013 at 02:13:06PM +0800, Xiao Guangrong wrote: > >>>> On 05/23/2013 01:57 PM, Gleb Natapov wrote: > >>>>> On Thu, May 23, 2013 at 03:55:58AM +0800, Xiao Guangrong wrote: > >>>>>> It is only used to zap the obsolete page. Since the obsolete page > >>>>>> will not be used, we need not spend time to find its unsync children > >>>>>> out. Also, we delete the page from shadow page cache so that the page > >>>>>> is completely isolated after call this function. > >>>>>> > >>>>>> The later patch will use it to collapse tlb flushes > >>>>>> > >>>>>> Signed-off-by: Xiao Guangrong > >>>>>> --- > >>>>>> arch/x86/kvm/mmu.c | 46 +++++++++++++++++++++++++++++++++++++++++----- > >>>>>> 1 files changed, 41 insertions(+), 5 deletions(-) > >>>>>> > >>>>>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > >>>>>> index 9b57faa..e676356 100644 > >>>>>> --- a/arch/x86/kvm/mmu.c > >>>>>> +++ b/arch/x86/kvm/mmu.c > >>>>>> @@ -1466,7 +1466,7 @@ static inline void kvm_mod_used_mmu_pages(struct kvm *kvm, int nr) > >>>>>> static void kvm_mmu_free_page(struct kvm_mmu_page *sp) > >>>>>> { > >>>>>> ASSERT(is_empty_shadow_page(sp->spt)); > >>>>>> - hlist_del(&sp->hash_link); > >>>>>> + hlist_del_init(&sp->hash_link); > >>>>> Why do you need hlist_del_init() here? Why not move it into > >>>> > >>>> Since the hlist will be double freed. We will it like this: > >>>> > >>>> kvm_mmu_prepare_zap_obsolete_page(page, list); > >>>> kvm_mmu_commit_zap_page(list); > >>>> kvm_mmu_free_page(page); > >>>> > >>>> The first place is kvm_mmu_prepare_zap_obsolete_page(page), which have > >>>> deleted the hash list. > >>>> > >>>>> kvm_mmu_prepare_zap_page() like we discussed it here: > >>>>> https://patchwork.kernel.org/patch/2580351/ instead of doing > >>>>> it differently for obsolete and non obsolete pages? > >>>> > >>>> It is can break the hash-list walking: we should rescan the > >>>> hash list once the page is prepared-ly zapped. > >>>> > >>>> I mentioned it in the changelog: > >>>> > >>>> 4): drop the patch which deleted page from hash list at the "prepare" > >>>> time since it can break the walk based on hash list. > >>> Can you elaborate on how this can happen? > >> > >> There is a example: > >> > >> int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn) > >> { > >> struct kvm_mmu_page *sp; > >> LIST_HEAD(invalid_list); > >> int r; > >> > >> pgprintk("%s: looking for gfn %llx\n", __func__, gfn); > >> r = 0; > >> spin_lock(&kvm->mmu_lock); > >> for_each_gfn_indirect_valid_sp(kvm, sp, gfn) { > >> pgprintk("%s: gfn %llx role %x\n", __func__, gfn, > >> sp->role.word); > >> r = 1; > >> kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list); > >> } > >> kvm_mmu_commit_zap_page(kvm, &invalid_list); > >> spin_unlock(&kvm->mmu_lock); > >> > >> return r; > >> } > >> > >> It works fine since kvm_mmu_prepare_zap_page does not touch the hash list. > >> If we delete hlist in kvm_mmu_prepare_zap_page(), this kind of codes should > >> be changed to: > >> > >> restart: > >> for_each_gfn_indirect_valid_sp(kvm, sp, gfn) { > >> pgprintk("%s: gfn %llx role %x\n", __func__, gfn, > >> sp->role.word); > >> r = 1; > >> if (kvm_mmu_prepare_zap_page(kvm, sp, &invalid_list)) > >> goto restart; > >> } > >> kvm_mmu_commit_zap_page(kvm, &invalid_list); > >> > > Hmm, yes. So lets leave it as is and always commit invalid_list before > > So, you mean drop this patch and the patch of > KVM: MMU: collapse TLB flushes when zap all pages? > We still want to add kvm_reload_remote_mmus() to kvm_mmu_invalidate_zap_all_pages(). But yes, we disable a nice optimization here. So may be skipping obsolete pages while walking hashtable is better solution. > But, we only introduced less code in this patch, most of them is reusing > the code of __kvm_mmu_prepare_zap_page... > > Furthermore, maybe not related to this patch, i do not think calling > mmu_zap_unsync_children() in kvm_mmu_prepare_zap_page() is necessary, > but i need to test it very carefully. Why not let > kvm_mmu_prepare_zap_obsolete_page for the first step? :( Yes, I want Marcelo opinion on skipping mmu_zap_unsync_children() first. > > releasing lock in kvm_zap_obsolete_pages() or skip obsolete pages while > > walking hash table. Former is clearer I think. > > > > -- > > Gleb. > > > > > > -- Gleb. -- 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/