[PATCH v3 00/10] selftests/mm: Some cleanups from trying to run them

[PATCH v3 00/10] selftests/mm: Some cleanups from trying to run them

[Brendan Jackman]

I never had much luck running mm selftests so I spent a few 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_longterm, gup_test, mmap,
userfaultfd and memfd_secret.

It's a bit unfortunate to have to skip those tests when ftruncate()
fails, but I don't have time to dig deep enough into it to actually make
them pass. I have observed the issue on 9pfs and heard rumours that NFS
has a similar problem.

I'm now able to run these test groups successfully:

- mmap
- gup_test
- compaction
- migration
- page_frag
- userfaultfd

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
Changes in v3:
- Added fix for userfaultfd tests.
- Dropped attempts to use sudo.
- Fixed garbage printf in uffd-stress.
  (Added EXTRA_CFLAGS=-Werror FORCE_TARGETS=1 to my scripts to prevent
   such errors happening again).
- Fixed missing newlines in ksft_test_result_skip() calls.
- Link to v2: https://lore.kernel.org/r/20250221-mm-selftests-v2-0-28c4d66383c5@google.com

Changes in v2 (Thanks to Dev for the reviews):
- Improve and cleanup some error messages
- Add some extra SKIPs
- Fix misnaming of nr_cpus variable in uffd tests
- Link to v1: https://lore.kernel.org/r/20250220-mm-selftests-v1-0-9bbf57d64463@google.com

---
Brendan Jackman (10):
      selftests/mm: Report errno when things fail in gup_longterm
      selftests/mm: Skip uffd-stress if userfaultfd not available
      selftests/mm: Skip uffd-wp-mremap if userfaultfd not available
      selftests/mm/uffd: Rename nr_cpus -> nr_threads
      selftests/mm: Print some details when uffd-stress gets bad params
      selftests/mm: Don't fail uffd-stress if too many CPUs
      selftests/mm: Skip map_populate on weird filesystems
      selftests/mm: Skip gup_longerm tests on weird filesystems
      selftests/mm: Drop unnecessary sudo usage
      selftests/mm: Ensure uffd-wp-mremap gets pages of each size

 tools/testing/selftests/mm/gup_longterm.c    | 45 ++++++++++++++++++----------
 tools/testing/selftests/mm/map_populate.c    |  7 +++++
 tools/testing/selftests/mm/run_vmtests.sh    | 25 ++++++++++++++--
 tools/testing/selftests/mm/uffd-common.c     |  8 ++---
 tools/testing/selftests/mm/uffd-common.h     |  2 +-
 tools/testing/selftests/mm/uffd-stress.c     | 42 ++++++++++++++++----------
 tools/testing/selftests/mm/uffd-unit-tests.c |  2 +-
 tools/testing/selftests/mm/uffd-wp-mremap.c  |  5 +++-
 8 files changed, 95 insertions(+), 41 deletions(-)
---
base-commit: 76544811c850a1f4c055aa182b513b7a843868ea
change-id: 20250220-mm-selftests-2d7d0542face

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

[PATCH v3 01/10] selftests/mm: Report errno when things fail in gup_longterm

[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 | 37 ++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
index 9423ad439a6140163bdef2974615bb86406a8c14..879e9e4e8cce8127656fabe098abf7db5f6c5e23 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 (%s)\n", strerror(errno));
 		return;
 	}
 
 	if (fallocate(fd, 0, 0, size)) {
 		if (size == pagesize)
-			ksft_test_result_fail("fallocate() failed\n");
+			ksft_test_result_fail("fallocate() failed (%s)\n", strerror(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 (%s)\n", strerror(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 (%s)\n", strerror(errno));
 			goto munmap;
 		}
 		/* FALLTHROUGH */
@@ -165,18 +165,20 @@ 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 (%s)\n",
+					      strerror(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 (%s)\n",
+				       strerror(errno));
 
 		/*
 		 * TODO: if the kernel ever supports long-term R/W pinning on
@@ -202,7 +204,8 @@ 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 (%s)\n",
+					      strerror(errno));
 			break;
 		}
 		/*
@@ -215,13 +218,15 @@ 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 (%s)\n",
+					 strerror(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 (%s)\n",
+					      strerror(errno));
 		} else {
 			ksft_test_result(should_work, "Should have worked\n");
 			io_uring_unregister_buffers(&ring);
@@ -249,7 +254,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 (%s)\n", strerror(errno));
 		return;
 	}
 
@@ -266,13 +271,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 (%s)\n", strerror(errno));
 		return;
 	}
 
 	fd = fileno(file);
 	if (fd < 0) {
-		ksft_test_result_fail("fileno() failed\n");
+		ksft_test_result_fail("fileno() failed (%s)\n", strerror(errno));
 		goto close;
 	}
 
@@ -290,12 +295,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 (%s)\n", strerror(errno));
 		return;
 	}
 
 	if (unlink(filename)) {
-		ksft_test_result_fail("unlink() failed\n");
+		ksft_test_result_fail("unlink() failed (%s)\n", strerror(errno));
 		goto close;
 	}
 
@@ -317,7 +322,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 (%s)\n", strerror(errno));
 		return;
 	}
 

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v3 02/10] 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..ed68436fac62c76e2ca7060c661487f2f8a6ab45 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 available features (%d)\n", errno);
 
 	test_uffdio_wp = test_uffdio_wp &&
 		(features & UFFD_FEATURE_PAGEFAULT_FLAG_WP);

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v3 03/10] 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.711.g2feabab25a-goog

[PATCH v3 04/10] selftests/mm/uffd: Rename nr_cpus -> nr_threads

[Brendan Jackman]

A later commit will bound this variable so it no longer necessarily
matches the number of CPUs. Rename it appropriately.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/mm/uffd-common.c     |  8 ++++----
 tools/testing/selftests/mm/uffd-common.h     |  2 +-
 tools/testing/selftests/mm/uffd-stress.c     | 28 ++++++++++++++--------------
 tools/testing/selftests/mm/uffd-unit-tests.c |  2 +-
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
index 717539eddf98754250e70e564cd9a59f398bd7ea..a72a2ed5e89480ed06c81b034967ed5ae5f8cad5 100644
--- a/tools/testing/selftests/mm/uffd-common.c
+++ b/tools/testing/selftests/mm/uffd-common.c
@@ -10,7 +10,7 @@
 #define BASE_PMD_ADDR ((void *)(1UL << 30))
 
 volatile bool test_uffdio_copy_eexist = true;
-unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size;
+unsigned long nr_threads, nr_pages, nr_pages_per_cpu, page_size;
 char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap;
 int uffd = -1, uffd_flags, finished, *pipefd, test_type;
 bool map_shared;
@@ -269,7 +269,7 @@ void uffd_test_ctx_clear(void)
 	size_t i;
 
 	if (pipefd) {
-		for (i = 0; i < nr_cpus * 2; ++i) {
+		for (i = 0; i < nr_threads * 2; ++i) {
 			if (close(pipefd[i]))
 				err("close pipefd");
 		}
@@ -365,10 +365,10 @@ int uffd_test_ctx_init(uint64_t features, const char **errmsg)
 	 */
 	uffd_test_ops->release_pages(area_dst);
 
-	pipefd = malloc(sizeof(int) * nr_cpus * 2);
+	pipefd = malloc(sizeof(int) * nr_threads * 2);
 	if (!pipefd)
 		err("pipefd");
-	for (cpu = 0; cpu < nr_cpus; cpu++)
+	for (cpu = 0; cpu < nr_threads; cpu++)
 		if (pipe2(&pipefd[cpu * 2], O_CLOEXEC | O_NONBLOCK))
 			err("pipe");
 
diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
index a70ae10b5f6206daecc8e19ed3e3bbb388e265aa..604e3572fe17280ae346b031e2e867e039578f95 100644
--- a/tools/testing/selftests/mm/uffd-common.h
+++ b/tools/testing/selftests/mm/uffd-common.h
@@ -98,7 +98,7 @@ struct uffd_test_case_ops {
 };
 typedef struct uffd_test_case_ops uffd_test_case_ops_t;
 
-extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size;
+extern unsigned long nr_threads, nr_pages, nr_pages_per_cpu, page_size;
 extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap;
 extern int uffd, uffd_flags, finished, *pipefd, test_type;
 extern bool map_shared;
diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index ed68436fac62c76e2ca7060c661487f2f8a6ab45..ec842bbb9f18e291fa51de0ed8d1fbf9aaf14372 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -179,12 +179,12 @@ static void *background_thread(void *arg)
 static int stress(struct uffd_args *args)
 {
 	unsigned long cpu;
-	pthread_t locking_threads[nr_cpus];
-	pthread_t uffd_threads[nr_cpus];
-	pthread_t background_threads[nr_cpus];
+	pthread_t locking_threads[nr_threads];
+	pthread_t uffd_threads[nr_threads];
+	pthread_t background_threads[nr_threads];
 
 	finished = 0;
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
+	for (cpu = 0; cpu < nr_threads; cpu++) {
 		if (pthread_create(&locking_threads[cpu], &attr,
 				   locking_thread, (void *)cpu))
 			return 1;
@@ -202,7 +202,7 @@ static int stress(struct uffd_args *args)
 				   background_thread, (void *)cpu))
 			return 1;
 	}
