Re: [PATCH 13/29] selftests/mm: Split uffd tests into uffd-stress and uffd-unit-tests

[Mike Rapoport <rppt@linux.ibm.com>]

On Thu, Mar 30, 2023 at 12:07:23PM -0400, Peter Xu wrote:
> In many ways it's weird and unwanted to keep all the tests in the same
> userfaultfd.c at least when still in the current way.
> 
> For example, it doesn't make much sense to run the stress test for each
> method we can create an userfaultfd handle (either via syscall or /dev/
> node).  It's a waste of time running this twice for the whole stress as the
> stress paths are the same, only the open path is different.
> 
> It's also just weird to need to manually specify different types of memory
> to run all unit tests for the userfaultfd interface.  We should be able to
> just run a single program and that should go through all functional uffd
> tests without running the stress test at all.  The stress test was more for
> torturing and finding race conditions.  We don't want to wait for stress to
> finish just to regress test a functional test.
> 
> When we start to pile up more things on top of the same file and same
> functions, things start to go a bit chaos and the code is just harder to
> maintain too with tons of global variables.
> 
> This patch creates a new test uffd-unit-tests to keep userfaultfd unit
> tests in the future, currently empty.
> 
> Meanwhile rename the old userfaultfd.c test to uffd-stress.c.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org>

> ---
>  tools/testing/selftests/mm/.gitignore         |  3 ++-
>  tools/testing/selftests/mm/Makefile           |  8 +++---
>  tools/testing/selftests/mm/run_vmtests.sh     | 10 ++++---
>  .../mm/{userfaultfd.c => uffd-stress.c}       |  0
>  tools/testing/selftests/mm/uffd-unit-tests.c  | 27 +++++++++++++++++++
>  5 files changed, 40 insertions(+), 8 deletions(-)
>  rename tools/testing/selftests/mm/{userfaultfd.c => uffd-stress.c} (100%)
>  create mode 100644 tools/testing/selftests/mm/uffd-unit-tests.c
> 
> diff --git a/tools/testing/selftests/mm/.gitignore b/tools/testing/selftests/mm/.gitignore
> index 347277f2adc3..8917455f4f51 100644
> --- a/tools/testing/selftests/mm/.gitignore
> +++ b/tools/testing/selftests/mm/.gitignore
> @@ -21,7 +21,8 @@ protection_keys
>  protection_keys_32
>  protection_keys_64
>  madv_populate
> -userfaultfd
> +uffd-stress
> +uffd-unit-tests
>  mlock-intersect-test
>  mlock-random-test
>  virtual_address_range
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> index 9c3737285f8a..ca8bc4ac71e6 100644
> --- a/tools/testing/selftests/mm/Makefile
> +++ b/tools/testing/selftests/mm/Makefile
> @@ -20,7 +20,7 @@ MACHINE ?= $(shell echo $(uname_M) | sed -e 's/aarch64.*/arm64/' -e 's/ppc64.*/p
>  # Avoid accidental wrong builds, due to built-in rules working just a little
>  # bit too well--but not quite as well as required for our situation here.
>  #
> -# In other words, "make userfaultfd" is supposed to fail to build at all,
> +# In other words, "make $SOME_TEST" is supposed to fail to build at all,
>  # because this Makefile only supports either "make" (all), or "make /full/path".
>  # However,  the built-in rules, if not suppressed, will pick up CFLAGS and the
>  # initial LDLIBS (but not the target-specific LDLIBS, because those are only
> @@ -56,7 +56,8 @@ TEST_GEN_PROGS += mremap_test
>  TEST_GEN_PROGS += on-fault-limit
>  TEST_GEN_PROGS += thuge-gen
>  TEST_GEN_PROGS += transhuge-stress
> -TEST_GEN_PROGS += userfaultfd
> +TEST_GEN_PROGS += uffd-stress
> +TEST_GEN_PROGS += uffd-unit-tests
>  TEST_GEN_PROGS += soft-dirty
>  TEST_GEN_PROGS += split_huge_page_test
>  TEST_GEN_PROGS += ksm_tests
> @@ -107,7 +108,8 @@ include ../lib.mk
> 
>  $(TEST_GEN_PROGS): vm_util.c
> 
> -$(OUTPUT)/userfaultfd: uffd-common.c
> +$(OUTPUT)/uffd-stress: uffd-common.c
> +$(OUTPUT)/uffd-unit-tests: uffd-common.c
> 
>  ifeq ($(MACHINE),x86_64)
>  BINARIES_32 := $(patsubst %,$(OUTPUT)/%,$(BINARIES_32))
> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> index 9cc33984aa9f..af7bbc74cd83 100644
> --- a/tools/testing/selftests/mm/run_vmtests.sh
> +++ b/tools/testing/selftests/mm/run_vmtests.sh
> @@ -196,14 +196,16 @@ CATEGORY="gup_test" run_test ./gup_test -a
>  # Dump pages 0, 19, and 4096, using pin_user_pages:
>  CATEGORY="gup_test" run_test ./gup_test -ct -F 0x1 0 19 0x1000
> 
> +CATEGORY="userfaultfd" run_test ./uffd-unit-tests
>  uffd_mods=("" ":dev")
> +uffd_stress_bin=./uffd-stress
>  for mod in "${uffd_mods[@]}"; do
> -	CATEGORY="userfaultfd" run_test ./userfaultfd anon${mod} 20 16
> +	CATEGORY="userfaultfd" run_test ${uffd_stress_bin} anon${mod} 20 16
>  	# Hugetlb tests require source and destination huge pages. Pass in half
>  	# the size ($half_ufd_size_MB), which is used for *each*.
> -	CATEGORY="userfaultfd" run_test ./userfaultfd hugetlb${mod} "$half_ufd_size_MB" 32
> -	CATEGORY="userfaultfd" run_test ./userfaultfd hugetlb_shared${mod} "$half_ufd_size_MB" 32
> -	CATEGORY="userfaultfd" run_test ./userfaultfd shmem${mod} 20 16
> +	CATEGORY="userfaultfd" run_test ${uffd_stress_bin} hugetlb${mod} "$half_ufd_size_MB" 32
> +	CATEGORY="userfaultfd" run_test ${uffd_stress_bin} hugetlb_shared${mod} "$half_ufd_size_MB" 32
> +	CATEGORY="userfaultfd" run_test ${uffd_stress_bin} shmem${mod} 20 16
>  done
> 
>  #cleanup
> diff --git a/tools/testing/selftests/mm/userfaultfd.c b/tools/testing/selftests/mm/uffd-stress.c
> similarity index 100%
> rename from tools/testing/selftests/mm/userfaultfd.c
> rename to tools/testing/selftests/mm/uffd-stress.c
> diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
> new file mode 100644
> index 000000000000..6857388783be
> --- /dev/null
> +++ b/tools/testing/selftests/mm/uffd-unit-tests.c
> @@ -0,0 +1,27 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Userfaultfd unit tests.
> + *
> + *  Copyright (C) 2015-2023  Red Hat, Inc.
> + */
> +
> +#include "uffd-common.h"
> +
> +#ifdef __NR_userfaultfd
> +
> +int main(int argc, char *argv[])
> +{
> +	return KSFT_PASS;
> +}
> +
> +#else /* __NR_userfaultfd */
> +
> +#warning "missing __NR_userfaultfd definition"
> +
> +int main(void)
> +{
> +	printf("Skipping %s (missing __NR_userfaultfd)\n", __file__);
> +	return KSFT_SKIP;
> +}
> +
> +#endif /* __NR_userfaultfd */
> -- 
> 2.39.1
> 

-- 
Sincerely yours,
Mike.