Re: [PATCH v7 09/11] KVM: MMU: introduce kvm_mmu_prepare_zap_obsolete_page

[Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>]

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.
> 
>>  	list_del(&sp->link);
>>  	free_page((unsigned long)sp->spt);
>>  	if (!sp->role.direct)
>> @@ -2069,14 +2069,19 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
>>  	return zapped;
>>  }
>>  
>> -static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
>> -				    struct list_head *invalid_list)
>> +static int
>> +__kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
>> +			   bool zap_unsync_children,
>> +			   struct list_head *invalid_list)
>>  {
>> -	int ret;
>> +	int ret = 0;
>>  
>>  	trace_kvm_mmu_prepare_zap_page(sp);
>>  	++kvm->stat.mmu_shadow_zapped;
>> -	ret = mmu_zap_unsync_children(kvm, sp, invalid_list);
>> +
>> +	if (likely(zap_unsync_children))
>> +		ret = mmu_zap_unsync_children(kvm, sp, invalid_list);
>> +
>>  	kvm_mmu_page_unlink_children(kvm, sp);
>>  	kvm_mmu_unlink_parents(kvm, sp);
>>  
>> @@ -2099,6 +2104,37 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
>>  	return ret;
>>  }
>>  
>> +/*
>> + * 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.
>> + *
>> + * Note: if we use this function in for_each_gfn_xxx macros, we should
>> + * re-walk the list when it successfully zaps one page.
>> + */
>> +static int
>> +kvm_mmu_prepare_zap_obsolete_page(struct kvm *kvm, struct kvm_mmu_page *sp,
>> +				  struct list_head *invalid_list)
>> +{
>> +	int ret;
>> +
>> +	WARN_ON(!is_obsolete_sp(kvm, sp));
>> +
>> +	ret = __kvm_mmu_prepare_zap_page(kvm, sp, false, invalid_list);
>> +	if (ret)
>> +		hlist_del_init(&sp->hash_link);
> Why hlist_del() is not enough?

Since it will be deleted again in kvm_mmu_free_page().
I am not sure if has another better way to do this.