DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 7254285540
Subject: Re: [PATCH] KVM: SVM: refactor avic VM ID allocation
To: Denys Vlasenko <dvlasenk@redhat.com>,
        =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= <rkrcmar@redhat.com>,
        Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
Cc: Joerg Roedel <joro@8bytes.org>, tglx@linutronix.de, mingo@redhat.com,
        hpa@zytor.com, x86@kernel.org, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20170811201158.24715-1-dvlasenk@redhat.com>
 <20170815201159.GC5975@flask>
 <c352062c-fb9f-4468-297d-42a87b53fc8c@redhat.com>
 <4ca2cb8a-3743-fe42-2542-797493fd819f@redhat.com>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <41de72c5-af96-5e53-ccf7-5fc969748fbd@redhat.com>
Date: Fri, 18 Aug 2017 18:05:32 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <4ca2cb8a-3743-fe42-2542-797493fd819f@redhat.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2551
Lines: 68

On 18/08/2017 17:22, Denys Vlasenko wrote:
> On 08/17/2017 04:33 PM, Paolo Bonzini wrote:
>> On 15/08/2017 22:12, Radim Krčmář wrote:
>>> 2017-08-11 22:11+0200, Denys Vlasenko:
>>>> With lightly tweaked defconfig:
>>>>
>>>>     text    data     bss      dec     hex filename
>>>> 11259661 5109408 2981888 19350957 12745ad vmlinux.before
>>>> 11259661 5109408  884736 17253805 10745ad vmlinux.after
>>>>
>>>> Only compile-tested.
>>>>
>>>> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
>>>> Cc: Joerg Roedel <joro@8bytes.org>
>>>> Cc: pbonzini@redhat.com
>>>> Cc: rkrcmar@redhat.com
>>>> Cc: tglx@linutronix.de
>>>> Cc: mingo@redhat.com
>>>> Cc: hpa@zytor.com
>>>> Cc: x86@kernel.org
>>>> Cc: kvm@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> ---
>>>
>>> Ah, I suspected my todo wasn't this short;  thanks for the patch!
>>>
>>>> @@ -1468,6 +1433,22 @@ static int avic_vm_init(struct kvm *kvm)
>>>>      clear_page(page_address(l_page));
>>>>
>>>>      spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
>>>> + again:
>>>> +    vm_id = next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK;
>>>> +    if (vm_id == 0) { /* id is 1-based, zero is not okay */
>>>
>>> Suravee, did the reserved zero mean something?
>>>
>>>> +        next_vm_id_wrapped = 1;
>>>> +        goto again;
>>>> +    }
>>>> +    /* Is it still in use? Only possible if wrapped at least once */
>>>> +    if (next_vm_id_wrapped) {
>>>> +        hash_for_each_possible(svm_vm_data_hash, ka, hnode, vm_id) {
>>>> +            struct kvm *k2 = container_of(ka, struct kvm, arch);
>>>> +            struct kvm_arch *vd2 = &k2->arch;
>>>> +            if (vd2->avic_vm_id == vm_id)
>>>> +                goto again;
>>>
>>> Although hitting the case where all 2^24 ids are already used is
>>> practically impossible, I don't like the loose end ...
>>
>> I think even the case where 2^16 ids are used deserves a medal.  Why
>> don't we just make the bitmap 8 KiB and call it a day? :)
> 
> Well, the RAM is cheap, but a 4-byte variable is still thousands of times
> smaller than a 8K bitmap.
> 
> Since a 256 element hash table is used here, you need to have ~one hundred
> VMs to start seeing (very small) degradation in speed of creation of new
> VMs compared to bitmap approach, and that is only after 16777216 VMs
> were created since reboot.

I guess that's fair since we already have the hash table for other uses.

Paolo

> If you want to spend RAM on speeding this up, you can increase hash
> table size
> instead. That would speed up avic_ga_log_notifier() too.