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

["Zhoujian (jay)" <jianjay.zhou@huawei.com>]

> -----Original Message-----
> From: Peter Xu [mailto:peterx@redhat.com]
> Sent: Friday, February 21, 2020 11:19 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>
> Cc: kvm@vger.kernel.org; pbonzini@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>; Huangweidong (C)
> <weidong.huang@huawei.com>; sean.j.christopherson@intel.com; Liujinsong
> (Paul) <liu.jinsong@huawei.com>
> Subject: Re: [PATCH v2] KVM: x86: enable dirty log gradually in small chunks
> 
> 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.

OK, I got your meaning now.

> 
> >
> > > 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.

Yeah, will drop it in the next version.

> 
> >
> > >
> > > (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.

OK, will do in v3.

Regards,
Jay Zhou

> 
> Thanks,
> 
> --
> Peter Xu