Date: Thu, 23 May 2013 09:18:18 +0300
From: Gleb Natapov <gleb@redhat.com>
To: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
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: <20130523061818.GC26157@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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <519DB372.3080803@linux.vnet.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2290
Lines: 56

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 <xiaoguangrong@linux.vnet.ibm.com>
> >> ---
> >>  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?

--
			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/