2021-05-19 20:24:33

by Axel Rasmussen

Subject: [PATCH v2 00/10] KVM: selftests: exercise userfaultfd minor faults

Base
====

These patches are based upon Andrew Morton's v5.13-rc1-mmots-2021-05-13-17-23
tag. This is because this series depends on:

- UFFD minor fault support for hugetlbfs (in v5.13-rc1) [1]
- UFFD minor fault support for shmem (in Andrew's tree) [2]

[1] https://lore.kernel.org/linux-fsdevel/[email protected]/
[2] https://lore.kernel.org/patchwork/cover/1420967/

Changelog
=========

v1->v2:
- Picked up Reviewed-by's.
- Change backing_src_is_shared() to check the flags, instead of the type. This
makes it robust to adding new backing source types in the future.
- Add another commit which refactors setup_demand_paging() error handling.
- Print UFFD ioctl type once in setup_demand_paging, instead of on every page-in
operation.
- Expand comment on why we use MFD_HUGETLB instead of MAP_HUGETLB.
- Reworded comment on addr_gpa2alias.
- Moved demand_paging_test.c timing calls outside of the if (), deduplicating
them.
- Split trivial comment / logging fixups into a separate commit.
- Add another commit which prints a clarifying message on test skip.
- Split the commit allowing backing src_type to be modified in two.
- Split the commit adding the shmem backing type in two.
- Rebased onto v5.13-rc1-mmots-2021-05-13-17-23.

Overview
========

Minor fault handling is a new userfaultfd feature whose goal is generally to
improve performance. In particular, it is intended for use with demand paging.
There are more details in the cover letters for this new feature (linked above),
but at a high level the idea is that we think of these three phases of live
migration of a VM:

1. Precopy, where we copy "some" pages from the source to the target, while the
VM is still running on the source machine.
2. Blackout, where execution stops on the source, and begins on the target.
3. Postcopy, where the VM is running on the target, some pages are already up
to date, and others are not (because they weren't copied, or were modified
after being copied).

During postcopy, the first time the guest touches memory, we intercept a minor
fault. Userspace checks whether or not the page is already up to date. If
needed, we copy the final version of the page from the soure machine. This
could be done with RDMA for example, to do it truly in place / with no copying.
At this point, all that's left is to setup PTEs for the guest: so we issue
UFFDIO_CONTINUE. No copying or page allocation needed.

Because of this use case, it's useful to exercise this as part of the demand
paging test. It lets us ensure the use case works correctly end-to-end, and also
gives us an in-tree way to profile the end-to-end flow for future performance
improvements.

Axel Rasmussen (10):
KVM: selftests: trivial comment/logging fixes
KVM: selftests: simplify setup_demand_paging error handling
KVM: selftests: print a message when skipping KVM tests
KVM: selftests: compute correct demand paging size
KVM: selftests: allow different backing source types
KVM: selftests: refactor vm_mem_backing_src_type flags
KVM: selftests: add shmem backing source type
KVM: selftests: create alias mappings when using shared memory
KVM: selftests: allow using UFFD minor faults for demand paging
KVM: selftests: add shared hugetlbfs backing source type

.../selftests/kvm/demand_paging_test.c | 175 +++++++++++-------
.../testing/selftests/kvm/include/kvm_util.h | 1 +
.../testing/selftests/kvm/include/test_util.h | 12 ++
tools/testing/selftests/kvm/lib/kvm_util.c | 84 ++++++++-
.../selftests/kvm/lib/kvm_util_internal.h | 2 +
tools/testing/selftests/kvm/lib/test_util.c | 51 +++--
6 files changed, 238 insertions(+), 87 deletions(-)

--
2.31.1.751.gd2f1c929bd-goog

2021-05-19 20:24:52

by Axel Rasmussen

Subject: [PATCH v2 10/10] KVM: selftests: add shared hugetlbfs backing source type

This lets us run the demand paging test on top of a shared
hugetlbfs-backed area. The "shared" is key, as this allows us to
exercise userfaultfd minor faults on hugetlbfs.

Signed-off-by: Axel Rasmussen <[email protected]>
---
tools/testing/selftests/kvm/demand_paging_test.c | 6 ++++--
tools/testing/selftests/kvm/include/test_util.h | 11 +++++++++++
tools/testing/selftests/kvm/lib/kvm_util.c | 9 +++++++--
tools/testing/selftests/kvm/lib/test_util.c | 11 +++++++++++
4 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
index df7190261923..60d9b5223b9d 100644
--- a/tools/testing/selftests/kvm/demand_paging_test.c
+++ b/tools/testing/selftests/kvm/demand_paging_test.c
@@ -485,8 +485,10 @@ int main(int argc, char *argv[])
}
}

- TEST_ASSERT(p.uffd_mode != UFFDIO_REGISTER_MODE_MINOR || p.src_type == VM_MEM_SRC_SHMEM,
- "userfaultfd MINOR mode requires shared memory; pick a different -t");
+ if (p.uffd_mode == UFFDIO_REGISTER_MODE_MINOR &&
+ !backing_src_is_shared(p.src_type)) {
+ TEST_FAIL("userfaultfd MINOR mode requires shared memory; pick a different -t");
+ }

for_each_guest_mode(run_test, &p);

diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
index 7377f00469ef..d79be15dd3d2 100644
--- a/tools/testing/selftests/kvm/include/test_util.h
+++ b/tools/testing/selftests/kvm/include/test_util.h
@@ -17,6 +17,7 @@
#include <errno.h>
#include <unistd.h>
#include <fcntl.h>
+#include <sys/mman.h>
#include "kselftest.h"

static inline int _no_printf(const char *format, ...) { return 0; }
@@ -85,6 +86,7 @@ enum vm_mem_backing_src_type {
VM_MEM_SRC_ANONYMOUS_HUGETLB_2GB,
VM_MEM_SRC_ANONYMOUS_HUGETLB_16GB,
VM_MEM_SRC_SHMEM,
+ VM_MEM_SRC_SHARED_HUGETLB,
NUM_SRC_TYPES,
};

@@ -101,4 +103,13 @@ size_t get_backing_src_pagesz(uint32_t i);
void backing_src_help(void);
enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name);

