[PATCH 0/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[PATCH 0/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Takuya Yoshikawa]

Hi Avi,

I want you look at this patch before discussing about our patch set.

This patch sould itself worth it, I belive, and shows how much improvements
we can expect from our dirty bitmap works.

Note: this will not conflict with our future works!

Thanks,
  Takuya

** Simple test **
1. What we did
I measured the time needed for the get dirty log ioctl during playing with
Ubuntu installer, in which VGA is logging, and compared the result to that
of the original version.

2. Test environment
Intel(R) Core(TM)2 Duo CPU     P8700  @ 2.53GHz
(No EPT support)
With latest qemu-kvm.git

This test is for clarifying the micro performance, and is not intended to
verify what we can expect on Enterprize servers: so I used my laptop.

3. Results
I picked up three typical parts for comparison.

- TYPE1
original
----------------------------------------
slot=6, slot.len=     32768, usec=   65
slot=7, slot.len=     32768, usec=   24
slot=6, slot.len=     32768, usec=   26
slot=7, slot.len=     32768, usec=   23
slot=6, slot.len=     32768, usec=   24
slot=7, slot.len=     32768, usec=   24
slot=6, slot.len=     32768, usec=   25
slot=7, slot.len=     32768, usec=   25
----------------------------------------
with my patch
----------------------------------------
slot=6, slot.len=     32768, usec=    3
slot=7, slot.len=     32768, usec=    3
slot=6, slot.len=     32768, usec=    3
slot=7, slot.len=     32768, usec=    2
slot=6, slot.len=     32768, usec=    3
slot=7, slot.len=     32768, usec=    2
slot=6, slot.len=     32768, usec=    3
slot=7, slot.len=     32768, usec=    3
----------------------------------------

- TYPE2
original
----------------------------------------
slot=6, slot.len=     32768, usec=  158
slot=7, slot.len=     32768, usec=   26
slot=6, slot.len=     32768, usec=  157
slot=7, slot.len=     32768, usec=   26
slot=6, slot.len=     32768, usec=  157
slot=7, slot.len=     32768, usec=   26
slot=6, slot.len=     32768, usec=  158
slot=7, slot.len=     32768, usec=   27
----------------------------------------
with my patch
----------------------------------------
slot=6, slot.len=     32768, usec=  117
slot=7, slot.len=     32768, usec=    2
slot=6, slot.len=     32768, usec=  124
slot=7, slot.len=     32768, usec=    1
slot=6, slot.len=     32768, usec=  121
slot=7, slot.len=     32768, usec=    1
slot=6, slot.len=     32768, usec=   72
slot=7, slot.len=     32768, usec=    2
----------------------------------------

- TYPE3
original
----------------------------------------
slot=5, slot.len=  16777216, usec=    9
slot=6, slot.len=     32768, usec=    6
slot=7, slot.len=     32768, usec=    7
slot=5, slot.len=  16777216, usec=   11
slot=6, slot.len=     32768, usec=    7
slot=7, slot.len=     32768, usec=    7
slot=5, slot.len=  16777216, usec=   14
slot=6, slot.len=     32768, usec=    8
slot=7, slot.len=     32768, usec=    8
slot=5, slot.len=  16777216, usec=   11
slot=6, slot.len=     32768, usec=    9
slot=7, slot.len=     32768, usec=    9
----------------------------------------
with my patch
----------------------------------------
slot=5, slot.len=  16777216, usec=    2
slot=6, slot.len=     32768, usec=    2
slot=7, slot.len=     32768, usec=    1
slot=5, slot.len=  16777216, usec=    2
slot=6, slot.len=     32768, usec=    2
slot=7, slot.len=     32768, usec=    2
slot=5, slot.len=  16777216, usec=    2
slot=6, slot.len=     32768, usec=    1
slot=7, slot.len=     32768, usec=    1
slot=5, slot.len=  16777216, usec=    3
slot=6, slot.len=     32768, usec=    2
slot=7, slot.len=     32768, usec=    2
----------------------------------------

[PATCH 1/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Takuya Yoshikawa]

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.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 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);
 
+		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);
+			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;
-- 
1.6.3.3

Re: [PATCH 1/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Marcelo Tosatti]

On Mon, Apr 26, 2010 at 06:58:54PM +0900, 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.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>

Looks good to me.

Re: [PATCH 1/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Avi Kivity]

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<yoshikawa.takuya@oss.ntt.co.jp>
> ---
>   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

Re: [PATCH 1/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Takuya Yoshikawa]

(2010/04/27 22:18), Avi Kivity wrote:

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

I cannot do exactly, but vmalloc() might affect the following allocations?

But actually, I could check how much one vmalloc() consumed our time,
and this will be completely removed by our "moving dirty bitmaps to user space".


