| Message ID | 20210506184241.618958-3-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Lazily allocate memslot rmaps | expand |
On 06.05.21 20:42, 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 | 41 ++++++++++++++++++++++++++++++++--------- > 1 file changed, 32 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 5bcf07465c47..fc32a7dbe4c4 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -10842,10 +10842,37 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot) > kvm_page_track_free_memslot(slot); > } > > +static int alloc_memslot_rmap(struct kvm_memory_slot *slot, > + unsigned long npages) I'd have called the functions memslot_rmap_alloc() and memslot_rmap_free() > +{ > + 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]) > + goto out_free; you can just avoid the goto here and do the free_memslot_rmap() right away. > + } > + > + return 0; > + > +out_free: > + free_memslot_rmap(slot); > + return -ENOMEM; > +} > + > 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 > @@ -10854,7 +10881,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 = alloc_memslot_rmap(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; > @@ -10863,14 +10894,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; > apart from that LGTM
On Fri, May 7, 2021 at 12:47 AM David Hildenbrand <david@redhat.com> wrote: > > On 06.05.21 20:42, 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 | 41 ++++++++++++++++++++++++++++++++--------- > > 1 file changed, 32 insertions(+), 9 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 5bcf07465c47..fc32a7dbe4c4 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -10842,10 +10842,37 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot) > > kvm_page_track_free_memslot(slot); > > } > > > > +static int alloc_memslot_rmap(struct kvm_memory_slot *slot, > > + unsigned long npages) > > I'd have called the functions memslot_rmap_alloc() and memslot_rmap_free() > > > > +{ > > + 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]) > > + goto out_free; > > you can just avoid the goto here and do the free_memslot_rmap() right away. Oh good catch. I'll incorporate your suggestions in v4. > > > + } > > + > > + return 0; > > + > > +out_free: > > + free_memslot_rmap(slot); > > + return -ENOMEM; > > +} > > + > > 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 > > @@ -10854,7 +10881,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 = alloc_memslot_rmap(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; > > @@ -10863,14 +10894,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; > > > > apart from that LGTM > > -- > Thanks, > > David / dhildenb >
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 5bcf07465c47..fc32a7dbe4c4 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10842,10 +10842,37 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot) kvm_page_track_free_memslot(slot); } +static int alloc_memslot_rmap(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]) + goto out_free; + } + + return 0; + +out_free: + free_memslot_rmap(slot); + return -ENOMEM; +} + 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 @@ -10854,7 +10881,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 = alloc_memslot_rmap(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; @@ -10863,14 +10894,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 | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-)