Messages in this thread | | | Date | Mon, 20 May 2013 17:12:39 +0800 | From | Xiao Guangrong <> | Subject | Re: [PATCH v6 3/7] KVM: MMU: fast invalidate all pages |
| |
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. ;)
| |