[PATCH 0/6] selftests/mm: Some cleanups from trying to run them

[PATCH 0/6] selftests/mm: Some cleanups from trying to run them

[Brendan Jackman]

I never had much luck running mm selftests so I spent a couple of hours
digging into why.

Looks like most of the reason is missing SKIP checks, so this series is
just adding a bunch of those that I found. I did not do anything like
all of them, just the ones I spotted in gup_test, mmap, userfaultfd and
memfd_secret.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
Brendan Jackman (6):
      selftests/mm: Report errno when things fail
      selftests/mm: Fix assumption that sudo is present
      selftests/mm: Skip uffd-stress if userfaultfd not available
      selftests/mm: Skip uffd-wp-mremap if userfaultfd not available
      selftests/mm: Print some details when uffd-stress gets bad params
      selftests/mm: Don't fail uffd-stress if too many CPUs

 tools/testing/selftests/mm/gup_longterm.c   | 32 ++++++++++++++---------------
 tools/testing/selftests/mm/run_vmtests.sh   | 22 ++++++++++++++++----
 tools/testing/selftests/mm/uffd-stress.c    | 11 +++++++---
 tools/testing/selftests/mm/uffd-wp-mremap.c |  5 ++++-
 4 files changed, 46 insertions(+), 24 deletions(-)
---
base-commit: 87a132e73910e8689902aed7f2fc229d6908383b
change-id: 20250220-mm-selftests-2d7d0542face

Best regards,
-- 
Brendan Jackman <jackmanb@google.com>

[PATCH 1/6] selftests/mm: Report errno when things fail

[Brendan Jackman]