+/*
+ * Whether or not the given source type is shared memory (as opposed to
+ * anonymous).
+ */
+static inline bool backing_src_is_shared(enum vm_mem_backing_src_type t)
+{
+ return vm_mem_backing_src_alias(t)->flag & MAP_SHARED;
+}
+
#endif /* SELFTEST_KVM_TEST_UTIL_H */
diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index 0b88d1bbc1e0..8373aec1fb02 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -758,8 +758,13 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
region->mmap_size += alignment;

region->fd = -1;
- if (src_type == VM_MEM_SRC_SHMEM) {
- region->fd = memfd_create("kvm_selftest", MFD_CLOEXEC);
+ if (backing_src_is_shared(src_type)) {
+ int memfd_flags = MFD_CLOEXEC;
+
+ if (src_type == VM_MEM_SRC_SHARED_HUGETLB)
+ memfd_flags |= MFD_HUGETLB;
+
+ region->fd = memfd_create("kvm_selftest", memfd_flags);
TEST_ASSERT(region->fd != -1,
"memfd_create failed, errno: %i", errno);

diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index c7a265da5090..6ad6c8276b2e 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -240,6 +240,16 @@ const struct vm_mem_backing_src_alias *vm_mem_backing_src_alias(uint32_t i)
.name = "shmem",
.flag = MAP_SHARED,
},
+ [VM_MEM_SRC_SHARED_HUGETLB] = {
+ .name = "shared_hugetlb",
+ /*
+ * No MAP_HUGETLB, we use MFD_HUGETLB instead. Since
+ * we're using "file backed" memory, we need to specify
+ * this when the FD is created, not when the area is
+ * mapped.
+ */
+ .flag = MAP_SHARED,
+ },
};
_Static_assert(ARRAY_SIZE(aliases) == NUM_SRC_TYPES,
"Missing new backing src types?");
@@ -262,6 +272,7 @@ size_t get_backing_src_pagesz(uint32_t i)
case VM_MEM_SRC_ANONYMOUS_THP:
return get_trans_hugepagesz();
case VM_MEM_SRC_ANONYMOUS_HUGETLB:
+ case VM_MEM_SRC_SHARED_HUGETLB:
return get_def_hugetlb_pagesz();
default:
return MAP_HUGE_PAGE_SIZE(flag);
--
2.31.1.751.gd2f1c929bd-goog

2021-05-19 20:25:08

by Axel Rasmussen

Subject: [PATCH v2 01/10] KVM: selftests: trivial comment/logging fixes

Some trivial fixes I found while touching related code in this series,
factored out into a separate commit for easier reviewing:

- s/gor/got/ and add a newline in demand_paging_test.c
- s/backing_src/src_type/ in a comment to be consistent with the real
function signature in kvm_util.c

Signed-off-by: Axel Rasmussen <[email protected]>
---
tools/testing/selftests/kvm/demand_paging_test.c | 2 +-
tools/testing/selftests/kvm/lib/kvm_util.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
index 5f7a229c3af1..9398ba6ef023 100644
--- a/tools/testing/selftests/kvm/demand_paging_test.c
+++ b/tools/testing/selftests/kvm/demand_paging_test.c
@@ -169,7 +169,7 @@ static void *uffd_handler_thread_fn(void *arg)
if (r == -1) {
if (errno == EAGAIN)
continue;
- pr_info("Read of uffd gor errno %d", errno);
+ pr_info("Read of uffd got errno %d\n", errno);
return NULL;
}

diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
index fc83f6c5902d..f05ca919cccb 100644
--- a/tools/testing/selftests/kvm/lib/kvm_util.c
+++ b/tools/testing/selftests/kvm/lib/kvm_util.c
@@ -663,8 +663,8 @@ int kvm_memcmp_hva_gva(void *hva, struct kvm_vm *vm, vm_vaddr_t gva, size_t len)
*
* Input Args:
* vm - Virtual Machine
- * backing_src - Storage source for this region.
- * NULL to use anonymous memory.
+ * src_type - Storage source for this region.
+ * NULL to use anonymous memory.
* guest_paddr - Starting guest physical address
* slot - KVM region slot
* npages - Number of physical pages
--
2.31.1.751.gd2f1c929bd-goog

2021-05-19 20:26:02

by Axel Rasmussen

Subject: [PATCH v2 02/10] KVM: selftests: simplify setup_demand_paging error handling

A small cleanup. Our caller writes:

r = setup_demand_paging(...);
if (r < 0) exit(-r);

Since we're just going to exit anyway, instead of returning an error we
can just re-use TEST_ASSERT. This makes the caller simpler, as well as
the function itself - no need to write our branches, etc.

Signed-off-by: Axel Rasmussen <[email protected]>
---
.../selftests/kvm/demand_paging_test.c | 51 +++++++------------
1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
index 9398ba6ef023..601a1df24dd2 100644
--- a/tools/testing/selftests/kvm/demand_paging_test.c
+++ b/tools/testing/selftests/kvm/demand_paging_test.c
@@ -9,6 +9,8 @@

#define _GNU_SOURCE /* for pipe2 */

+#include <inttypes.h>
+#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <time.h>
@@ -198,42 +200,32 @@ static void *uffd_handler_thread_fn(void *arg)
return NULL;
}

-static int setup_demand_paging(struct kvm_vm *vm,
- pthread_t *uffd_handler_thread, int pipefd,
- useconds_t uffd_delay,
- struct uffd_handler_args *uffd_args,
- void *hva, uint64_t len)
+static void setup_demand_paging(struct kvm_vm *vm,
+ pthread_t *uffd_handler_thread, int pipefd,
+ useconds_t uffd_delay,
+ struct uffd_handler_args *uffd_args,
+ void *hva, uint64_t len)
{
int uffd;
struct uffdio_api uffdio_api;
struct uffdio_register uffdio_register;

uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
- if (uffd == -1) {
- pr_info("uffd creation failed\n");
- return -1;
- }
+ TEST_ASSERT(uffd >= 0, "uffd creation failed, errno: %d", errno);

uffdio_api.api = UFFD_API;
uffdio_api.features = 0;
- if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1) {
- pr_info("ioctl uffdio_api failed\n");
- return -1;
- }
+ TEST_ASSERT(ioctl(uffd, UFFDIO_API, &uffdio_api) != -1,
+ "ioctl UFFDIO_API failed: %" PRIu64,
+ (uint64_t)uffdio_api.api);

