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

Zhoujian (jay)
2020-02-22T08:11:30Z

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