Re: [PATCH] KVM: Defer remote tlb flushes on invlpg (v4)

[Marcelo Tosatti <mtosatti@redhat.com>]

On Sat, Apr 18, 2009 at 05:34:27PM +0200, Andrea Arcangeli wrote:
> On Sun, Apr 12, 2009 at 07:31:58PM -0300, Marcelo Tosatti wrote:
> > mmu_sync_children needs to find any _reachable_ sptes that are unsync,
> > read the guest pagetable, and sync the sptes. Before it can inspect the
> > guest pagetable, it needs to write protect it, with rmap_write_protect.
> 
> So far so good.
> 
> > In theory it wouldnt be necesarry to call
> > kvm_flush_remote_tlbs_cond(protected) here, but only
> > kvm_flush_remote_tlbs(), since the "kvm->remote_tlbs_dirty" information
> > is not pertinent to mmu_sync_children.
> 
> Hmm I'm not sure I fully understand how it is not pertinent.
> 
> When we have a cr3 write in a remote vcpu, mmu_sync_children runs and
> it resyncs all sptes reachaeble for that remote vcpu context. But to
> resync the sptes, it also has to get rid of any old writable tlb entry
> for its remote vcpu where cr3 write is running. Checking only sptes to
> find writable ones, updating the sptes that are mapped by the writable
> sptes, and marking them wrprotected, isn't enough, as old spte
> contents may still be cached in the tlb if remote_tlbs_dirty is true!
>
> Think if the invlpg'd spte that got nuked by rmap_remove in the invlpg
> handler running in the current vcpu has a writable tlb entry active in
> the vcpu that later does cr3 overwrite. mmu_sync_children running in
> the remote vcpu will find no writable spte in the rmap chain
> representing that spte (because that spte that is still cached in the
> remote tlb, has already been zapped by the current vcpu) but it is
> still cached and writable in the remote vcpu TLB cache, when cr3
> overwrite runs.

Right, so you have to cope with the fact that invlpg can skip a TLB
flush. OK.

> > But this is done here to "clear" remote_tlbs_dirty (after a
> > kvm_flush_remote_tlbs remote_tlbs_dirty is clean), ie: as an
> > optimization.
> 
> The whole point of remote_tlbs_dirty is to defer any smp tlb flush at
> the least possible time, so how can it be an optimization to run a
> kvm_flush_remote_tlbs earlier than necessary? The only way this can be
> an optimization, is to never run kvm_flush_remote_tlbs unless
> absolutely necessary, and to leave remote_tlbs_dirty true instead of
> calling kvm_flush_remote_tlbs. Calling kvm_flush_remote_tlbs_cond
> instead of kvm_flush_remote_tlbs cannot ever be an optimization.

Right.

> > > >>  @@ -465,7 +464,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, 
> > > >> gva_t gva)
> > > >>  				rmap_remove(vcpu->kvm, sptep);
> > > >>  				if (is_large_pte(*sptep))
> > > >>  					--vcpu->kvm->stat.lpages;
> > > >> -				need_flush = 1;
> > > >> +				vcpu->kvm->remote_tlbs_dirty = true;
> > > >>  			}
> > > >>  			set_shadow_pte(sptep, shadow_trap_nonpresent_pte);
> > > >>  			break;
> > > >> @@ -475,8 +474,6 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t 
> > > >> gva)
> > > >>  			break;
> > > >>  	}
> > > >>  -	if (need_flush)
> > > >> -		kvm_flush_remote_tlbs(vcpu->kvm);
> > > >>  	spin_unlock(&vcpu->kvm->mmu_lock);
> > > 
> > > AFIK to be compliant with lowlevel archs (without ASN it doesn't
> > > matter I think as vmx always flush on exit), we have to flush the
> > > local tlb here, with set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests). I
> > > don't see why it's missing. Or am I wrong?
> > 
> > Caller does it:
> > 
> > void kvm_mmu_invlpg(struct kvm_vcpu *vcpu, gva_t gva)
> > {
> >         vcpu->arch.mmu.invlpg(vcpu, gva);
> >         kvm_mmu_flush_tlb(vcpu);
> >         ++vcpu->stat.invlpg;
> > }
> 
> Ah great! Avi also mentioned it I recall but I didn't figure out it
> was after FNAME(invlpg) returns. But isn't always more efficient to
> set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests) instead like I'm doing?