uffdio_register.range.start = (uint64_t)hva;
uffdio_register.range.len = len;
uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
- pr_info("ioctl uffdio_register failed\n");
- return -1;
- }
-
- if ((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) !=
- UFFD_API_RANGE_IOCTLS) {
- pr_info("unexpected userfaultfd ioctl set\n");
- return -1;
- }
+ TEST_ASSERT(ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) != -1,
+ "ioctl UFFDIO_REGISTER failed");
+ TEST_ASSERT((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) ==
+ UFFD_API_RANGE_IOCTLS, "unexpected userfaultfd ioctl set");

uffd_args->uffd = uffd;
uffd_args->pipefd = pipefd;
@@ -243,8 +235,6 @@ static int setup_demand_paging(struct kvm_vm *vm,

PER_VCPU_DEBUG("Created uffd thread for HVA range [%p, %p)\n",
hva, hva + len);
-
- return 0;
}

struct test_params {
@@ -321,13 +311,10 @@ static void run_test(enum vm_guest_mode mode, void *arg)
O_CLOEXEC | O_NONBLOCK);
TEST_ASSERT(!r, "Failed to set up pipefd");

- r = setup_demand_paging(vm,
- &uffd_handler_threads[vcpu_id],
- pipefds[vcpu_id * 2],
- p->uffd_delay, &uffd_args[vcpu_id],
- vcpu_hva, vcpu_mem_size);
- if (r < 0)
- exit(-r);
+ setup_demand_paging(vm, &uffd_handler_threads[vcpu_id],
+ pipefds[vcpu_id * 2], p->uffd_delay,
+ &uffd_args[vcpu_id], vcpu_hva,
+ vcpu_mem_size);
}
}

--
2.31.1.751.gd2f1c929bd-goog

2021-05-19 20:26:05

by Axel Rasmussen

Subject: [PATCH v2 05/10] KVM: selftests: allow different backing source types

Add an argument which lets us specify a different backing memory type
for the test. The default is just to use anonymous, matching existing
behavior.

This is in preparation for testing UFFD minor faults. For that, we'll
need to use a new backing memory type which is setup with MAP_SHARED.

Signed-off-by: Axel Rasmussen <[email protected]>
---
tools/testing/selftests/kvm/demand_paging_test.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
index 94cf047358d5..01890a7b0155 100644
--- a/tools/testing/selftests/kvm/demand_paging_test.c
+++ b/tools/testing/selftests/kvm/demand_paging_test.c
@@ -241,6 +241,7 @@ static void setup_demand_paging(struct kvm_vm *vm,
struct test_params {
bool use_uffd;
useconds_t uffd_delay;
+ enum vm_mem_backing_src_type src_type;
bool partition_vcpu_memory_access;
};

@@ -258,11 +259,11 @@ static void run_test(enum vm_guest_mode mode, void *arg)
int r;

vm = perf_test_create_vm(mode, nr_vcpus, guest_percpu_mem_size,
- VM_MEM_SRC_ANONYMOUS);
+ p->src_type);

perf_test_args.wr_fract = 1;

- demand_paging_size = get_backing_src_pagesz(VM_MEM_SRC_ANONYMOUS);
+ demand_paging_size = get_backing_src_pagesz(p->src_type);