Just reporting failure doesn't tell you what went wrong. This can fail
in different ways so report errno to help the reader get started
debugging.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/mm/gup_longterm.c | 32 +++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
index 9423ad439a6140163bdef2974615bb86406a8c14..46a2139b3a646f6c050eb031a770f615be76c433 100644
--- a/tools/testing/selftests/mm/gup_longterm.c
+++ b/tools/testing/selftests/mm/gup_longterm.c
@@ -96,13 +96,13 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
 	int ret;
 
 	if (ftruncate(fd, size)) {
-		ksft_test_result_fail("ftruncate() failed\n");
+		ksft_test_result_fail("ftruncate() failed (%d)\n", errno);
 		return;
 	}
 
 	if (fallocate(fd, 0, 0, size)) {
 		if (size == pagesize)
-			ksft_test_result_fail("fallocate() failed\n");
+			ksft_test_result_fail("fallocate() failed (%d)\n", errno);
 		else
 			ksft_test_result_skip("need more free huge pages\n");
 		return;
@@ -112,7 +112,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
 		   shared ? MAP_SHARED : MAP_PRIVATE, fd, 0);
 	if (mem == MAP_FAILED) {
 		if (size == pagesize || shared)
-			ksft_test_result_fail("mmap() failed\n");
+			ksft_test_result_fail("mmap() failed (%d)\n", errno);
 		else
 			ksft_test_result_skip("need more free huge pages\n");
 		return;
@@ -130,7 +130,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
 		 */
 		ret = mprotect(mem, size, PROT_READ);
 		if (ret) {
-			ksft_test_result_fail("mprotect() failed\n");
+			ksft_test_result_fail("mprotect() failed (%d)\n", errno);
 			goto munmap;
 		}
 		/* FALLTHROUGH */
@@ -165,18 +165,18 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
 		args.flags |= rw ? PIN_LONGTERM_TEST_FLAG_USE_WRITE : 0;
 		ret = ioctl(gup_fd, PIN_LONGTERM_TEST_START, &args);
 		if (ret && errno == EINVAL) {
-			ksft_test_result_skip("PIN_LONGTERM_TEST_START failed\n");
+			ksft_test_result_skip("PIN_LONGTERM_TEST_START failed (EINVAL)n");
 			break;
 		} else if (ret && errno == EFAULT) {
 			ksft_test_result(!should_work, "Should have failed\n");
 			break;
 		} else if (ret) {
-			ksft_test_result_fail("PIN_LONGTERM_TEST_START failed\n");
+			ksft_test_result_fail("PIN_LONGTERM_TEST_START failed (%d)\n", errno);
 			break;
 		}
 
 		if (ioctl(gup_fd, PIN_LONGTERM_TEST_STOP))
-			ksft_print_msg("[INFO] PIN_LONGTERM_TEST_STOP failed\n");
+			ksft_print_msg("[INFO] PIN_LONGTERM_TEST_STOP failed (%d)\n", errno);
 
 		/*
 		 * TODO: if the kernel ever supports long-term R/W pinning on
@@ -202,7 +202,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
 		/* Skip on errors, as we might just lack kernel support. */
 		ret = io_uring_queue_init(1, &ring, 0);
 		if (ret < 0) {
-			ksft_test_result_skip("io_uring_queue_init() failed\n");
+			ksft_test_result_skip("io_uring_queue_init() failed (%d)\n", errno);
 			break;
 		}
 		/*
@@ -215,13 +215,13 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
 		/* Only new kernels return EFAULT. */
 		if (ret && (errno == ENOSPC || errno == EOPNOTSUPP ||
 			    errno == EFAULT)) {
-			ksft_test_result(!should_work, "Should have failed\n");
+			ksft_test_result(!should_work, "Should have failed (%d)\n", errno);
 		} else if (ret) {
 			/*
 			 * We might just lack support or have insufficient
 			 * MEMLOCK limits.
 			 */
-			ksft_test_result_skip("io_uring_register_buffers() failed\n");
+			ksft_test_result_skip("io_uring_register_buffers() failed (%d)\n", errno);
 		} else {
 			ksft_test_result(should_work, "Should have worked\n");
 			io_uring_unregister_buffers(&ring);
@@ -249,7 +249,7 @@ static void run_with_memfd(test_fn fn, const char *desc)
 
 	fd = memfd_create("test", 0);
 	if (fd < 0) {
-		ksft_test_result_fail("memfd_create() failed\n");
+		ksft_test_result_fail("memfd_create() failed (%d)\n", errno);
 		return;
 	}
 
@@ -266,13 +266,13 @@ static void run_with_tmpfile(test_fn fn, const char *desc)
 
 	file = tmpfile();
 	if (!file) {
-		ksft_test_result_fail("tmpfile() failed\n");
+		ksft_test_result_fail("tmpfile() failed (%d)\n", errno);
 		return;
 	}
 
 	fd = fileno(file);
 	if (fd < 0) {
-		ksft_test_result_fail("fileno() failed\n");
+		ksft_test_result_fail("fileno() failed (%d)\n", errno);
 		goto close;
 	}
 
@@ -290,12 +290,12 @@ static void run_with_local_tmpfile(test_fn fn, const char *desc)
 
 	fd = mkstemp(filename);
 	if (fd < 0) {
-		ksft_test_result_fail("mkstemp() failed\n");
+		ksft_test_result_fail("mkstemp() failed (%d)\n", errno);
 		return;
 	}
 
 	if (unlink(filename)) {
-		ksft_test_result_fail("unlink() failed\n");
+		ksft_test_result_fail("unlink() failed (%d)\n", errno);
 		goto close;
 	}
 
@@ -317,7 +317,7 @@ static void run_with_memfd_hugetlb(test_fn fn, const char *desc,
 
 	fd = memfd_create("test", flags);
 	if (fd < 0) {
-		ksft_test_result_skip("memfd_create() failed\n");
+		ksft_test_result_skip("memfd_create() failed (%d)\n", errno);
 		return;
 	}
 

-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH 1/6] selftests/mm: Report errno when things fail

[Dev Jain]

On 20/02/25 8:33 pm, Brendan Jackman wrote:
> Just reporting failure doesn't tell you what went wrong. This can fail
> in different ways so report errno to help the reader get started
> debugging.

IMHO it would be even better if we reported strerror(errno).

> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>   tools/testing/selftests/mm/gup_longterm.c | 32 +++++++++++++++----------------
>   1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
> index 9423ad439a6140163bdef2974615bb86406a8c14..46a2139b3a646f6c050eb031a770f615be76c433 100644
> --- a/tools/testing/selftests/mm/gup_longterm.c
> +++ b/tools/testing/selftests/mm/gup_longterm.c
> @@ -96,13 +96,13 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
>   	int ret;
>   
>   	if (ftruncate(fd, size)) {
> -		ksft_test_result_fail("ftruncate() failed\n");
> +		ksft_test_result_fail("ftruncate() failed (%d)\n", errno);
>   		return;
>   	}
>   
>   	if (fallocate(fd, 0, 0, size)) {
>   		if (size == pagesize)
> -			ksft_test_result_fail("fallocate() failed\n");
> +			ksft_test_result_fail("fallocate() failed (%d)\n", errno);
>   		else
>   			ksft_test_result_skip("need more free huge pages\n");
>   		return;
> @@ -112,7 +112,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
>   		   shared ? MAP_SHARED : MAP_PRIVATE, fd, 0);
>   	if (mem == MAP_FAILED) {
>   		if (size == pagesize || shared)
> -			ksft_test_result_fail("mmap() failed\n");
> +			ksft_test_result_fail("mmap() failed (%d)\n", errno);
>   		else
>   			ksft_test_result_skip("need more free huge pages\n");
>   		return;
> @@ -130,7 +130,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
>   		 */
>   		ret = mprotect(mem, size, PROT_READ);
>   		if (ret) {
> -			ksft_test_result_fail("mprotect() failed\n");
> +			ksft_test_result_fail("mprotect() failed (%d)\n", errno);
>   			goto munmap;
>   		}
>   		/* FALLTHROUGH */
> @@ -165,18 +165,18 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
>   		args.flags |= rw ? PIN_LONGTERM_TEST_FLAG_USE_WRITE : 0;
>   		ret = ioctl(gup_fd, PIN_LONGTERM_TEST_START, &args);
>   		if (ret && errno == EINVAL) {
> -			ksft_test_result_skip("PIN_LONGTERM_TEST_START failed\n");
> +			ksft_test_result_skip("PIN_LONGTERM_TEST_START failed (EINVAL)n");
>   			break;
>   		} else if (ret && errno == EFAULT) {
>   			ksft_test_result(!should_work, "Should have failed\n");
>   			break;
>   		} else if (ret) {
> -			ksft_test_result_fail("PIN_LONGTERM_TEST_START failed\n");
> +			ksft_test_result_fail("PIN_LONGTERM_TEST_START failed (%d)\n", errno);
>   			break;
>   		}
>   
>   		if (ioctl(gup_fd, PIN_LONGTERM_TEST_STOP))
> -			ksft_print_msg("[INFO] PIN_LONGTERM_TEST_STOP failed\n");
> +			ksft_print_msg("[INFO] PIN_LONGTERM_TEST_STOP failed (%d)\n", errno);
>   
>   		/*
>   		 * TODO: if the kernel ever supports long-term R/W pinning on
> @@ -202,7 +202,7 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
>   		/* Skip on errors, as we might just lack kernel support. */
>   		ret = io_uring_queue_init(1, &ring, 0);
>   		if (ret < 0) {
> -			ksft_test_result_skip("io_uring_queue_init() failed\n");
> +			ksft_test_result_skip("io_uring_queue_init() failed (%d)\n", errno);
>   			break;
>   		}
>   		/*
> @@ -215,13 +215,13 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
>   		/* Only new kernels return EFAULT. */
>   		if (ret && (errno == ENOSPC || errno == EOPNOTSUPP ||
>   			    errno == EFAULT)) {
> -			ksft_test_result(!should_work, "Should have failed\n");
> +			ksft_test_result(!should_work, "Should have failed (%d)\n", errno);
>   		} else if (ret) {
>   			/*
>   			 * We might just lack support or have insufficient
>   			 * MEMLOCK limits.
>   			 */
> -			ksft_test_result_skip("io_uring_register_buffers() failed\n");
> +			ksft_test_result_skip("io_uring_register_buffers() failed (%d)\n", errno);
>   		} else {
>   			ksft_test_result(should_work, "Should have worked\n");
>   			io_uring_unregister_buffers(&ring);
> @@ -249,7 +249,7 @@ static void run_with_memfd(test_fn fn, const char *desc)
>   
>   	fd = memfd_create("test", 0);
>   	if (fd < 0) {
> -		ksft_test_result_fail("memfd_create() failed\n");
> +		ksft_test_result_fail("memfd_create() failed (%d)\n", errno);
>   		return;
>   	}
>   
> @@ -266,13 +266,13 @@ static void run_with_tmpfile(test_fn fn, const char *desc)
>   
>   	file = tmpfile();
>   	if (!file) {
> -		ksft_test_result_fail("tmpfile() failed\n");
> +		ksft_test_result_fail("tmpfile() failed (%d)\n", errno);
>   		return;
>   	}
>   
>   	fd = fileno(file);
>   	if (fd < 0) {
> -		ksft_test_result_fail("fileno() failed\n");
> +		ksft_test_result_fail("fileno() failed (%d)\n", errno);
>   		goto close;
>   	}
>   
> @@ -290,12 +290,12 @@ static void run_with_local_tmpfile(test_fn fn, const char *desc)
>   
>   	fd = mkstemp(filename);
>   	if (fd < 0) {
> -		ksft_test_result_fail("mkstemp() failed\n");
> +		ksft_test_result_fail("mkstemp() failed (%d)\n", errno);
>   		return;
>   	}
>   
>   	if (unlink(filename)) {
> -		ksft_test_result_fail("unlink() failed\n");
> +		ksft_test_result_fail("unlink() failed (%d)\n", errno);
>   		goto close;
>   	}
>   
> @@ -317,7 +317,7 @@ static void run_with_memfd_hugetlb(test_fn fn, const char *desc,
>   
>   	fd = memfd_create("test", flags);
>   	if (fd < 0) {
> -		ksft_test_result_skip("memfd_create() failed\n");
> +		ksft_test_result_skip("memfd_create() failed (%d)\n", errno);
>   		return;
>   	}
>   
> 

[PATCH 2/6] selftests/mm: Fix assumption that sudo is present

[Brendan Jackman]

If we are root, sudo isn't needed. If we are not root, we need sudo, so
skip the test if it isn't present.

We already do this for on-fault-limit, but this uses separate
infrastructure since that is specifically for sudo-ing to the nobody
user.

Note this ptrace_skip configuration still fails if that file doesn't
exist, but in that case the test is still fine, so this just prints an
error but doesn't break anything. I suspect that's probably deliberate.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/mm/run_vmtests.sh | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
index da7e266681031d2772fb0c4139648904a18e0bf9..9c963f50927ab2b10c3f942cedd087087d4d0def 100755
--- a/tools/testing/selftests/mm/run_vmtests.sh
+++ b/tools/testing/selftests/mm/run_vmtests.sh
@@ -89,6 +89,17 @@ RUN_ALL=false
 RUN_DESTRUCTIVE=false
 TAP_PREFIX="# "
 
+# We can do stuff as root either if we are already root, or if sudo exists.
+if [ "$(id -u)" == 0 ]; then
+	HAVE_SUDO_ROOT=true
+	SUDO_ROOT=
+elif command -v sudo >/dev/null 2>&1; then
+	HAVE_SUDO_ROOT=true
+	SUDO_ROOT=sudo
+else
+	HAVE_SUDO_ROOT=false
+fi
+
 while getopts "aht:n" OPT; do
 	case ${OPT} in
 		"a") RUN_ALL=true ;;
@@ -384,10 +395,13 @@ CATEGORY="madv_guard" run_test ./guard-pages
 # MADV_POPULATE_READ and MADV_POPULATE_WRITE tests
 CATEGORY="madv_populate" run_test ./madv_populate
 
-if [ -x ./memfd_secret ]
-then
-(echo 0 | sudo tee /proc/sys/kernel/yama/ptrace_scope 2>&1) | tap_prefix
-CATEGORY="memfd_secret" run_test ./memfd_secret
+if [ -x ./memfd_secret ]; then
+	if $HAVE_SUDO_ROOT; then
+		(echo 0 | $SUDO_ROOT tee /proc/sys/kernel/yama/ptrace_scope 2>&1) | tap_prefix
+		CATEGORY="memfd_secret" run_test ./memfd_secret
+	else
+		echo "# SKIP ./memfd_secret"
+	fi
 fi
 
 # KSM KSM_MERGE_TIME_HUGE_PAGES test with size of 100

-- 
2.48.1.601.g30ceb7b040-goog

[PATCH 3/6] selftests/mm: Skip uffd-stress if userfaultfd not available

[Brendan Jackman]

It's pretty obvious that the test wouldn't work if you don't have the
feature enabled. But, it's still useful to SKIP instead of failing so
the reader can immediately tell that this is the reason why.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/mm/uffd-stress.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index a4b83280998ab7ce8d31e91d8f9fbb47ef11d742..db5366b4766e5bfa2d1150d2f3c2d32469a6e28b 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -411,8 +411,8 @@ static void parse_test_type_arg(const char *raw_type)
 	 * feature.
 	 */
 
-	if (uffd_get_features(&features))
-		err("failed to get available features");
+	if (uffd_get_features(&features) && errno == ENOENT)
+		ksft_exit_skip("failed to get avialable features (%d)\n", errno);
 
 	test_uffdio_wp = test_uffdio_wp &&
 		(features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);

-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH 3/6] selftests/mm: Skip uffd-stress if userfaultfd not available

[Dev Jain]

On 20/02/25 8:33 pm, Brendan Jackman wrote:
> It's pretty obvious that the test wouldn't work if you don't have the
> feature enabled. But, it's still useful to SKIP instead of failing so
> the reader can immediately tell that this is the reason why.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>   tools/testing/selftests/mm/uffd-stress.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index a4b83280998ab7ce8d31e91d8f9fbb47ef11d742..db5366b4766e5bfa2d1150d2f3c2d32469a6e28b 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -411,8 +411,8 @@ static void parse_test_type_arg(const char *raw_type)
>   	 * feature.
>   	 */
>   
> -	if (uffd_get_features(&features))
> -		err("failed to get available features");
> +	if (uffd_get_features(&features) && errno == ENOENT)
> +		ksft_exit_skip("failed to get avialable features (%d)\n", errno);
>   

s/avialable/available

>   	test_uffdio_wp = test_uffdio_wp &&
>   		(features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);
> 

Re: [PATCH 3/6] selftests/mm: Skip uffd-stress if userfaultfd not available

[Brendan Jackman]

On Thu, 20 Feb 2025 at 19:06, Dev Jain <dev.jain@arm.com> wrote:
> > -     if (uffd_get_features(&features))
> > -             err("failed to get available features");
> > +     if (uffd_get_features(&features) && errno == ENOENT)
> > +             ksft_exit_skip("failed to get avialable features (%d)\n", errno);
> >
>
> s/avialable/available

Oh thanks. I thought I had codespell running automatically on my
diffs, I'll have to look at why that wasn't caught.

[PATCH 4/6] selftests/mm: Skip uffd-wp-mremap if userfaultfd not available

[Brendan Jackman]

It's obvious that this should fail in that case, but still, save the
reader the effort of figuring out that they've run into this by just
SKIPping

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/mm/uffd-wp-mremap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/uffd-wp-mremap.c b/tools/testing/selftests/mm/uffd-wp-mremap.c
index 2c4f984bd73caa17e12b9f4a5bb71e7fdf5d8554..c2ba7d46c7b4581a3c32a6b6acd148e3e89c2172 100644
--- a/tools/testing/selftests/mm/uffd-wp-mremap.c
+++ b/tools/testing/selftests/mm/uffd-wp-mremap.c
@@ -182,7 +182,10 @@ static void test_one_folio(size_t size, bool private, bool swapout, bool hugetlb
 
 	/* Register range for uffd-wp. */
 	if (userfaultfd_open(&features)) {
-		ksft_test_result_fail("userfaultfd_open() failed\n");
+		if (errno == ENOENT)
+			ksft_test_result_skip("userfaultfd not available\n");
+		else
+			ksft_test_result_fail("userfaultfd_open() failed\n");
 		goto out;
 	}
 	if (uffd_register(uffd, mem, size, false, true, false)) {

-- 
2.48.1.601.g30ceb7b040-goog

[PATCH 5/6] selftests/mm: Print some details when uffd-stress gets bad params

[Brendan Jackman]

So this can be debugged more easily.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/mm/uffd-stress.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index db5366b4766e5bfa2d1150d2f3c2d32469a6e28b..1facfb79e09aa4113e344d7d90dec06a37264058 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -456,7 +456,8 @@ int main(int argc, char **argv)
 
 	nr_pages_per_cpu = bytes / page_size / nr_cpus;
 	if (!nr_pages_per_cpu) {
-		_err("invalid MiB");
+		_err("invalid MiB %lu (%lu / %lu / %lu)",
+			nr_pages_per_cpu, bytes, page_size, nr_cpus);
 		usage();
 	}
 

-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH 5/6] selftests/mm: Print some details when uffd-stress gets bad params

[Brendan Jackman]

On Thu, 20 Feb 2025 at 16:03, Brendan Jackman <jackmanb@google.com> wrote:
>         nr_pages_per_cpu = bytes / page_size / nr_cpus;
>         if (!nr_pages_per_cpu) {
> -               _err("invalid MiB");
> +               _err("invalid MiB %lu (%lu / %lu / %lu)",
> +                       nr_pages_per_cpu, bytes, page_size, nr_cpus);

Oh this is actually wrong - the number it's printing isn't MiB.

Assuming there's a v2 I'll fix that up. Otherwise honestly this patch
could just be dropped, it's not important.

[PATCH 6/6] selftests/mm: Don't fail uffd-stress if too many CPUs

[Brendan Jackman]

This calculation divides a fixed parameter by an environment-dependent
parameter i.e. the number of CPUs.

The simple way to avoid machine-specific failures here is to just put a
cap on the max value of the latter.

Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/mm/uffd-stress.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index 1facfb79e09aa4113e344d7d90dec06a37264058..f306accbef255c79bc3eeba8b9e42161a88fc10e 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -453,6 +453,10 @@ int main(int argc, char **argv)
 	}
 
 	nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+	if (nr_cpus > 32) {
+		/* Don't let calculation below go to zero. */
+		nr_cpus = 32;
+	}
 
 	nr_pages_per_cpu = bytes / page_size / nr_cpus;
 	if (!nr_pages_per_cpu) {

-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH 6/6] selftests/mm: Don't fail uffd-stress if too many CPUs

[Dev Jain]

On 20/02/25 8:33 pm, Brendan Jackman wrote:
> This calculation divides a fixed parameter by an environment-dependent
> parameter i.e. the number of CPUs.
> 
> The simple way to avoid machine-specific failures here is to just put a
> cap on the max value of the latter.

I haven't read the test, but if nr_cpus is being computed, then this 
value must be important to the test somehow? Would it potentially be 
wrong to let the test run for nr_cpus != actual number of cpus?

Also, if the patch is correct then will it be better to also print a 
diagnostic telling the user that the number of cpus is going to be 
capped for the test to run?

> 
> Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>   tools/testing/selftests/mm/uffd-stress.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index 1facfb79e09aa4113e344d7d90dec06a37264058..f306accbef255c79bc3eeba8b9e42161a88fc10e 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -453,6 +453,10 @@ int main(int argc, char **argv)
>   	}
>   
>   	nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
> +	if (nr_cpus > 32) {
> +		/* Don't let calculation below go to zero. */
> +		nr_cpus = 32;
> +	}
>   
>   	nr_pages_per_cpu = bytes / page_size / nr_cpus;
>   	if (!nr_pages_per_cpu) {
> 

Re: [PATCH 6/6] selftests/mm: Don't fail uffd-stress if too many CPUs

[Brendan Jackman]

On Thu, 20 Feb 2025 at 16:48, Dev Jain <dev.jain@arm.com> wrote:
>
>
>
> On 20/02/25 8:33 pm, Brendan Jackman wrote:
> > This calculation divides a fixed parameter by an environment-dependent
> > parameter i.e. the number of CPUs.
> >
> > The simple way to avoid machine-specific failures here is to just put a
> > cap on the max value of the latter.
>
> I haven't read the test, but if nr_cpus is being computed, then this
> value must be important to the test somehow? Would it potentially be
> wrong to let the test run for nr_cpus != actual number of cpus?

Based on my _extremely hasty_ reading, the variable is misnamed and
it's actually a thread count not a CPU count. I can double check
that's the case and rename it.

> Also, if the patch is correct then will it be better to also print a
> diagnostic telling the user that the number of cpus is going to be
> capped for the test to run?

Sure. The level of detail in the  logging and error messages is
extremely low here so I didn't feel like being too anomalous, but why
not.

Re: [PATCH 6/6] selftests/mm: Don't fail uffd-stress if too many CPUs

[Brendan Jackman]

On Thu, 20 Feb 2025 at 16:55, Brendan Jackman <jackmanb@google.com> wrote:
>
> On Thu, 20 Feb 2025 at 16:48, Dev Jain <dev.jain@arm.com> wrote:
> >
> >
> >
> > On 20/02/25 8:33 pm, Brendan Jackman wrote:
> > > This calculation divides a fixed parameter by an environment-dependent
> > > parameter i.e. the number of CPUs.
> > >
> > > The simple way to avoid machine-specific failures here is to just put a
> > > cap on the max value of the latter.
> >
> > I haven't read the test, but if nr_cpus is being computed, then this
> > value must be important to the test somehow? Would it potentially be
> > wrong to let the test run for nr_cpus != actual number of cpus?
>
> Based on my _extremely hasty_ reading, the variable is misnamed and
> it's actually a thread count not a CPU count. I can double check
> that's the case and rename it.

Oh yeah actually, it's only misnamed because I made it misnamed. So
this patch needs to rename it for sure, thanks for pointing it out.

(But yeah I upgraded my extremely hasty reading to an only hasty
reading and I still don't think this test cares about the actual CPU
topology).