Re: [RFC PATCH] KVM: Fix __set_bit() race in mark_page_dirty() during dirty logging

[Takuya Yoshikawa <takuya.yoshikawa@gmail.com>]

On Thu, 5 Jan 2012 09:48:37 -0200
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> On Wed, Jan 04, 2012 at 03:06:43PM +0900, Takuya Yoshikawa wrote:
> > It is possible that the __set_bit() in mark_page_dirty() is called
> > simultaneously on the same region of memory, which may result in only
> > one bit being set, because some callers do not take mmu_lock before
> > mark_page_dirty().
> > 
> > This problem is hard to produce because when we reach mark_page_dirty()
> > beginning from, e.g., tdp_page_fault(), mmu_lock is being held during
> > __direct_map():  making kvm-unit-tests' dirty log api test write to two
> > pages concurrently was not useful for this reason.
> > 
> > So we have confirmed that there can actually be race condition by
> > checking if some callers really reach there without holding mmu_lock
> > using spin_is_locked():  probably they were from kvm_write_guest_page().
> > 
> > To fix this race, this patch changes the bit operation to the atomic
> > version:  note that nr_dirty_pages also suffers from the race but we do
> > not need exactly correct numbers for now.
> > 
> > Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
> > ---
> >  virt/kvm/kvm_main.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 7287bf5..a91f980 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1543,7 +1543,7 @@ void mark_page_dirty_in_slot(struct kvm *kvm, struct kvm_memory_slot *memslot,
> >  	if (memslot && memslot->dirty_bitmap) {
> >  		unsigned long rel_gfn = gfn - memslot->base_gfn;
> >  
> > -		if (!__test_and_set_bit_le(rel_gfn, memslot->dirty_bitmap))
> > +		if (!test_and_set_bit_le(rel_gfn, memslot->dirty_bitmap))
> >  			memslot->nr_dirty_pages++;
> >  	}
> >  }
> 
> Looks good to me.
> 

I was planning to avoid bitmap flip based on srcu by clearing each bit
right before rmap write protection.

for each gfn_offset
	__clear_bit();
	nr_dirty_pages--;
	rmap_write_protect();

This way, I could eliminate memslots allocations and srcu synchronizations
and get dirty log became faster.

Furthermore get dirty log could be called for selected pages without
affecting others.

Now I need to re-think how to achieve the same thing, and measure the effect
again.

	Takuya