Better to remove this explanation about dirty slots?


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

Oh, I read your similar comment to other person. Sorry, I should have.


BTW, maybe good news for us!

1. I found one place in which set_bit_to_user() is locally defined using
current helpers.

So we become easier to explain why set_bit_user_non_atomic() is needed.


2. I found copy_user helper of x86_32 asm which may be useful for copy_in_user().

This is used both for *_from_user() and *_to_user() and my patch worked with
it in more than enough speed.

With Fernando, I'm checking if this is OK for worst fault cases too.


Thanks,
   Takuya



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

Re: [PATCH 1/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Takuya Yoshikawa]

(2010/04/27 22:46), Takuya Yoshikawa wrote:
> (2010/04/27 22:18), Avi Kivity wrote:
>
>>> 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?
>
> I cannot do exactly, but vmalloc() might affect the following allocations?
>


Oh, this might be the effect of tlb flush or something?
  - I'm not so confident about mm, but vmalloc(),vfree() does this kind of thing?

If so, the effect has more generic sense, and my explanation was misleading
in that sense.


And one more difference is from clear_user(). This probably uses registers as
source as opposed to copy_to_user()'s case.





> But actually, I could check how much one vmalloc() consumed our time,
> and this will be completely removed by our "moving dirty bitmaps to user
> space".
>
>
> Better to remove this explanation about dirty slots?
>
>
>>>
>>> - 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.
>
> Oh, I read your similar comment to other person. Sorry, I should have.
>
>
> BTW, maybe good news for us!
>
> 1. I found one place in which set_bit_to_user() is locally defined using
> current helpers.
>
> So we become easier to explain why set_bit_user_non_atomic() is needed.
>
>
> 2. I found copy_user helper of x86_32 asm which may be useful for
> copy_in_user().
>
> This is used both for *_from_user() and *_to_user() and my patch worked
> with
> it in more than enough speed.
>
> With Fernando, I'm checking if this is OK for worst fault cases too.
>
>
> Thanks,
> Takuya
>
>
>

Re: [PATCH 1/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Avi Kivity]

On 04/28/2010 07:17 AM, Takuya Yoshikawa wrote:
> (2010/04/27 22:46), Takuya Yoshikawa wrote:
>> (2010/04/27 22:18), Avi Kivity wrote:
>>
>>>> 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?
>>
>> I cannot do exactly, but vmalloc() might affect the following 
>> allocations?
>>
>
>
> Oh, this might be the effect of tlb flush or something?
>  - I'm not so confident about mm, but vmalloc(),vfree() does this kind 
> of thing?

Yes, vmalloc() does tlb flushes, perhaps that's the cause.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/1] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Avi Kivity]

On 04/27/2010 04:46 PM, Takuya Yoshikawa wrote:
> (2010/04/27 22:18), Avi Kivity wrote:
>
>>> 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?
>
> I cannot do exactly, but vmalloc() might affect the following 
> allocations?
>
> But actually, I could check how much one vmalloc() consumed our time,
> and this will be completely removed by our "moving dirty bitmaps to 
> user space".
>
>
> Better to remove this explanation about dirty slots?

I guess so.

>
> BTW, maybe good news for us!
>
> 1. I found one place in which set_bit_to_user() is locally defined using
> current helpers.

Interesting, where?

-- 
error compiling committee.c: too many arguments to function

[PATCH v2] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Takuya Yoshikawa]

Hi Marcelo, Avi,

I updated the patch as follows.

Changelog:
 1. Inserted one "r = -ENOMEM;" line following Avi's advice.
 2. Little change of explanation about performance improvements.

I'm now testing and cleaning up my next patch series based on this,
so please apply this if this makes sense and has no problems.

Thanks,
  Takuya

===

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, reducing memory allocations and copies will produce good
effects to caches too.

Signed-off-by: Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>
---
 arch/x86/kvm/x86.c |   37 +++++++++++++++++++++++--------------
 1 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6b2ce1d..b95a211 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,30 @@ 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);
 
+		r = -ENOMEM;
+		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 +2790,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);
+			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;
-- 
1.6.3.3

Re: [PATCH v2] KVM: x86: avoid unnecessary bitmap allocation when memslot is clean

[Avi Kivity]

On 04/28/2010 12:50 PM, Takuya Yoshikawa wrote:
> Hi Marcelo, Avi,
>
> I updated the patch as follows.
>
> Changelog:
>   1. Inserted one "r = -ENOMEM;" line following Avi's advice.
>   2. Little change of explanation about performance improvements.
>
> I'm now testing and cleaning up my next patch series based on this,
> so please apply this if this makes sense and has no problems.
>
> Thanks,
>    Takuya
>
> ===
>
> 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, reducing memory allocations and copies will produce good
> effects to caches too.
>
>    

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function