| Message ID | 20210519200339.829146-3-axelrasmussen@google.com |
|---|---|
| State | Accepted |
| Commit | 25408e5a0246048e3e36d2cd513565ebcc481f51 |
| Headers | show |
| Series | KVM: selftests: exercise userfaultfd minor faults | expand |
On Wed, May 19, 2021 at 2:45 PM Ben Gardon <bgardon@google.com> wrote: > > On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <axelrasmussen@google.com> 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 <axelrasmussen@google.com> > > --- > > .../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 > >
On Wed, May 19, 2021 at 3:14 PM Axel Rasmussen <axelrasmussen@google.com> wrote: > > On Wed, May 19, 2021 at 2:45 PM Ben Gardon <bgardon@google.com> wrote: > > > > On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <axelrasmussen@google.com> 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 <axelrasmussen@google.com> Reviewed-by: Ben Gardon <bgardon@google.com> > > > --- > > > .../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 > > >
On 20/05/21 00:14, Axel Rasmussen wrote: > On Wed, May 19, 2021 at 2:45 PM Ben Gardon <bgardon@google.com> wrote: >> >> On Wed, May 19, 2021 at 1:03 PM Axel Rasmussen <axelrasmussen@google.com> 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 <axelrasmussen@google.com> >>> --- >>> .../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
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); } }
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 <axelrasmussen@google.com> --- .../selftests/kvm/demand_paging_test.c | 51 +++++++------------ 1 file changed, 19 insertions(+), 32 deletions(-)