Add two helpers to register/unregister to an uffd. Use them to drop
duplicate codes.
This patch also drops assert_expected_ioctls_present() and
get_expected_ioctls(). Reasons:
- It'll need a lot of effort to pass test_type==HUGETLB into it from the
upper, so it's the simplest way to get rid of another global var
- The ioctls returned in UFFDIO_REGISTER is hardly useful at all, because
any app can already detect kernel support on any ioctl via its
corresponding UFFD_FEATURE_*. The check here is for sanity mostly but
it's probably destined no user app will even use it.
- It's not friendly to one future goal of uffd to run on old kernels, the
problem is get_expected_ioctls() compiles against UFFD_API_RANGE_IOCTLS,
which is a value that can change depending on where the test is compiled,
rather than reflecting what the kernel underneath has. It means it'll
report false negatives on old kernels so it's against our will.
So let's make our live easier.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tools/testing/selftests/mm/hugepage-mremap.c | 7 +-
.../selftests/mm/ksm_functional_tests.c | 6 +-
tools/testing/selftests/mm/uffd-common.c | 28 +------
tools/testing/selftests/mm/uffd-common.h | 2 -
tools/testing/selftests/mm/uffd-stress.c | 83 +++++--------------
tools/testing/selftests/mm/vm_util.c | 37 +++++++++
tools/testing/selftests/mm/vm_util.h | 7 ++
7 files changed, 66 insertions(+), 104 deletions(-)
diff --git a/tools/testing/selftests/mm/hugepage-mremap.c b/tools/testing/selftests/mm/hugepage-mremap.c
index e53b5eaa8fce..2084692fe1c4 100644
--- a/tools/testing/selftests/mm/hugepage-mremap.c
+++ b/tools/testing/selftests/mm/hugepage-mremap.c
@@ -60,7 +60,6 @@ static void register_region_with_uffd(char *addr, size_t len)
{
long uffd; /* userfaultfd file descriptor */
struct uffdio_api uffdio_api;
- struct uffdio_register uffdio_register;
/* Create and enable userfaultfd object. */
@@ -96,11 +95,7 @@ static void register_region_with_uffd(char *addr, size_t len)
* handling by the userfaultfd object. In mode, we request to track
* missing pages (i.e., pages that have not yet been faulted in).
*/
-
- uffdio_register.range.start = (unsigned long)addr;
- uffdio_register.range.len = len;
- uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
+ if (uffd_register(uffd, addr, len, true, false, false)) {
perror("ioctl-UFFDIO_REGISTER");
exit(1);
}
diff --git a/tools/testing/selftests/mm/ksm_functional_tests.c b/tools/testing/selftests/mm/ksm_functional_tests.c
index d8b5b4930412..d3f26050dfd7 100644
--- a/tools/testing/selftests/mm/ksm_functional_tests.c
+++ b/tools/testing/selftests/mm/ksm_functional_tests.c
@@ -178,7 +178,6 @@ static void test_unmerge_discarded(void)
static void test_unmerge_uffd_wp(void)
{
struct uffdio_writeprotect uffd_writeprotect;
- struct uffdio_register uffdio_register;
const unsigned int size = 2 * MiB;
struct uffdio_api uffdio_api;
char *map;
@@ -210,10 +209,7 @@ static void test_unmerge_uffd_wp(void)
}
/* Register UFFD-WP, no need for an actual handler. */
- uffdio_register.range.start = (unsigned long) map;
- uffdio_register.range.len = size;
- uffdio_register.mode = UFFDIO_REGISTER_MODE_WP;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) < 0) {
+ if (uffd_register(uffd, map, size, false, true, false)) {
ksft_test_result_fail("UFFDIO_REGISTER_MODE_WP failed\n");
goto close_uffd;
}
diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
index c57757c2a36f..17f2bb82c3db 100644
--- a/tools/testing/selftests/mm/uffd-common.c
+++ b/tools/testing/selftests/mm/uffd-common.c
@@ -330,33 +330,6 @@ void uffd_test_ctx_init(uint64_t features)
err("pipe");
}
-uint64_t get_expected_ioctls(uint64_t mode)
-{
- uint64_t ioctls = UFFD_API_RANGE_IOCTLS;
-
- if (test_type == TEST_HUGETLB)
- ioctls &= ~(1 << _UFFDIO_ZEROPAGE);
-
- if (!((mode & UFFDIO_REGISTER_MODE_WP) && test_uffdio_wp))
- ioctls &= ~(1 << _UFFDIO_WRITEPROTECT);
-
- if (!((mode & UFFDIO_REGISTER_MODE_MINOR) && test_uffdio_minor))
- ioctls &= ~(1 << _UFFDIO_CONTINUE);
-
- return ioctls;
-}
-
-void assert_expected_ioctls_present(uint64_t mode, uint64_t ioctls)
-{
- uint64_t expected = get_expected_ioctls(mode);
- uint64_t actual = ioctls & expected;
-
- if (actual != expected) {
- err("missing ioctl(s): expected %"PRIx64" actual: %"PRIx64,
- expected, actual);
- }
-}
-
void wp_range(int ufd, __u64 start, __u64 len, bool wp)
{
struct uffdio_writeprotect prms;
@@ -609,3 +582,4 @@ int copy_page(int ufd, unsigned long offset)
{
return __copy_page(ufd, offset, false);
}
+
diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
index d9430cfdcb19..11f770391bd9 100644
--- a/tools/testing/selftests/mm/uffd-common.h
+++ b/tools/testing/selftests/mm/uffd-common.h
@@ -101,8 +101,6 @@ extern uffd_test_ops_t *uffd_test_ops;
void uffd_stats_report(struct uffd_stats *stats, int n_cpus);
void uffd_test_ctx_init(uint64_t features);
void userfaultfd_open(uint64_t *features);
-uint64_t get_expected_ioctls(uint64_t mode);
-void assert_expected_ioctls_present(uint64_t mode, uint64_t ioctls);
int uffd_read_msg(int ufd, struct uffd_msg *msg);
void wp_range(int ufd, __u64 start, __u64 len, bool wp);
void uffd_handle_page_fault(struct uffd_msg *msg, struct uffd_stats *stats);
diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index c68a9aeefc41..e6d39a755082 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -463,28 +463,19 @@ static int uffdio_zeropage(int ufd, unsigned long offset)
/* exercise UFFDIO_ZEROPAGE */
static int userfaultfd_zeropage_test(void)
{
- struct uffdio_register uffdio_register;
-
printf("testing UFFDIO_ZEROPAGE: ");
fflush(stdout);
uffd_test_ctx_init(0);
- uffdio_register.range.start = (unsigned long) area_dst;
- uffdio_register.range.len = nr_pages * page_size;
- uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
- if (test_uffdio_wp)
- uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
+ if (uffd_register(uffd, area_dst, nr_pages * page_size,
+ true, test_uffdio_wp, false))
err("register failure");
- assert_expected_ioctls_present(
- uffdio_register.mode, uffdio_register.ioctls);
-
if (area_dst_alias) {
/* Needed this to test zeropage-retry on shared memory */
- uffdio_register.range.start = (unsigned long) area_dst_alias;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
+ if (uffd_register(uffd, area_dst_alias, nr_pages * page_size,
+ true, test_uffdio_wp, false))
err("register failure");
}
@@ -498,7 +489,6 @@ static int userfaultfd_zeropage_test(void)
static int userfaultfd_events_test(void)
{
- struct uffdio_register uffdio_register;
pthread_t uffd_mon;
int err, features;
pid_t pid;
@@ -514,17 +504,10 @@ static int userfaultfd_events_test(void)
fcntl(uffd, F_SETFL, uffd_flags | O_NONBLOCK);
- uffdio_register.range.start = (unsigned long) area_dst;
- uffdio_register.range.len = nr_pages * page_size;
- uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
- if (test_uffdio_wp)
- uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
+ if (uffd_register(uffd, area_dst, nr_pages * page_size,
+ true, test_uffdio_wp, false))
err("register failure");
- assert_expected_ioctls_present(
- uffdio_register.mode, uffdio_register.ioctls);
-
if (pthread_create(&uffd_mon, &attr, uffd_poll_thread, &stats))
err("uffd_poll_thread create");
@@ -550,7 +533,6 @@ static int userfaultfd_events_test(void)
static int userfaultfd_sig_test(void)
{
- struct uffdio_register uffdio_register;
unsigned long userfaults;
pthread_t uffd_mon;
int err, features;
@@ -566,17 +548,10 @@ static int userfaultfd_sig_test(void)
fcntl(uffd, F_SETFL, uffd_flags | O_NONBLOCK);
- uffdio_register.range.start = (unsigned long) area_dst;
- uffdio_register.range.len = nr_pages * page_size;
- uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
- if (test_uffdio_wp)
- uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
+ if (uffd_register(uffd, area_dst, nr_pages * page_size,
+ true, test_uffdio_wp, false))
err("register failure");
- assert_expected_ioctls_present(
- uffdio_register.mode, uffdio_register.ioctls);
-
if (faulting_process(1))
err("faulting process failed");
@@ -629,7 +604,6 @@ void check_memory_contents(char *p)
static int userfaultfd_minor_test(void)
{
unsigned long p;
- struct uffdio_register uffdio_register;
pthread_t uffd_mon;
char c;
struct uffd_stats stats = { 0 };
@@ -642,17 +616,10 @@ static int userfaultfd_minor_test(void)
uffd_test_ctx_init(uffd_minor_feature());
- uffdio_register.range.start = (unsigned long)area_dst_alias;
- uffdio_register.range.len = nr_pages * page_size;
- uffdio_register.mode = UFFDIO_REGISTER_MODE_MINOR;
- if (test_uffdio_wp)
- uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
+ if (uffd_register(uffd, area_dst_alias, nr_pages * page_size,
+ false, test_uffdio_wp, true))
err("register failure");
- assert_expected_ioctls_present(
- uffdio_register.mode, uffdio_register.ioctls);
-
/*
* After registering with UFFD, populate the non-UFFD-registered side of
* the shared mapping. This should *not* trigger any UFFD minor faults.
@@ -777,7 +744,6 @@ static void userfaultfd_wp_unpopulated_test(int pagemap_fd)
static void userfaultfd_pagemap_test(unsigned int test_pgsize)
{
- struct uffdio_register uffdio_register;
int pagemap_fd;
uint64_t value;
@@ -805,10 +771,8 @@ static void userfaultfd_pagemap_test(unsigned int test_pgsize)
err("madvise(MADV_NOHUGEPAGE) failed");
}
- uffdio_register.range.start = (unsigned long) area_dst;
- uffdio_register.range.len = nr_pages * page_size;
- uffdio_register.mode = UFFDIO_REGISTER_MODE_WP;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
+ if (uffd_register(uffd, area_dst, nr_pages * page_size,
+ false, true, false))
err("register failed");
pagemap_fd = pagemap_open();
@@ -858,8 +822,8 @@ static int userfaultfd_stress(void)
{
void *area;
unsigned long nr;
- struct uffdio_register uffdio_register;
struct uffd_stats uffd_stats[nr_cpus];
+ uint64_t mem_size = nr_pages * page_size;
uffd_test_ctx_init(UFFD_FEATURE_WP_UNPOPULATED);
@@ -894,20 +858,13 @@ static int userfaultfd_stress(void)
fcntl(uffd, F_SETFL, uffd_flags & ~O_NONBLOCK);
/* register */
- uffdio_register.range.start = (unsigned long) area_dst;
- uffdio_register.range.len = nr_pages * page_size;
- uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
- if (test_uffdio_wp)
- uffdio_register.mode |= UFFDIO_REGISTER_MODE_WP;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
+ if (uffd_register(uffd, area_dst, mem_size,
+ true, test_uffdio_wp, false))
err("register failure");
- assert_expected_ioctls_present(
- uffdio_register.mode, uffdio_register.ioctls);
if (area_dst_alias) {
- uffdio_register.range.start = (unsigned long)
- area_dst_alias;
- if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register))
+ if (uffd_register(uffd, area_dst_alias, mem_size,
+ true, test_uffdio_wp, false))
err("register failure alias");
}
@@ -949,12 +906,10 @@ static int userfaultfd_stress(void)
nr_pages * page_size, false);
/* unregister */
- if (ioctl(uffd, UFFDIO_UNREGISTER, &uffdio_register.range))
+ if (uffd_unregister(uffd, area_dst, mem_size))
err("unregister failure");
if (area_dst_alias) {
- uffdio_register.range.start = (unsigned long) area_dst;
- if (ioctl(uffd, UFFDIO_UNREGISTER,
- &uffdio_register.range))
+ if (uffd_unregister(uffd, area_dst_alias, mem_size))
err("unregister failure alias");
}
diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index 8e9da621764a..10e76400ed70 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -1,6 +1,8 @@
// SPDX-License-Identifier: GPL-2.0
#include <string.h>
#include <fcntl.h>
+#include <sys/ioctl.h>
+#include <linux/userfaultfd.h>
#include "../kselftest.h"
#include "vm_util.h"
@@ -193,3 +195,38 @@ unsigned long default_huge_page_size(void)
fclose(f);
return hps;
}
+
+int uffd_register(int uffd, void *addr, uint64_t len,
+ bool miss, bool wp, bool minor)
+{
+ struct uffdio_register uffdio_register = { 0 };
+ uint64_t mode = 0;
+ int ret = 0;
+
+ if (miss)
+ mode |= UFFDIO_REGISTER_MODE_MISSING;
+ if (wp)
+ mode |= UFFDIO_REGISTER_MODE_WP;
+ if (minor)
+ mode |= UFFDIO_REGISTER_MODE_MINOR;
+
+ uffdio_register.range.start = (unsigned long)addr;
+ uffdio_register.range.len = len;
+ uffdio_register.mode = mode;
+
+ if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
+ ret = -errno;
+
+ return ret;
+}
+
+int uffd_unregister(int uffd, void *addr, uint64_t len)
+{
+ struct uffdio_range range = { .start = (uintptr_t)addr, .len = len };
+ int ret = 0;
+
+ if (ioctl(uffd, UFFDIO_UNREGISTER, &range) == -1)
+ ret = -errno;
+
+ return ret;
+}
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index d9fadddb5c69..a67db8432855 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -45,6 +45,13 @@ bool check_huge_shmem(void *addr, int nr_hpages, uint64_t hpage_size);
int64_t allocate_transhuge(void *ptr, int pagemap_fd);
unsigned long default_huge_page_size(void);
+int uffd_open_dev(unsigned int flags);
+int uffd_open_sys(unsigned int flags);
+int uffd_open(unsigned int flags);
+int uffd_register(int uffd, void *addr, uint64_t len,
+ bool miss, bool wp, bool minor);
+int uffd_unregister(int uffd, void *addr, uint64_t len);
+
/*
* On ppc64 this will only work with radix 2M hugepage size
*/
--
2.39.1On Thu, Mar 30, 2023 at 12:07:47PM -0400, Peter Xu wrote:
> Add two helpers to register/unregister to an uffd. Use them to drop
> duplicate codes.
>
> This patch also drops assert_expected_ioctls_present() and
> get_expected_ioctls(). Reasons:
>
> - It'll need a lot of effort to pass test_type==HUGETLB into it from the
> upper, so it's the simplest way to get rid of another global var
>
> - The ioctls returned in UFFDIO_REGISTER is hardly useful at all, because
> any app can already detect kernel support on any ioctl via its
> corresponding UFFD_FEATURE_*. The check here is for sanity mostly but
> it's probably destined no user app will even use it.
>
> - It's not friendly to one future goal of uffd to run on old kernels, the
> problem is get_expected_ioctls() compiles against UFFD_API_RANGE_IOCTLS,
> which is a value that can change depending on where the test is compiled,
> rather than reflecting what the kernel underneath has. It means it'll
> report false negatives on old kernels so it's against our will.
>
> So let's make our live easier.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> tools/testing/selftests/mm/hugepage-mremap.c | 7 +-
> .../selftests/mm/ksm_functional_tests.c | 6 +-
> tools/testing/selftests/mm/uffd-common.c | 28 +------
> tools/testing/selftests/mm/uffd-common.h | 2 -
> tools/testing/selftests/mm/uffd-stress.c | 83 +++++--------------
> tools/testing/selftests/mm/vm_util.c | 37 +++++++++
> tools/testing/selftests/mm/vm_util.h | 7 ++
> 7 files changed, 66 insertions(+), 104 deletions(-)
>
> diff --git a/tools/testing/selftests/mm/hugepage-mremap.c b/tools/testing/selftests/mm/hugepage-mremap.c
> index e53b5eaa8fce..2084692fe1c4 100644
> --- a/tools/testing/selftests/mm/hugepage-mremap.c
> +++ b/tools/testing/selftests/mm/hugepage-mremap.c
> @@ -60,7 +60,6 @@ static void register_region_with_uffd(char *addr, size_t len)
> {
> long uffd; /* userfaultfd file descriptor */
> struct uffdio_api uffdio_api;
> - struct uffdio_register uffdio_register;
>
> /* Create and enable userfaultfd object. */
>
> @@ -96,11 +95,7 @@ static void register_region_with_uffd(char *addr, size_t len)
> * handling by the userfaultfd object. In mode, we request to track
> * missing pages (i.e., pages that have not yet been faulted in).
> */
> -
> - uffdio_register.range.start = (unsigned long)addr;
> - uffdio_register.range.len = len;
> - uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> - if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> + if (uffd_register(uffd, addr, len, true, false, false)) {
I'd replace booleans with a bit flags as it easier to read.
Other than that LGTM.
> perror("ioctl-UFFDIO_REGISTER");
> exit(1);
> }
--
Sincerely yours,
Mike.
On Fri, Apr 07, 2023 at 02:08:54PM +0300, Mike Rapoport wrote:
> > @@ -96,11 +95,7 @@ static void register_region_with_uffd(char *addr, size_t len)
> > * handling by the userfaultfd object. In mode, we request to track
> > * missing pages (i.e., pages that have not yet been faulted in).
> > */
> > -
> > - uffdio_register.range.start = (unsigned long)addr;
> > - uffdio_register.range.len = len;
> > - uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> > - if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> > + if (uffd_register(uffd, addr, len, true, false, false)) {
>
> I'd replace booleans with a bit flags as it easier to read.
> Other than that LGTM.
It was mostly for no need to remember the long names of macros, and easier
when conditionally set with some modes. E.g., we have 5 callers have things
like:
uffd_register(..., test_uffdio_wp ? UFFDIO_REGISTER_MODE_WP : 0);
The bools simplifes it to:
uffd_register(..., test_uffdio_wp, ...);
But let me know if you still think that's better - I can switch here.
Thanks,
--
Peter Xu
On Tue, Apr 11, 2023 at 03:13:18PM -0400, Peter Xu wrote:
> On Fri, Apr 07, 2023 at 02:08:54PM +0300, Mike Rapoport wrote:
> > > @@ -96,11 +95,7 @@ static void register_region_with_uffd(char *addr, size_t len)
> > > * handling by the userfaultfd object. In mode, we request to track
> > > * missing pages (i.e., pages that have not yet been faulted in).
> > > */
> > > -
> > > - uffdio_register.range.start = (unsigned long)addr;
> > > - uffdio_register.range.len = len;
> > > - uffdio_register.mode = UFFDIO_REGISTER_MODE_MISSING;
> > > - if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1) {
> > > + if (uffd_register(uffd, addr, len, true, false, false)) {
> >
> > I'd replace booleans with a bit flags as it easier to read.
> > Other than that LGTM.
>
> It was mostly for no need to remember the long names of macros, and easier
> when conditionally set with some modes. E.g., we have 5 callers have things
> like:
>
> uffd_register(..., test_uffdio_wp ? UFFDIO_REGISTER_MODE_WP : 0);
>
> The bools simplifes it to:
>
> uffd_register(..., test_uffdio_wp, ...);
>
> But let me know if you still think that's better - I can switch here.
No strong feelings, however you prefer.
> Thanks,
>
> --
> Peter Xu
>
--
Sincerely yours,
Mike.
On Thu, Mar 30, 2023 at 12:07:47PM -0400, Peter Xu wrote:
> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
> index c57757c2a36f..17f2bb82c3db 100644
> --- a/tools/testing/selftests/mm/uffd-common.c
> +++ b/tools/testing/selftests/mm/uffd-common.c
> @@ -330,33 +330,6 @@ void uffd_test_ctx_init(uint64_t features)
> err("pipe");
> }
>
> -uint64_t get_expected_ioctls(uint64_t mode)
> -{
> - uint64_t ioctls = UFFD_API_RANGE_IOCTLS;
> -
> - if (test_type == TEST_HUGETLB)
> - ioctls &= ~(1 << _UFFDIO_ZEROPAGE);
> -
> - if (!((mode & UFFDIO_REGISTER_MODE_WP) && test_uffdio_wp))
> - ioctls &= ~(1 << _UFFDIO_WRITEPROTECT);
> -
> - if (!((mode & UFFDIO_REGISTER_MODE_MINOR) && test_uffdio_minor))
> - ioctls &= ~(1 << _UFFDIO_CONTINUE);
> -
> - return ioctls;
> -}
> -
> -void assert_expected_ioctls_present(uint64_t mode, uint64_t ioctls)
> -{
> - uint64_t expected = get_expected_ioctls(mode);
> - uint64_t actual = ioctls & expected;
> -
> - if (actual != expected) {
> - err("missing ioctl(s): expected %"PRIx64" actual: %"PRIx64,
> - expected, actual);
> - }
> -}
Here I dropped the other reference of get_expected_ioctls(), so I also
dropped this test which I think is kind of flawed IMHO - as I replied in
the other thread, we should probably not reference UFFD_API_RANGE_IOCTLS.
But I can feel (from the comments in the other patch that removed the other
reference of get_expected_ioctls()) that a lot of us would still care about
this test.
So I added a new patch / test on top of the series (so it'll have one more
patch in the next version at last), just to test all possible combinations
of UFFDIO_REGISTER alongside with its returned uffdio_register.ioctls.
This is IMHO better than get_expected_ioctls() because:
- It's much cleaner to have a separate test on this rather than testing
it randomly in the code with random values passed in.
- It tests all combinations. It not only includes shmem-private that this
series introduced while wasn't there before, but also all combinations
of (miss, wp, minor) tuples.
- It doesn't rely on UFFD_API_RANGE_IOCTLS anymore.
It'll be something like this:
===8<===
/*
* Test the returned uffdio_register.ioctls with different register modes.
* Note that _UFFDIO_ZEROPAGE is tested separately in the zeropage test.
*/
static void
do_register_ioctls_test(uffd_test_args_t *args, bool miss, bool wp, bool minor)
{
uint64_t ioctls = 0, expected = BIT_ULL(_UFFDIO_WAKE);
mem_type_t *mem_type = args->mem_type;
int ret;
ret = uffd_register_with_ioctls(uffd, area_dst, page_size,
miss, wp, minor, &ioctls);
/*
* Handle special cases of UFFDIO_REGISTER here where it should
* just fail with -EINVAL first..
*
* Case 1: register MINOR on anon
* Case 2: register with no mode selected
*/
if ((minor && (mem_type->mem_flag == MEM_ANON)) ||
(!miss && !wp && !minor)) {
if (ret != -EINVAL)
err("register (miss=%d, wp=%d, minor=%d) failed "
"with wrong errno=%d", miss, wp, minor, ret);
return;
}
/* UFFDIO_REGISTER should succeed, then check ioctls returned */
if (miss)
expected |= BIT_ULL(_UFFDIO_COPY);
if (wp)
expected |= BIT_ULL(_UFFDIO_WRITEPROTECT);
if (minor)
expected |= BIT_ULL(_UFFDIO_CONTINUE);
if ((ioctls & expected) != expected)
err("unexpected uffdio_register.ioctls "
"(miss=%d, wp=%d, minor=%d): expected=0x%"PRIx64", "
"returned=0x%"PRIx64, miss, wp, minor, expected, ioctls);
uffd_unregister(uffd, area_dst, page_size);
}
static void uffd_register_ioctls_test(uffd_test_args_t *args)
{
int miss, wp, minor;
for (miss = 0; miss <= 1; miss++)
for (wp = 0; wp <= 1; wp++)
for (minor = 0; minor <= 1; minor++)
do_register_ioctls_test(args, miss, wp, minor);
uffd_test_pass();
}
===8<===
Side note: the _UFFDIO_ZEROPAGE test will be left in the specific zeropage
test.
I considered moving get_expected_ioctls() rather than dropping in the same
patch, but that's just over-complicated when without the unit test
frameworks being ready. I hope this addresses the concern here, otherwise
please shoot.
I've also attached the two patches that will test uffdio_register.ioctls as
a whole, just in case helpful for discussion before I post v2.
Thanks,
--
Peter Xu
From bea1534894c8edca4d7132c2d2676a9faf94a346 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Wed, 29 Mar 2023 16:34:15 -0400
Subject: [PATCH] selftests/mm: Move zeropage test into uffd unit tests
Simplifies it a bit along the way, e.g., drop the never used offset
field (which was always the 1st page so offset=0).
Introduce uffd_register_with_ioctls() out of uffd_register() to detect
uffdio_register.ioctls got returned. Check that automatically when testing
UFFDIO_ZEROPAGE on different types of memory (and kernel).
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tools/testing/selftests/mm/uffd-stress.c | 94 +-------------------
tools/testing/selftests/mm/uffd-unit-tests.c | 93 +++++++++++++++++++
tools/testing/selftests/mm/vm_util.c | 14 ++-
tools/testing/selftests/mm/vm_util.h | 2 +
4 files changed, 108 insertions(+), 95 deletions(-)
diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index f3046ae13a90..a6f3609c1ad1 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -110,15 +110,6 @@ static inline uint64_t uffd_minor_feature(void)
return 0;
}
-static int my_bcmp(char *str1, char *str2, size_t n)
-{
- unsigned long i;
- for (i = 0; i < n; i++)
- if (str1[i] != str2[i])
- return 1;
- return 0;
-}
-
static void *locking_thread(void *arg)
{
unsigned long cpu = (unsigned long) arg;
@@ -274,89 +265,6 @@ static int stress(struct uffd_stats *uffd_stats)
return 0;
}
-static void retry_uffdio_zeropage(int ufd,
- struct uffdio_zeropage *uffdio_zeropage,
- unsigned long offset)
-{
- uffd_test_ops->alias_mapping(&uffdio_zeropage->range.start,
- uffdio_zeropage->range.len,
- offset);
- if (ioctl(ufd, UFFDIO_ZEROPAGE, uffdio_zeropage)) {
- if (uffdio_zeropage->zeropage != -EEXIST)
- err("UFFDIO_ZEROPAGE error: %"PRId64,
- (int64_t)uffdio_zeropage->zeropage);
- } else {
- err("UFFDIO_ZEROPAGE error: %"PRId64,
- (int64_t)uffdio_zeropage->zeropage);
- }
-}
-
-static int __uffdio_zeropage(int ufd, unsigned long offset)
-{
- struct uffdio_zeropage uffdio_zeropage;
- int ret;
- bool has_zeropage = !(test_type == TEST_HUGETLB);
- __s64 res;
-
- if (offset >= nr_pages * page_size)
- err("unexpected offset %lu", offset);
- uffdio_zeropage.range.start = (unsigned long) area_dst + offset;
- uffdio_zeropage.range.len = page_size;
- uffdio_zeropage.mode = 0;
- ret = ioctl(ufd, UFFDIO_ZEROPAGE, &uffdio_zeropage);
- res = uffdio_zeropage.zeropage;
- if (ret) {
- /* real retval in ufdio_zeropage.zeropage */
- if (has_zeropage)
- err("UFFDIO_ZEROPAGE error: %"PRId64, (int64_t)res);
- else if (res != -EINVAL)
- err("UFFDIO_ZEROPAGE not -EINVAL");
- } else if (has_zeropage) {
- if (res != page_size) {
- err("UFFDIO_ZEROPAGE unexpected size");
- } else {
- retry_uffdio_zeropage(ufd, &uffdio_zeropage,
- offset);
- return 1;
- }
- } else
- err("UFFDIO_ZEROPAGE succeeded");
-
- return 0;
-}
-
-static int uffdio_zeropage(int ufd, unsigned long offset)
-{
- return __uffdio_zeropage(ufd, offset);
-}
-
-/* exercise UFFDIO_ZEROPAGE */
-static int userfaultfd_zeropage_test(void)
-{
- printf("testing UFFDIO_ZEROPAGE: ");
- fflush(stdout);
-
- uffd_test_ctx_init(0);
-
- if (uffd_register(uffd, area_dst, nr_pages * page_size,
- true, test_uffdio_wp, false))
- err("register failure");
-
- if (area_dst_alias) {
- /* Needed this to test zeropage-retry on shared memory */
- if (uffd_register(uffd, area_dst_alias, nr_pages * page_size,
- true, test_uffdio_wp, false))
- err("register failure");
- }
-
- if (uffdio_zeropage(uffd, 0))
- if (my_bcmp(area_dst, zeropage, page_size))
- err("zeropage is not zero");
-
- printf("done.\n");
- return 0;
-}
-
static int userfaultfd_stress(void)
{
void *area;
@@ -468,7 +376,7 @@ static int userfaultfd_stress(void)
uffd_stats_report(uffd_stats, nr_cpus);
}
- return userfaultfd_zeropage_test();
+ return 0;
}
static void set_test_type(const char *type)
diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
index 20fa73730b51..676edfdcbbe9 100644
--- a/tools/testing/selftests/mm/uffd-unit-tests.c
+++ b/tools/testing/selftests/mm/uffd-unit-tests.c
@@ -660,7 +660,100 @@ static void uffd_events_wp_test(void)
uffd_events_test_common(true);
}
+static void retry_uffdio_zeropage(int ufd,
+ struct uffdio_zeropage *uffdio_zeropage)
+{
+ uffd_test_ops->alias_mapping(&uffdio_zeropage->range.start,
+ uffdio_zeropage->range.len,
+ 0);
+ if (ioctl(ufd, UFFDIO_ZEROPAGE, uffdio_zeropage)) {
+ if (uffdio_zeropage->zeropage != -EEXIST)
+ err("UFFDIO_ZEROPAGE error: %"PRId64,
+ (int64_t)uffdio_zeropage->zeropage);
+ } else {
+ err("UFFDIO_ZEROPAGE error: %"PRId64,
+ (int64_t)uffdio_zeropage->zeropage);
+ }
+}
+
+static bool do_uffdio_zeropage(int ufd, bool has_zeropage)
+{
+ struct uffdio_zeropage uffdio_zeropage = { 0 };
+ int ret;
+ __s64 res;
+
+ uffdio_zeropage.range.start = (unsigned long) area_dst;
+ uffdio_zeropage.range.len = page_size;
+ uffdio_zeropage.mode = 0;
+ ret = ioctl(ufd, UFFDIO_ZEROPAGE, &uffdio_zeropage);
+ res = uffdio_zeropage.zeropage;
+ if (ret) {
+ /* real retval in ufdio_zeropage.zeropage */
+ if (has_zeropage)
+ err("UFFDIO_ZEROPAGE error: %"PRId64, (int64_t)res);
+ else if (res != -EINVAL)
+ err("UFFDIO_ZEROPAGE not -EINVAL");
+ } else if (has_zeropage) {
+ if (res != page_size)
+ err("UFFDIO_ZEROPAGE unexpected size");
+ else
+ retry_uffdio_zeropage(ufd, &uffdio_zeropage);
+ return true;
+ } else
+ err("UFFDIO_ZEROPAGE succeeded");
+
+ return false;
+}
+
+/*
+ * Registers a range with MISSING mode only for zeropage test. Return true
+ * if UFFDIO_ZEROPAGE supported, false otherwise. Can't use uffd_register()
+ * because we want to detect .ioctls along the way.
+ */
+static bool
+uffd_register_detect_zp(int uffd, void *addr, uint64_t len)
+{
+ uint64_t ioctls = 0;
+
+ if (uffd_register_with_ioctls(uffd, addr, len, true,
+ false, false, &ioctls))
+ err("zeropage register fail");
+
+ return ioctls & (1 << _UFFDIO_ZEROPAGE);
+}
+
+/* exercise UFFDIO_ZEROPAGE */
+static void uffd_zeropage_test(void)
+{
+ bool has_zeropage;
+ int i;
+
+ has_zeropage = uffd_register_detect_zp(uffd, area_dst, page_size);
+ if (area_dst_alias)
+ /* Ignore the retval; we already have it */
+ uffd_register_detect_zp(uffd, area_dst_alias, page_size);
+
+ if (do_uffdio_zeropage(uffd, has_zeropage))
+ for (i = 0; i < page_size; i++)
+ if (area_dst[i] != 0)
+ err("data non-zero at offset %d\n", i);
+
+ if (uffd_unregister(uffd, area_dst, page_size))
+ err("unregister");
+
+ if (area_dst_alias && uffd_unregister(uffd, area_dst_alias, page_size))
+ err("unregister");
+
+ uffd_test_pass();
+}
+
uffd_test_case_t uffd_tests[] = {
+ {
+ .name = "zeropage",
+ .uffd_fn = uffd_zeropage_test,
+ .mem_targets = MEM_ALL,
+ .uffd_feature_required = 0,
+ },
{
.name = "pagemap",
.uffd_fn = uffd_pagemap_test,
diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
index dad1f62a7ecd..6b2bc17efe13 100644
--- a/tools/testing/selftests/mm/vm_util.c
+++ b/tools/testing/selftests/mm/vm_util.c
@@ -198,8 +198,9 @@ unsigned long default_huge_page_size(void)
return hps;
}
-int uffd_register(int uffd, void *addr, uint64_t len,
- bool miss, bool wp, bool minor)
+/* If `ioctls' non-NULL, the allowed ioctls will be returned into the var */
+int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
+ bool miss, bool wp, bool minor, uint64_t *ioctls)
{
struct uffdio_register uffdio_register = { 0 };
uint64_t mode = 0;
@@ -218,10 +219,19 @@ int uffd_register(int uffd, void *addr, uint64_t len,
if (ioctl(uffd, UFFDIO_REGISTER, &uffdio_register) == -1)
ret = -errno;
+ else if (ioctls)
+ *ioctls = uffdio_register.ioctls;
return ret;
}
+int uffd_register(int uffd, void *addr, uint64_t len,
+ bool miss, bool wp, bool minor)
+{
+ return uffd_register_with_ioctls(uffd, addr, len,
+ miss, wp, minor, NULL);
+}
+
int uffd_unregister(int uffd, void *addr, uint64_t len)
{
struct uffdio_range range = { .start = (uintptr_t)addr, .len = len };
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
index 634eb2f41145..b950bd16083a 100644
--- a/tools/testing/selftests/mm/vm_util.h
+++ b/tools/testing/selftests/mm/vm_util.h
@@ -52,6 +52,8 @@ int uffd_open_dev(unsigned int flags);
int uffd_open_sys(unsigned int flags);
int uffd_open(unsigned int flags);
int uffd_get_features(uint64_t *features);
+int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len,
+ bool miss, bool wp, bool minor, uint64_t *ioctls);
/*
* On ppc64 this will only work with radix 2M hugepage size
--
2.39.1
From f575e3d26dedce19ef9790c2ac0a5e7e17bcf2bd Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Wed, 5 Apr 2023 14:20:18 -0400
Subject: [PATCH] selftests/mm: Add uffdio register ioctls test
This new test tests against the returned ioctls from UFFDIO_REGISTER, where
put into uffdio_register.ioctls.
This also tests the expected failure cases of UFFDIO_REGISTER, aka:
- Register with empty mode should fail with -EINVAL
- Register anon without page cache (anon) should fail with -EINVAL
Signed-off-by: Peter Xu <peterx@redhat.com>
---
tools/testing/selftests/mm/uffd-unit-tests.c | 111 ++++++++++++++++---
1 file changed, 96 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
index 37a844bafe19..88968ed5f57d 100644
--- a/tools/testing/selftests/mm/uffd-unit-tests.c
+++ b/tools/testing/selftests/mm/uffd-unit-tests.c
@@ -62,8 +62,14 @@ mem_type_t mem_types[] = {
},
};
+/* Arguments to be passed over to each uffd unit test */
+struct uffd_test_args {
+ mem_type_t *mem_type;
+};
+typedef struct uffd_test_args uffd_test_args_t;
+
/* Returns: UFFD_TEST_* */
-typedef void (*uffd_test_fn)(void);
+typedef void (*uffd_test_fn)(uffd_test_args_t *);
typedef struct {
const char *name;
@@ -172,8 +178,9 @@ static int test_uffd_api(bool use_dev)
* This function initializes the global variables. TODO: remove global
* vars and then remove this.
*/
-static int uffd_setup_environment(uffd_test_case_t *test, mem_type_t *mem_type,
- const char **errmsg)
+static int
+uffd_setup_environment(uffd_test_args_t *args, uffd_test_case_t *test,
+ mem_type_t *mem_type, const char **errmsg)
{
map_shared = mem_type->shared;
uffd_test_ops = mem_type->mem_ops;
@@ -187,6 +194,9 @@ static int uffd_setup_environment(uffd_test_case_t *test, mem_type_t *mem_type,
/* TODO: remove this global var.. it's so ugly */
nr_cpus = 1;
+ /* Initialize test arguments */
+ args->mem_type = mem_type;
+
return uffd_test_ctx_init(test->uffd_feature_required, errmsg);
}
@@ -239,7 +249,7 @@ static int pagemap_test_fork(bool present)
return result;
}
-static void uffd_wp_unpopulated_test(void)
+static void uffd_wp_unpopulated_test(uffd_test_args_t *args)
{
uint64_t value;
int pagemap_fd;
@@ -285,7 +295,7 @@ static void uffd_wp_unpopulated_test(void)
uffd_test_pass();
}
-static void uffd_pagemap_test(void)
+static void uffd_pagemap_test(uffd_test_args_t *args)
{
int pagemap_fd;
uint64_t value;
@@ -415,17 +425,17 @@ static void uffd_minor_test_common(bool test_collapse, bool test_wp)
uffd_test_pass();
}
-void uffd_minor_test(void)
+void uffd_minor_test(uffd_test_args_t *args)
{
uffd_minor_test_common(false, false);
}
-void uffd_minor_wp_test(void)
+void uffd_minor_wp_test(uffd_test_args_t *args)
{
uffd_minor_test_common(false, true);
}
-void uffd_minor_collapse_test(void)
+void uffd_minor_collapse_test(uffd_test_args_t *args)
{
uffd_minor_test_common(true, false);
}
@@ -603,12 +613,12 @@ static void uffd_sigbus_test_common(bool wp)
uffd_test_pass();
}
-static void uffd_sigbus_test(void)
+static void uffd_sigbus_test(uffd_test_args_t *args)
{
uffd_sigbus_test_common(false);
}
-static void uffd_sigbus_wp_test(void)
+static void uffd_sigbus_wp_test(uffd_test_args_t *args)
{
uffd_sigbus_test_common(true);
}
@@ -651,12 +661,12 @@ static void uffd_events_test_common(bool wp)
uffd_test_pass();
}
-static void uffd_events_test(void)
+static void uffd_events_test(uffd_test_args_t *args)
{
uffd_events_test_common(false);
}
-static void uffd_events_wp_test(void)
+static void uffd_events_wp_test(uffd_test_args_t *args)
{
uffd_events_test_common(true);
}
@@ -724,7 +734,7 @@ uffd_register_detect_zp(int uffd, void *addr, uint64_t len)
}
/* exercise UFFDIO_ZEROPAGE */
-static void uffd_zeropage_test(void)
+static void uffd_zeropage_test(uffd_test_args_t *args)
{
bool has_zeropage;
int i;
@@ -748,7 +758,76 @@ static void uffd_zeropage_test(void)
uffd_test_pass();
}
+/*
+ * Test the returned uffdio_register.ioctls with different register modes.
+ * Note that _UFFDIO_ZEROPAGE is tested separately in the zeropage test.
+ */
+static void
+do_register_ioctls_test(uffd_test_args_t *args, bool miss, bool wp, bool minor)
+{
+ uint64_t ioctls = 0, expected = BIT_ULL(_UFFDIO_WAKE);
+ mem_type_t *mem_type = args->mem_type;
+ int ret;
+
+ ret = uffd_register_with_ioctls(uffd, area_dst, page_size,
+ miss, wp, minor, &ioctls);
+
+ /*
+ * Handle special cases of UFFDIO_REGISTER here where it should
+ * just fail with -EINVAL first..
+ *
+ * Case 1: register MINOR on anon
+ * Case 2: register with no mode selected
+ */
+ if ((minor && (mem_type->mem_flag == MEM_ANON)) ||
+ (!miss && !wp && !minor)) {
+ if (ret != -EINVAL)
+ err("register (miss=%d, wp=%d, minor=%d) failed "
+ "with wrong errno=%d", miss, wp, minor, ret);
+ return;
+ }
+
+ /* UFFDIO_REGISTER should succeed, then check ioctls returned */
+ if (miss)
+ expected |= BIT_ULL(_UFFDIO_COPY);
+ if (wp)
+ expected |= BIT_ULL(_UFFDIO_WRITEPROTECT);
+ if (minor)
+ expected |= BIT_ULL(_UFFDIO_CONTINUE);
+
+ if ((ioctls & expected) != expected)
+ err("unexpected uffdio_register.ioctls "
+ "(miss=%d, wp=%d, minor=%d): expected=0x%"PRIx64", "
+ "returned=0x%"PRIx64, miss, wp, minor, expected, ioctls);
+
+ uffd_unregister(uffd, area_dst, page_size);
+}
+
+static void uffd_register_ioctls_test(uffd_test_args_t *args)
+{
+ int miss, wp, minor;
+
+ for (miss = 0; miss <= 1; miss++)
+ for (wp = 0; wp <= 1; wp++)
+ for (minor = 0; minor <= 1; minor++)
+ do_register_ioctls_test(args, miss, wp, minor);
+
+ uffd_test_pass();
+}
+
uffd_test_case_t uffd_tests[] = {
+ {
+ /* Test returned uffdio_register.ioctls. */
+ .name = "register-ioctls",
+ .uffd_fn = uffd_register_ioctls_test,
+ .mem_targets = MEM_ALL,
+ .uffd_feature_required = UFFD_FEATURE_MISSING_HUGETLBFS |
+ UFFD_FEATURE_MISSING_SHMEM |
+ UFFD_FEATURE_PAGEFAULT_FLAG_WP |
+ UFFD_FEATURE_WP_HUGETLBFS_SHMEM |
+ UFFD_FEATURE_MINOR_HUGETLBFS |
+ UFFD_FEATURE_MINOR_SHMEM,
+ },
{
.name = "zeropage",
.uffd_fn = uffd_zeropage_test,
@@ -835,6 +914,7 @@ int main(int argc, char *argv[])
int n_mems = sizeof(mem_types) / sizeof(mem_type_t);
uffd_test_case_t *test;
mem_type_t *mem_type;
+ uffd_test_args_t args;
char test_name[128];
const char *errmsg;
int has_uffd;
@@ -862,11 +942,12 @@ int main(int argc, char *argv[])
uffd_test_skip("feature missing");
continue;
}
- if (uffd_setup_environment(test, mem_type, &errmsg)) {
+ if (uffd_setup_environment(&args, test, mem_type,
+ &errmsg)) {
uffd_test_skip(errmsg);
continue;
}
- test->uffd_fn();
+ test->uffd_fn(&args);
}
}
--
2.39.1
© 2016 - 2025 Red Hat, Inc.