Re: [PATCH 1/4] KVM: Add APIs for unlocked TLB flush

[Avi Kivity <avi@redhat.com>]

On 05/08/2012 05:25 AM, Marcelo Tosatti wrote:
> > >
> > > We need call kvm_cond_flush_remote_tlbs in rmap_write-protect
> > > unconditionally?
> > 
> > Yes, that's the point.  We change
> > 
> >    spin_lock(mmu_lock)
> >    conditionally flush the tlb
> >    spin_unlock(mmu_lock)
> > 
> > to
> > 
> >   spin_lock(mmu_lock)
> >   conditionally mark the tlb as dirty
> >   spin_unlock(mmu_lock)
> >   kvm_cond_flush_remote_tlbs()
> > 
> > but that means the entire codebase has to be converted.
>
> Is there any other site which expects sptes and TLBs to be in sync,     
> other than rmap_write_protect?                                          

I wouldn't say rmap_write_protect() expects sptes and TLBs to be in
sync.  Rather its callers.

> Please convert the flush_remote_tlbs at the end of set_spte (RW->RO) to
> mark_dirty.

I'd like to take an incremental approach, since there are many paths.  I
don't have a concrete plan though.

> Looks good in general (patchset is incomplete though). One thing that
> is annoying is that there is no guarantee of progress for flushed_count
> increment (it can, in theory, always race with a mark_dirty). But since
> that would be only a performance, and not correctness, aspect, it is
> fine.

We don't have a while () look in cond_flush(), so it won't be slowed
down by the race; whoever caused the race will have to flush on their
own, but they do that anyway now.

We can regress if vcpu 0 marks the tlb as dirty, and then vcpu 0 and 1
simultaneously flush, even though vcpu 1 did nothing to deserve it.  I
don't see a way around it except to hope its a rare event.

> It has the advantage that it requires code to explicitly document where
> the TLB must be flushed and the sites which expect sptes to be in sync
> with TLBs.

I'm looking for an idea of how to make the flush in those cases not hold
mmu_lock.

-- 
error compiling committee.c: too many arguments to function