On Tue, Aug 21, 2018 at 03:52:40PM +0200, Paolo Bonzini wrote:
>On 21/08/2018 14:57, Wei Yang wrote:
>> On Tue, Aug 21, 2018 at 12:31:31PM +0200, Paolo Bonzini wrote:
>>> On 21/08/2018 00:53, Wei Yang wrote:
>>>> On Mon, Aug 20, 2018 at 04:44:25PM -0400, Konrad Rzeszutek Wilk wrote:
>>>>> On Mon, Aug 20, 2018 at 08:23:29PM +0000, Wei Yang wrote:
>>>>>> On Mon, Aug 20, 2018 at 09:47:20AM -0400, Konrad Rzeszutek Wilk wrote:
>>>>>>> On Sun, Aug 19, 2018 at 08:17:45AM +0800, Wei Yang wrote:
>>>>>>>> update_memslots() is only called by __kvm_set_memory_region(), in which
>>>>>>>> change is calculated to indicate the following behavior. With this
>>>>>>>
>>>>>>> What is the 'following behavior' you mention?
>>>>>>
>>>>>> Ah, 'following behavior' means what need to do in update_memslots() to
>>>>>> used_slots.
>>>>>>
>>>>>> For example, if change equals KVM_MR_CREATE, it means update_memslots()
>>>>>> will add a new slot and used_slots need to increase by one.
>>>>>>
>>>>>> With this information, we don't need to do the calculation in
>>>>>> update_memslots() again, but just do an addition.
>>>>>
>>>>> Could you update the commit ot have this description please?
>>>>>>
>>>>
>>>> Sure.
>>>>
>>>> Per comments from Paolo in patch 2, only this one is necessary in next
>>>> spin, right?
>>>
>>> I don't know... Encoding the delta in bits 0-7 is not something I would
>>> really do unless it's super important for performance.
>>
>> The original idea is just pass the change to update_memslots() and
>> adjust used_slots based on change instead of re-calculate it.
>
>Looks good, and perhaps:
>
>> How about something like this:
>>
>> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
>> index 3d233ebfbee9..efecb0489dbc 100644
>> --- a/virt/kvm/kvm_main.c
>> +++ b/virt/kvm/kvm_main.c
>> @@ -808,13 +808,15 @@ static void update_memslots(struct kvm_memslots *slots,
>> struct kvm_memory_slot *mslots = slots->memslots;
>>
>> WARN_ON(mslots[i].id != id);
>> - if (!new->npages) {
>> - WARN_ON(!mslots[i].npages);
>> - if (mslots[i].npages)
>> - slots->used_slots--;
>> - } else {
>> - if (!mslots[i].npages)
>> - slots->used_slots++;
>> + switch (change) {
>> + case KVM_MR_CREATE:
>> + slots->used_slots++;
>
>WARN_ON(mslots[i].npages || !new->npages);
>
>> + break;
>> + case KVM_MR_DELETE:
>> + slots->used_slots--;
>
>WARN_ON(new->npages || !mslots[i].npages);
>
>> + break;
>> + default:
>> + break;
>> }
>
>Ensuring the invariants is really more important here than being fast.
Ok, get your point. I will spin a patch based on this format.
BTW, I am willing to measure the impact of those change. Also I have
another idea to improve the code. Do you have some case or method to
measure this?
>
>Paolo
--
Wei Yang
Help you, Help me
