Re: [PATCH v4 2/7] KVM: x86/mmu: Factor out allocating memslot rmap

From: Sean Christopherson <seanjc@google.com>
To: Ben Gardon <bgardon@google.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>,
	Peter Shier <pshier@google.com>,
	Yulei Zhang <yulei.kernel@gmail.com>,
	Wanpeng Li <kernellwp@gmail.com>,
	Xiao Guangrong <xiaoguangrong.eric@gmail.com>,
	Kai Huang <kai.huang@intel.com>,
	Keqian Zhu <zhukeqian1@huawei.com>,
	David Hildenbrand <david@redhat.com>
Subject: Re: [PATCH v4 2/7] KVM: x86/mmu: Factor out allocating memslot rmap
Date: Tue, 11 May 2021 17:56:09 +0000	[thread overview]
Message-ID: <YJrFOXW3mM3WjGT5@google.com> (raw)
In-Reply-To: <20210511171610.170160-3-bgardon@google.com>

On Tue, May 11, 2021, Ben Gardon wrote:
> Small refactor to facilitate allocating rmaps for all memslots at once.
> 
> No functional change expected.
> 
> Signed-off-by: Ben Gardon <bgardon@google.com>
> ---
>  arch/x86/kvm/x86.c | 39 ++++++++++++++++++++++++++++++---------
>  1 file changed, 30 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 1e1f4f31e586..cc0440b5b35d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10911,10 +10911,35 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot)
>  	kvm_page_track_free_memslot(slot);
>  }
>  
> +static int memslot_rmap_alloc(struct kvm_memory_slot *slot,
> +			      unsigned long npages)
> +{
> +	int i;
> +
> +	for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
> +		int lpages;
> +		int level = i + 1;
> +
> +		lpages = gfn_to_index(slot->base_gfn + npages - 1,
> +				      slot->base_gfn, level) + 1;

Might as well assign lpages at its declaration, i.e.

		int lpages = gfn_to_index(slot->base_gfn + npages - 1,
					  slot->base_gfn, level) + 1;
> +
> +		slot->arch.rmap[i] =
> +			kvcalloc(lpages, sizeof(*slot->arch.rmap[i]),
> +				 GFP_KERNEL_ACCOUNT);

Eh, I don't think avoiding a 3 char overrun is worth splitting across three lines.
E.g. this is perfectly readable

		slot->arch.rmap[i] = kvcalloc(lpages, sizeof(*slot->arch.rmap[i]),
					      GFP_KERNEL_ACCOUNT);

Alternatively, the rmap size could be captured in a local var, e.g.

	const int sz = sizeof(*slot->arch.rmap[0]);

	...

		slot->arch.rmap[i] = kvcalloc(lpages, sz, GFP_KERNEL_ACCOUNT);
		if (!slot->arch.rmap[i]) {
			memslot_rmap_free(slot);
			return -ENOMEM;
		}

> +		if (!slot->arch.rmap[i]) {
> +			memslot_rmap_free(slot);
> +			return -ENOMEM;

Reaaaally getting into nitpicks, what do you think about changing this to a goto
with the error handling at the bottom?  Obviously not necessary by any means,
but for me it makes it easier to see that all rmaps are freed on failure.  My
eyes skipped over that on the first read through.  E.g.

		if (!slot_arch.rmap[i])
			goto err;
	}

	return 0;

err:
	memslot_rmap_free(slot);
	return -ENOMEM;

> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int kvm_alloc_memslot_metadata(struct kvm_memory_slot *slot,
>  				      unsigned long npages)
>  {
>  	int i;
> +	int r;

Personal preference, for short declarations like this I like putting 'em on a
single line.

>  	/*
>  	 * Clear out the previous array pointers for the KVM_MR_MOVE case.  The
> @@ -10923,7 +10948,11 @@ static int kvm_alloc_memslot_metadata(struct kvm_memory_slot *slot,
>  	 */
>  	memset(&slot->arch, 0, sizeof(slot->arch));
>  
> -	for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) {
> +	r = memslot_rmap_alloc(slot, npages);
> +	if (r)
> +		return r;
> +
> +	for (i = 1; i < KVM_NR_PAGE_SIZES; ++i) {
>  		struct kvm_lpage_info *linfo;
>  		unsigned long ugfn;
>  		int lpages;
> @@ -10932,14 +10961,6 @@ static int kvm_alloc_memslot_metadata(struct kvm_memory_slot *slot,
>  		lpages = gfn_to_index(slot->base_gfn + npages - 1,
>  				      slot->base_gfn, level) + 1;
>  
> -		slot->arch.rmap[i] =
> -			kvcalloc(lpages, sizeof(*slot->arch.rmap[i]),
> -				 GFP_KERNEL_ACCOUNT);
> -		if (!slot->arch.rmap[i])
> -			goto out_free;
> -		if (i == 0)
> -			continue;
> -
>  		linfo = kvcalloc(lpages, sizeof(*linfo), GFP_KERNEL_ACCOUNT);
>  		if (!linfo)
>  			goto out_free;
> -- 
> 2.31.1.607.g51e8a6a459-goog
> 