guest_data_prototype = malloc(demand_paging_size);
TEST_ASSERT(guest_data_prototype,
@@ -378,7 +379,7 @@ static void help(char *name)
{
puts("");
printf("usage: %s [-h] [-m mode] [-u] [-d uffd_delay_usec]\n"
- " [-b memory] [-v vcpus] [-o]\n", name);
+ " [-b memory] [-t type] [-v vcpus] [-o]\n", name);
guest_modes_help();
printf(" -u: use User Fault FD to handle vCPU page\n"
" faults.\n");
@@ -388,6 +389,8 @@ static void help(char *name)
printf(" -b: specify the size of the memory region which should be\n"
" demand paged by each vCPU. e.g. 10M or 3G.\n"
" Default: 1G\n");
+ printf(" -t: The type of backing memory to use. Default: anonymous\n");
+ backing_src_help();
printf(" -v: specify the number of vCPUs to run.\n");
printf(" -o: Overlap guest memory accesses instead of partitioning\n"
" them into a separate region of memory for each vCPU.\n");
@@ -399,13 +402,14 @@ int main(int argc, char *argv[])
{
int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS);
struct test_params p = {
+ .src_type = VM_MEM_SRC_ANONYMOUS,
.partition_vcpu_memory_access = true,
};
int opt;

guest_modes_append_default();

- while ((opt = getopt(argc, argv, "hm:ud:b:v:o")) != -1) {
+ while ((opt = getopt(argc, argv, "hm:ud:b:t:v:o")) != -1) {
switch (opt) {
case 'm':
guest_modes_cmdline(optarg);
@@ -420,6 +424,9 @@ int main(int argc, char *argv[])
case 'b':
guest_percpu_mem_size = parse_size(optarg);
break;
+ case 't':
+ p.src_type = parse_backing_src_type(optarg);
+ break;
case 'v':
nr_vcpus = atoi(optarg);
TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
--
2.31.1.751.gd2f1c929bd-goog

2021-05-19 21:43:21

by Ben Gardon

Subject: Re: [PATCH v2 01/10] KVM: selftests: trivial comment/logging fixes

On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <[email protected]> wrote:
>
> Some trivial fixes I found while touching related code in this series,
> factored out into a separate commit for easier reviewing:
>
> - s/gor/got/ and add a newline in demand_paging_test.c
> - s/backing_src/src_type/ in a comment to be consistent with the real
> function signature in kvm_util.c
>
> Signed-off-by: Axel Rasmussen <[email protected]>

Reviewed-by: Ben Gardon <[email protected]>

Thanks for doing this!

> ---
> tools/testing/selftests/kvm/demand_paging_test.c | 2 +-
> tools/testing/selftests/kvm/lib/kvm_util.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> index 5f7a229c3af1..9398ba6ef023 100644
> --- a/tools/testing/selftests/kvm/demand_paging_test.c
> +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> @@ -169,7 +169,7 @@ static void *uffd_handler_thread_fn(void *arg)
> if (r == -1) {
> if (errno == EAGAIN)
> continue;
> - pr_info("Read of uffd gor errno %d", errno);
> + pr_info("Read of uffd got errno %d\n", errno);
> return NULL;
> }
>
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index fc83f6c5902d..f05ca919cccb 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -663,8 +663,8 @@ int kvm_memcmp_hva_gva(void *hva, struct kvm_vm *vm, vm_vaddr_t gva, size_t len)
> *
> * Input Args:
> * vm - Virtual Machine
> - * backing_src - Storage source for this region.
> - * NULL to use anonymous memory.
> + * src_type - Storage source for this region.
> + * NULL to use anonymous memory.
> * guest_paddr - Starting guest physical address
> * slot - KVM region slot
> * npages - Number of physical pages
> --
> 2.31.1.751.gd2f1c929bd-goog
>

2021-05-19 21:47:29

by Ben Gardon

Subject: Re: [PATCH v2 02/10] KVM: selftests: simplify setup_demand_paging error handling

On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <[email protected]> wrote:
>
> A small cleanup. Our caller writes:
>
> r = setup_demand_paging(...);
> if (r < 0) exit(-r);
>
> Since we're just going to exit anyway, instead of returning an error we
> can just re-use TEST_ASSERT. This makes the caller simpler, as well as
> the function itself - no need to write our branches, etc.
>
> Signed-off-by: Axel Rasmussen <[email protected]>
> ---
> .../selftests/kvm/demand_paging_test.c | 51 +++++++------------
> 1 file changed, 19 insertions(+), 32 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> index 9398ba6ef023..601a1df24dd2 100644
> --- a/tools/testing/selftests/kvm/demand_paging_test.c
> +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> @@ -9,6 +9,8 @@
>
> #define _GNU_SOURCE /* for pipe2 */
>
> +#include <inttypes.h>
> +#include <stdint.h>

Why do the includes need to change in this commit? Is it for the PRIu64 below?

> #include <stdio.h>
> #include <stdlib.h>
> #include <time.h>
> @@ -198,42 +200,32 @@ static void *uffd_handler_thread_fn(void *arg)
> return NULL;
> }
>
> -static int setup_demand_paging(struct kvm_vm *vm,
> - pthread_t *uffd_handler_thread, int pipefd,
> - useconds_t uffd_delay,
> - struct uffd_handler_args *uffd_args,
> - void *hva, uint64_t len)
> +static void setup_demand_paging(struct kvm_vm *vm,
> + pthread_t *uffd_handler_thread, int pipefd,
> + useconds_t uffd_delay,
> + struct uffd_handler_args *uffd_args,
> + void *hva, uint64_t len)
> {
> int uffd;
> struct uffdio_api uffdio_api;
> struct uffdio_register uffdio_register;
>
> uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> - if (uffd == -1) {
> - pr_info("uffd creation failed\n");
> - return -1;
> - }
> + TEST_ASSERT(uffd >= 0, "uffd creation failed, errno: %d", errno);
>
> uffdio_api.api = UFFD_API;
> uffdio_api.features = 0;
> - if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1) {
> - pr_info("ioctl uffdio_api failed\n");
> - return -1;
> - }
> + TEST_ASSERT(ioctl(uffd, UFFDIO_API, &uffdio_api) != -1,
> + "ioctl UFFDIO_API failed: %" PRIu64,
> + (uint64_t)uffdio_api.api);
>
> uffdio_register.range.start = (uint64_t)hva;
> uffdio_register.range.len = len;
> uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> - if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> - pr_info("ioctl uffdio_register failed\n");
> - return -1;
> - }
> -
> - if ((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) !=
> - UFFD_API_RANGE_IOCTLS) {
> - pr_info("unexpected userfaultfd ioctl set\n");
> - return -1;
> - }
> + TEST_ASSERT(ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) != -1,
> + "ioctl UFFDIO_REGISTER failed");
> + TEST_ASSERT((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) ==
> + UFFD_API_RANGE_IOCTLS, "unexpected userfaultfd ioctl set");
>
> uffd_args->uffd = uffd;
> uffd_args->pipefd = pipefd;
> @@ -243,8 +235,6 @@ static int setup_demand_paging(struct kvm_vm *vm,
>
> PER_VCPU_DEBUG("Created uffd thread for HVA range [%p, %p)\n",
> hva, hva + len);
> -
> - return 0;
> }
>
> struct test_params {
> @@ -321,13 +311,10 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> O_CLOEXEC | O_NONBLOCK);
> TEST_ASSERT(!r, "Failed to set up pipefd");
>
> - r = setup_demand_paging(vm,
> - &uffd_handler_threads[vcpu_id],
> - pipefds[vcpu_id * 2],
> - p->uffd_delay, &uffd_args[vcpu_id],
> - vcpu_hva, vcpu_mem_size);
> - if (r < 0)
> - exit(-r);
> + setup_demand_paging(vm, &uffd_handler_threads[vcpu_id],
> + pipefds[vcpu_id * 2], p->uffd_delay,
> + &uffd_args[vcpu_id], vcpu_hva,
> + vcpu_mem_size);
> }
> }
>
> --
> 2.31.1.751.gd2f1c929bd-goog
>

2021-05-19 21:56:51

by Ben Gardon

Subject: Re: [PATCH v2 05/10] KVM: selftests: allow different backing source types

On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <[email protected]> wrote:
>
> Add an argument which lets us specify a different backing memory type
> for the test. The default is just to use anonymous, matching existing
> behavior.
>
> This is in preparation for testing UFFD minor faults. For that, we'll
> need to use a new backing memory type which is setup with MAP_SHARED.
>
> Signed-off-by: Axel Rasmussen <[email protected]>

Reviewed-by: Ben Gardon <[email protected]>

