Re: [PATCH 4/6] KVM: selftests: memslot_perf_test: Support variable guest page size
From: Gavin Shan
Date: Mon Oct 17 2022 - 20:46:53 EST
On 10/18/22 5:31 AM, Maciej S. Szmigiero wrote:
On 14.10.2022 09:19, Gavin Shan wrote:
The test case is obviously broken on aarch64 because non-4KB guest
page size is supported. The guest page size on aarch64 could be 4KB,
16KB or 64KB.
This supports variable guest page size, mostly for aarch64.
- The host determines the guest page size when virtual machine is
created. The value is also passed to guest through the synchronization
area.
- The number of guest pages are unknown until the virtual machine
is to be created. So all the related macros are dropped. Instead,
their values are dynamically calculated based on the guest page
size.
- The static checks on memory sizes and pages becomes dependent
on guest page size, which is unknown until the virtual machine
is about to be created. So all the static checks are converted
to dynamic checks, done in check_memory_sizes().
- As the address passed to madvise() should be aligned to host page,
the size of page chunk is automatically selected, other than one
page.
- All other changes included in this patch are almost mechanical
replacing '4096' with 'guest_page_size'.
Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
---
.../testing/selftests/kvm/memslot_perf_test.c | 191 +++++++++++-------
1 file changed, 115 insertions(+), 76 deletions(-)
diff --git a/tools/testing/selftests/kvm/memslot_perf_test.c b/tools/testing/selftests/kvm/memslot_perf_test.c
index d5aa9148f96f..d587bd952ff9 100644
--- a/tools/testing/selftests/kvm/memslot_perf_test.c
+++ b/tools/testing/selftests/kvm/memslot_perf_test.c
@@ -26,14 +26,11 @@
#include <processor.h>
#define MEM_SIZE ((512U << 20) + 4096)
-#define MEM_SIZE_PAGES (MEM_SIZE / 4096)
#define MEM_GPA 0x10000000UL
#define MEM_AUX_GPA MEM_GPA
#define MEM_SYNC_GPA MEM_AUX_GPA
#define MEM_TEST_GPA (MEM_AUX_GPA + 4096)
#define MEM_TEST_SIZE (MEM_SIZE - 4096)
-static_assert(MEM_SIZE % 4096 == 0, "invalid mem size");
-static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
/*
* 32 MiB is max size that gets well over 100 iterations on 509 slots.
@@ -42,29 +39,16 @@ static_assert(MEM_TEST_SIZE % 4096 == 0, "invalid mem test size");
* limited resolution).
*/
#define MEM_SIZE_MAP ((32U << 20) + 4096)
-#define MEM_SIZE_MAP_PAGES (MEM_SIZE_MAP / 4096)
#define MEM_TEST_MAP_SIZE (MEM_SIZE_MAP - 4096)
-#define MEM_TEST_MAP_SIZE_PAGES (MEM_TEST_MAP_SIZE / 4096)
-static_assert(MEM_SIZE_MAP % 4096 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE % 4096 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE_PAGES % 2 == 0, "invalid map test region size");
-static_assert(MEM_TEST_MAP_SIZE_PAGES > 2, "invalid map test region size");
/*
* 128 MiB is min size that fills 32k slots with at least one page in each
* while at the same time gets 100+ iterations in such test
+ *
+ * 2 MiB chunk size like a typical huge page
*/
#define MEM_TEST_UNMAP_SIZE (128U << 20)
-#define MEM_TEST_UNMAP_SIZE_PAGES (MEM_TEST_UNMAP_SIZE / 4096)
-/* 2 MiB chunk size like a typical huge page */
-#define MEM_TEST_UNMAP_CHUNK_PAGES (2U << (20 - 12))
-static_assert(MEM_TEST_UNMAP_SIZE <= MEM_TEST_SIZE,
- "invalid unmap test region size");
-static_assert(MEM_TEST_UNMAP_SIZE % 4096 == 0,
- "invalid unmap test region size");
-static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
- (2 * MEM_TEST_UNMAP_CHUNK_PAGES) == 0,
- "invalid unmap test region size");
+#define MEM_TEST_UNMAP_CHUNK_SIZE (2U << 20)
/*
* For the move active test the middle of the test area is placed on
@@ -77,8 +61,7 @@ static_assert(MEM_TEST_UNMAP_SIZE_PAGES %
* for the total size of 25 pages.
* Hence, the maximum size here is 50 pages.
*/
-#define MEM_TEST_MOVE_SIZE_PAGES (50)
-#define MEM_TEST_MOVE_SIZE (MEM_TEST_MOVE_SIZE_PAGES * 4096)
+#define MEM_TEST_MOVE_SIZE 0x32000
The above number seems less readable than an explicit value of 50 pages.
In addition to that, it's 50 pages only with 4k page size, so at least
the comment above needs to be updated to reflect this fact.
Yeah, I will change the comments like below in next revision.
/*
* When running this test with 32k memslots, actually 32763 excluding
* the reserved memory slot 0, the memory for each slot is 0x4000 bytes.
* The last slot contains 0x19000 bytes memory. Hence, the maximum size
* here is 0x32000 bytes.
*/
#define MEM_TEST_MOVE_GPA_DEST (MEM_GPA + MEM_SIZE)
static_assert(MEM_TEST_MOVE_SIZE <= MEM_TEST_SIZE,
"invalid move test region size");
(...)
@@ -242,33 +229,34 @@ static struct vm_data *alloc_vm(void)
}
static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
- void *guest_code, uint64_t mempages,
+ void *guest_code, uint64_t mem_size,
struct timespec *slot_runtime)
{
- uint64_t rempages;
+ uint64_t mempages, rempages;
uint64_t guest_addr;
- uint32_t slot;
+ uint32_t slot, guest_page_size;
struct timespec tstart;
struct sync_area *sync;
- TEST_ASSERT(mempages > 1,
- "Can't test without any memory");
+ guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
+ mempages = mem_size / guest_page_size;
+
+ data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
+ ucall_init(data->vm, NULL);
TEST_ASSERT(data->vm->page_size == guest_page_size, "Invalid VM page size")
here would catch the case if someone accidentally modifies
__vm_create_with_one_vcpu() to use other page size than specified for
VM_MODE_DEFAULT.
Sure, it's not harmful at least.
data->npages = mempages;
+ TEST_ASSERT(data->npages > 1, "Can't test without any memory");
data->nslots = nslots;
- data->pages_per_slot = mempages / data->nslots;
+ data->pages_per_slot = data->npages / data->nslots;
if (!data->pages_per_slot) {
- *maxslots = mempages + 1;
+ *maxslots = data->npages + 1;
return false;
}
- rempages = mempages % data->nslots;
+ rempages = data->npages % data->nslots;
data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
TEST_ASSERT(data->hva_slots, "malloc() fail");
- data->vm = __vm_create_with_one_vcpu(&data->vcpu, mempages, guest_code);
- ucall_init(data->vm, NULL);
-
pr_info_v("Adding slots 1..%i, each slot with %"PRIu64" pages + %"PRIu64" extra pages last\n",
data->nslots, data->pages_per_slot, rempages);
(...)
@@ -856,6 +863,35 @@ static void help(char *name, struct test_args *targs)
pr_info("%d: %s\n", ctr, tests[ctr].name);
}
+static bool check_memory_sizes(void)
+{
+ uint32_t guest_page_size = vm_guest_mode_params[VM_MODE_DEFAULT].page_size;
+
+ if (MEM_SIZE % guest_page_size ||
+ MEM_TEST_SIZE % guest_page_size) {
+ pr_info("invalid MEM_SIZE or MEM_TEST_SIZE\n");
+ return false;
+ }
+
+ if (MEM_SIZE_MAP % guest_page_size ||
+ MEM_TEST_MAP_SIZE % guest_page_size ||
+ (MEM_TEST_MAP_SIZE / guest_page_size) <= 2 ||
+ (MEM_TEST_MAP_SIZE / guest_page_size) % 2) {
+ pr_info("invalid MEM_SIZE_MAP or MEM_TEST_MAP_SIZE\n");
+ return false;
+ }
+
+ if (MEM_TEST_UNMAP_SIZE > MEM_TEST_SIZE ||
+ MEM_TEST_UNMAP_SIZE % guest_page_size ||
+ (MEM_TEST_UNMAP_SIZE / guest_page_size) %
+ (MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size)) {
This should be (MEM_TEST_UNMAP_SIZE / guest_page_size) % (2 * MEM_TEST_UNMAP_CHUNK_SIZE / guest_page_size))
to match the old static_assert().
Nice catch! I will fix it up in next revision :)
Thanks,
Gavin