From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 1/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean Date: Tue, 27 Apr 2010 16:18:10 +0300 Message-ID: <4BD6E412.7090202@redhat.com> References: <20100426185657.7f68c3ad.yoshikawa.takuya@oss.ntt.co.jp> <20100426185854.5d606a04.yoshikawa.takuya@oss.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: mtosatti@redhat.com, kvm@vger.kernel.org To: Takuya Yoshikawa Return-path: Received: from mx1.redhat.com ([209.132.183.28]:10832 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750883Ab0D0NSS (ORCPT ); Tue, 27 Apr 2010 09:18:18 -0400 In-Reply-To: <20100426185854.5d606a04.yoshikawa.takuya@oss.ntt.co.jp> Sender: kvm-owner@vger.kernel.org List-ID: On 04/26/2010 12:58 PM, Takuya Yoshikawa wrote: > Although we always allocate a new dirty bitmap in x86's get_dirty_log(), > it is only used as a zero-source of copy_to_user() and freed right after > that when memslot is clean. This patch uses clear_user() instead of doing > this unnecessary zero-source allocation. > > Performance improvement: as we can expect easily, the time needed to > allocate a bitmap is completely reduced. In my test, the improved ioctl > was about 4 to 10 times faster than the original one for clean slots. > Furthermore, the reduced allocations seem to produce good effects for > other cases too. Actually, I observed that the time for the ioctl was > more stable than the original one and the average time for dirty slots > was also reduced by some extent. > Can you explain why the dirty slots were improved? > Signed-off-by: Takuya Yoshikawa > --- > arch/x86/kvm/x86.c | 36 ++++++++++++++++++++++-------------- > 1 files changed, 22 insertions(+), 14 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 6b2ce1d..0086d64 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -2744,7 +2744,6 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, > struct kvm_memory_slot *memslot; > unsigned long n; > unsigned long is_dirty = 0; > - unsigned long *dirty_bitmap = NULL; > > mutex_lock(&kvm->slots_lock); > > @@ -2759,27 +2758,29 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, > > n = kvm_dirty_bitmap_bytes(memslot); > > - r = -ENOMEM; > - dirty_bitmap = vmalloc(n); > - if (!dirty_bitmap) > - goto out; > - memset(dirty_bitmap, 0, n); > - > for (i = 0; !is_dirty&& i< n/sizeof(long); i++) > is_dirty = memslot->dirty_bitmap[i]; > > /* If nothing is dirty, don't bother messing with page tables. */ > if (is_dirty) { > struct kvm_memslots *slots, *old_slots; > + unsigned long *dirty_bitmap; > > spin_lock(&kvm->mmu_lock); > kvm_mmu_slot_remove_write_access(kvm, log->slot); > spin_unlock(&kvm->mmu_lock); > > - slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL); > - if (!slots) > - goto out_free; > + r = -ENOMEM; > + dirty_bitmap = vmalloc(n); > + if (!dirty_bitmap) > + goto out; > + memset(dirty_bitmap, 0, n); > > Better to keep r = -ENOMEM here, so if something else is inserted, we don't lose the error. It logically belongs with the allocation, not inherited. > + slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL); > + if (!slots) { > + vfree(dirty_bitmap); > + goto out; > + } > memcpy(slots, kvm->memslots, sizeof(struct kvm_memslots)); > slots->memslots[log->slot].dirty_bitmap = dirty_bitmap; > > @@ -2788,13 +2789,20 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, > synchronize_srcu_expedited(&kvm->srcu); > dirty_bitmap = old_slots->memslots[log->slot].dirty_bitmap; > kfree(old_slots); > + > + r = -EFAULT; > + if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) { > + vfree(dirty_bitmap); > What about slots? ah, I see they were already freed. > + goto out; > + } > + vfree(dirty_bitmap); > + } else { > + r = -EFAULT; > + if (clear_user(log->dirty_bitmap, n)) > + goto out; > } > > r = 0; > - if (copy_to_user(log->dirty_bitmap, dirty_bitmap, n)) > - r = -EFAULT; > -out_free: > - vfree(dirty_bitmap); > out: > mutex_unlock(&kvm->slots_lock); > return r; > -- error compiling committee.c: too many arguments to function