| Message ID | 20210511171610.170160-3-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Lazily allocate memslot rmaps | expand |
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 >
On Tue, May 11, 2021 at 10:56 AM Sean Christopherson <seanjc@google.com> 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@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; 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. > > > + } > > + } > > + > > + 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 > >
On 11.05.21 20:17, Ben Gardon wrote: > On Tue, May 11, 2021 at 10:56 AM Sean Christopherson <seanjc@google.com> 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@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; > > 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.
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; + + slot->arch.rmap[i] = + kvcalloc(lpages, sizeof(*slot->arch.rmap[i]), + GFP_KERNEL_ACCOUNT); + if (!slot->arch.rmap[i]) { + 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; /* * 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;
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(-)