Re: [PATCH 14/29] selftests/mm: uffd_[un]register()

Mike Rapoport
2023-04-07T11:09:13Z

On 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.