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

[Takuya Yoshikawa <yoshikawa.takuya@oss.ntt.co.jp>]

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