Re: [PATCH 3/4 changelog-v2] KVM: Switch to srcu-less get_dirty_log()

[Marcelo Tosatti <mtosatti@redhat.com>]

On Tue, Mar 06, 2012 at 11:43:17PM +0900, Takuya Yoshikawa wrote:
> Marcelo Tosatti <mtosatti@redhat.com> wrote:
> 
> > > +	spin_lock(&kvm->mmu_lock);
> > 
> > It is not clear why mmu_lock is needed. Dropping it across the xchg loop
> > should be similar to srcu implementation, in that concurrent updates
> > will be visible only on the next get_dirty call? Well, it is necessary
> > anyway for write protecting the sptes.
> 
> My implementation does write protection inside the xchg loop.
> Then, after that loop, flushes TLB.
> 
> mmu_lock must protect both of these together.
> 
> If we do not mind scanning the bitmap twice, we can decouple the
> xchg loop and write protection, but it will be a bit slower, and in
> any case we need to hold mmu_lock until TLB is flushed.

Why is it necessary to scan twice? Simply continuing to the next set 
of pages, after dropping the lock, should be enough.

The potential problem i am referring to is:

- kvm.git next + srcu-less series
average(ns)    stdev     ns/page    pages    improvement(%)

8497356.4    16441.0        32.4     256K     -29

So 8ms for 1GB. Assuming it increases linearly, it would take 
400ms for get_dirty on a 50GB slot (most of that time spent 
with mmu_lock held). Is this correct?

> As can be seen from the unit-test result the majority of time
> is being spent on write protecting sptes, so decoupling xchg loop
> alone will not alleviate the problem so much -- my guess.
> 
> > A cond_resched_lock() would alleviate the potentially long held 
> > times for mmu_lock (can you measure it with large memslots?)
> 
> How to move TLB flush out of mmu_lock critical sections was discussed
> before, and there seemed to be some proposals.
> 
> Anyone is working on that?
> 
> After that we can do many things.
> 
> One idea is to make the extra bitmap buffer size shrink to one page
> or so and do xchg and write protection loop by that limited size.
> 
> Because we can drop mmu_lock, it is possible to copy_to_user part of
> the dirty bitmap, and then go to the next part.
> 
> After everything is protected, we can then do TLB flush after dropping
> mmu_lock.
> 
> > Otherwise looks nice.
> 
> Thanks,
> 	Takuya
> 
> 
> > > -		r = -ENOMEM;
> > > -		slots = kmemdup(kvm->memslots, sizeof(*kvm->memslots), GFP_KERNEL);
> > > -		if (!slots)
> > > -			goto out;
> > > +	for (i = 0; i < n / sizeof(long); i++) {
> > > +		unsigned long mask;
> > > +		gfn_t offset;
> > >  
> > > -		memslot = id_to_memslot(slots, log->slot);
> > > -		memslot->nr_dirty_pages = 0;
> > > -		memslot->dirty_bitmap = dirty_bitmap_head;
> > > -		update_memslots(slots, NULL);
> > > +		if (!dirty_bitmap[i])
> > > +			continue;
> > >  
> > > -		old_slots = kvm->memslots;
> > > -		rcu_assign_pointer(kvm->memslots, slots);
> > > -		synchronize_srcu_expedited(&kvm->srcu);
> > > -		kfree(old_slots);
> > > +		is_dirty = true;
> > >  
> > > -		write_protect_slot(kvm, memslot, dirty_bitmap, nr_dirty_pages);
> > > +		mask = xchg(&dirty_bitmap[i], 0);
> > > +		dirty_bitmap_buffer[i] = mask;
> > >  
> > > -		r = -EFAULT;
> > > -		if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n))
> > > -			goto out;
> > > -	} else {
> > > -		r = -EFAULT;
> > > -		if (clear_user(log->dirty_bitmap, n))
> > > -			goto out;
> > > +		offset = i * BITS_PER_LONG;
> > > +		kvm_mmu_write_protect_pt_masked(kvm, memslot, offset, mask);
> > >  	}
> > > +	if (is_dirty)
> > > +		kvm_flush_remote_tlbs(kvm);
> > > +
> > > +	spin_unlock(&kvm->mmu_lock);