-	for (cpu = 0; cpu < nr_cpus; cpu++)
+	for (cpu = 0; cpu < nr_threads; cpu++)
 		if (pthread_join(background_threads[cpu], NULL))
 			return 1;
 
@@ -218,11 +218,11 @@ static int stress(struct uffd_args *args)
 	uffd_test_ops->release_pages(area_src);
 
 	finished = 1;
-	for (cpu = 0; cpu < nr_cpus; cpu++)
+	for (cpu = 0; cpu < nr_threads; cpu++)
 		if (pthread_join(locking_threads[cpu], NULL))
 			return 1;
 
-	for (cpu = 0; cpu < nr_cpus; cpu++) {
+	for (cpu = 0; cpu < nr_threads; cpu++) {
 		char c;
 		if (bounces & BOUNCE_POLL) {
 			if (write(pipefd[cpu*2+1], &c, 1) != 1)
@@ -245,11 +245,11 @@ static int userfaultfd_stress(void)
 {
 	void *area;
 	unsigned long nr;
-	struct uffd_args args[nr_cpus];
+	struct uffd_args args[nr_threads];
 	uint64_t mem_size = nr_pages * page_size;
 	int flags = 0;
 
-	memset(args, 0, sizeof(struct uffd_args) * nr_cpus);
+	memset(args, 0, sizeof(struct uffd_args) * nr_threads);
 
 	if (features & UFFD_FEATURE_WP_UNPOPULATED && test_type == TEST_ANON)
 		flags = UFFD_FEATURE_WP_UNPOPULATED;
@@ -324,7 +324,7 @@ static int userfaultfd_stress(void)
 		 */
 		uffd_test_ops->release_pages(area_dst);
 
-		uffd_stats_reset(args, nr_cpus);
+		uffd_stats_reset(args, nr_threads);
 
 		/* bounce pass */
 		if (stress(args)) {
@@ -358,7 +358,7 @@ static int userfaultfd_stress(void)
 
 		swap(area_src_alias, area_dst_alias);
 
-		uffd_stats_report(args, nr_cpus);
+		uffd_stats_report(args, nr_threads);
 	}
 	uffd_test_ctx_clear();
 
@@ -452,9 +452,9 @@ int main(int argc, char **argv)
 		return KSFT_SKIP;
 	}
 
-	nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+	nr_threads = sysconf(_SC_NPROCESSORS_ONLN);
 
-	nr_pages_per_cpu = bytes / page_size / nr_cpus;
+	nr_pages_per_cpu = bytes / page_size / nr_threads;
 	if (!nr_pages_per_cpu) {
 		_err("invalid MiB");
 		usage();
@@ -465,7 +465,7 @@ int main(int argc, char **argv)
 		_err("invalid bounces");
 		usage();
 	}
-	nr_pages = nr_pages_per_cpu * nr_cpus;
+	nr_pages = nr_pages_per_cpu * nr_threads;
 
 	printf("nr_pages: %lu, nr_pages_per_cpu: %lu\n",
 	       nr_pages, nr_pages_per_cpu);
diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
index 9ff71fa1f9bf09b3ae599250663a25bbe2c13b8a..2f84fae5642c6f91b75fbf5f5d59ae64a1c15f92 100644
--- a/tools/testing/selftests/mm/uffd-unit-tests.c
+++ b/tools/testing/selftests/mm/uffd-unit-tests.c
@@ -197,7 +197,7 @@ uffd_setup_environment(uffd_test_args_t *args, uffd_test_case_t *test,
 
 	nr_pages = UFFD_TEST_MEM_SIZE / page_size;
 	/* TODO: remove this global var.. it's so ugly */
-	nr_cpus = 1;
+	nr_threads = 1;
 
 	/* Initialize test arguments */
 	args->mem_type = mem_type;

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v3 05/10] 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 ec842bbb9f18e291fa51de0ed8d1fbf9aaf14372..efe2051c393096e237d942c04a264b6611a6e127 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_threads;
 	if (!nr_pages_per_cpu) {
-		_err("invalid MiB");
+		_err("pages_per_cpu = 0, cannot test (%lu / %lu / %lu)",
+			bytes, page_size, nr_threads);
 		usage();
 	}
 

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v3 06/10] 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 | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
index efe2051c393096e237d942c04a264b6611a6e127..5656128590373ed376b3b5d9259e5ca3867a4099 100644
--- a/tools/testing/selftests/mm/uffd-stress.c
+++ b/tools/testing/selftests/mm/uffd-stress.c
@@ -434,6 +434,7 @@ static void sigalrm(int sig)
 
 int main(int argc, char **argv)
 {
+	unsigned long nr_cpus;
 	size_t bytes;
 
 	if (argc < 4)
@@ -452,7 +453,15 @@ int main(int argc, char **argv)
 		return KSFT_SKIP;
 	}
 
-	nr_threads = sysconf(_SC_NPROCESSORS_ONLN);
+	nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
+	if (nr_cpus > 32) {
+		/* Don't let calculation below go to zero. */
+		ksft_print_msg("_SC_NPROCESSORS_ONLN (%lu) too large, capping nr_threads to 32\n",
+			       nr_cpus);
+		nr_threads = 32;
+	} else {
+		nr_cpus = nr_threads;
+	}
 
 	nr_pages_per_cpu = bytes / page_size / nr_threads;
 	if (!nr_pages_per_cpu) {

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v3 07/10] selftests/mm: Skip map_populate on weird filesystems

[Brendan Jackman]

It seems that 9pfs does not allow truncating unlinked files, Mark Brown
has noted that NFS may also behave this way.

It doesn't seem quite right to call this a "bug" but it's probably a
special enough case that it makes sense for the test to just SKIP if it
happens.

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

diff --git a/tools/testing/selftests/mm/map_populate.c b/tools/testing/selftests/mm/map_populate.c
index 5c8a53869b1bd287b09a250edf628a66c25c2439..433e54fb634f793f2eb4c53ba6b791045c9f4986 100644
--- a/tools/testing/selftests/mm/map_populate.c
+++ b/tools/testing/selftests/mm/map_populate.c
@@ -87,6 +87,13 @@ int main(int argc, char **argv)
 	BUG_ON(!ftmp, "tmpfile()");
 
 	ret = ftruncate(fileno(ftmp), MMAP_SZ);
+	if (ret < 0 && errno == ENOENT) {
+		/*
+		 * This probably means tmpfile() made a file on a filesystem
+		 * that doesn't handle temporary files the way we want.
+		 */
+		ksft_exit_skip("ftruncate(fileno(tmpfile())) gave ENOENT, weird filesystem?\n");
+	}
 	BUG_ON(ret, "ftruncate()");
 
 	smap = mmap(0, MMAP_SZ, PROT_READ | PROT_WRITE,

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[Brendan Jackman]

Some filesystems don't support funtract()ing unlinked files. They return
ENOENT. In that case, skip the test.

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

diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
index 879e9e4e8cce8127656fabe098abf7db5f6c5e23..494ec4102111b9c96fb4947b29c184735ceb8e1c 100644
--- a/tools/testing/selftests/mm/gup_longterm.c
+++ b/tools/testing/selftests/mm/gup_longterm.c
@@ -96,7 +96,15 @@ 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 (%s)\n", strerror(errno));
+		if (errno == ENOENT) {
+			/*
+			 * This can happen if the file has been unlinked and the
+			 * filesystem doesn't support truncating unlinked files.
+			 */
+			ksft_test_result_skip("ftruncate() failed with ENOENT\n");
+		} else {
+			ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno));
+		}
 		return;
 	}
 

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v3 09/10] selftests/mm: Drop unnecessary sudo usage

