From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takuya Yoshikawa Subject: Re: [PATCH RFC v2 5/6] KVM: moving dirty bitmaps to user space Date: Thu, 22 Apr 2010 18:07:52 +0900 Message-ID: <4BD011E8.6020502@oss.ntt.co.jp> References: <20100420195349.dab60b1d.yoshikawa.takuya@oss.ntt.co.jp> <20100420200225.efca602f.yoshikawa.takuya@oss.ntt.co.jp> <4BCEE0E4.6060707@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: mtosatti-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kvm-ia64-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, fernando-gVGce1chcLdL9jVzuh4AOg@public.gmane.org To: Avi Kivity Return-path: In-Reply-To: <4BCEE0E4.6060707-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> Sender: kvm-ppc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: kvm.vger.kernel.org (2010/04/21 20:26), Avi Kivity wrote: >> r = 0; >> @@ -1858,7 +1866,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, >> if (memslot->is_dirty) { >> kvm_flush_remote_tlbs(kvm); >> n = kvm_dirty_bitmap_bytes(memslot); >> - memset(memslot->dirty_bitmap, 0, n); >> + clear_user(memslot->dirty_bitmap, n); >> memslot->is_dirty = false; > > Does this need an error check? Yes, sorry I forgot. > > >> @@ -468,8 +480,12 @@ void kvm_free_physmem(struct kvm *kvm) >> int i; >> struct kvm_memslots *slots = kvm->memslots; >> >> - for (i = 0; i< slots->nmemslots; ++i) >> + for (i = 0; i< slots->nmemslots; ++i) { >> + /* We don't munmap dirty bitmaps by ourselves. */ > > Why not? If we allocated them, we have to free them. In the case of VM destruction, when qemu process exits, it conflicts with the work of the usual (process) destructor's job. I struggled with this problem before sending the first version. So I have to differentiate the slot release and qemu process exit. > >> + slots->memslots[i].dirty_bitmap = NULL; >> + slots->memslots[i].dirty_bitmap_old = NULL; >> kvm_free_physmem_slot(&slots->memslots[i], NULL); >> + } >> >> >> +/* >> + * Please use generic *_user bitops once they become available. >> + * Be careful setting the bit won't be done atomically. >> + */ > > Please introduce the user bitops as part of this patchset. > OK, I will do in the next version. In this RFC, I would be happy if I can know the overall design is right or not. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takuya Yoshikawa Date: Thu, 22 Apr 2010 09:07:52 +0000 Subject: Re: [PATCH RFC v2 5/6] KVM: moving dirty bitmaps to user space Message-Id: <4BD011E8.6020502@oss.ntt.co.jp> List-Id: References: <20100420195349.dab60b1d.yoshikawa.takuya@oss.ntt.co.jp> <20100420200225.efca602f.yoshikawa.takuya@oss.ntt.co.jp> <4BCEE0E4.6060707@redhat.com> In-Reply-To: <4BCEE0E4.6060707-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Avi Kivity Cc: mtosatti-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, kvm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kvm-ia64-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, kvm-ppc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, fernando-gVGce1chcLdL9jVzuh4AOg@public.gmane.org (2010/04/21 20:26), Avi Kivity wrote: >> r = 0; >> @@ -1858,7 +1866,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, >> if (memslot->is_dirty) { >> kvm_flush_remote_tlbs(kvm); >> n = kvm_dirty_bitmap_bytes(memslot); >> - memset(memslot->dirty_bitmap, 0, n); >> + clear_user(memslot->dirty_bitmap, n); >> memslot->is_dirty = false; > > Does this need an error check? Yes, sorry I forgot. > > >> @@ -468,8 +480,12 @@ void kvm_free_physmem(struct kvm *kvm) >> int i; >> struct kvm_memslots *slots = kvm->memslots; >> >> - for (i = 0; i< slots->nmemslots; ++i) >> + for (i = 0; i< slots->nmemslots; ++i) { >> + /* We don't munmap dirty bitmaps by ourselves. */ > > Why not? If we allocated them, we have to free them. In the case of VM destruction, when qemu process exits, it conflicts with the work of the usual (process) destructor's job. I struggled with this problem before sending the first version. So I have to differentiate the slot release and qemu process exit. > >> + slots->memslots[i].dirty_bitmap = NULL; >> + slots->memslots[i].dirty_bitmap_old = NULL; >> kvm_free_physmem_slot(&slots->memslots[i], NULL); >> + } >> >> >> +/* >> + * Please use generic *_user bitops once they become available. >> + * Be careful setting the bit won't be done atomically. >> + */ > > Please introduce the user bitops as part of this patchset. > OK, I will do in the next version. In this RFC, I would be happy if I can know the overall design is right or not. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takuya Yoshikawa Date: Thu, 22 Apr 2010 09:07:52 +0000 Subject: Re: [PATCH RFC v2 5/6] KVM: moving dirty bitmaps to user space Message-Id: <4BD011E8.6020502@oss.ntt.co.jp> List-Id: References: <20100420200225.efca602f.yoshikawa.takuya@oss.ntt.co.jp> In-Reply-To: <20100420200225.efca602f.yoshikawa.takuya@oss.ntt.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: kvm-ia64@vger.kernel.org (2010/04/21 20:26), Avi Kivity wrote: >> r = 0; >> @@ -1858,7 +1866,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, >> if (memslot->is_dirty) { >> kvm_flush_remote_tlbs(kvm); >> n = kvm_dirty_bitmap_bytes(memslot); >> - memset(memslot->dirty_bitmap, 0, n); >> + clear_user(memslot->dirty_bitmap, n); >> memslot->is_dirty = false; > > Does this need an error check? Yes, sorry I forgot. > > >> @@ -468,8 +480,12 @@ void kvm_free_physmem(struct kvm *kvm) >> int i; >> struct kvm_memslots *slots = kvm->memslots; >> >> - for (i = 0; i< slots->nmemslots; ++i) >> + for (i = 0; i< slots->nmemslots; ++i) { >> + /* We don't munmap dirty bitmaps by ourselves. */ > > Why not? If we allocated them, we have to free them. In the case of VM destruction, when qemu process exits, it conflicts with the work of the usual (process) destructor's job. I struggled with this problem before sending the first version. So I have to differentiate the slot release and qemu process exit. > >> + slots->memslots[i].dirty_bitmap = NULL; >> + slots->memslots[i].dirty_bitmap_old = NULL; >> kvm_free_physmem_slot(&slots->memslots[i], NULL); >> + } >> >> >> +/* >> + * Please use generic *_user bitops once they become available. >> + * Be careful setting the bit won't be done atomically. >> + */ > > Please introduce the user bitops as part of this patchset. > OK, I will do in the next version. In this RFC, I would be happy if I can know the overall design is right or not.