Re: [PATCH v6 3/7] KVM: MMU: fast invalidate all pages

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

On 05/19/2013 06:04 PM, Gleb Natapov wrote:

>> +		/*
>> +		 * Do not repeatedly zap a root page to avoid unnecessary
>> +		 * KVM_REQ_MMU_RELOAD, otherwise we may not be able to
>> +		 * progress:
>> +		 *    vcpu 0                        vcpu 1
>> +		 *                         call vcpu_enter_guest():
>> +		 *                            1): handle KVM_REQ_MMU_RELOAD
>> +		 *                                and require mmu-lock to
>> +		 *                                load mmu
>> +		 * repeat:
>> +		 *    1): zap root page and
>> +		 *        send KVM_REQ_MMU_RELOAD
>> +		 *
>> +		 *    2): if (cond_resched_lock(mmu-lock))
>> +		 *
>> +		 *                            2): hold mmu-lock and load mmu
>> +		 *
>> +		 *                            3): see KVM_REQ_MMU_RELOAD bit
>> +		 *                                on vcpu->requests is set
>> +		 *                                then return 1 to call
>> +		 *                                vcpu_enter_guest() again.
>> +		 *            goto repeat;
>> +		 *
>> +		 */
> I am not sure why the above scenario will prevent us from progressing.
> There is finite number of root pages with invalid generation number, so
> eventually we will zap them all and vcpu1 will stop seeing KVM_REQ_MMU_RELOAD
> request.

This patch does not "zap pages in batch", so kvm_zap_obsolete_pages() can
just zap invalid root pages and lock-break due to the lock contention on the
path of handing KVM_REQ_MMU_RELOAD.

Yes, after "zap pages in batch", this issue does not exist any more. I should
update this into that patch.

> 
> This check here prevent unnecessary KVM_REQ_MMU_RELOAD as you say, but
> this races the question, why don't we check for sp->role.invalid in
> kvm_mmu_prepare_zap_page before calling kvm_reload_remote_mmus()?
> Something like this:
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 40d7b2d..d2ae3a4 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2081,7 +2081,8 @@ static int kvm_mmu_prepare_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
>  		kvm_mod_used_mmu_pages(kvm, -1);
>  	} else {
>  		list_move(&sp->link, &kvm->arch.active_mmu_pages);
> -		kvm_reload_remote_mmus(kvm);
> +		if (!sp->role.invalid)
> +			kvm_reload_remote_mmus(kvm);
>  	}
> 
>  	sp->role.invalid = 1;

Yes, it is better.

> 
> Actually we can add check for is_obsolete_sp() there too since
> kvm_mmu_invalidate_all_pages() already calls kvm_reload_remote_mmus()
> after incrementing mmu_valid_gen.

Yes, I agree.

> 
> Or do I miss something?

No, you are right. ;)