On Tue, Nov 10, 2020 at 02:13:14PM -0800, Ben Gardon wrote:
> On Tue, Nov 10, 2020 at 12:48 PM Andrew Jones <drjones@xxxxxxxxxx> wrote:
> >
> > Introduce new vm_create variants that also takes a number of vcpus,
> > an amount of per-vcpu pages, and optionally a list of vcpuids. These
> > variants will create default VMs with enough additional pages to
> > cover the vcpu stacks, per-vcpu pages, and pagetable pages for all.
> > The new 'default' variant uses VM_MODE_DEFAULT, whereas the other
> > new variant accepts the mode as a parameter.
> >
> > Reviewed-by: Peter Xu <peterx@xxxxxxxxxx>
> > Reviewed-by: Ben Gardon <bgardon@xxxxxxxxxx>
> > Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> > ---
> > .../testing/selftests/kvm/include/kvm_util.h | 10 ++++++
> > tools/testing/selftests/kvm/lib/kvm_util.c | 35 ++++++++++++++++---
> > 2 files changed, 40 insertions(+), 5 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h
> > index 48b48a0014e2..bc8db80309f5 100644
> > --- a/tools/testing/selftests/kvm/include/kvm_util.h
> > +++ b/tools/testing/selftests/kvm/include/kvm_util.h
> > @@ -261,6 +261,16 @@ vm_paddr_t vm_phy_pages_alloc(struct kvm_vm *vm, size_t num,
> > struct kvm_vm *vm_create_default(uint32_t vcpuid, uint64_t extra_mem_pages,
> > void *guest_code);
> >
> > +/* Same as vm_create_default, but can be used for more than one vcpu */
> > +struct kvm_vm *vm_create_default_with_vcpus(uint32_t nr_vcpus, uint64_t extra_mem_pages,
> > + uint32_t num_percpu_pages, void *guest_code,
> > + uint32_t vcpuids[]);
> > +
> > +/* Like vm_create_default_with_vcpus, but accepts mode as a parameter */
> > +struct kvm_vm *vm_create_with_vcpus(enum vm_guest_mode mode, uint32_t nr_vcpus,
> > + uint64_t extra_mem_pages, uint32_t num_percpu_pages,
> > + void *guest_code, uint32_t vcpuids[]);
> > +
> > /*
> > * Adds a vCPU with reasonable defaults (e.g. a stack)
> > *
> > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> > index a7e28e33fc3b..b31a4e988a5d 100644
> > --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> > @@ -272,8 +272,9 @@ struct kvm_vm *vm_create(enum vm_guest_mode mode, uint64_t phy_pages, int perm)
> > return vm;
> > }
> >
> > -struct kvm_vm *vm_create_default(uint32_t vcpuid, uint64_t extra_mem_pages,
> > - void *guest_code)
> > +struct kvm_vm *vm_create_with_vcpus(enum vm_guest_mode mode, uint32_t nr_vcpus,
> > + uint64_t extra_mem_pages, uint32_t num_percpu_pages,
> > + void *guest_code, uint32_t vcpuids[])
> > {
> > /* The maximum page table size for a memory region will be when the
> > * smallest pages are used. Considering each page contains x page
> > @@ -281,10 +282,18 @@ struct kvm_vm *vm_create_default(uint32_t vcpuid, uint64_t extra_mem_pages,
> > * N pages) will be: N/x+N/x^2+N/x^3+... which is definitely smaller
> > * than N/x*2.
> > */
> > - uint64_t extra_pg_pages = (extra_mem_pages / PTES_PER_MIN_PAGE) * 2;
> > + uint64_t vcpu_pages = (DEFAULT_STACK_PGS + num_percpu_pages) * nr_vcpus;
> > + uint64_t extra_pg_pages = (extra_mem_pages + vcpu_pages) / PTES_PER_MIN_PAGE * 2;
> > + uint64_t pages = DEFAULT_GUEST_PHY_PAGES + vcpu_pages + extra_pg_pages;
> > struct kvm_vm *vm;
> > + int i;
> > +
> > + TEST_ASSERT(nr_vcpus <= kvm_check_cap(KVM_CAP_MAX_VCPUS),
> > + "nr_vcpus = %d too large for host, max-vcpus = %d",
> > + nr_vcpus, kvm_check_cap(KVM_CAP_MAX_VCPUS));
> >
> > - vm = vm_create(VM_MODE_DEFAULT, DEFAULT_GUEST_PHY_PAGES + extra_pg_pages, O_RDWR);
> > + pages = vm_adjust_num_guest_pages(mode, pages);
> > + vm = vm_create(mode, pages, O_RDWR);
>
> I think this will substantially change the behavior of this function
> to create a much larger memslot 0. In the existing code, the memslot
> created in vm_create is just sized large enough for the stacks and
> page tables. Another memslot is then created for the memory under
> test.
>
> I think separating the memslots is a good arrangement because it
> limits the extent to which kernel bugs could screw up the test and
> makes it easier to debug if you're testing something like dirty
> logging. It's also useful if you wanted to back the memslot under test
> with a different kind of memory from memslot 0. e.g. memslot 0 could
> use anonymous pages and the slot(s) under test could use hugetlbfs.
> You might also want multiple memslots to assign them to different NUMA
> nodes.
>
> Is that change intentional? I would suggest not adding vcpu_pages to
> the calculation for pages above, similar to what it was before:
> uint64_t pages = DEFAULT_GUEST_PHY_PAGES + extra_pg_pages;
It was definitely intentional to create API that allows us to allocate
per-vcpu pages for the VM created with vcpus. But, to do what you want,
we just need to change the
vm = vm_create_with_vcpus(mode, vcpus, 0,
vcpu_memory_bytes / perf_test_args.guest_page_size,
guest_code, NULL);
line in perf_test_util.h in the "KVM: selftests: Use vm_create_with_vcpus
in create_vm" patch to
vm = vm_create_with_vcpus(mode, vcpus,
(vcpus * vcpu_memory_bytes) / perf_test_args.guest_page_size,
0, guest_code, NULL);
Notice how that moves the per-vcpu pages to the extra_mem_pages, which
only get used to calculate page table pages.
I'll do that in v2.
Thanks,
drew