> ---
> tools/testing/selftests/kvm/demand_paging_test.c | 15 +++++++++++----
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> index 94cf047358d5..01890a7b0155 100644
> --- a/tools/testing/selftests/kvm/demand_paging_test.c
> +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> @@ -241,6 +241,7 @@ static void setup_demand_paging(struct kvm_vm *vm,
> struct test_params {
> bool use_uffd;
> useconds_t uffd_delay;
> + enum vm_mem_backing_src_type src_type;
> bool partition_vcpu_memory_access;
> };
>
> @@ -258,11 +259,11 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> int r;
>
> vm = perf_test_create_vm(mode, nr_vcpus, guest_percpu_mem_size,
> - VM_MEM_SRC_ANONYMOUS);
> + p->src_type);
>
> perf_test_args.wr_fract = 1;
>
> - demand_paging_size = get_backing_src_pagesz(VM_MEM_SRC_ANONYMOUS);
> + demand_paging_size = get_backing_src_pagesz(p->src_type);
>
> guest_data_prototype = malloc(demand_paging_size);
> TEST_ASSERT(guest_data_prototype,
> @@ -378,7 +379,7 @@ static void help(char *name)
> {
> puts("");
> printf("usage: %s [-h] [-m mode] [-u] [-d uffd_delay_usec]\n"
> - " [-b memory] [-v vcpus] [-o]\n", name);
> + " [-b memory] [-t type] [-v vcpus] [-o]\n", name);
> guest_modes_help();
> printf(" -u: use User Fault FD to handle vCPU page\n"
> " faults.\n");
> @@ -388,6 +389,8 @@ static void help(char *name)
> printf(" -b: specify the size of the memory region which should be\n"
> " demand paged by each vCPU. e.g. 10M or 3G.\n"
> " Default: 1G\n");
> + printf(" -t: The type of backing memory to use. Default: anonymous\n");
> + backing_src_help();
> printf(" -v: specify the number of vCPUs to run.\n");
> printf(" -o: Overlap guest memory accesses instead of partitioning\n"
> " them into a separate region of memory for each vCPU.\n");
> @@ -399,13 +402,14 @@ int main(int argc, char *argv[])
> {
> int max_vcpus = kvm_check_cap(KVM_CAP_MAX_VCPUS);
> struct test_params p = {
> + .src_type = VM_MEM_SRC_ANONYMOUS,
> .partition_vcpu_memory_access = true,
> };
> int opt;
>
> guest_modes_append_default();
>
> - while ((opt = getopt(argc, argv, "hm:ud:b:v:o")) != -1) {
> + while ((opt = getopt(argc, argv, "hm:ud:b:t:v:o")) != -1) {
> switch (opt) {
> case 'm':
> guest_modes_cmdline(optarg);
> @@ -420,6 +424,9 @@ int main(int argc, char *argv[])
> case 'b':
> guest_percpu_mem_size = parse_size(optarg);
> break;
> + case 't':
> + p.src_type = parse_backing_src_type(optarg);
> + break;
> case 'v':
> nr_vcpus = atoi(optarg);
> TEST_ASSERT(nr_vcpus > 0 && nr_vcpus <= max_vcpus,
> --
> 2.31.1.751.gd2f1c929bd-goog
>

2021-05-19 22:15:47

by Axel Rasmussen

Subject: Re: [PATCH v2 02/10] KVM: selftests: simplify setup_demand_paging error handling

On Wed, May 19, 2021 at 2:45 PM Ben Gardon <[email protected]> wrote:
>
> On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <[email protected]> wrote:
> >
> > A small cleanup. Our caller writes:
> >
> > r = setup_demand_paging(...);
> > if (r < 0) exit(-r);
> >
> > Since we're just going to exit anyway, instead of returning an error we
> > can just re-use TEST_ASSERT. This makes the caller simpler, as well as
> > the function itself - no need to write our branches, etc.
> >
> > Signed-off-by: Axel Rasmussen <[email protected]>
> > ---
> > .../selftests/kvm/demand_paging_test.c | 51 +++++++------------
> > 1 file changed, 19 insertions(+), 32 deletions(-)
> >
> > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> > index 9398ba6ef023..601a1df24dd2 100644
> > --- a/tools/testing/selftests/kvm/demand_paging_test.c
> > +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> > @@ -9,6 +9,8 @@
> >
> > #define _GNU_SOURCE /* for pipe2 */
> >
> > +#include <inttypes.h>
> > +#include <stdint.h>
>
> Why do the includes need to change in this commit? Is it for the PRIu64 below?

Right, I didn't actually try compiling without these, but inttypes.h
defines PRIu64 and stdint.h defines uint64_t. In general I tend to
prefer including things like this because we're using their
definitions directly, even if we might be picking them up transiently
some other way.

>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <time.h>
> > @@ -198,42 +200,32 @@ static void *uffd_handler_thread_fn(void *arg)
> > return NULL;
> > }
> >
> > -static int setup_demand_paging(struct kvm_vm *vm,
> > - pthread_t *uffd_handler_thread, int pipefd,
> > - useconds_t uffd_delay,
> > - struct uffd_handler_args *uffd_args,
> > - void *hva, uint64_t len)
> > +static void setup_demand_paging(struct kvm_vm *vm,
> > + pthread_t *uffd_handler_thread, int pipefd,
> > + useconds_t uffd_delay,
> > + struct uffd_handler_args *uffd_args,
> > + void *hva, uint64_t len)
> > {
> > int uffd;
> > struct uffdio_api uffdio_api;
> > struct uffdio_register uffdio_register;
> >
> > uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> > - if (uffd == -1) {
> > - pr_info("uffd creation failed\n");
> > - return -1;
> > - }
> > + TEST_ASSERT(uffd >= 0, "uffd creation failed, errno: %d", errno);
> >
> > uffdio_api.api = UFFD_API;
> > uffdio_api.features = 0;
> > - if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1) {
> > - pr_info("ioctl uffdio_api failed\n");
> > - return -1;
> > - }
> > + TEST_ASSERT(ioctl(uffd, UFFDIO_API, &uffdio_api) != -1,
> > + "ioctl UFFDIO_API failed: %" PRIu64,
> > + (uint64_t)uffdio_api.api);
> >
> > uffdio_register.range.start = (uint64_t)hva;
> > uffdio_register.range.len = len;
> > uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> > - if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> > - pr_info("ioctl uffdio_register failed\n");
> > - return -1;
> > - }
> > -
> > - if ((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) !=
> > - UFFD_API_RANGE_IOCTLS) {
> > - pr_info("unexpected userfaultfd ioctl set\n");
> > - return -1;
> > - }
> > + TEST_ASSERT(ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) != -1,
> > + "ioctl UFFDIO_REGISTER failed");
> > + TEST_ASSERT((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) ==
> > + UFFD_API_RANGE_IOCTLS, "unexpected userfaultfd ioctl set");
> >
> > uffd_args->uffd = uffd;
> > uffd_args->pipefd = pipefd;
> > @@ -243,8 +235,6 @@ static int setup_demand_paging(struct kvm_vm *vm,
> >
> > PER_VCPU_DEBUG("Created uffd thread for HVA range [%p, %p)\n",
> > hva, hva + len);
> > -
> > - return 0;
> > }
> >
> > struct test_params {
> > @@ -321,13 +311,10 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> > O_CLOEXEC | O_NONBLOCK);
> > TEST_ASSERT(!r, "Failed to set up pipefd");
> >
> > - r = setup_demand_paging(vm,
> > - &uffd_handler_threads[vcpu_id],
> > - pipefds[vcpu_id * 2],
> > - p->uffd_delay, &uffd_args[vcpu_id],
> > - vcpu_hva, vcpu_mem_size);
> > - if (r < 0)
> > - exit(-r);
> > + setup_demand_paging(vm, &uffd_handler_threads[vcpu_id],
> > + pipefds[vcpu_id * 2], p->uffd_delay,
> > + &uffd_args[vcpu_id], vcpu_hva,
> > + vcpu_mem_size);
> > }
> > }
> >
> > --
> > 2.31.1.751.gd2f1c929bd-goog
> >

2021-05-19 22:24:16

by Ben Gardon

Subject: Re: [PATCH v2 10/10] KVM: selftests: add shared hugetlbfs backing source type

On Wed, May 19, 2021 at 1:04 PM Axel Rasmussen <[email protected]> wrote:
>
> This lets us run the demand paging test on top of a shared
> hugetlbfs-backed area. The "shared" is key, as this allows us to
> exercise userfaultfd minor faults on hugetlbfs.
>
> Signed-off-by: Axel Rasmussen <[email protected]>

Reviewed-by: Ben Gardon <[email protected]>

> ---
> tools/testing/selftests/kvm/demand_paging_test.c | 6 ++++--
> tools/testing/selftests/kvm/include/test_util.h | 11 +++++++++++
> tools/testing/selftests/kvm/lib/kvm_util.c | 9 +++++++--
> tools/testing/selftests/kvm/lib/test_util.c | 11 +++++++++++
> 4 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> index df7190261923..60d9b5223b9d 100644
> --- a/tools/testing/selftests/kvm/demand_paging_test.c
> +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> @@ -485,8 +485,10 @@ int main(int argc, char *argv[])
> }
> }
>
> - TEST_ASSERT(p.uffd_mode != UFFDIO_REGISTER_MODE_MINOR || p.src_type == VM_MEM_SRC_SHMEM,
> - "userfaultfd MINOR mode requires shared memory; pick a different -t");
> + if (p.uffd_mode == UFFDIO_REGISTER_MODE_MINOR &&
> + !backing_src_is_shared(p.src_type)) {
> + TEST_FAIL("userfaultfd MINOR mode requires shared memory; pick a different -t");
> + }
>
> for_each_guest_mode(run_test, &p);
>
> diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
> index 7377f00469ef..d79be15dd3d2 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -17,6 +17,7 @@
> #include <errno.h>
> #include <unistd.h>
> #include <fcntl.h>
> +#include <sys/mman.h>
> #include "kselftest.h"
>
> static inline int _no_printf(const char *format, ...) { return 0; }
> @@ -85,6 +86,7 @@ enum vm_mem_backing_src_type {
> VM_MEM_SRC_ANONYMOUS_HUGETLB_2GB,
> VM_MEM_SRC_ANONYMOUS_HUGETLB_16GB,
> VM_MEM_SRC_SHMEM,
> + VM_MEM_SRC_SHARED_HUGETLB,
> NUM_SRC_TYPES,
> };
>
> @@ -101,4 +103,13 @@ size_t get_backing_src_pagesz(uint32_t i);
> void backing_src_help(void);
> enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name);
>
> +/*
> + * Whether or not the given source type is shared memory (as opposed to
> + * anonymous).
> + */
> +static inline bool backing_src_is_shared(enum vm_mem_backing_src_type t)
> +{
> + return vm_mem_backing_src_alias(t)->flag & MAP_SHARED;
> +}
> +
> #endif /* SELFTEST_KVM_TEST_UTIL_H */
> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
> index 0b88d1bbc1e0..8373aec1fb02 100644
> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
> @@ -758,8 +758,13 @@ void vm_userspace_mem_region_add(struct kvm_vm *vm,
> region->mmap_size += alignment;
>
> region->fd = -1;
> - if (src_type == VM_MEM_SRC_SHMEM) {
> - region->fd = memfd_create("kvm_selftest", MFD_CLOEXEC);
> + if (backing_src_is_shared(src_type)) {
> + int memfd_flags = MFD_CLOEXEC;
> +
> + if (src_type == VM_MEM_SRC_SHARED_HUGETLB)
> + memfd_flags |= MFD_HUGETLB;
> +
> + region->fd = memfd_create("kvm_selftest", memfd_flags);
> TEST_ASSERT(region->fd != -1,
> "memfd_create failed, errno: %i", errno);
>
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index c7a265da5090..6ad6c8276b2e 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -240,6 +240,16 @@ const struct vm_mem_backing_src_alias *vm_mem_backing_src_alias(uint32_t i)
> .name = "shmem",
> .flag = MAP_SHARED,
> },
> + [VM_MEM_SRC_SHARED_HUGETLB] = {
> + .name = "shared_hugetlb",
> + /*
> + * No MAP_HUGETLB, we use MFD_HUGETLB instead. Since
> + * we're using "file backed" memory, we need to specify
> + * this when the FD is created, not when the area is
> + * mapped.
> + */
> + .flag = MAP_SHARED,
> + },
> };
> _Static_assert(ARRAY_SIZE(aliases) == NUM_SRC_TYPES,
> "Missing new backing src types?");
> @@ -262,6 +272,7 @@ size_t get_backing_src_pagesz(uint32_t i)
> case VM_MEM_SRC_ANONYMOUS_THP:
> return get_trans_hugepagesz();
> case VM_MEM_SRC_ANONYMOUS_HUGETLB:
> + case VM_MEM_SRC_SHARED_HUGETLB:
> return get_def_hugetlb_pagesz();
> default:
> return MAP_HUGE_PAGE_SIZE(flag);
> --
> 2.31.1.751.gd2f1c929bd-goog
>

2021-05-19 22:25:24

by Ben Gardon

Subject: Re: [PATCH v2 02/10] KVM: selftests: simplify setup_demand_paging error handling

On Wed, May 19, 2021 at 3:14 PM Axel Rasmussen <[email protected]> wrote:
>
> On Wed, May 19, 2021 at 2:45 PM Ben Gardon <[email protected]> wrote:
> >
> > On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <[email protected]> wrote:
> > >
> > > A small cleanup. Our caller writes:
> > >
> > > r = setup_demand_paging(...);
> > > if (r < 0) exit(-r);
> > >
> > > Since we're just going to exit anyway, instead of returning an error we
> > > can just re-use TEST_ASSERT. This makes the caller simpler, as well as
> > > the function itself - no need to write our branches, etc.
> > >
> > > Signed-off-by: Axel Rasmussen <[email protected]>

Reviewed-by: Ben Gardon <[email protected]>

> > > ---
> > > .../selftests/kvm/demand_paging_test.c | 51 +++++++------------
> > > 1 file changed, 19 insertions(+), 32 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
> > > index 9398ba6ef023..601a1df24dd2 100644
> > > --- a/tools/testing/selftests/kvm/demand_paging_test.c
> > > +++ b/tools/testing/selftests/kvm/demand_paging_test.c
> > > @@ -9,6 +9,8 @@
> > >
> > > #define _GNU_SOURCE /* for pipe2 */
> > >
> > > +#include <inttypes.h>
> > > +#include <stdint.h>
> >
> > Why do the includes need to change in this commit? Is it for the PRIu64 below?
>
> Right, I didn't actually try compiling without these, but inttypes.h
> defines PRIu64 and stdint.h defines uint64_t. In general I tend to
> prefer including things like this because we're using their
> definitions directly, even if we might be picking them up transiently
> some other way.

Makes sense to me.

>
> >
> > > #include <stdio.h>
> > > #include <stdlib.h>
> > > #include <time.h>
> > > @@ -198,42 +200,32 @@ static void *uffd_handler_thread_fn(void *arg)
> > > return NULL;
> > > }
> > >
> > > -static int setup_demand_paging(struct kvm_vm *vm,
> > > - pthread_t *uffd_handler_thread, int pipefd,
> > > - useconds_t uffd_delay,
> > > - struct uffd_handler_args *uffd_args,
> > > - void *hva, uint64_t len)
> > > +static void setup_demand_paging(struct kvm_vm *vm,
> > > + pthread_t *uffd_handler_thread, int pipefd,
> > > + useconds_t uffd_delay,
> > > + struct uffd_handler_args *uffd_args,
> > > + void *hva, uint64_t len)
> > > {
> > > int uffd;
> > > struct uffdio_api uffdio_api;
> > > struct uffdio_register uffdio_register;
> > >
> > > uffd = syscall(__NR_userfaultfd, O_CLOEXEC | O_NONBLOCK);
> > > - if (uffd == -1) {
> > > - pr_info("uffd creation failed\n");
> > > - return -1;
> > > - }
> > > + TEST_ASSERT(uffd >= 0, "uffd creation failed, errno: %d", errno);
> > >
> > > uffdio_api.api = UFFD_API;
> > > uffdio_api.features = 0;
> > > - if (ioctl(uffd, UFFDIO_API, &uffdio_api) == -1) {
> > > - pr_info("ioctl uffdio_api failed\n");
> > > - return -1;
> > > - }
> > > + TEST_ASSERT(ioctl(uffd, UFFDIO_API, &uffdio_api) != -1,
> > > + "ioctl UFFDIO_API failed: %" PRIu64,
> > > + (uint64_t)uffdio_api.api);
> > >
> > > uffdio_register.range.start = (uint64_t)hva;
> > > uffdio_register.range.len = len;
> > > uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> > > - if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> > > - pr_info("ioctl uffdio_register failed\n");
> > > - return -1;
> > > - }
> > > -
> > > - if ((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) !=
> > > - UFFD_API_RANGE_IOCTLS) {
> > > - pr_info("unexpected userfaultfd ioctl set\n");
> > > - return -1;
> > > - }
> > > + TEST_ASSERT(ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) != -1,
> > > + "ioctl UFFDIO_REGISTER failed");
> > > + TEST_ASSERT((uffdio_register.ioctls & UFFD_API_RANGE_IOCTLS) ==
> > > + UFFD_API_RANGE_IOCTLS, "unexpected userfaultfd ioctl set");
> > >
> > > uffd_args->uffd = uffd;
> > > uffd_args->pipefd = pipefd;
> > > @@ -243,8 +235,6 @@ static int setup_demand_paging(struct kvm_vm *vm,
> > >
> > > PER_VCPU_DEBUG("Created uffd thread for HVA range [%p, %p)\n",
> > > hva, hva + len);
> > > -
> > > - return 0;
> > > }
> > >
> > > struct test_params {
> > > @@ -321,13 +311,10 @@ static void run_test(enum vm_guest_mode mode, void *arg)
> > > O_CLOEXEC | O_NONBLOCK);
> > > TEST_ASSERT(!r, "Failed to set up pipefd");
> > >
> > > - r = setup_demand_paging(vm,
> > > - &uffd_handler_threads[vcpu_id],
> > > - pipefds[vcpu_id * 2],
> > > - p->uffd_delay, &uffd_args[vcpu_id],
> > > - vcpu_hva, vcpu_mem_size);
> > > - if (r < 0)
> > > - exit(-r);
> > > + setup_demand_paging(vm, &uffd_handler_threads[vcpu_id],
> > > + pipefds[vcpu_id * 2], p->uffd_delay,
> > > + &uffd_args[vcpu_id], vcpu_hva,
> > > + vcpu_mem_size);
> > > }
> > > }
> > >
> > > --
> > > 2.31.1.751.gd2f1c929bd-goog
> > >