[Brendan Jackman]

This script must be run as root anyway (see all the writing to
privileged files in /proc etc).

Remove the unnecessary use of sudo to avoid breaking on single-user
systems that don't have sudo. This also avoids confusing readers.

Signed-off-by: Brendan Jackman <jackmanb@google.com>
---
 tools/testing/selftests/mm/run_vmtests.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
index da7e266681031d2772fb0c4139648904a18e0bf9..0f9fe757c3320a6551e39b6d4552fd4874b0bf43 100755
--- a/tools/testing/selftests/mm/run_vmtests.sh
+++ b/tools/testing/selftests/mm/run_vmtests.sh
@@ -386,7 +386,7 @@ 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
+(echo 0 > /proc/sys/kernel/yama/ptrace_scope 2>&1) | tap_prefix
 CATEGORY="memfd_secret" run_test ./memfd_secret
 fi
 

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v3 10/10] selftests/mm: Ensure uffd-wp-mremap gets pages of each size

[Brendan Jackman]

This test allocates a page of every available size and doesn't have any
SKIP logic if the allocation fails. So, ensure it's available and skip
the test if we can't do so.

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

diff --git a/tools/testing/selftests/mm/run_vmtests.sh b/tools/testing/selftests/mm/run_vmtests.sh
index 0f9fe757c3320a6551e39b6d4552fd4874b0bf43..e86ef8cb37d00e572be8cf0ea9cc8246d4eecd7e 100755
--- a/tools/testing/selftests/mm/run_vmtests.sh
+++ b/tools/testing/selftests/mm/run_vmtests.sh
@@ -309,9 +309,30 @@ CATEGORY="userfaultfd" run_test ${uffd_stress_bin} hugetlb "$half_ufd_size_MB" 3
 CATEGORY="userfaultfd" run_test ${uffd_stress_bin} hugetlb-private "$half_ufd_size_MB" 32
 CATEGORY="userfaultfd" run_test ${uffd_stress_bin} shmem 20 16
 CATEGORY="userfaultfd" run_test ${uffd_stress_bin} shmem-private 20 16
-CATEGORY="userfaultfd" run_test ./uffd-wp-mremap
+# uffd-wp-mremap requires at least one page of each size.
+have_all_size_hugepgs=true
+declare -A nr_size_hugepgs
+for f in /sys/kernel/mm/hugepages/**/nr_hugepages; do
+	old=$(cat $f)
+	nr_size_hugepgs["$f"]="$old"
+	if [ "$old" == 0 ]; then
+		echo 1 > "$f"
+	fi
+	if [ $(cat "$f") == 0 ]; then
+		have_all_size_hugepgs=false
+		break
+	fi
+done
+if $have_all_size_hugepgs; then
+	CATEGORY="userfaultfd" run_test ./uffd-wp-mremap
+else
+	echo "# SKIP ./uffd-wp-mremap"
+fi
 
 #cleanup
+for f in "${!nr_size_hugepgs[@]}"; do
+	echo "${nr_size_hugepgs["$f"]}" > "$f"
+done
 echo "$nr_hugepgs" > /proc/sys/vm/nr_hugepages
 
 CATEGORY="compaction" run_test ./compaction_test

-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v3 01/10] selftests/mm: Report errno when things fail in gup_longterm

[Dev Jain]

On 28/02/25 10:24 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.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Reviewed-by: Dev Jain <dev.jain@arm.com>

Re: [PATCH v3 02/10] selftests/mm: Skip uffd-stress if userfaultfd not available

[Dev Jain]

On 28/02/25 10:24 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..ed68436fac62c76e2ca7060c661487f2f8a6ab45 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 available features (%d)\n", errno);

Is it possible that uffd_get_features(&features) returns non-zero 
without errno == ENOENT?

Re: [PATCH v3 03/10] selftests/mm: Skip uffd-wp-mremap if userfaultfd not available

[Dev Jain]

