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