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 <xiaoguangr...@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