2021-05-24 13:28:38

by Paolo Bonzini

Subject: Re: [PATCH v2 02/10] KVM: selftests: simplify setup_demand_paging error handling

On 20/05/21 00:14, Axel Rasmussen wrote:
> On Wed, May 19, 2021 at 2:45 PM Ben Gardon <[email protected]> wrote:
>>
>> On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <[email protected]> wrote:
>>>
>>> A small cleanup. Our caller writes:
>>>
>>> r = setup_demand_paging(...);
>>> if (r < 0) exit(-r);
>>>
>>> Since we're just going to exit anyway, instead of returning an error we
>>> can just re-use TEST_ASSERT. This makes the caller simpler, as well as
>>> the function itself - no need to write our branches, etc.
>>>
>>> Signed-off-by: Axel Rasmussen <[email protected]>
>>> ---
>>> .../selftests/kvm/demand_paging_test.c | 51 +++++++------------
>>> 1 file changed, 19 insertions(+), 32 deletions(-)
>>>
>>> diff --git a/tools/testing/selftests/kvm/demand_paging_test.c b/tools/testing/selftests/kvm/demand_paging_test.c
>>> index 9398ba6ef023..601a1df24dd2 100644
>>> --- a/tools/testing/selftests/kvm/demand_paging_test.c
>>> +++ b/tools/testing/selftests/kvm/demand_paging_test.c
>>> @@ -9,6 +9,8 @@
>>>
>>> #define _GNU_SOURCE /* for pipe2 */
>>>
>>> +#include <inttypes.h>
>>> +#include <stdint.h>
>>
>> Why do the includes need to change in this commit? Is it for the PRIu64 below?
>
> Right, I didn't actually try compiling without these, but inttypes.h
> defines PRIu64 and stdint.h defines uint64_t. In general I tend to
> prefer including things like this because we're using their
> definitions directly, even if we might be picking them up transiently
> some other way.

