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,
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; }
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
/* * 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.
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);
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?
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)) {
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");elseksft_test_result_fail("userfaultfd_open() failed\n");
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.
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");elseksft_test_result_fail("userfaultfd_open() failed\n");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
+ 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");elseksft_test_result_fail("userfaultfd_open() failed\n"); goto out;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:
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
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
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;
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);@@ -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.
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!
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 :)
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.
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
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(); }
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
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) {
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
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.
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,
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; }
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));}
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!
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.
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.
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.
- 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 :(
- 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.
On Tue, Mar 11, 2025 at 08:53:02PM +0100, David Hildenbrand wrote:
- 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).
On 12.03.25 09:34, Brendan Jackman wrote:
On Tue, Mar 11, 2025 at 08:53:02PM +0100, David Hildenbrand wrote:
- 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
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:
- 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;
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.c...
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?
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.c...
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!
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
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
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
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,
linux-kselftest-mirror@lists.linaro.org
-
Brendan Jackman -
David Hildenbrand -
Dev Jain -
Muhammad Usama Anjum