Re: [PATCH v2] KVM: x86: enable dirty log gradually in small chunks

[Peter Xu <peterx@redhat.com>]

On Fri, Feb 21, 2020 at 09:31:52AM +0000, Zhoujian (jay) wrote:

[...]

> > > diff --git a/tools/testing/selftests/kvm/dirty_log_test.c
> > > b/tools/testing/selftests/kvm/dirty_log_test.c
> > > index 5614222..2a493c1 100644
> > > --- a/tools/testing/selftests/kvm/dirty_log_test.c
> > > +++ b/tools/testing/selftests/kvm/dirty_log_test.c
> > > @@ -317,10 +317,11 @@ static void run_test(enum vm_guest_mode mode,
> > unsigned long iterations,
> > >  	host_bmap_track = bitmap_alloc(host_num_pages);
> > >
> > >  #ifdef USE_CLEAR_DIRTY_LOG
> > > +	int ret = kvm_check_cap(KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2);
> > >  	struct kvm_enable_cap cap = {};
> > >
> > >  	cap.cap = KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2;
> > > -	cap.args[0] = 1;
> > > +	cap.args[0] = ret;
> > 
> > You enabled the initial-all-set but didn't really check it, so it didn't help much
> > from the testcase pov...  
> 
> vm_enable_cap is called afterwards, the return value is checked inside it,
> may I ask this check is enough, or it is needed to get the value through
> something like vm_get_cap ?

I meant to check what has offered by the initial-all-set feature bit,
which is, we should get the bitmap before dirtying and verify that
it's all ones.

> 
> > I'd suggest you drop this change, and you can work
> > on top after this patch can be accepted.
> 
> OK, some input parameters for cap.args[0] should be tested I think: 0, 1, 3
> should be accepted, the other numbers will not.

Yes. I think it'll be fine too if you want to put the test changes
into this patch. It's just not required so this patch could
potentially get merged easier, since the test may not be an oneliner
change.

> 
> > 
> > (Not to mention the original test actually verified that we don't  break, which
> > seems good..)
> > 
> > >  	vm_enable_cap(vm, &cap);
> > >  #endif
> > >
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index
> > > 70f03ce..f2631d0 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -862,7 +862,7 @@ static int kvm_vm_release(struct inode *inode,
> > struct file *filp)
> > >   * Allocation size is twice as large as the actual dirty bitmap size.
> > >   * See x86's kvm_vm_ioctl_get_dirty_log() why this is needed.
> > >   */
> > > -static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot)
> > > +static int kvm_alloc_dirty_bitmap(struct kvm_memory_slot *memslot)
> > 
> > This change seems irrelevant..
> > 
> > >  {
> > >  	unsigned long dirty_bytes = 2 * kvm_dirty_bitmap_bytes(memslot);
> > >
> > > @@ -1094,8 +1094,11 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > >
> > >  	/* Allocate page dirty bitmap if needed */
> > >  	if ((new.flags & KVM_MEM_LOG_DIRTY_PAGES) && !new.dirty_bitmap) {
> > > -		if (kvm_create_dirty_bitmap(&new) < 0)
> > > +		if (kvm_alloc_dirty_bitmap(&new))
> > 
> > Same here.
> 
> s/kvm_create_dirty_bitmap/kvm_alloc_dirty_bitmap is Sean's suggestion to make
> it clear that the helper is only responsible for allocation, then set all the bitmap bits
> to 1 using bitmap_set afterwards, which seems reasonable. Do you still think it's
> better to keep this name untouched?

No strong opinion, feel free to take your preference (because we've
got three here and if you like it too then it's already 2:1 :).

> 
> > 
> > >  			goto out_free;
> > > +
> > > +		if (kvm->manual_dirty_log_protect &
> > KVM_DIRTY_LOG_INITIALLY_SET)
> > 
> > (Maybe time to introduce a helper to shorten this check. :)
> 
> Yeah, but could this be done on top of this patch?

You're going to repost after all, right?  If so, IMO it's easier to
just add the helper in the same patch.

Thanks,

-- 
Peter Xu