Re: [PATCH] KVM: SVM: refactor avic VM ID allocation

[Paolo Bonzini]

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@xxxxxxxxxx>
>> Cc: Joerg Roedel <joro@xxxxxxxxxx>
>> Cc: pbonzini@xxxxxxxxxx
>> Cc: rkrcmar@xxxxxxxxxx
>> Cc: tglx@xxxxxxxxxxxxx
>> Cc: mingo@xxxxxxxxxx
>> Cc: hpa@xxxxxxxxx
>> Cc: x86@xxxxxxxxxx
>> Cc: kvm@xxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> ---
> 
> 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? :)

Paolo


> What about applying something like the following on top?
> ---8<---
> Subject: [PATCH] KVM: SVM: refactor avic VM ID allocation
> 
> We missed protection in case someone deserving a medal allocated 2^24
> VMs and got a deadlock instead.  I think it is nicer without the goto
> even if we droppped the error handling.
> 
> Signed-off-by: Radim KrÄmÃÅ <rkrcmar@xxxxxxxxxx>
> ---
>  arch/x86/kvm/svm.c | 58 +++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index c21b49b5ee96..4d9ee8d76db3 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -985,8 +985,6 @@ static void disable_nmi_singlestep(struct vcpu_svm *svm)
>   */
>  #define SVM_VM_DATA_HASH_BITS	8
>  static DEFINE_HASHTABLE(svm_vm_data_hash, SVM_VM_DATA_HASH_BITS);
> -static u32 next_vm_id = 0;
> -static bool next_vm_id_wrapped = 0;
>  static DEFINE_SPINLOCK(svm_vm_data_hash_lock);
>  
>  /* Note:
> @@ -1385,6 +1383,38 @@ static int avic_init_backing_page(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> +static bool avic_vm_id_is_used(u32 vm_id)
> +{
> +	struct kvm_arch *ka;
> +
> +	hash_for_each_possible(svm_vm_data_hash, ka, hnode, vm_id)
> +		if (ka->avic_vm_id == vm_id)
> +			return true;
> +
> +	return false;
> +}
> +
> +static u32 avic_get_next_vm_id(void)
> +{
> +	static u32 next_vm_id = 0;
> +	static bool next_vm_id_wrapped = false;
> +	unsigned i;
> +
> +	for (i = 0; i < AVIC_VM_ID_NR; i++) {
> +		next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK;
> +
> +		if (next_vm_id == 0) { /* id is 1-based, zero is not okay */
> +			next_vm_id = 1;
> +			next_vm_id_wrapped = true;
> +		}
> +
> +		if (!next_vm_id_wrapped || !avic_vm_id_is_used(next_vm_id))
> +			return next_vm_id;
> +	}
> +
> +	return 0;
> +}
> +
>  static void avic_vm_destroy(struct kvm *kvm)
>  {
>  	unsigned long flags;
> @@ -1410,8 +1440,6 @@ static int avic_vm_init(struct kvm *kvm)
>  	struct kvm_arch *vm_data = &kvm->arch;
>  	struct page *p_page;
>  	struct page *l_page;
> -	struct kvm_arch *ka;
> -	u32 vm_id;
>  
>  	if (!avic)
>  		return 0;
> @@ -1432,28 +1460,18 @@ static int avic_vm_init(struct kvm *kvm)
>  	vm_data->avic_logical_id_table_page = l_page;
>  	clear_page(page_address(l_page));
>  
> +	err = -EAGAIN;
>  	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 */
> -		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;
> -		}
> -	}
> -	vm_data->avic_vm_id = vm_id;
> +	vm_data->avic_vm_id = avic_get_next_vm_id();
> +	if (!vm_data->avic_vm_id)
> +		goto unlock;
>  	hash_add(svm_vm_data_hash, &vm_data->hnode, vm_data->avic_vm_id);
>  	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>  
>  	return 0;
>  
> +unlock:
> +	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>  free_avic:
>  	avic_vm_destroy(kvm);
>  	return err;
>