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. ;)
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