On 28/02/25 10:24 pm, Brendan Jackman wrote:
> 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)) {
> 

I think you are correct, just want to confirm whether "uffd not 
available" if and only if "errno == ENOENT" is true. That is,
is it possible that errno can be something else and uffd is still not 
available, or errno can be ENOENT even if uffd is available.

Re: [PATCH v3 05/10] selftests/mm: Print some details when uffd-stress gets bad params

[Dev Jain]

On 28/02/25 10:24 pm, Brendan Jackman wrote:
> So this can be debugged more easily.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Reviewed-by: Dev Jain <dev.jain@arm.com>

Re: [PATCH v3 09/10] selftests/mm: Drop unnecessary sudo usage

[Dev Jain]

On 28/02/25 10:24 pm, Brendan Jackman wrote:
> This script must be run as root anyway (see all the writing to
> privileged files in /proc etc).
> 
> Remove the unnecessary use of sudo to avoid breaking on single-user
> systems that don't have sudo. This also avoids confusing readers.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>

Reviewed-by: Dev Jain <dev.jain@arm.com>

Re: [PATCH v3 04/10] selftests/mm/uffd: Rename nr_cpus -> nr_threads

[Dev Jain]

On 28/02/25 10:24 pm, Brendan Jackman wrote:
> A later commit will bound this variable so it no longer necessarily
> matches the number of CPUs. Rename it appropriately.
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>   tools/testing/selftests/mm/uffd-common.c     |  8 ++++----
>   tools/testing/selftests/mm/uffd-common.h     |  2 +-
>   tools/testing/selftests/mm/uffd-stress.c     | 28 ++++++++++++++--------------
>   tools/testing/selftests/mm/uffd-unit-tests.c |  2 +-
>   4 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/mm/uffd-common.c b/tools/testing/selftests/mm/uffd-common.c
> index 717539eddf98754250e70e564cd9a59f398bd7ea..a72a2ed5e89480ed06c81b034967ed5ae5f8cad5 100644
> --- a/tools/testing/selftests/mm/uffd-common.c
> +++ b/tools/testing/selftests/mm/uffd-common.c
> @@ -10,7 +10,7 @@
>   #define BASE_PMD_ADDR ((void *)(1UL << 30))
>   
>   volatile bool test_uffdio_copy_eexist = true;
> -unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size;
> +unsigned long nr_threads, nr_pages, nr_pages_per_cpu, page_size;
>   char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap;
>   int uffd = -1, uffd_flags, finished, *pipefd, test_type;
>   bool map_shared;
> @@ -269,7 +269,7 @@ void uffd_test_ctx_clear(void)
>   	size_t i;
>   
>   	if (pipefd) {
> -		for (i = 0; i < nr_cpus * 2; ++i) {
> +		for (i = 0; i < nr_threads * 2; ++i) {
>   			if (close(pipefd[i]))
>   				err("close pipefd");
>   		}
> @@ -365,10 +365,10 @@ int uffd_test_ctx_init(uint64_t features, const char **errmsg)
>   	 */
>   	uffd_test_ops->release_pages(area_dst);
>   
> -	pipefd = malloc(sizeof(int) * nr_cpus * 2);
> +	pipefd = malloc(sizeof(int) * nr_threads * 2);
>   	if (!pipefd)
>   		err("pipefd");
> -	for (cpu = 0; cpu < nr_cpus; cpu++)
> +	for (cpu = 0; cpu < nr_threads; cpu++)
>   		if (pipe2(&pipefd[cpu * 2], O_CLOEXEC | O_NONBLOCK))
>   			err("pipe");
>   
> diff --git a/tools/testing/selftests/mm/uffd-common.h b/tools/testing/selftests/mm/uffd-common.h
> index a70ae10b5f6206daecc8e19ed3e3bbb388e265aa..604e3572fe17280ae346b031e2e867e039578f95 100644
> --- a/tools/testing/selftests/mm/uffd-common.h
> +++ b/tools/testing/selftests/mm/uffd-common.h
> @@ -98,7 +98,7 @@ struct uffd_test_case_ops {
>   };
>   typedef struct uffd_test_case_ops uffd_test_case_ops_t;
>   
> -extern unsigned long nr_cpus, nr_pages, nr_pages_per_cpu, page_size;
> +extern unsigned long nr_threads, nr_pages, nr_pages_per_cpu, page_size;
>   extern char *area_src, *area_src_alias, *area_dst, *area_dst_alias, *area_remap;
>   extern int uffd, uffd_flags, finished, *pipefd, test_type;
>   extern bool map_shared;
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index ed68436fac62c76e2ca7060c661487f2f8a6ab45..ec842bbb9f18e291fa51de0ed8d1fbf9aaf14372 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -179,12 +179,12 @@ static void *background_thread(void *arg)
>   static int stress(struct uffd_args *args)
>   {
>   	unsigned long cpu;
> -	pthread_t locking_threads[nr_cpus];
> -	pthread_t uffd_threads[nr_cpus];
> -	pthread_t background_threads[nr_cpus];
> +	pthread_t locking_threads[nr_threads];
> +	pthread_t uffd_threads[nr_threads];
> +	pthread_t background_threads[nr_threads];
>   
>   	finished = 0;
> -	for (cpu = 0; cpu < nr_cpus; cpu++) {
> +	for (cpu = 0; cpu < nr_threads; cpu++) {
>   		if (pthread_create(&locking_threads[cpu], &attr,
>   				   locking_thread, (void *)cpu))
>   			return 1;
> @@ -202,7 +202,7 @@ static int stress(struct uffd_args *args)
>   				   background_thread, (void *)cpu))
>   			return 1;
>   	}
> -	for (cpu = 0; cpu < nr_cpus; cpu++)
> +	for (cpu = 0; cpu < nr_threads; cpu++)
>   		if (pthread_join(background_threads[cpu], NULL))
>   			return 1;
>   
> @@ -218,11 +218,11 @@ static int stress(struct uffd_args *args)
>   	uffd_test_ops->release_pages(area_src);
>   
>   	finished = 1;
> -	for (cpu = 0; cpu < nr_cpus; cpu++)
> +	for (cpu = 0; cpu < nr_threads; cpu++)
>   		if (pthread_join(locking_threads[cpu], NULL))
>   			return 1;
>   
> -	for (cpu = 0; cpu < nr_cpus; cpu++) {
> +	for (cpu = 0; cpu < nr_threads; cpu++) {
>   		char c;
>   		if (bounces & BOUNCE_POLL) {
>   			if (write(pipefd[cpu*2+1], &c, 1) != 1)
> @@ -245,11 +245,11 @@ static int userfaultfd_stress(void)
>   {
>   	void *area;
>   	unsigned long nr;
> -	struct uffd_args args[nr_cpus];
> +	struct uffd_args args[nr_threads];
>   	uint64_t mem_size = nr_pages * page_size;
>   	int flags = 0;
>   
> -	memset(args, 0, sizeof(struct uffd_args) * nr_cpus);
> +	memset(args, 0, sizeof(struct uffd_args) * nr_threads);
>   
>   	if (features & UFFD_FEATURE_WP_UNPOPULATED && test_type == TEST_ANON)
>   		flags = UFFD_FEATURE_WP_UNPOPULATED;
> @@ -324,7 +324,7 @@ static int userfaultfd_stress(void)
>   		 */
>   		uffd_test_ops->release_pages(area_dst);
>   
> -		uffd_stats_reset(args, nr_cpus);
> +		uffd_stats_reset(args, nr_threads);
>   
>   		/* bounce pass */
>   		if (stress(args)) {
> @@ -358,7 +358,7 @@ static int userfaultfd_stress(void)
>   
>   		swap(area_src_alias, area_dst_alias);
>   
> -		uffd_stats_report(args, nr_cpus);
> +		uffd_stats_report(args, nr_threads);
>   	}
>   	uffd_test_ctx_clear();
>   
> @@ -452,9 +452,9 @@ int main(int argc, char **argv)
>   		return KSFT_SKIP;
>   	}
>   
> -	nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
> +	nr_threads = sysconf(_SC_NPROCESSORS_ONLN);
>   
> -	nr_pages_per_cpu = bytes / page_size / nr_cpus;
> +	nr_pages_per_cpu = bytes / page_size / nr_threads;
>   	if (!nr_pages_per_cpu) {
>   		_err("invalid MiB");
>   		usage();
> @@ -465,7 +465,7 @@ int main(int argc, char **argv)
>   		_err("invalid bounces");
>   		usage();
>   	}
> -	nr_pages = nr_pages_per_cpu * nr_cpus;
> +	nr_pages = nr_pages_per_cpu * nr_threads;
>   
>   	printf("nr_pages: %lu, nr_pages_per_cpu: %lu\n",
>   	       nr_pages, nr_pages_per_cpu);
> diff --git a/tools/testing/selftests/mm/uffd-unit-tests.c b/tools/testing/selftests/mm/uffd-unit-tests.c
> index 9ff71fa1f9bf09b3ae599250663a25bbe2c13b8a..2f84fae5642c6f91b75fbf5f5d59ae64a1c15f92 100644
> --- a/tools/testing/selftests/mm/uffd-unit-tests.c
> +++ b/tools/testing/selftests/mm/uffd-unit-tests.c
> @@ -197,7 +197,7 @@ uffd_setup_environment(uffd_test_args_t *args, uffd_test_case_t *test,
>   
>   	nr_pages = UFFD_TEST_MEM_SIZE / page_size;
>   	/* TODO: remove this global var.. it's so ugly */
> -	nr_cpus = 1;
> +	nr_threads = 1;
>   
>   	/* Initialize test arguments */
>   	args->mem_type = mem_type;
> 

Taking a cursory look at the test, it creates three threads for each 
cpu. The bounding of the variable is fine but that being the reason to 
rename the variable is not making sense to me.

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

[Dev Jain]

On 28/02/25 10:24 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.
> 
> Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---

Reviewed-by: Dev Jain <dev.jain@arm.com>

Re: [PATCH v3 04/10] selftests/mm/uffd: Rename nr_cpus -> nr_threads

[Brendan Jackman]

On Fri, Feb 28, 2025 at 11:06:35PM +0530, Dev Jain wrote:
> Taking a cursory look at the test, it creates three threads for each cpu.
> The bounding of the variable is fine but that being the reason to rename the
> variable is not making sense to me.

Hmm yeah the name needs to be more abstract. Do you think nr_workers
would be confusing? Or even just "parallelism" or nr_parallel? Or any
other ideas?

FWIW I briefly looked at just cleaning this up to remove the global
variable but that's a bigger time investment than I can afford here I
think. (The local variable in stress() would still need a better name
anyway).

Thanks for the review BTW!

Re: [PATCH v3 04/10] selftests/mm/uffd: Rename nr_cpus -> nr_threads

[Dev Jain]

On 03/03/25 3:17 pm, Brendan Jackman wrote:
> On Fri, Feb 28, 2025 at 11:06:35PM +0530, Dev Jain wrote:
>> Taking a cursory look at the test, it creates three threads for each cpu.
>> The bounding of the variable is fine but that being the reason to rename the
>> variable is not making sense to me.
> 
> Hmm yeah the name needs to be more abstract. Do you think nr_workers
> would be confusing? Or even just "parallelism" or nr_parallel? Or any
> other ideas?
> 
> FWIW I briefly looked at just cleaning this up to remove the global
> variable but that's a bigger time investment than I can afford here I
> think. (The local variable in stress() would still need a better name
> anyway).
> 
> Thanks for the review BTW!

Your welcome.

I personally prefer leaving it as is; unless someone comes up and 
completely cleans up the structure, let us save our collective brain 
cycles for more meaningful battles than renaming variables :)

Re: [PATCH v3 04/10] selftests/mm/uffd: Rename nr_cpus -> nr_threads

[Brendan Jackman]

On Mon, Mar 03, 2025 at 03:48:38PM +0530, Dev Jain wrote:
> 
> 
> On 03/03/25 3:17 pm, Brendan Jackman wrote:
> > On Fri, Feb 28, 2025 at 11:06:35PM +0530, Dev Jain wrote:
> > > Taking a cursory look at the test, it creates three threads for each cpu.
> > > The bounding of the variable is fine but that being the reason to rename the
> > > variable is not making sense to me.
> > 
> > Hmm yeah the name needs to be more abstract. Do you think nr_workers
> > would be confusing? Or even just "parallelism" or nr_parallel? Or any
> > other ideas?
> > 
> > FWIW I briefly looked at just cleaning this up to remove the global
> > variable but that's a bigger time investment than I can afford here I
> > think. (The local variable in stress() would still need a better name
> > anyway).
> > 
> > Thanks for the review BTW!
> 
> Your welcome.
> 
> I personally prefer leaving it as is; unless someone comes up and completely
> cleans up the structure, let us save our collective brain cycles for more
> meaningful battles than renaming variables :)

Hmm, I think that's a false economy on brain cycles. A variable called
nr_cpus that isn't a number of CPUs is bound to waste a bunch of
mental energy at some point in the future.

Unless you strongly object I'll go for nr_parallel. It's not a great
name but, well... I think that probably just suggests it's not a great
variable, and I don't have time to fix that.

Re: [PATCH v3 04/10] selftests/mm/uffd: Rename nr_cpus -> nr_threads

[Dev Jain]

On 03/03/25 4:04 pm, Brendan Jackman wrote:
> On Mon, Mar 03, 2025 at 03:48:38PM +0530, Dev Jain wrote:
>>
>>
>> On 03/03/25 3:17 pm, Brendan Jackman wrote:
>>> On Fri, Feb 28, 2025 at 11:06:35PM +0530, Dev Jain wrote:
>>>> Taking a cursory look at the test, it creates three threads for each cpu.
>>>> The bounding of the variable is fine but that being the reason to rename the
>>>> variable is not making sense to me.
>>>
>>> Hmm yeah the name needs to be more abstract. Do you think nr_workers
>>> would be confusing? Or even just "parallelism" or nr_parallel? Or any
>>> other ideas?
>>>
>>> FWIW I briefly looked at just cleaning this up to remove the global
>>> variable but that's a bigger time investment than I can afford here I
>>> think. (The local variable in stress() would still need a better name
>>> anyway).
>>>
>>> Thanks for the review BTW!
>>
>> Your welcome.
>>
>> I personally prefer leaving it as is; unless someone comes up and completely
>> cleans up the structure, let us save our collective brain cycles for more
>> meaningful battles than renaming variables :)
> 
> Hmm, I think that's a false economy on brain cycles. A variable called
> nr_cpus that isn't a number of CPUs is bound to waste a bunch of
> mental energy at some point in the future.
> 
> Unless you strongly object I'll go for nr_parallel. It's not a great
> name but, well... I think that probably just suggests it's not a great
> variable, and I don't have time to fix that.

nr_parallel sounds better for sure. In case you send out a new patch:

Reviewed-by: Dev Jain <dev.jain@arm.com>

Re: [PATCH v3 03/10] selftests/mm: Skip uffd-wp-mremap if userfaultfd not available

[Brendan Jackman]

On Fri, Feb 28, 2025 at 10:55:00PM +0530, Dev Jain wrote:
> 
> 
> On 28/02/25 10:24 pm, Brendan Jackman wrote:
> > 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)) {
> > 
> 
> I think you are correct, just want to confirm whether "uffd not available"
> if and only if "errno == ENOENT" is true. That is,
> is it possible that errno can be something else and uffd is still not
> available, 

Yeah, I strongly suspect this can happen. This is an attempt to
improve things but I don't think it's a full solution.

I've been pondering this a bit and I think it's impractical to solve
problems like this in the code of individual testst. I think the right
thing to do is either:

1. Have a centralised facility for detecting conditions like
   "userfaultfd not available" that tests can just query it, so they
   say something like:

   ksft_test_requires("userfaultfd");

   Which would do some sort of actual principled check for presence
   and then skip the test with an informative message when it's not
   there. There would be a list of these "system requirements" in the
   code so you can easily see in one place what things might be needed
   to successfully run all the tests.

or

2. Specify out of band that there's a fixed set of requirements for
   running the tests and document that you shouldn't run them without
   satisfying them. Then just don't bother with SKIPs and call it user
   error.

   This would require some reasonably usable tooling for actually
   getting a system that satisfies the requirements.

But both of them require a deeper investment. I would quite like to
explore option 1 a bit but that's for a future Brendan. 

In the meantime I'm just trying to get these tests running on
virtme-ng. (I'm not even gonna add all of them, because e.g. once I
noticed this one I added a `scripts/config -e USERFAULTFD` to my
script, so I won't notice if anything else is missing the check).

> or errno can be ENOENT even if uffd is available.

I think it's probably posible for this to happen too, e.g. if the
system has a perverted /dev or something. But again I think that can
only be solved with the kinda stuff I mentioned above.

Sorry for the essay :D

Re: [PATCH v3 03/10] selftests/mm: Skip uffd-wp-mremap if userfaultfd not available

[Dev Jain]

+ Muhammad, I guess he has been working on selftests, maybe he can chime in.

On 03/03/25 4:18 pm, Brendan Jackman wrote:
> On Fri, Feb 28, 2025 at 10:55:00PM +0530, Dev Jain wrote:
>>
>>
>> On 28/02/25 10:24 pm, Brendan Jackman wrote:
>>> 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)) {
>>>
>>
>> I think you are correct, just want to confirm whether "uffd not available"
>> if and only if "errno == ENOENT" is true. That is,
>> is it possible that errno can be something else and uffd is still not
>> available,
> 
> Yeah, I strongly suspect this can happen. This is an attempt to
> improve things but I don't think it's a full solution.
> 
> I've been pondering this a bit and I think it's impractical to solve
> problems like this in the code of individual testst. I think the right
> thing to do is either:
> 
> 1. Have a centralised facility for detecting conditions like
>     "userfaultfd not available" that tests can just query it, so they
>     say something like:
> 
>     ksft_test_requires("userfaultfd");

Agreed, there should be a single point of reporting whether the facility 
is available.

> 
>     Which would do some sort of actual principled check for presence
>     and then skip the test with an informative message when it's not
>     there. There would be a list of these "system requirements" in the
>     code so you can easily see in one place what things might be needed
>     to successfully run all the tests.
> 
> or
> 
> 2. Specify out of band that there's a fixed set of requirements for
>     running the tests and document that you shouldn't run them without
>     satisfying them. Then just don't bother with SKIPs and call it user
>     error.
> 
>     This would require some reasonably usable tooling for actually
>     getting a system that satisfies the requirements.
> 
> But both of them require a deeper investment. I would quite like to
> explore option 1 a bit but that's for a future Brendan.
> 
> In the meantime I'm just trying to get these tests running on
> virtme-ng. (I'm not even gonna add all of them, because e.g. once I
> noticed this one I added a `scripts/config -e USERFAULTFD` to my
> script, so I won't notice if anything else is missing the check).
> 
>> or errno can be ENOENT even if uffd is available.
> 
> I think it's probably posible for this to happen too, e.g. if the
> system has a perverted /dev or something. But again I think that can
> only be solved with the kinda stuff I mentioned above.
> 
> Sorry for the essay :D

Re: [PATCH v3 00/10] selftests/mm: Some cleanups from trying to run them

[Muhammad Usama Anjum]

Hi,

Thanks for adding to the series Dev Jain. The series looks good. Thanks for doing
such a series. It helps everyone.

For the series:
Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

On 2/28/25 9:54 PM, Brendan Jackman wrote:
> I never had much luck running mm selftests so I spent a few 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_longterm, gup_test, mmap,
> userfaultfd and memfd_secret.
> 
> It's a bit unfortunate to have to skip those tests when ftruncate()
> fails, but I don't have time to dig deep enough into it to actually make
> them pass. I have observed the issue on 9pfs and heard rumours that NFS
> has a similar problem.
> 
> I'm now able to run these test groups successfully:
> 
> - mmap
> - gup_test
> - compaction
> - migration
> - page_frag
> - userfaultfd
> 
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
> Changes in v3:
> - Added fix for userfaultfd tests.
> - Dropped attempts to use sudo.
> - Fixed garbage printf in uffd-stress.
>   (Added EXTRA_CFLAGS=-Werror FORCE_TARGETS=1 to my scripts to prevent
>    such errors happening again).
> - Fixed missing newlines in ksft_test_result_skip() calls.
> - Link to v2: https://lore.kernel.org/r/20250221-mm-selftests-v2-0-28c4d66383c5@google.com
> 
> Changes in v2 (Thanks to Dev for the reviews):
> - Improve and cleanup some error messages
> - Add some extra SKIPs
> - Fix misnaming of nr_cpus variable in uffd tests
> - Link to v1: https://lore.kernel.org/r/20250220-mm-selftests-v1-0-9bbf57d64463@google.com
> 
> ---
> Brendan Jackman (10):
>       selftests/mm: Report errno when things fail in gup_longterm
>       selftests/mm: Skip uffd-stress if userfaultfd not available
>       selftests/mm: Skip uffd-wp-mremap if userfaultfd not available
>       selftests/mm/uffd: Rename nr_cpus -> nr_threads
>       selftests/mm: Print some details when uffd-stress gets bad params
>       selftests/mm: Don't fail uffd-stress if too many CPUs
>       selftests/mm: Skip map_populate on weird filesystems
>       selftests/mm: Skip gup_longerm tests on weird filesystems
>       selftests/mm: Drop unnecessary sudo usage
>       selftests/mm: Ensure uffd-wp-mremap gets pages of each size
> 
>  tools/testing/selftests/mm/gup_longterm.c    | 45 ++++++++++++++++++----------
>  tools/testing/selftests/mm/map_populate.c    |  7 +++++
>  tools/testing/selftests/mm/run_vmtests.sh    | 25 ++++++++++++++--
>  tools/testing/selftests/mm/uffd-common.c     |  8 ++---
>  tools/testing/selftests/mm/uffd-common.h     |  2 +-
>  tools/testing/selftests/mm/uffd-stress.c     | 42 ++++++++++++++++----------
>  tools/testing/selftests/mm/uffd-unit-tests.c |  2 +-
>  tools/testing/selftests/mm/uffd-wp-mremap.c  |  5 +++-
>  8 files changed, 95 insertions(+), 41 deletions(-)
> ---
> base-commit: 76544811c850a1f4c055aa182b513b7a843868ea
> change-id: 20250220-mm-selftests-2d7d0542face
> 
> Best regards,


-- 
BR,
Muhammad Usama Anjum

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

[Brendan Jackman]

On Fri, 28 Feb 2025 at 17:55, Brendan Jackman <jackmanb@google.com> 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.
>
> Suggested-by: Mateusz Guzik <mjguzik@gmail.com>
> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>  tools/testing/selftests/mm/uffd-stress.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/mm/uffd-stress.c b/tools/testing/selftests/mm/uffd-stress.c
> index efe2051c393096e237d942c04a264b6611a6e127..5656128590373ed376b3b5d9259e5ca3867a4099 100644
> --- a/tools/testing/selftests/mm/uffd-stress.c
> +++ b/tools/testing/selftests/mm/uffd-stress.c
> @@ -434,6 +434,7 @@ static void sigalrm(int sig)
>
>  int main(int argc, char **argv)
>  {
> +       unsigned long nr_cpus;
>         size_t bytes;
>
>         if (argc < 4)
> @@ -452,7 +453,15 @@ int main(int argc, char **argv)
>                 return KSFT_SKIP;
>         }
>
> -       nr_threads = sysconf(_SC_NPROCESSORS_ONLN);
> +       nr_cpus = sysconf(_SC_NPROCESSORS_ONLN);
> +       if (nr_cpus > 32) {
> +               /* Don't let calculation below go to zero. */
> +               ksft_print_msg("_SC_NPROCESSORS_ONLN (%lu) too large, capping nr_threads to 32\n",
> +                              nr_cpus);
> +               nr_threads = 32;
> +       } else {
> +               nr_cpus = nr_threads;

Won't have time to send v4 for a few days so I'll just note here: this
shoudl be nr_thread = nr_cpus. This causes a division by zero on
machines with less than 30 CPUs.

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[David Hildenbrand]

On 28.02.25 17:54, Brendan Jackman wrote:
> Some filesystems don't support funtract()ing unlinked files. They return
> ENOENT. In that case, skip the test.
> 

That's not documented in the man page, so is this a bug of these 
filesystems?

What are examples for these weird filesystems?

As we have the fstype available, we could instead simply reject more 
filesystems earlier. See fs_is_unknown().

> Signed-off-by: Brendan Jackman <jackmanb@google.com>
> ---
>   tools/testing/selftests/mm/gup_longterm.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
> index 879e9e4e8cce8127656fabe098abf7db5f6c5e23..494ec4102111b9c96fb4947b29c184735ceb8e1c 100644
> --- a/tools/testing/selftests/mm/gup_longterm.c
> +++ b/tools/testing/selftests/mm/gup_longterm.c
> @@ -96,7 +96,15 @@ 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 (%s)\n", strerror(errno));
> +		if (errno == ENOENT) {
> +			/*
> +			 * This can happen if the file has been unlinked and the
> +			 * filesystem doesn't support truncating unlinked files.
> +			 */
> +			ksft_test_result_skip("ftruncate() failed with ENOENT\n");
> +		} else {
> +			ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno));
> +		}
>   		return;
>   	}
>   
> 


-- 
Cheers,

David / dhildenb

Re: [PATCH v3 01/10] selftests/mm: Report errno when things fail in gup_longterm

[David Hildenbrand]

>   
>   		/*
>   		 * TODO: if the kernel ever supports long-term R/W pinning on
> @@ -202,7 +204,8 @@ 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 (%s)\n",
> +					      strerror(errno));

This function is documented to return -errno. I'm not sure if errno is 
guaranteed to be left unmodified (not clearly documented in the man 
page). So you might just want to use strerror(-ret) here.

Same applies to the other io_uring functions.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[Brendan Jackman]

On Thu, Mar 06, 2025 at 10:28:09AM +0100, David Hildenbrand wrote:
> On 28.02.25 17:54, Brendan Jackman wrote:
> > Some filesystems don't support funtract()ing unlinked files. They return
> > ENOENT. In that case, skip the test.
> > 
> 
> That's not documented in the man page, so is this a bug of these
> filesystems?

Um...

unlink(2) does say:

  If the name was the last link to a file but any processes still have
  the file open, the file will remain in existence until the last file
  descriptor referring to it is closed.

And POSIX says

  If one or more processes have the file open when the last link is
  removed, the link shall be removed before unlink() returns, but the
  removal of the file contents shall be postponed until all references
  to the file are closed

I didn't call it a bug in the commit message because my impression was
always that filesystem semantics are broadly determined by vibes. But
looking at the above I do feel more confident that the "unlink isn't
delete" thing is actually a pretty solid expectation.

> What are examples for these weird filesystems?

My experience of the issue is with 9pfs. broonie reported on #mm that
NFS can display similar issues but I haven't hit it myself.

> As we have the fstype available, we could instead simply reject more
> filesystems earlier. See fs_is_unknown().

Oh. I didn't know this was so easy, I thought that checking the
filesystem type would require some awful walk to find the mountpoint
and join it against the mount list. (Now I think about it, I should
have recorded this rationale in the commit message, so you could
easily see my bogus reasoning).

If there's a syscall to just say "what FS is this file on please?"
we should just do that and explicitly denylist the systems that are
known to have issues. I will just do 9pfs for now. Maybe we can log
warning if the error shows up on systems that aren't listed, then if
someone does run into it on NFS they should get a strong clue about
what the problem is.

Thanks!

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[David Hildenbrand]

On 06.03.25 13:42, Brendan Jackman wrote:
> On Thu, Mar 06, 2025 at 10:28:09AM +0100, David Hildenbrand wrote:
>> On 28.02.25 17:54, Brendan Jackman wrote:
>>> Some filesystems don't support funtract()ing unlinked files. They return
>>> ENOENT. In that case, skip the test.
>>>
>>
>> That's not documented in the man page, so is this a bug of these
>> filesystems?
> 
Note that I meant that ftruncate doesn't mention this in the man page.
The only occurrence is

"ENOENT The named file does not exist.", and that only applies to
truncate, not ftruncate.

> Um...
> 
> unlink(2) does say:
> 
>    If the name was the last link to a file but any processes still have
>    the file open, the file will remain in existence until the last file
>    descriptor referring to it is closed.
> 
> And POSIX says
> 
>    If one or more processes have the file open when the last link is
>    removed, the link shall be removed before unlink() returns, but the
>    removal of the file contents shall be postponed until all references
>    to the file are closed

Right, it's supposed to just stay around, it simply cannot be looked up anymore.

> I didn't call it a bug in the commit message because my impression was
> always that filesystem semantics are broadly determined by vibes. But
> looking at the above I do feel more confident that the "unlink isn't
> delete" thing is actually a pretty solid expectation.

I have a faint recollection that 9pfs is problematic with unlink ...
and indeed:

https://gitlab.com/qemu-project/qemu/-/issues/103

I'm not sure at this point what's expected to work and what not with
9pfs at this point.

> 
>> What are examples for these weird filesystems?
> 
> My experience of the issue is with 9pfs. broonie reported on #mm that
> NFS can display similar issues but I haven't hit it myself.
> >> As we have the fstype available, we could instead simply reject more
>> filesystems earlier. See fs_is_unknown().
> 
> Oh. I didn't know this was so easy, I thought that checking the
> filesystem type would require some awful walk to find the mountpoint
> and join it against the mount list. (Now I think about it, I should
> have recorded this rationale in the commit message, so you could
> easily see my bogus reasoning).
> 
> If there's a syscall to just say "what FS is this file on please?"
> we should just do that and explicitly denylist the systems that are
> known to have issues. I will just do 9pfs for now. Maybe we can log
> warning if the error shows up on systems that aren't listed, then if
> someone does run into it on NFS they should get a strong clue about
> what the problem is.

Yes, just skip 9pfs early, and mention in the commit message that 9pfs
has a history of being probematic with "use-after-unlink", maybe
mentioning the discussion I linked above.

Maybe something like this would work?

diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
index 9423ad439a614..349e40d3979f2 100644
--- a/tools/testing/selftests/mm/gup_longterm.c
+++ b/tools/testing/selftests/mm/gup_longterm.c
@@ -47,6 +47,16 @@ static __fsword_t get_fs_type(int fd)
         return ret ? 0 : fs.f_type;
  }
  
+static bool fs_is_problematic(__fsword_t fs_type)
+{
+       switch (fs_type) {
+       case V9FS_MAGIC:
+               return false;
+       default:
+               return true;
+       }
+}
+
  static bool fs_is_unknown(__fsword_t fs_type)
  {
         /*
@@ -95,6 +105,11 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
         char *mem;
         int ret;
  
+       if (fs_is_problematic(fs_type)) {
+               ksft_test_result_skip("problematic fs\n");
+               return;
+       }
+
         if (ftruncate(fd, size)) {
                 ksft_test_result_fail("ftruncate() failed\n");
                 return;


I am not 100% sure if V9FS_MAGIC is what we should be using? "man fstatfs" lists
most magic values.

-- 
Cheers,

David / dhildenb

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[Brendan Jackman]

On Thu, 6 Mar 2025 at 15:40, David Hildenbrand <david@redhat.com> wrote:
> Yes, just skip 9pfs early, and mention in the commit message that 9pfs
> has a history of being probematic with "use-after-unlink", maybe
> mentioning the discussion I linked above.
>
> Maybe something like this would work?
>
> diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
> index 9423ad439a614..349e40d3979f2 100644
> --- a/tools/testing/selftests/mm/gup_longterm.c
> +++ b/tools/testing/selftests/mm/gup_longterm.c
> @@ -47,6 +47,16 @@ static __fsword_t get_fs_type(int fd)
>          return ret ? 0 : fs.f_type;q
>   }
>
> +static bool fs_is_problematic(__fsword_t fs_type)
> +{
> +       switch (fs_type) {
> +       case V9FS_MAGIC:
> +               return false;
> +       default:
> +               return true;
> +       }
> +}

Ugh, some fun discoveries.

1. fstatfs() seems to have the same bug as ftruncate() i.e. it doesn't
work on unlinked files on 9pfs. This can be worked around by calling
it on the parent directory, but...

2. 9pfs seems to pass the f_type through from the host. So you can't
detect it this way anyway.

[3. I guess overlayfs & friends would also be an issue here although
that doesn't affect my usecase.]

Anyway, I think we would have to scrape /proc/mounts to do this :(

I think the proper way to deal with this is something like what I've
described here[0]. I.e. have a central facility as part of kselftest
to detect relevant characteristics of the platform. This logic could
be written in a proper programming language or in Bash, then the
relevant info could be passed in via the environment or whatever (e.g.
export KSFT_SYSENV_cwd_ftruncate_unlinked_works=1).

[0] https://lore.kernel.org/all/Z8WJEsEAwUPeMkqy@google.com/

But, to find an immediate way to get these tests working, I think we
are stuck with just peeking at errno and guessing for the time being.

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[David Hildenbrand]

On 11.03.25 14:00, Brendan Jackman wrote:
> On Thu, 6 Mar 2025 at 15:40, David Hildenbrand <david@redhat.com> wrote:
>> Yes, just skip 9pfs early, and mention in the commit message that 9pfs
>> has a history of being probematic with "use-after-unlink", maybe
>> mentioning the discussion I linked above.
>>
>> Maybe something like this would work?
>>
>> diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c
>> index 9423ad439a614..349e40d3979f2 100644
>> --- a/tools/testing/selftests/mm/gup_longterm.c
>> +++ b/tools/testing/selftests/mm/gup_longterm.c
>> @@ -47,6 +47,16 @@ static __fsword_t get_fs_type(int fd)
>>           return ret ? 0 : fs.f_type;q
>>    }
>>
>> +static bool fs_is_problematic(__fsword_t fs_type)
>> +{
>> +       switch (fs_type) {
>> +       case V9FS_MAGIC:
>> +               return false;
>> +       default:
>> +               return true;
>> +       }
>> +}
> 
> Ugh, some fun discoveries.
> 
> 1. fstatfs() seems to have the same bug as ftruncate() i.e. it doesn't
> work on unlinked files on 9pfs. This can be worked around by calling
> it on the parent directory, but...

oO what a piece of bad software :(

> 
> 2. 9pfs seems to pass the f_type through from the host. So you can't
> detect it this way anyway.
> 
> [3. I guess overlayfs & friends would also be an issue here although
> that doesn't affect my usecase.]
> 
> Anyway, I think we would have to scrape /proc/mounts to do this :(
> 

The question I am asking myself: is this a 9pfs design bug or is it a 
9pfs hypervisor bug. Because we shouldn't try too hard to work around 
hypervisor bugs.

Which 9pfs implementation are you using in the hypervisor?

> I think the proper way to deal with this is something like what I've
> described here[0]. I.e. have a central facility as part of kselftest
> to detect relevant characteristics of the platform. This logic could
> be written in a proper programming language or in Bash, then the
> relevant info could be passed in via the environment or whatever (e.g.
> export KSFT_SYSENV_cwd_ftruncate_unlinked_works=1).
> 
> [0] https://lore.kernel.org/all/Z8WJEsEAwUPeMkqy@google.com/
> 
> But, to find an immediate way to get these tests working, I think we
> are stuck with just peeking at errno and guessing for the time being.



-- 
Cheers,

David / dhildenb

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[Brendan Jackman]

On Tue, Mar 11, 2025 at 08:53:02PM +0100, David Hildenbrand wrote:
> > 2. 9pfs seems to pass the f_type through from the host. So you can't
> > detect it this way anyway.
> > 
> > [3. I guess overlayfs & friends would also be an issue here although
> > that doesn't affect my usecase.]
> > 
> > Anyway, I think we would have to scrape /proc/mounts to do this :(
> > 
> 
> The question I am asking myself: is this a 9pfs design bug or is it a 9pfs
> hypervisor bug. Because we shouldn't try too hard to work around hypervisor
> bugs.
> 
> Which 9pfs implementation are you using in the hypervisor?

I'm using QEMU via virtme-ng. IIUC virtme-ng knows how to use viortfs
for the rootfs, but for individually-mounted directories with
--rwdir/--rodir it uses 9pfs unconditionally.

Even if it's a bug in QEMU, I think it is worth working around this
one way or another. QEMU by far the most practical way to run these
tests, and virtme-ng is probably the most popular/practical way to do
that. I think even if we are confident it's just a bunch of broken
code that isn't even in Linux, it's pragmatic to spend a certain
amount of energy on having green tests there.

(Also, this f_type thing might be totally intentional specified
filesystem behaviour, I don't know).

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[David Hildenbrand]

On 12.03.25 09:34, Brendan Jackman wrote:
> On Tue, Mar 11, 2025 at 08:53:02PM +0100, David Hildenbrand wrote:
>>> 2. 9pfs seems to pass the f_type through from the host. So you can't
>>> detect it this way anyway.
>>>
>>> [3. I guess overlayfs & friends would also be an issue here although
>>> that doesn't affect my usecase.]
>>>
>>> Anyway, I think we would have to scrape /proc/mounts to do this :(
>>>
>>
>> The question I am asking myself: is this a 9pfs design bug or is it a 9pfs
>> hypervisor bug. Because we shouldn't try too hard to work around hypervisor
>> bugs.
>>
>> Which 9pfs implementation are you using in the hypervisor?
> 
> I'm using QEMU via virtme-ng. IIUC virtme-ng knows how to use viortfs
> for the rootfs, but for individually-mounted directories with
> --rwdir/--rodir it uses 9pfs unconditionally.

Ah okay, that makes sense.

> 
> Even if it's a bug in QEMU, I think it is worth working around this
> one way or another. QEMU by far the most practical way to run these
> tests, and virtme-ng is probably the most popular/practical way to do
> that.

I'm afraid yes. Although allocating temp files form 9pfs is rather ... 
weird. :) One would assume that /tmp is usually backed by tmpfs. But 
well, a disto can do what it wants.

> I think even if we are confident it's just a bunch of broken
> code that isn't even in Linux, it's pragmatic to spend a certain
> amount of energy on having green tests there.
> 

Yeah, we're trying ...

> (Also, this f_type thing might be totally intentional specified
> filesystem behaviour, I don't know).

I assume it's broken in various ways to mimic that you are a file system 
which you are not.

Your approach is likely the easiest approach to deal with this 9pfs crap.

Can you document in the code+description better what we learned, and why 
we cannot even trust f_type with crappy 9pfs?

---
Cheers,

David / dhildenb

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[David Hildenbrand]

On 14.03.25 13:10, David Hildenbrand wrote:
> On 12.03.25 09:34, Brendan Jackman wrote:
>> On Tue, Mar 11, 2025 at 08:53:02PM +0100, David Hildenbrand wrote:
>>>> 2. 9pfs seems to pass the f_type through from the host. So you can't
>>>> detect it this way anyway.
>>>>
>>>> [3. I guess overlayfs & friends would also be an issue here although
>>>> that doesn't affect my usecase.]
>>>>
>>>> Anyway, I think we would have to scrape /proc/mounts to do this :(
>>>>
>>>
>>> The question I am asking myself: is this a 9pfs design bug or is it a 9pfs
>>> hypervisor bug. Because we shouldn't try too hard to work around hypervisor
>>> bugs.
>>>
>>> Which 9pfs implementation are you using in the hypervisor?
>>
>> I'm using QEMU via virtme-ng. IIUC virtme-ng knows how to use viortfs
>> for the rootfs, but for individually-mounted directories with
>> --rwdir/--rodir it uses 9pfs unconditionally.
> 
> Ah okay, that makes sense.
> 
>>
>> Even if it's a bug in QEMU, I think it is worth working around this
>> one way or another. QEMU by far the most practical way to run these
>> tests, and virtme-ng is probably the most popular/practical way to do
>> that.
> 
> I'm afraid yes. Although allocating temp files form 9pfs is rather ...
> weird. :) One would assume that /tmp is usually backed by tmpfs. But
> well, a disto can do what it wants.
> 
>> I think even if we are confident it's just a bunch of broken
>> code that isn't even in Linux, it's pragmatic to spend a certain
>> amount of energy on having green tests there.
>>
> 
> Yeah, we're trying ...
> 
>> (Also, this f_type thing might be totally intentional specified
>> filesystem behaviour, I don't know).
> 
> I assume it's broken in various ways to mimic that you are a file system
> which you are not.
> 
> Your approach is likely the easiest approach to deal with this 9pfs crap.
> 
> Can you document in the code+description better what we learned, and why
> we cannot even trust f_type with crappy 9pfs?

Staring a bit at that code, it's mostly 9p specific I think.

t14s: ~/git/linux s390x-file-thp2 $ git grep "= NFS_SUPER_MAGIC"
fs/nfs/super.c: buf->f_type = NFS_SUPER_MAGIC;
fs/nfs/super.c: sb->s_magic = NFS_SUPER_MAGIC;

t14s: ~/git/linux s390x-file-thp2 $ git grep "= V9FS_MAGIC"
fs/9p/vfs_super.c:      sb->s_magic = V9FS_MAGIC;
  $ git grep "f_type" | grep 9p
fs/9p/vfs_super.c:                      buf->f_type = rs.type;


-- 
Cheers,

David / dhildenb

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[Brendan Jackman]

> > Even if it's a bug in QEMU, I think it is worth working around this
> > one way or another. QEMU by far the most practical way to run these
> > tests, and virtme-ng is probably the most popular/practical way to do
> > that.
> 
> I'm afraid yes. Although allocating temp files form 9pfs is rather ...
> weird. :) One would assume that /tmp is usually backed by tmpfs. But well, a
> disto can do what it wants.

Ah yeah but these tests also use mkstemp() in the CWD i.e. the
location of run_vmtests.sh, it isn't /tmp that is causing this at the
moment. (At some point I thought I was hitting the issue there too,
but I think I was mistaken, like just not reading the test logs
properly or something).

> > I think even if we are confident it's just a bunch of broken
> > code that isn't even in Linux, it's pragmatic to spend a certain
> > amount of energy on having green tests there.
> > 
> 
> Yeah, we're trying ...
> 
> > (Also, this f_type thing might be totally intentional specified
> > filesystem behaviour, I don't know).
> 
> I assume it's broken in various ways to mimic that you are a file system
> which you are not.
> 
> Your approach is likely the easiest approach to deal with this 9pfs crap.
> 
> Can you document in the code+description better what we learned, and why we
> cannot even trust f_type with crappy 9pfs?

Sure, I will be more verbose about it.

I've already sent v4 here:

https://lore.kernel.org/all/20250311-mm-selftests-v4-7-dec210a658f5@google.com/

So I will wait and see if there are any comments on the v4, if there
are I'll spin the extra commentary into v5 otherwise send it as a
followup, does that sound OK?

Re: [PATCH v3 08/10] selftests/mm: Skip gup_longerm tests on weird filesystems

[David Hildenbrand]

On 14.03.25 16:56, Brendan Jackman wrote:
>>> Even if it's a bug in QEMU, I think it is worth working around this
>>> one way or another. QEMU by far the most practical way to run these
>>> tests, and virtme-ng is probably the most popular/practical way to do
>>> that.
>>
>> I'm afraid yes. Although allocating temp files form 9pfs is rather ...
>> weird. :) One would assume that /tmp is usually backed by tmpfs. But well, a
>> disto can do what it wants.
> 
> Ah yeah but these tests also use mkstemp() in the CWD i.e. the
> location of run_vmtests.sh, it isn't /tmp that is causing this at the
> moment. (At some point I thought I was hitting the issue there too,
> but I think I was mistaken, like just not reading the test logs
> properly or something).

Ah, yes run_with_local_tmpfile() ... jep, I wrote that test, now my 
memory comes back; we wanted to test with actual filesystems (e.g., 
ext4, xfs) easily.

> 
>>> I think even if we are confident it's just a bunch of broken
>>> code that isn't even in Linux, it's pragmatic to spend a certain
>>> amount of energy on having green tests there.
>>>
>>
>> Yeah, we're trying ...
>>
>>> (Also, this f_type thing might be totally intentional specified
>>> filesystem behaviour, I don't know).
>>
>> I assume it's broken in various ways to mimic that you are a file system
>> which you are not.
>>
>> Your approach is likely the easiest approach to deal with this 9pfs crap.
>>
>> Can you document in the code+description better what we learned, and why we
>> cannot even trust f_type with crappy 9pfs?
> 
> Sure, I will be more verbose about it.
> 
> I've already sent v4 here:
> 
> https://lore.kernel.org/all/20250311-mm-selftests-v4-7-dec210a658f5@google.com/
> 
> So I will wait and see if there are any comments on the v4, if there
> are I'll spin the extra commentary into v5 otherwise send it as a
> followup, does that sound OK?


You can just ask Andrew to fixup the comment or description in a reply 
to the v4 patch. Andrew will let you know if he prefers a resend.

Thanks!


-- 
Cheers,

David / dhildenb