Sure that works too.

> See my version of kvm_flush_local_tlb, that's a bit different from
> kvm_mmu_flush_tlb and I'm made the old no-mmu notifier kernel safe too
> which I think is worth it. If you like to keep my version of
> kvm_flush_local_tlb, then I've simply to remove the kvm_mmu_flush_tlb
> from kvm_mmu_invlpg and move it inside the arch.mmu.invlpg handler so
> each shadow implementation does it its way. Comments?

I'm fine with your kvm_flush_local_tlb. Just one minor nit:

+   /* get new asid before returning to guest mode */
+   if (!test_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests))
+               set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests);

Whats the test_bit for?

> If you disagree, and you want to run kvm_mmu_flush_tlb in
> kvm_mmu_invlpg instead of set_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests),
> then I can expand the kvm_flush_local_tlb with the smp_wmb() in the
> FNAME(invlpg) code. Like:
> 
> 	      if (need_flush) {
> 	         smp_wmb();
> 		 remote_tlbs_dirty = true;
> 	      }
> 	      spin_unlock(mmu_lock);
> 
> Then the local tlb flush will run when we return from FNAME(invlpg)
> and remote_tlbs_dirty is set _after_ set_shadow_pte and _before_
> releasing mmu_lock, making it still safe against
> mmu_notifier_invalidate_page/range.
> 
> > What about protecting remote_tlbs_dirty with mmu_lock? Only caller of
> > kvm_flush_remote_tlbs that lacks mmu_notifier is kvm_mmu_zap_all, which
> > is not performance sensitive anyway.
> 
> I thought it too I've to say. Tried a bit too, then I figured out why
> Avi wanted to do out of order. The reason is that we want to allow
> kvm_flush_remote_tlbs to run outside the mmu_lock too. So this
> actually results in more self-containment of the remote_tlb_dirty
> logic and simpler code. But I documented at least what the smb_wmb is
> serializing and how it works.

OK.

> > > -		if (protected)
> > > +		/*
> > > +		 * Avoid leaving stale tlb entries in this vcpu representing
> > > +		 * sptes rmap_removed by invlpg without vcpu smp tlb flush.
> > > +		 */
> > > +		if (protected || vcpu->kvm->remote_tlbs_dirty)
> > >  			kvm_flush_remote_tlbs(vcpu->kvm);
> > 
> > +void kvm_flush_remote_tlbs_cond(struct kvm *kvm, bool cond)
> > +{
> > +   if (cond || kvm->remote_tlbs_dirty)
> > +       kvm_flush_remote_tlbs(kvm);
> > +}
> > 
> > Aren't they the same?
> 
> I didn't particularly like the kvm_flush_remote_tlbs_cond, and because
> I ended up editing the patch by hand in my code to fix it as I
> understood it, I ended up refactoring some bits like this. But if you
> like I can add it back, I don't really care.

It was nice to hide explicit knowledge about
vcpu->kvm->remote_tlbs_dirty behind the interface instead of exposing
it.

> 
> > > diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> > > index 09782a9..060b4a3 100644
> > > --- a/arch/x86/kvm/paging_tmpl.h
> > > +++ b/arch/x86/kvm/paging_tmpl.h
> > > @@ -483,7 +483,7 @@ static void FNAME(invlpg)(struct kvm_vcpu *vcpu, gva_t gva)
> > >  	}
> > >  
> > >  	if (need_flush)
> > > -		kvm_flush_remote_tlbs(vcpu->kvm);
> > > +		kvm_flush_local_tlb(vcpu);
> > >  	spin_unlock(&vcpu->kvm->mmu_lock);
> > 
> > No need, caller does it for us.
> 
> Right but caller does it different, as commented above. Clearly one of the two
> has to go ;).
> 
> > OOO ? Out Of Options?
> 
> Eheeh, I meant out of order.
> 
> > If remote_tlbs_dirty is protected by mmu_lock, most of this complexity 
> > is unecessary?
> 
> Answered above.
> 
> Thanks a lot for review!! My current thought is that we should just
> move the unnecessary ->tlb_flush from the common invlpg handler to the
> other lowlevel .invlpg handler and then I hope this optimization will
> be measurable ;).

Depends on how often the guest uses invlpg right. I believe its a
worthwhile optimization.

TIA