inttypes.h is defined to include stdint.h (stdint.h is mostly useful in
freestanding environments and is usually provided by the C compiler,
while inttypes.h is provided by libc).

Paolo

2021-05-24 13:38:59

by Paolo Bonzini

Subject: Re: [PATCH v2 00/10] KVM: selftests: exercise userfaultfd minor faults

On 19/05/21 22:03, Axel Rasmussen wrote:
> Base
> ====
>
> These patches are based upon Andrew Morton's v5.13-rc1-mmots-2021-05-13-17-23
> tag. This is because this series depends on:
>
> - UFFD minor fault support for hugetlbfs (in v5.13-rc1) [1]
> - UFFD minor fault support for shmem (in Andrew's tree) [2]
>
> [1] https://lore.kernel.org/linux-fsdevel/[email protected]/
> [2] https://lore.kernel.org/patchwork/cover/1420967/
>
> Changelog
> =========
>
> v1->v2:
> - Picked up Reviewed-by's.
> - Change backing_src_is_shared() to check the flags, instead of the type. This
> makes it robust to adding new backing source types in the future.
> - Add another commit which refactors setup_demand_paging() error handling.
> - Print UFFD ioctl type once in setup_demand_paging, instead of on every page-in
> operation.
> - Expand comment on why we use MFD_HUGETLB instead of MAP_HUGETLB.
> - Reworded comment on addr_gpa2alias.
> - Moved demand_paging_test.c timing calls outside of the if (), deduplicating
> them.
> - Split trivial comment / logging fixups into a separate commit.
> - Add another commit which prints a clarifying message on test skip.
> - Split the commit allowing backing src_type to be modified in two.
> - Split the commit adding the shmem backing type in two.
> - Rebased onto v5.13-rc1-mmots-2021-05-13-17-23.
>
> Overview
> ========
>
> Minor fault handling is a new userfaultfd feature whose goal is generally to
> improve performance. In particular, it is intended for use with demand paging.
> There are more details in the cover letters for this new feature (linked above),
> but at a high level the idea is that we think of these three phases of live
> migration of a VM:
>
> 1. Precopy, where we copy "some" pages from the source to the target, while the
> VM is still running on the source machine.
> 2. Blackout, where execution stops on the source, and begins on the target.
> 3. Postcopy, where the VM is running on the target, some pages are already up
> to date, and others are not (because they weren't copied, or were modified
> after being copied).
>
> During postcopy, the first time the guest touches memory, we intercept a minor
> fault. Userspace checks whether or not the page is already up to date. If
> needed, we copy the final version of the page from the soure machine. This
> could be done with RDMA for example, to do it truly in place / with no copying.
> At this point, all that's left is to setup PTEs for the guest: so we issue
> UFFDIO_CONTINUE. No copying or page allocation needed.
>
> Because of this use case, it's useful to exercise this as part of the demand
> paging test. It lets us ensure the use case works correctly end-to-end, and also
> gives us an in-tree way to profile the end-to-end flow for future performance
> improvements.
>
> Axel Rasmussen (10):
> KVM: selftests: trivial comment/logging fixes
> KVM: selftests: simplify setup_demand_paging error handling
> KVM: selftests: print a message when skipping KVM tests
> KVM: selftests: compute correct demand paging size
> KVM: selftests: allow different backing source types
> KVM: selftests: refactor vm_mem_backing_src_type flags
> KVM: selftests: add shmem backing source type
> KVM: selftests: create alias mappings when using shared memory
> KVM: selftests: allow using UFFD minor faults for demand paging
> KVM: selftests: add shared hugetlbfs backing source type
>
> .../selftests/kvm/demand_paging_test.c | 175 +++++++++++-------
> .../testing/selftests/kvm/include/kvm_util.h | 1 +
> .../testing/selftests/kvm/include/test_util.h | 12 ++
> tools/testing/selftests/kvm/lib/kvm_util.c | 84 ++++++++-
> .../selftests/kvm/lib/kvm_util_internal.h | 2 +
> tools/testing/selftests/kvm/lib/test_util.c | 51 +++--
> 6 files changed, 238 insertions(+), 87 deletions(-)
>
> --
> 2.31.1.751.gd2f1c929bd-goog
>

Queued, thanks (with region->fd moved to the right patch).

Paolo