Re: [PATCH v9 6/6] selftests/mm: add a GUP selftest

[David Hildenbrand (Arm)]

On 9/4/26 14:36, Sarthak Sharma wrote:
> Add a new GUP selftest which uses kselftest_harness.h. Cover
> 12 mapping configurations: THP enabled, THP disabled and
> HugeTLB, each across private/shared mappings and with/without
> FOLL_WRITE. Run 5 test cases for every variant: get_user_pages,
> get_user_pages_fast, pin_user_pages, pin_user_pages_fast and
> pin_user_pages_longterm.
> 
> Choose the number of default hugeTLB pages using a 256 MiB target,
> with a minimum of 1 page and derive the mapping size from that
> number. This avoids reserving excess memory when the hugeTLB page
> size is too large and retains 128 pages for the most common case
> of 2MiB hugeTLB pages.
> 
> Sweep four nr_pages_per_call values for each test: 1, 512, 123 and
> all pages. This preserves the coverage previously provided by
> run_gup_matrix(): 12 mapping combinations x 5 GUP/PUP operations x 4
> batch sizes. In total the selftest reports 60 TAP cases and issues
> 240 ioctls.
> 
> Do not carry DUMP_USER_PAGES_TEST into the new selftest because its
> output is written to the kernel log and the selftest does not verify
> that output.
> 
> Add the new gup binary to the selftests/mm build, run_vmtests.sh and
> MAINTAINERS. Update mm/Kconfig to describe the benchmark and
> selftest split.
> 
> Suggested-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> Acked-by: Mike Rapoport (Microsoft) <rppt@xxxxxxxxxx>
> Tested-by: Muhammad Usama Anjum <usama.anjum@xxxxxxx>
> Signed-off-by: Sarthak Sharma <sarthak.sharma@xxxxxxx>
> ---
>  MAINTAINERS                               |   1 +
>  mm/Kconfig                                |  21 +-
>  tools/testing/selftests/mm/Makefile       |   1 +
>  tools/testing/selftests/mm/gup.c          | 287 ++++++++++++++++++++++
>  tools/testing/selftests/mm/run_vmtests.sh |   1 +
>  5 files changed, 300 insertions(+), 11 deletions(-)
>  create mode 100644 tools/testing/selftests/mm/gup.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d7a146b093de..1ca7de3e440e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17189,6 +17189,7 @@ F:	mm/gup.c
>  F:	mm/gup_test.c
>  F:	mm/gup_test.h
>  F:	tools/mm/gup_bench.c
> +F:	tools/testing/selftests/mm/gup.c
>  F:	tools/testing/selftests/mm/gup_longterm.c
>  
>  MEMORY MANAGEMENT - KSM (Kernel Samepage Merging)
> diff --git a/mm/Kconfig b/mm/Kconfig
> index c1ddf59c0d71..79163b7d795a 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -1287,24 +1287,23 @@ config PERCPU_STATS
>  	  be used to help understand percpu memory usage.
>  
>  config GUP_TEST
> -	bool "Enable infrastructure for get_user_pages()-related unit tests"
> +	bool "Enable infrastructure for get_user_pages()-related unit tests and benchmarks"
>  	depends on DEBUG_FS
>  	help
>  	  Provides /sys/kernel/debug/gup_test, which in turn provides a way
> -	  to make ioctl calls that can launch kernel-based unit tests for
> -	  the get_user_pages*() and pin_user_pages*() family of API calls.
> +	  to make ioctl calls that can launch kernel-based unit tests and
> +	  benchmarks for the get_user_pages*() and pin_user_pages*() families
> +	  of API calls.
>  
> -	  These tests include benchmark testing of the _fast variants of
> -	  get_user_pages*() and pin_user_pages*(), as well as smoke tests of
> +	  These include benchmark testing of the _fast variants of
> +	  get_user_pages*() and pin_user_pages*(), as well as tests of
>  	  the non-_fast variants.
>  
> -	  There is also a sub-test that allows running dump_page() on any
> -	  of up to eight pages (selected by command line args) within the
> -	  range of user-space addresses. These pages are either pinned via
> -	  pin_user_pages*(), or pinned via get_user_pages*(), as specified
> -	  by other command line arguments.
> +	  There is also a test that allows running dump_page() on any of up
> +	  to eight pages within the range of user-space addresses. These
> +	  pages are either acquired via pin_user_pages*() or get_user_pages*().
>  
> -	  See tools/testing/selftests/mm/gup_test.c
> +	  See tools/testing/selftests/mm/gup.c and tools/mm/gup_bench.c.


BTW, I was wondering what it would take to:

1) Turn mm/gup_test.o into an OOT module (would we need more EXPORT_SYMBOL_GPL?
EXPORT_SYMBOL_FOOR_MODULE ?)

2) Move it to tools/mm/modules or sth like that.

3) Build it with the selftests etc

4) Remove GUP_TEST

5) Try insmod'ing it from the tools+selftests that need it.

[...]

> +int main(int argc, char **argv)
> +{
> +	char *file = "/dev/zero";
> +	int fd;
> +
> +	fd = open(file, O_RDWR);
> +	if (fd < 0) {
> +		ksft_print_header();
> +		ksft_exit_fail_msg("Unable to open %s: %s\n", file, strerror(errno));
> +	}
> +	close(fd);


I'm confused. Why do we have to open+close /dev/zero?

> +
> +	fd = open(GUP_TEST_FILE, O_RDWR);
> +	if (fd == -1) {
> +		ksft_print_header();
> +		if (errno == EACCES)
> +			ksft_exit_skip("Please run this test as root\n");

Wouldn't we want to fail here?

> +		if (errno == ENOENT) {
> +			DIR *debugfs = opendir("/sys/kernel/debug");
> +
> +			if (!debugfs) {
> +				ksft_exit_skip("Mount debugfs at /sys/kernel/debug\n");
> +			} else {
> +				closedir(debugfs);
> +				ksft_exit_skip("Check CONFIG_GUP_TEST in kernel config\n");
> +			}

You can remove the } else { part as you skip on !debugfs.

> +		}
> +		ksft_exit_fail_msg("Failed to open %s: %s\n", GUP_TEST_FILE, strerror(errno));
> +	}
> +	close(fd);
> +
> +	hp_size = default_huge_page_size();
> +	if (hp_size) {
> +		nr_huge_pages = HUGETLB_TARGET_SIZE / hp_size;
> +		if (!nr_huge_pages)
> +			nr_huge_pages = 1;
> +
> +		hugetlb_setup_succeeded = hugetlb_setup_default(nr_huge_pages);
> +	}

BTW, why are we using HUGETLB_TARGET_SIZE instead of just using the
default_huge_page_size()?

> +
> +	return test_harness_run(argc, argv);
> +}
> diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
> index 8f1e828e4f39..ae0ab5efabae 100755
> --- a/tools/testing/selftests/mm/run_vmtests.sh
> +++ b/tools/testing/selftests/mm/run_vmtests.sh
> @@ -251,6 +251,7 @@ fi
>  
>  CATEGORY="mmap" run_test ./map_fixed_noreplace
>  
> +CATEGORY="gup_test" run_test ./gup
>  CATEGORY="gup_test" run_test ./gup_longterm

Nice

-- 
Cheers,

David