(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;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
