From mboxrd@z Thu Jan  1 00:00:00 1970
From: Gui Jianfeng <guijianfeng@cn.fujitsu.com>
Subject: Re: [PATCH 1/3] KVM MMU: make kvm_mmu_zap_page() return the number
 of zapped sp in total.
Date: Fri, 23 Apr 2010 13:03:57 +0800
Message-ID: <4BD12A3D.8050605@cn.fujitsu.com>
References: <4BD017CD.1090500@cn.fujitsu.com> <4BD118D7.4040102@cn.fujitsu.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Cc: Avi Kivity <avi@redhat.com>, kvm@vger.kernel.org
To: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from cn.fujitsu.com ([222.73.24.84]:60468 "EHLO song.cn.fujitsu.com"
	rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP
	id S1755438Ab0DWFFB (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 23 Apr 2010 01:05:01 -0400
In-Reply-To: <4BD118D7.4040102@cn.fujitsu.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Xiao Guangrong wrote:
> 
> Gui Jianfeng wrote:
>> Currently, in kvm_mmu_change_mmu_pages(kvm, page), "used_pages--" is  performed after calling
>> kvm_mmu_zap_page() in spite of that whether "page" is actually reclaimed. Because root sp won't be 
>> reclaimed by kvm_mmu_zap_page(). So making kvm_mmu_zap_page() return total number of reclaimed sp
>> makes more sense. A new flag is put into kvm_mmu_zap_page() to indicate whether the top page is reclaimed.
>>
> 
> This bug only hurts kvm_mmu_change_mmu_pages() function, we'd better allow 'self_deleted' is
> NULL, then we can pass NULL at other place.

Ok, will change. Will send a updated version.

> 
>> @@ -1571,7 +1584,8 @@ restart:
>>  			pgprintk("%s: gfn %lx role %x\n", __func__, gfn,
>>  				 sp->role.word);
>>  			r = 1;
>> -			if (kvm_mmu_zap_page(kvm, sp))
>> +			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
>> +			if (ret > 1 || (ret == 1 && self_deleted == 0))
>>  				goto restart;
> 
> Maybe we can keep kvm_mmu_zap_page() returns the number of zapped children,
> and 'self_deleted' indicates whether self is zapped, then we no need modify
> those function, just fix kvm_mmu_change_mmu_pages() that is if 'self_deleted == 1',
> inc 'used_pages'

I think kvm_mmu_zap_page() returning the total zapped number is more intuitive, so i'd
prefer to retain the original code. Thanks.

Gui,
Thanks

> 
> Xiao
> 
> 
>