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

From: David Hildenbrand
Date: Tue May 11 2021 - 14:56:55 EST


On 11.05.21 20:17, Ben Gardon wrote:
On Tue, May 11, 2021 at 10:56 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:

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@xxxxxxxxxx>
---
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;

I'll do this if I end up sending out a v5.

+
+ 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);

I like this suggestion. Much nicer. Will incorporate if I send a v5.

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;


Lol, I had a goto in v3, but David Hildenbrand suggested removing it
and putting the free in the loop. I think I like it more this way too.

No strong opinion, I tend to stick to Documentation/process/coding-style.rst which states

"The goto statement comes in handy when a function exits from multiple locations and some common work such as cleanup has to be done."

As we only have a single error exit and no complicated locking, at least for me the "goto" makes it unnecessary hard to read.


--
Thanks,

David / dhildenb