The bulk of these changes modify the cow and gup_longterm tests to report unique and stable names for each test, bringing them into line with the expectations of tooling that works with kselftest. The string reported as a test result is used by tooling to both deduplicate tests and track tests between test runs, using the same string for multiple tests or changing the string depending on test result causes problems for user interfaces and automation such as bisection.
It was suggested that converting to use kselftest_harness.h would be a good way of addressing this, however that really wants the set of tests to run to be known at compile time but both test programs dynamically enumarate the set of huge page sizes the system supports and test each. Refactoring to handle this would be even more invasive than these changes which are large but straightforward and repetitive.
A version of the main gup_longterm cleanup was previously sent separately, this version factors out the helpers for logging the start of the test since the cow test looks very similar.
Signed-off-by: Mark Brown broonie@kernel.org --- Changes in v2: - Typo fixes. - Link to v1: https://lore.kernel.org/r/20250522-selftests-mm-cow-dedupe-v1-0-713cee2fdd6d...
--- Mark Brown (4): selftests/mm: Use standard ksft_finished() in cow and gup_longterm selftests/mm: Add helper for logging test start and results selftests/mm: Report unique test names for each cow test selftests/mm: Fix test result reporting in gup_longterm
tools/testing/selftests/mm/cow.c | 340 +++++++++++++++++++----------- tools/testing/selftests/mm/gup_longterm.c | 158 ++++++++------ tools/testing/selftests/mm/vm_util.h | 20 ++ 3 files changed, 334 insertions(+), 184 deletions(-) --- base-commit: a5806cd506af5a7c19bcd596e4708b5c464bfd21 change-id: 20250521-selftests-mm-cow-dedupe-33dcab034558
Best regards,
The cow and gup_longterm test programs open code something that looks a lot like the standard ksft_finished() helper to summarise the test results and provide an exit code, convert to use ksft_finished().
Acked-by: David Hildenbrand david@redhat.com Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/mm/cow.c | 7 +------ tools/testing/selftests/mm/gup_longterm.c | 8 ++------ 2 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index b6cfe0a4b7df..e70cd3d900cc 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -1771,7 +1771,6 @@ static int tests_per_non_anon_test_case(void)
int main(int argc, char **argv) { - int err; struct thp_settings default_settings;
ksft_print_header(); @@ -1811,9 +1810,5 @@ int main(int argc, char **argv) thp_restore_settings(); }
- err = ksft_get_fail_cnt(); - if (err) - ksft_exit_fail_msg("%d out of %d tests failed\n", - err, ksft_test_num()); - ksft_exit_pass(); + ksft_finished(); } diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c index 21595b20bbc3..e60e62809186 100644 --- a/tools/testing/selftests/mm/gup_longterm.c +++ b/tools/testing/selftests/mm/gup_longterm.c @@ -455,7 +455,7 @@ static int tests_per_test_case(void)
int main(int argc, char **argv) { - int i, err; + int i;
pagesize = getpagesize(); nr_hugetlbsizes = detect_hugetlb_page_sizes(hugetlbsizes, @@ -469,9 +469,5 @@ int main(int argc, char **argv) for (i = 0; i < ARRAY_SIZE(test_cases); i++) run_test_case(&test_cases[i]);
- err = ksft_get_fail_cnt(); - if (err) - ksft_exit_fail_msg("%d out of %d tests failed\n", - err, ksft_test_num()); - ksft_exit_pass(); + ksft_finished(); }
Several of the MM tests have a pattern of printing a description of the test to be run then reporting the actual TAP result using a generic string not connected to the specific test, often in a shared function used by many tests. The name reported typically varies depending on the specific result rather than the test too. This causes problems for tooling that works with test results, the names reported with the results are used to deduplicate tests and track them between runs so both duplicated names and changing names cause trouble for things like UIs and automated bisection.
As a first step towards matching these tests better with the expectations of kselftest provide helpers which record the test name as part of the initial print and then use that as part of reporting a result.
This is not added as a generic kselftest helper partly because the use of a variable to store the test name doesn't fit well with the header only implementation of kselftest.h and partly because it's not really an intended pattern. Ideally at some point the mm tests that use it will be updated to not need it.
Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/mm/vm_util.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h index 6effafdc4d8a..4944e4c79051 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -3,6 +3,7 @@ #include <stdbool.h> #include <sys/mman.h> #include <err.h> +#include <stdarg.h> #include <strings.h> /* ffsl() */ #include <unistd.h> /* _SC_PAGESIZE */ #include "../kselftest.h" @@ -74,6 +75,25 @@ int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len, unsigned long get_free_hugepages(void); bool check_vmflag_io(void *addr);
+/* These helpers need to be inline to match the kselftest.h idiom. */ +static char test_name[1024]; + +static inline void log_test_start(const char *name, ...) +{ + va_list args; + va_start(args, name); + + vsnprintf(test_name, sizeof(test_name), name, args); + ksft_print_msg("[RUN] %s\n", test_name); + + va_end(args); +} + +static inline void log_test_result(int result) +{ + ksft_test_result_report(result, "%s\n", test_name); +} + /* * On ppc64 this will only work with radix 2M hugepage size */
On 27.05.25 18:04, Mark Brown wrote:
Several of the MM tests have a pattern of printing a description of the test to be run then reporting the actual TAP result using a generic string not connected to the specific test, often in a shared function used by many tests. The name reported typically varies depending on the specific result rather than the test too. This causes problems for tooling that works with test results, the names reported with the results are used to deduplicate tests and track them between runs so both duplicated names and changing names cause trouble for things like UIs and automated bisection.
As a first step towards matching these tests better with the expectations of kselftest provide helpers which record the test name as part of the initial print and then use that as part of reporting a result.
This is not added as a generic kselftest helper partly because the use of a variable to store the test name doesn't fit well with the header only implementation of kselftest.h and partly because it's not really an intended pattern. Ideally at some point the mm tests that use it will be updated to not need it.
Signed-off-by: Mark Brown broonie@kernel.org
tools/testing/selftests/mm/vm_util.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h index 6effafdc4d8a..4944e4c79051 100644 --- a/tools/testing/selftests/mm/vm_util.h +++ b/tools/testing/selftests/mm/vm_util.h @@ -3,6 +3,7 @@ #include <stdbool.h> #include <sys/mman.h> #include <err.h> +#include <stdarg.h> #include <strings.h> /* ffsl() */ #include <unistd.h> /* _SC_PAGESIZE */ #include "../kselftest.h" @@ -74,6 +75,25 @@ int uffd_register_with_ioctls(int uffd, void *addr, uint64_t len, unsigned long get_free_hugepages(void); bool check_vmflag_io(void *addr); +/* These helpers need to be inline to match the kselftest.h idiom. */ +static char test_name[1024];
+static inline void log_test_start(const char *name, ...) +{
- va_list args;
- va_start(args, name);
- vsnprintf(test_name, sizeof(test_name), name, args);
- ksft_print_msg("[RUN] %s\n", test_name);
- va_end(args);
+}
+static inline void log_test_result(int result) +{
- ksft_test_result_report(result, "%s\n", test_name);
+}
Won't win a beauty contest, but should get the job done.
We could allocate the array in log_test_start() and free it in log_test_result(). Then, we could assert more easily that we always have a log_test_result() follow exactly one log_test_start() etc.
On Tue, Jun 03, 2025 at 02:37:41PM +0200, David Hildenbrand wrote:
On 27.05.25 18:04, Mark Brown wrote:
+static char test_name[1024];
+static inline void log_test_start(const char *name, ...) +{
- va_list args;
- va_start(args, name);
- vsnprintf(test_name, sizeof(test_name), name, args);
- ksft_print_msg("[RUN] %s\n", test_name);
We could allocate the array in log_test_start() and free it in log_test_result(). Then, we could assert more easily that we always have a log_test_result() follow exactly one log_test_start() etc.
We could, however we don't have vasprintf() in nolibc and people have been doing work towards making nolibc more generally useful as a libc for the selftests (and/or the selftest interfaces more friendly to nolibc). I don't really know what the end goal with that is but given the fairly small gain and the hope that this won't be a long term framework for anything I'd rather not add something that gets in the way of whatever's going on there.
Ideally the test programs would be refactored and these helpers deleted, but as we said previously that's a bigger job that neither of us is likely to get to in the short term :(
On 03.06.25 20:27, Mark Brown wrote:
On Tue, Jun 03, 2025 at 02:37:41PM +0200, David Hildenbrand wrote:
On 27.05.25 18:04, Mark Brown wrote:
+static char test_name[1024];
+static inline void log_test_start(const char *name, ...) +{
- va_list args;
- va_start(args, name);
- vsnprintf(test_name, sizeof(test_name), name, args);
- ksft_print_msg("[RUN] %s\n", test_name);
We could allocate the array in log_test_start() and free it in log_test_result(). Then, we could assert more easily that we always have a log_test_result() follow exactly one log_test_start() etc.
We could, however we don't have vasprintf() in nolibc and people have been doing work towards making nolibc more generally useful as a libc for the selftests (and/or the selftest interfaces more friendly to nolibc). I don't really know what the end goal with that is but given the fairly small gain and the hope that this won't be a long term framework for anything I'd rather not add something that gets in the way of whatever's going on there.
Ideally the test programs would be refactored and these helpers deleted, but as we said previously that's a bigger job that neither of us is likely to get to in the short term :(
Jup ...
The kselftest framework uses the string logged when a test result is reported as the unique identifier for a test, using it to track test results between runs. The cow test completely fails to follow this pattern, it runs test functions repeatedly with various parameters with each result report from those functions being a string logging an error message which is fixed between runs.
Since the code already logs each test uniquely before it starts refactor to also print this to a buffer, then use that name as the test result. This isn't especially pretty but is relatively straightforward and is a great help to tooling.
Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/mm/cow.c | 333 +++++++++++++++++++++++++-------------- 1 file changed, 217 insertions(+), 116 deletions(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index e70cd3d900cc..dbbcc5eb3dce 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -112,9 +112,12 @@ struct comm_pipes {
static int setup_comm_pipes(struct comm_pipes *comm_pipes) { - if (pipe(comm_pipes->child_ready) < 0) + if (pipe(comm_pipes->child_ready) < 0) { + ksft_perror("pipe()"); return -errno; + } if (pipe(comm_pipes->parent_ready) < 0) { + ksft_perror("pipe()"); close(comm_pipes->child_ready[0]); close(comm_pipes->child_ready[1]); return -errno; @@ -207,13 +210,14 @@ static void do_test_cow_in_parent(char *mem, size_t size, bool do_mprotect,
ret = setup_comm_pipes(&comm_pipes); if (ret) { - ksft_test_result_fail("pipe() failed\n"); + log_test_result(KSFT_FAIL); return; }
ret = fork(); if (ret < 0) { - ksft_test_result_fail("fork() failed\n"); + ksft_perror("fork() failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; } else if (!ret) { exit(fn(mem, size, &comm_pipes)); @@ -228,9 +232,18 @@ static void do_test_cow_in_parent(char *mem, size_t size, bool do_mprotect, * write-faults by directly mapping pages writable. */ ret = mprotect(mem, size, PROT_READ); - ret |= mprotect(mem, size, PROT_READ|PROT_WRITE); if (ret) { - ksft_test_result_fail("mprotect() failed\n"); + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); + write(comm_pipes.parent_ready[1], "0", 1); + wait(&ret); + goto close_comm_pipes; + } + + ret = mprotect(mem, size, PROT_READ|PROT_WRITE); + if (ret) { + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); write(comm_pipes.parent_ready[1], "0", 1); wait(&ret); goto close_comm_pipes; @@ -248,16 +261,16 @@ static void do_test_cow_in_parent(char *mem, size_t size, bool do_mprotect, ret = -EINVAL;
if (!ret) { - ksft_test_result_pass("No leak from parent into child\n"); + log_test_result(KSFT_PASS); } else if (xfail) { /* * With hugetlb, some vmsplice() tests are currently expected to * fail because (a) harder to fix and (b) nobody really cares. * Flag them as expected failure for now. */ - ksft_test_result_xfail("Leak from parent into child\n"); + log_test_result(KSFT_XFAIL); } else { - ksft_test_result_fail("Leak from parent into child\n"); + log_test_result(KSFT_FAIL); } close_comm_pipes: close_comm_pipes(&comm_pipes); @@ -306,26 +319,29 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size,
ret = setup_comm_pipes(&comm_pipes); if (ret) { - ksft_test_result_fail("pipe() failed\n"); + log_test_result(KSFT_FAIL); goto free; }
if (pipe(fds) < 0) { - ksft_test_result_fail("pipe() failed\n"); + ksft_perror("pipe() failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; }
if (before_fork) { transferred = vmsplice(fds[1], &iov, 1, 0); if (transferred <= 0) { - ksft_test_result_fail("vmsplice() failed\n"); + ksft_print_msg("vmsplice() failed\n"); + log_test_result(KSFT_FAIL); goto close_pipe; } }
ret = fork(); if (ret < 0) { - ksft_test_result_fail("fork() failed\n"); + ksft_perror("fork() failed\n"); + log_test_result(KSFT_FAIL); goto close_pipe; } else if (!ret) { write(comm_pipes.child_ready[1], "0", 1); @@ -339,7 +355,8 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, if (!before_fork) { transferred = vmsplice(fds[1], &iov, 1, 0); if (transferred <= 0) { - ksft_test_result_fail("vmsplice() failed\n"); + ksft_perror("vmsplice() failed"); + log_test_result(KSFT_FAIL); wait(&ret); goto close_pipe; } @@ -348,7 +365,8 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, while (read(comm_pipes.child_ready[0], &buf, 1) != 1) ; if (munmap(mem, size) < 0) { - ksft_test_result_fail("munmap() failed\n"); + ksft_perror("munmap() failed"); + log_test_result(KSFT_FAIL); goto close_pipe; } write(comm_pipes.parent_ready[1], "0", 1); @@ -356,7 +374,8 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, /* Wait until the child is done writing. */ wait(&ret); if (!WIFEXITED(ret)) { - ksft_test_result_fail("wait() failed\n"); + ksft_perror("wait() failed"); + log_test_result(KSFT_FAIL); goto close_pipe; }
@@ -364,22 +383,23 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, for (total = 0; total < transferred; total += cur) { cur = read(fds[0], new + total, transferred - total); if (cur < 0) { - ksft_test_result_fail("read() failed\n"); + ksft_perror("read() failed"); + log_test_result(KSFT_FAIL); goto close_pipe; } }
if (!memcmp(old, new, transferred)) { - ksft_test_result_pass("No leak from child into parent\n"); + log_test_result(KSFT_PASS); } else if (xfail) { /* * With hugetlb, some vmsplice() tests are currently expected to * fail because (a) harder to fix and (b) nobody really cares. * Flag them as expected failure for now. */ - ksft_test_result_xfail("Leak from child into parent\n"); + log_test_result(KSFT_XFAIL); } else { - ksft_test_result_fail("Leak from child into parent\n"); + log_test_result(KSFT_FAIL); } close_pipe: close(fds[0]); @@ -416,13 +436,14 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork)
ret = setup_comm_pipes(&comm_pipes); if (ret) { - ksft_test_result_fail("pipe() failed\n"); + log_test_result(KSFT_FAIL); return; }
file = tmpfile(); if (!file) { - ksft_test_result_fail("tmpfile() failed\n"); + ksft_perror("tmpfile() failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; } fd = fileno(file); @@ -430,14 +451,16 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork)
tmp = malloc(size); if (!tmp) { - ksft_test_result_fail("malloc() failed\n"); + ksft_print_msg("malloc() failed\n"); + log_test_result(KSFT_FAIL); goto close_file; }
/* 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_print_msg("io_uring_queue_init() failed\n"); + log_test_result(KSFT_SKIP); goto free_tmp; }
@@ -452,7 +475,8 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork) iov.iov_len = size; ret = io_uring_register_buffers(&ring, &iov, 1); if (ret) { - ksft_test_result_skip("io_uring_register_buffers() failed\n"); + ksft_print_msg("io_uring_register_buffers() failed\n"); + log_test_result(KSFT_SKIP); goto queue_exit; }
@@ -463,7 +487,8 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork) */ ret = fork(); if (ret < 0) { - ksft_test_result_fail("fork() failed\n"); + ksft_perror("fork() failed"); + log_test_result(KSFT_FAIL); goto unregister_buffers; } else if (!ret) { write(comm_pipes.child_ready[1], "0", 1); @@ -483,10 +508,17 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork) * if the page is mapped R/O vs. R/W). */ ret = mprotect(mem, size, PROT_READ); + if (ret) { + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); + goto unregister_buffers; + } + clear_softdirty(); - ret |= mprotect(mem, size, PROT_READ | PROT_WRITE); + ret = mprotect(mem, size, PROT_READ | PROT_WRITE); if (ret) { - ksft_test_result_fail("mprotect() failed\n"); + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); goto unregister_buffers; } } @@ -498,25 +530,29 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork) memset(mem, 0xff, size); sqe = io_uring_get_sqe(&ring); if (!sqe) { - ksft_test_result_fail("io_uring_get_sqe() failed\n"); + ksft_print_msg("io_uring_get_sqe() failed\n"); + log_test_result(KSFT_FAIL); goto quit_child; } io_uring_prep_write_fixed(sqe, fd, mem, size, 0, 0);
ret = io_uring_submit(&ring); if (ret < 0) { - ksft_test_result_fail("io_uring_submit() failed\n"); + ksft_print_msg("io_uring_submit() failed\n"); + log_test_result(KSFT_FAIL); goto quit_child; }
ret = io_uring_wait_cqe(&ring, &cqe); if (ret < 0) { - ksft_test_result_fail("io_uring_wait_cqe() failed\n"); + ksft_print_msg("io_uring_wait_cqe() failed\n"); + log_test_result(KSFT_FAIL); goto quit_child; }
if (cqe->res != size) { - ksft_test_result_fail("write_fixed failed\n"); + ksft_print_msg("write_fixed failed\n"); + log_test_result(KSFT_FAIL); goto quit_child; } io_uring_cqe_seen(&ring, cqe); @@ -526,15 +562,18 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork) while (total < size) { cur = pread(fd, tmp + total, size - total, total); if (cur < 0) { - ksft_test_result_fail("pread() failed\n"); + ksft_print_msg("pread() failed\n"); + log_test_result(KSFT_FAIL); goto quit_child; } total += cur; }
/* Finally, check if we read what we expected. */ - ksft_test_result(!memcmp(mem, tmp, size), - "Longterm R/W pin is reliable\n"); + if (!memcmp(mem, tmp, size)) + log_test_result(KSFT_PASS); + else + log_test_result(KSFT_FAIL);
quit_child: if (use_fork) { @@ -582,19 +621,21 @@ static void do_test_ro_pin(char *mem, size_t size, enum ro_pin_test test, int ret;
if (gup_fd < 0) { - ksft_test_result_skip("gup_test not available\n"); + ksft_print_msg("gup_test not available\n"); + log_test_result(KSFT_SKIP); return; }
tmp = malloc(size); if (!tmp) { - ksft_test_result_fail("malloc() failed\n"); + ksft_print_msg("malloc() failed\n"); + log_test_result(KSFT_FAIL); return; }
ret = setup_comm_pipes(&comm_pipes); if (ret) { - ksft_test_result_fail("pipe() failed\n"); + log_test_result(KSFT_FAIL); goto free_tmp; }
@@ -609,7 +650,8 @@ static void do_test_ro_pin(char *mem, size_t size, enum ro_pin_test test, */ ret = fork(); if (ret < 0) { - ksft_test_result_fail("fork() failed\n"); + ksft_perror("fork() failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; } else if (!ret) { write(comm_pipes.child_ready[1], "0", 1); @@ -646,7 +688,8 @@ static void do_test_ro_pin(char *mem, size_t size, enum ro_pin_test test, clear_softdirty(); ret |= mprotect(mem, size, PROT_READ | PROT_WRITE); if (ret) { - ksft_test_result_fail("mprotect() failed\n"); + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; } break; @@ -661,9 +704,11 @@ static void do_test_ro_pin(char *mem, size_t size, enum ro_pin_test test, ret = ioctl(gup_fd, PIN_LONGTERM_TEST_START, &args); if (ret) { if (errno == EINVAL) - ksft_test_result_skip("PIN_LONGTERM_TEST_START failed\n"); + ret = KSFT_SKIP; else - ksft_test_result_fail("PIN_LONGTERM_TEST_START failed\n"); + ret = KSFT_FAIL; + ksft_perror("PIN_LONGTERM_TEST_START failed"); + log_test_result(ret); goto wait; }
@@ -676,22 +721,26 @@ static void do_test_ro_pin(char *mem, size_t size, enum ro_pin_test test, */ tmp_val = (__u64)(uintptr_t)tmp; ret = ioctl(gup_fd, PIN_LONGTERM_TEST_READ, &tmp_val); - if (ret) - ksft_test_result_fail("PIN_LONGTERM_TEST_READ failed\n"); - else - ksft_test_result(!memcmp(mem, tmp, size), - "Longterm R/O pin is reliable\n"); + if (ret) { + ksft_perror("PIN_LONGTERM_TEST_READ failed"); + log_test_result(KSFT_FAIL); + } else { + if (!memcmp(mem, tmp, size)) + log_test_result(KSFT_PASS); + else + log_test_result(KSFT_FAIL); + }
ret = ioctl(gup_fd, PIN_LONGTERM_TEST_STOP); if (ret) - ksft_print_msg("[INFO] PIN_LONGTERM_TEST_STOP failed\n"); + ksft_perror("PIN_LONGTERM_TEST_STOP failed"); wait: switch (test) { case RO_PIN_TEST_SHARED: write(comm_pipes.parent_ready[1], "0", 1); wait(&ret); if (!WIFEXITED(ret)) - ksft_print_msg("[INFO] wait() failed\n"); + ksft_perror("wait() failed"); break; default: break; @@ -746,14 +795,16 @@ static void do_run_with_base_page(test_fn fn, bool swapout) mem = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (mem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); return; }
ret = madvise(mem, pagesize, MADV_NOHUGEPAGE); /* Ignore if not around on a kernel. */ if (ret && errno != EINVAL) { - ksft_test_result_fail("MADV_NOHUGEPAGE failed\n"); + ksft_perror("MADV_NOHUGEPAGE failed"); + log_test_result(KSFT_FAIL); goto munmap; }
@@ -763,7 +814,8 @@ static void do_run_with_base_page(test_fn fn, bool swapout) if (swapout) { madvise(mem, pagesize, MADV_PAGEOUT); if (!pagemap_is_swapped(pagemap_fd, mem)) { - ksft_test_result_skip("MADV_PAGEOUT did not work, is swap enabled?\n"); + ksft_print_msg("MADV_PAGEOUT did not work, is swap enabled?\n"); + log_test_result(KSFT_SKIP); goto munmap; } } @@ -775,13 +827,13 @@ static void do_run_with_base_page(test_fn fn, bool swapout)
static void run_with_base_page(test_fn fn, const char *desc) { - ksft_print_msg("[RUN] %s ... with base page\n", desc); + log_test_start("%s ... with base page", desc); do_run_with_base_page(fn, false); }
static void run_with_base_page_swap(test_fn fn, const char *desc) { - ksft_print_msg("[RUN] %s ... with swapped out base page\n", desc); + log_test_start("%s ... with swapped out base page", desc); do_run_with_base_page(fn, true); }
@@ -807,7 +859,8 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize) mmap_mem = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (mmap_mem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); return; }
@@ -816,7 +869,8 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize)
ret = madvise(mem, thpsize, MADV_HUGEPAGE); if (ret) { - ksft_test_result_fail("MADV_HUGEPAGE failed\n"); + ksft_perror("MADV_HUGEPAGE failed"); + log_test_result(KSFT_FAIL); goto munmap; }
@@ -826,7 +880,8 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize) */ mem[0] = 1; if (!pagemap_is_populated(pagemap_fd, mem + thpsize - pagesize)) { - ksft_test_result_skip("Did not get a THP populated\n"); + ksft_print_msg("Did not get a THP populated\n"); + log_test_result(KSFT_SKIP); goto munmap; } memset(mem, 1, thpsize); @@ -846,12 +901,14 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize) */ ret = mprotect(mem + pagesize, pagesize, PROT_READ); if (ret) { - ksft_test_result_fail("mprotect() failed\n"); + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); goto munmap; } ret = mprotect(mem + pagesize, pagesize, PROT_READ | PROT_WRITE); if (ret) { - ksft_test_result_fail("mprotect() failed\n"); + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); goto munmap; } break; @@ -863,7 +920,8 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize) */ ret = madvise(mem + pagesize, thpsize - pagesize, MADV_DONTNEED); if (ret) { - ksft_test_result_fail("MADV_DONTNEED failed\n"); + ksft_perror("MADV_DONTNEED failed"); + log_test_result(KSFT_FAIL); goto munmap; } size = pagesize; @@ -877,13 +935,15 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize) mremap_mem = mmap(NULL, mremap_size, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (mremap_mem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); goto munmap; } tmp = mremap(mem + mremap_size, mremap_size, mremap_size, MREMAP_MAYMOVE | MREMAP_FIXED, mremap_mem); if (tmp != mremap_mem) { - ksft_test_result_fail("mremap() failed\n"); + ksft_perror("mremap() failed"); + log_test_result(KSFT_FAIL); goto munmap; } size = mremap_size; @@ -896,12 +956,14 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize) */ ret = madvise(mem + pagesize, thpsize - pagesize, MADV_DONTFORK); if (ret) { - ksft_test_result_fail("MADV_DONTFORK failed\n"); + ksft_perror("MADV_DONTFORK failed"); + log_test_result(KSFT_FAIL); goto munmap; } ret = fork(); if (ret < 0) { - ksft_test_result_fail("fork() failed\n"); + ksft_perror("fork() failed"); + log_test_result(KSFT_FAIL); goto munmap; } else if (!ret) { exit(0); @@ -910,7 +972,8 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize) /* Allow for sharing all pages again. */ ret = madvise(mem + pagesize, thpsize - pagesize, MADV_DOFORK); if (ret) { - ksft_test_result_fail("MADV_DOFORK failed\n"); + ksft_perror("MADV_DOFORK failed"); + log_test_result(KSFT_FAIL); goto munmap; } break; @@ -924,7 +987,8 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize) case THP_RUN_SINGLE_PTE_SWAPOUT: madvise(mem, size, MADV_PAGEOUT); if (!range_is_swapped(mem, size)) { - ksft_test_result_skip("MADV_PAGEOUT did not work, is swap enabled?\n"); + ksft_print_msg("MADV_PAGEOUT did not work, is swap enabled?\n"); + log_test_result(KSFT_SKIP); goto munmap; } break; @@ -941,56 +1005,56 @@ static void do_run_with_thp(test_fn fn, enum thp_run thp_run, size_t thpsize)
static void run_with_thp(test_fn fn, const char *desc, size_t size) { - ksft_print_msg("[RUN] %s ... with THP (%zu kB)\n", + log_test_start("%s ... with THP (%zu kB)", desc, size / 1024); do_run_with_thp(fn, THP_RUN_PMD, size); }
static void run_with_thp_swap(test_fn fn, const char *desc, size_t size) { - ksft_print_msg("[RUN] %s ... with swapped-out THP (%zu kB)\n", + log_test_start("%s ... with swapped-out THP (%zu kB)", desc, size / 1024); do_run_with_thp(fn, THP_RUN_PMD_SWAPOUT, size); }
static void run_with_pte_mapped_thp(test_fn fn, const char *desc, size_t size) { - ksft_print_msg("[RUN] %s ... with PTE-mapped THP (%zu kB)\n", + log_test_start("%s ... with PTE-mapped THP (%zu kB)", desc, size / 1024); do_run_with_thp(fn, THP_RUN_PTE, size); }
static void run_with_pte_mapped_thp_swap(test_fn fn, const char *desc, size_t size) { - ksft_print_msg("[RUN] %s ... with swapped-out, PTE-mapped THP (%zu kB)\n", + log_test_start("%s ... with swapped-out, PTE-mapped THP (%zu kB)", desc, size / 1024); do_run_with_thp(fn, THP_RUN_PTE_SWAPOUT, size); }
static void run_with_single_pte_of_thp(test_fn fn, const char *desc, size_t size) { - ksft_print_msg("[RUN] %s ... with single PTE of THP (%zu kB)\n", + log_test_start("%s ... with single PTE of THP (%zu kB)", desc, size / 1024); do_run_with_thp(fn, THP_RUN_SINGLE_PTE, size); }
static void run_with_single_pte_of_thp_swap(test_fn fn, const char *desc, size_t size) { - ksft_print_msg("[RUN] %s ... with single PTE of swapped-out THP (%zu kB)\n", + log_test_start("%s ... with single PTE of swapped-out THP (%zu kB)", desc, size / 1024); do_run_with_thp(fn, THP_RUN_SINGLE_PTE_SWAPOUT, size); }
static void run_with_partial_mremap_thp(test_fn fn, const char *desc, size_t size) { - ksft_print_msg("[RUN] %s ... with partially mremap()'ed THP (%zu kB)\n", + log_test_start("%s ... with partially mremap()'ed THP (%zu kB)", desc, size / 1024); do_run_with_thp(fn, THP_RUN_PARTIAL_MREMAP, size); }
static void run_with_partial_shared_thp(test_fn fn, const char *desc, size_t size) { - ksft_print_msg("[RUN] %s ... with partially shared THP (%zu kB)\n", + log_test_start("%s ... with partially shared THP (%zu kB)", desc, size / 1024); do_run_with_thp(fn, THP_RUN_PARTIAL_SHARED, size); } @@ -1000,14 +1064,15 @@ static void run_with_hugetlb(test_fn fn, const char *desc, size_t hugetlbsize) int flags = MAP_PRIVATE | MAP_ANONYMOUS | MAP_HUGETLB; char *mem, *dummy;
- ksft_print_msg("[RUN] %s ... with hugetlb (%zu kB)\n", desc, + log_test_start("%s ... with hugetlb (%zu kB)", desc, hugetlbsize / 1024);
flags |= __builtin_ctzll(hugetlbsize) << MAP_HUGE_SHIFT;
mem = mmap(NULL, hugetlbsize, PROT_READ | PROT_WRITE, flags, -1, 0); if (mem == MAP_FAILED) { - ksft_test_result_skip("need more free huge pages\n"); + ksft_perror("need more free huge pages"); + log_test_result(KSFT_SKIP); return; }
@@ -1020,7 +1085,8 @@ static void run_with_hugetlb(test_fn fn, const char *desc, size_t hugetlbsize) */ dummy = mmap(NULL, hugetlbsize, PROT_READ | PROT_WRITE, flags, -1, 0); if (dummy == MAP_FAILED) { - ksft_test_result_skip("need more free huge pages\n"); + ksft_perror("need more free huge pages"); + log_test_result(KSFT_SKIP); goto munmap; } munmap(dummy, hugetlbsize); @@ -1226,7 +1292,7 @@ static void do_test_anon_thp_collapse(char *mem, size_t size,
ret = setup_comm_pipes(&comm_pipes); if (ret) { - ksft_test_result_fail("pipe() failed\n"); + log_test_result(KSFT_FAIL); return; }
@@ -1236,12 +1302,14 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, */ ret = mprotect(mem + pagesize, pagesize, PROT_READ); if (ret) { - ksft_test_result_fail("mprotect() failed\n"); + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; } ret = mprotect(mem + pagesize, pagesize, PROT_READ | PROT_WRITE); if (ret) { - ksft_test_result_fail("mprotect() failed\n"); + ksft_perror("mprotect() failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; }
@@ -1250,8 +1318,8 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, /* Collapse before actually COW-sharing the page. */ ret = madvise(mem, size, MADV_COLLAPSE); if (ret) { - ksft_test_result_skip("MADV_COLLAPSE failed: %s\n", - strerror(errno)); + ksft_perror("MADV_COLLAPSE failed"); + log_test_result(KSFT_SKIP); goto close_comm_pipes; } break; @@ -1262,7 +1330,8 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, /* Don't COW-share the upper part of the THP. */ ret = madvise(mem + size / 2, size / 2, MADV_DONTFORK); if (ret) { - ksft_test_result_fail("MADV_DONTFORK failed\n"); + ksft_perror("MADV_DONTFORK failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; } break; @@ -1270,7 +1339,8 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, /* Don't COW-share the lower part of the THP. */ ret = madvise(mem, size / 2, MADV_DONTFORK); if (ret) { - ksft_test_result_fail("MADV_DONTFORK failed\n"); + ksft_perror("MADV_DONTFORK failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; } break; @@ -1280,7 +1350,8 @@ static void do_test_anon_thp_collapse(char *mem, size_t size,
ret = fork(); if (ret < 0) { - ksft_test_result_fail("fork() failed\n"); + ksft_perror("fork() failed"); + log_test_result(KSFT_FAIL); goto close_comm_pipes; } else if (!ret) { switch (test) { @@ -1314,7 +1385,8 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, */ ret = madvise(mem, size, MADV_DOFORK); if (ret) { - ksft_test_result_fail("MADV_DOFORK failed\n"); + ksft_perror("MADV_DOFORK failed"); + log_test_result(KSFT_FAIL); write(comm_pipes.parent_ready[1], "0", 1); wait(&ret); goto close_comm_pipes; @@ -1324,8 +1396,8 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, /* Collapse before anyone modified the COW-shared page. */ ret = madvise(mem, size, MADV_COLLAPSE); if (ret) { - ksft_test_result_skip("MADV_COLLAPSE failed: %s\n", - strerror(errno)); + ksft_perror("MADV_COLLAPSE failed"); + log_test_result(KSFT_SKIP); write(comm_pipes.parent_ready[1], "0", 1); wait(&ret); goto close_comm_pipes; @@ -1345,7 +1417,10 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, else ret = -EINVAL;
- ksft_test_result(!ret, "No leak from parent into child\n"); + if (!ret) + log_test_result(KSFT_PASS); + else + log_test_result(KSFT_FAIL); close_comm_pipes: close_comm_pipes(&comm_pipes); } @@ -1430,7 +1505,7 @@ static void run_anon_thp_test_cases(void) for (i = 0; i < ARRAY_SIZE(anon_thp_test_cases); i++) { struct test_case const *test_case = &anon_thp_test_cases[i];
- ksft_print_msg("[RUN] %s\n", test_case->desc); + log_test_start("%s", test_case->desc); do_run_with_thp(test_case->fn, THP_RUN_PMD, pmdsize); } } @@ -1453,8 +1528,10 @@ static void test_cow(char *mem, const char *smem, size_t size) memset(mem, 0xff, size);
/* See if we still read the old values via the other mapping. */ - ksft_test_result(!memcmp(smem, old, size), - "Other mapping not modified\n"); + if (!memcmp(smem, old, size)) + log_test_result(KSFT_PASS); + else + log_test_result(KSFT_FAIL); free(old); }
@@ -1472,18 +1549,20 @@ static void run_with_zeropage(non_anon_test_fn fn, const char *desc) { char *mem, *smem, tmp;
- ksft_print_msg("[RUN] %s ... with shared zeropage\n", desc); + log_test_start("%s ... with shared zeropage", desc);
mem = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANON, -1, 0); if (mem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); return; }
smem = mmap(NULL, pagesize, PROT_READ, MAP_PRIVATE | MAP_ANON, -1, 0); if (smem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); goto munmap; }
@@ -1504,10 +1583,11 @@ static void run_with_huge_zeropage(non_anon_test_fn fn, const char *desc) size_t mmap_size; int ret;
- ksft_print_msg("[RUN] %s ... with huge zeropage\n", desc); + log_test_start("%s ... with huge zeropage", desc);
if (!has_huge_zeropage) { - ksft_test_result_skip("Huge zeropage not enabled\n"); + ksft_print_msg("Huge zeropage not enabled\n"); + log_test_result(KSFT_SKIP); return; }
@@ -1516,13 +1596,15 @@ static void run_with_huge_zeropage(non_anon_test_fn fn, const char *desc) mmap_mem = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (mmap_mem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); return; } mmap_smem = mmap(NULL, mmap_size, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); if (mmap_smem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); goto munmap; }
@@ -1531,9 +1613,15 @@ static void run_with_huge_zeropage(non_anon_test_fn fn, const char *desc) smem = (char *)(((uintptr_t)mmap_smem + pmdsize) & ~(pmdsize - 1));
ret = madvise(mem, pmdsize, MADV_HUGEPAGE); + if (ret != 0) { + ksft_perror("madvise()"); + log_test_result(KSFT_FAIL); + goto munmap; + } ret |= madvise(smem, pmdsize, MADV_HUGEPAGE); - if (ret) { - ksft_test_result_fail("MADV_HUGEPAGE failed\n"); + if (ret != 0) { + ksft_perror("madvise()"); + log_test_result(KSFT_FAIL); goto munmap; }
@@ -1562,29 +1650,33 @@ static void run_with_memfd(non_anon_test_fn fn, const char *desc) char *mem, *smem, tmp; int fd;
- ksft_print_msg("[RUN] %s ... with memfd\n", desc); + log_test_start("%s ... with memfd", desc);
fd = memfd_create("test", 0); if (fd < 0) { - ksft_test_result_fail("memfd_create() failed\n"); + ksft_perror("memfd_create() failed"); + log_test_result(KSFT_FAIL); return; }
/* File consists of a single page filled with zeroes. */ if (fallocate(fd, 0, 0, pagesize)) { - ksft_test_result_fail("fallocate() failed\n"); + ksft_perror("fallocate() failed"); + log_test_result(KSFT_FAIL); goto close; }
/* Create a private mapping of the memfd. */ mem = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); if (mem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); goto close; } smem = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0); if (smem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); goto munmap; }
@@ -1607,35 +1699,40 @@ static void run_with_tmpfile(non_anon_test_fn fn, const char *desc) FILE *file; int fd;
- ksft_print_msg("[RUN] %s ... with tmpfile\n", desc); + log_test_start("%s ... with tmpfile", desc);
file = tmpfile(); if (!file) { - ksft_test_result_fail("tmpfile() failed\n"); + ksft_perror("tmpfile() failed"); + log_test_result(KSFT_FAIL); return; }
fd = fileno(file); if (fd < 0) { - ksft_test_result_skip("fileno() failed\n"); + ksft_perror("fileno() failed"); + log_test_result(KSFT_SKIP); return; }
/* File consists of a single page filled with zeroes. */ if (fallocate(fd, 0, 0, pagesize)) { - ksft_test_result_fail("fallocate() failed\n"); + ksft_perror("fallocate() failed"); + log_test_result(KSFT_FAIL); goto close; }
/* Create a private mapping of the memfd. */ mem = mmap(NULL, pagesize, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); if (mem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); goto close; } smem = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0); if (smem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); goto munmap; }
@@ -1659,20 +1756,22 @@ static void run_with_memfd_hugetlb(non_anon_test_fn fn, const char *desc, char *mem, *smem, tmp; int fd;
- ksft_print_msg("[RUN] %s ... with memfd hugetlb (%zu kB)\n", desc, + log_test_start("%s ... with memfd hugetlb (%zu kB)", desc, hugetlbsize / 1024);
flags |= __builtin_ctzll(hugetlbsize) << MFD_HUGE_SHIFT;
fd = memfd_create("test", flags); if (fd < 0) { - ksft_test_result_skip("memfd_create() failed\n"); + ksft_perror("memfd_create() failed"); + log_test_result(KSFT_SKIP); return; }
/* File consists of a single page filled with zeroes. */ if (fallocate(fd, 0, 0, hugetlbsize)) { - ksft_test_result_skip("need more free huge pages\n"); + ksft_perror("need more free huge pages"); + log_test_result(KSFT_SKIP); goto close; }
@@ -1680,12 +1779,14 @@ static void run_with_memfd_hugetlb(non_anon_test_fn fn, const char *desc, mem = mmap(NULL, hugetlbsize, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 0); if (mem == MAP_FAILED) { - ksft_test_result_skip("need more free huge pages\n"); + ksft_perror("need more free huge pages"); + log_test_result(KSFT_SKIP); goto close; } smem = mmap(NULL, hugetlbsize, PROT_READ, MAP_SHARED, fd, 0); if (smem == MAP_FAILED) { - ksft_test_result_fail("mmap() failed\n"); + ksft_perror("mmap() failed"); + log_test_result(KSFT_FAIL); goto munmap; }
On 27.05.25 18:04, Mark Brown wrote:
The kselftest framework uses the string logged when a test result is reported as the unique identifier for a test, using it to track test results between runs. The cow test completely fails to follow this pattern, it runs test functions repeatedly with various parameters with each result report from those functions being a string logging an error message which is fixed between runs.
Since the code already logs each test uniquely before it starts refactor to also print this to a buffer, then use that name as the test result. This isn't especially pretty but is relatively straightforward and is a great help to tooling.
Signed-off-by: Mark Brown broonie@kernel.org
tools/testing/selftests/mm/cow.c | 333 +++++++++++++++++++++++++-------------- 1 file changed, 217 insertions(+), 116 deletions(-)
diff --git a/tools/testing/selftests/mm/cow.c b/tools/testing/selftests/mm/cow.c index e70cd3d900cc..dbbcc5eb3dce 100644 --- a/tools/testing/selftests/mm/cow.c +++ b/tools/testing/selftests/mm/cow.c @@ -112,9 +112,12 @@ struct comm_pipes { static int setup_comm_pipes(struct comm_pipes *comm_pipes) {
- if (pipe(comm_pipes->child_ready) < 0)
- if (pipe(comm_pipes->child_ready) < 0) {
ksft_perror("pipe()");
"pipe() failed"
return -errno;
- } if (pipe(comm_pipes->parent_ready) < 0) {
ksft_perror("pipe()");
"pipe() failed"
[...]
*/ ret = mprotect(mem, size, PROT_READ);
ret |= mprotect(mem, size, PROT_READ|PROT_WRITE);
Not sure if that change is really required: if the second mprotect succeeds, errno should not be updated. At least if my memory is correct :)
Same applies to similar cases below.
ksft_test_result_fail("mprotect() failed\n");
ksft_perror("mprotect() failed");log_test_result(KSFT_FAIL);write(comm_pipes.parent_ready[1], "0", 1);wait(&ret);goto close_comm_pipes;}ret = mprotect(mem, size, PROT_READ|PROT_WRITE);if (ret) {ksft_perror("mprotect() failed");log_test_result(KSFT_FAIL); write(comm_pipes.parent_ready[1], "0", 1); wait(&ret); goto close_comm_pipes;@@ -248,16 +261,16 @@ static void do_test_cow_in_parent(char *mem, size_t size, bool do_mprotect, ret = -EINVAL; if (!ret) {
ksft_test_result_pass("No leak from parent into child\n");
log_test_result(KSFT_PASS);> } else if (xfail) {*/
- With hugetlb, some vmsplice() tests are currently expected to
- fail because (a) harder to fix and (b) nobody really cares.
- Flag them as expected failure for now.
ksft_test_result_xfail("Leak from parent into child\n");
We're losing improtant information here.
log_test_result(KSFT_XFAIL);
ksft_test_result_fail("Leak from parent into child\n");
Same here and in other cases below (I probably didn't catch all).
We should log that somehow to indicate what exactly is going wrong, likely using ksft_print_msg().
log_test_result(KSFT_FAIL);@@ -306,26 +319,29 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, ret = setup_comm_pipes(&comm_pipes); if (ret) {
ksft_test_result_fail("pipe() failed\n");
log_test_result(KSFT_FAIL);if (pipe(fds) < 0) {
ksft_test_result_fail("pipe() failed\n");
ksft_perror("pipe() failed");log_test_result(KSFT_FAIL);if (before_fork) { transferred = vmsplice(fds[1], &iov, 1, 0); if (transferred <= 0) {
ksft_test_result_fail("vmsplice() failed\n");
ksft_print_msg("vmsplice() failed\n");
ksft_perror?
log_test_result(KSFT_FAIL); goto close_pipe;ret = fork(); if (ret < 0) {
ksft_test_result_fail("fork() failed\n");
ksft_perror("fork() failed\n");log_test_result(KSFT_FAIL);@@ -339,7 +355,8 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, if (!before_fork) { transferred = vmsplice(fds[1], &iov, 1, 0); if (transferred <= 0) {
ksft_test_result_fail("vmsplice() failed\n");
ksft_perror("vmsplice() failed");log_test_result(KSFT_FAIL); wait(&ret); goto close_pipe;@@ -348,7 +365,8 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, while (read(comm_pipes.child_ready[0], &buf, 1) != 1) ; if (munmap(mem, size) < 0) {
ksft_test_result_fail("munmap() failed\n");
ksft_perror("munmap() failed");log_test_result(KSFT_FAIL);@@ -356,7 +374,8 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, /* Wait until the child is done writing. */ wait(&ret); if (!WIFEXITED(ret)) {
ksft_test_result_fail("wait() failed\n");
ksft_perror("wait() failed");log_test_result(KSFT_FAIL);@@ -364,22 +383,23 @@ static void do_test_vmsplice_in_parent(char *mem, size_t size, for (total = 0; total < transferred; total += cur) { cur = read(fds[0], new + total, transferred - total); if (cur < 0) {
ksft_test_result_fail("read() failed\n");
ksft_perror("read() failed");log_test_result(KSFT_FAIL); goto close_pipe;if (!memcmp(old, new, transferred)) {
ksft_test_result_pass("No leak from child into parent\n");
log_test_result(KSFT_PASS);*/
- With hugetlb, some vmsplice() tests are currently expected to
- fail because (a) harder to fix and (b) nobody really cares.
- Flag them as expected failure for now.
ksft_test_result_xfail("Leak from child into parent\n");
log_test_result(KSFT_XFAIL);
Same comment as above.
} else {
ksft_test_result_fail("Leak from child into parent\n");
log_test_result(KSFT_FAIL);
Dito. [...]
*/ ret = mprotect(mem, size, PROT_READ);
if (ret) {ksft_perror("mprotect() failed");log_test_result(KSFT_FAIL);goto unregister_buffers;}- clear_softdirty();
ret |= mprotect(mem, size, PROT_READ | PROT_WRITE);
ret = mprotect(mem, size, PROT_READ | PROT_WRITE);
ksft_test_result_fail("mprotect() failed\n");
ksft_perror("mprotect() failed");log_test_result(KSFT_FAIL); goto unregister_buffers;@@ -498,25 +530,29 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork) memset(mem, 0xff, size); sqe = io_uring_get_sqe(&ring); if (!sqe) {
ksft_test_result_fail("io_uring_get_sqe() failed\n");
ksft_print_msg("io_uring_get_sqe() failed\n");log_test_result(KSFT_FAIL);ret = io_uring_submit(&ring); if (ret < 0) {
ksft_test_result_fail("io_uring_submit() failed\n");
ksft_print_msg("io_uring_submit() failed\n");log_test_result(KSFT_FAIL);ret = io_uring_wait_cqe(&ring, &cqe); if (ret < 0) {
ksft_test_result_fail("io_uring_wait_cqe() failed\n");
ksft_print_msg("io_uring_wait_cqe() failed\n");log_test_result(KSFT_FAIL);if (cqe->res != size) {
ksft_test_result_fail("write_fixed failed\n");
ksft_print_msg("write_fixed failed\n");log_test_result(KSFT_FAIL);> goto quit_child;@@ -526,15 +562,18 @@ static void do_test_iouring(char *mem, size_t size, bool use_fork) while (total < size) { cur = pread(fd, tmp + total, size - total, total); if (cur < 0) {
ksft_test_result_fail("pread() failed\n");
ksft_print_msg("pread() failed\n");
perror?
log_test_result(KSFT_FAIL); goto quit_child;/* Finally, check if we read what we expected. */
- ksft_test_result(!memcmp(mem, tmp, size),
"Longterm R/W pin is reliable\n");
- if (!memcmp(mem, tmp, size))
log_test_result(KSFT_PASS);- else
log_test_result(KSFT_FAIL);quit_child: if (use_fork) { @@ -582,19 +621,21 @@ static void do_test_ro_pin(char *mem, size_t size, enum ro_pin_test test, int ret; if (gup_fd < 0) {
ksft_test_result_skip("gup_test not available\n");
ksft_print_msg("gup_test not available\n");log_test_result(KSFT_SKIP);tmp = malloc(size); if (!tmp) {
ksft_test_result_fail("malloc() failed\n");
ksft_print_msg("malloc() failed\n");
perror?
Probably worth going over all your changes and double checking them, I'm sure I missed some.
log_test_result(KSFT_FAIL);ret = setup_comm_pipes(&comm_pipes); if (ret) {
ksft_test_result_fail("pipe() failed\n");
log_test_result(KSFT_FAIL);
[...]
ret = fork(); if (ret < 0) {
ksft_test_result_fail("fork() failed\n");
ksft_perror("fork() failed");log_test_result(KSFT_FAIL);@@ -1314,7 +1385,8 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, */ ret = madvise(mem, size, MADV_DOFORK); if (ret) {
ksft_test_result_fail("MADV_DOFORK failed\n");
ksft_perror("MADV_DOFORK failed");log_test_result(KSFT_FAIL); write(comm_pipes.parent_ready[1], "0", 1); wait(&ret); goto close_comm_pipes;@@ -1324,8 +1396,8 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, /* Collapse before anyone modified the COW-shared page. */ ret = madvise(mem, size, MADV_COLLAPSE); if (ret) {
ksft_test_result_skip("MADV_COLLAPSE failed: %s\n",strerror(errno));
ksft_perror("MADV_COLLAPSE failed");log_test_result(KSFT_SKIP); write(comm_pipes.parent_ready[1], "0", 1); wait(&ret); goto close_comm_pipes;@@ -1345,7 +1417,10 @@ static void do_test_anon_thp_collapse(char *mem, size_t size, else ret = -EINVAL;
- ksft_test_result(!ret, "No leak from parent into child\n");
- if (!ret)
log_test_result(KSFT_PASS);- else
log_test_result(KSFT_FAIL);
Are we losing the "No leak from parent into child\n" especially on the failure path?
close_comm_pipes: close_comm_pipes(&comm_pipes); } @@ -1430,7 +1505,7 @@ static void run_anon_thp_test_cases(void) for (i = 0; i < ARRAY_SIZE(anon_thp_test_cases); i++) { struct test_case const *test_case = &anon_thp_test_cases[i];
ksft_print_msg("[RUN] %s\n", test_case->desc);
log_test_start("%s", test_case->desc);@@ -1453,8 +1528,10 @@ static void test_cow(char *mem, const char *smem, size_t size) memset(mem, 0xff, size); /* See if we still read the old values via the other mapping. */
- ksft_test_result(!memcmp(smem, old, size),
"Other mapping not modified\n");
Are we losing the "Other mapping not modified\n" information especially on the failure path?
- if (!memcmp(smem, old, size))
log_test_result(KSFT_PASS);- else
log_test_result(KSFT_FAIL);
[...]
@@ -1531,9 +1613,15 @@ static void run_with_huge_zeropage(non_anon_test_fn fn, const char *desc) smem = (char *)(((uintptr_t)mmap_smem + pmdsize) & ~(pmdsize - 1)); ret = madvise(mem, pmdsize, MADV_HUGEPAGE);
- if (ret != 0) {
if (ret)
ksft_perror("madvise()");log_test_result(KSFT_FAIL);goto munmap;- } ret |= madvise(smem, pmdsize, MADV_HUGEPAGE);
- if (ret) {
ksft_test_result_fail("MADV_HUGEPAGE failed\n");
- if (ret != 0) {
if (ret) as it was
ksft_perror("madvise()");log_test_result(KSFT_FAIL);
On Tue, Jun 03, 2025 at 02:51:45PM +0200, David Hildenbrand wrote:
On 27.05.25 18:04, Mark Brown wrote:
ret = mprotect(mem, size, PROT_READ);
ret |= mprotect(mem, size, PROT_READ|PROT_WRITE);
Not sure if that change is really required: if the second mprotect succeeds, errno should not be updated. At least if my memory is correct :)
Same applies to similar cases below.
I thought about checking to see if that was guaranteed to be the case, then I thought that if that wasn't clear to me right now without checking it probably also wasn't going to be obvious to future readers so it was better to just write something clear. Previously we didn't report errno so it didn't matter.
} else {
ksft_test_result_fail("Leak from parent into child\n");
Same here and in other cases below (I probably didn't catch all).
We should log that somehow to indicate what exactly is going wrong, likely using ksft_print_msg().
Can you send a patch with the logging that you think would be clear please? I dropped these because they just seemed to be reporting the overall point of the test, unlike the cases where we ran into some error during the setup and didn't actually manage to perform the test we were trying to do. Perhaps the tests should be renamed.
tmp = malloc(size); if (!tmp) {
ksft_test_result_fail("malloc() failed\n");
ksft_print_msg("malloc() failed\n");
perror?
malloc() can only set one errno.
On 03.06.25 15:21, Mark Brown wrote:
On Tue, Jun 03, 2025 at 02:51:45PM +0200, David Hildenbrand wrote:
On 27.05.25 18:04, Mark Brown wrote:
ret = mprotect(mem, size, PROT_READ);
ret |= mprotect(mem, size, PROT_READ|PROT_WRITE); if (ret) {Not sure if that change is really required: if the second mprotect succeeds, errno should not be updated. At least if my memory is correct :)
Same applies to similar cases below.
I thought about checking to see if that was guaranteed to be the case, then I thought that if that wasn't clear to me right now without checking it probably also wasn't going to be obvious to future readers so it was better to just write something clear. Previously we didn't report errno so it didn't matter.
} else {
ksft_test_result_fail("Leak from parent into child\n");Same here and in other cases below (I probably didn't catch all).
We should log that somehow to indicate what exactly is going wrong, likely using ksft_print_msg().
Can you send a patch with the logging that you think would be clear please? I dropped these because they just seemed to be reporting the> overall
point of the test, unlike the cases where we ran into some error
during the setup and didn't actually manage to perform the test we were trying to do. Perhaps the tests should be renamed.
ksft_print_msg("Leak from parent into child");
On Tue, Jun 03, 2025 at 04:15:42PM +0200, David Hildenbrand wrote:
On 03.06.25 15:21, Mark Brown wrote:
} else {
ksft_test_result_fail("Leak from parent into child\n");
Same here and in other cases below (I probably didn't catch all).
We should log that somehow to indicate what exactly is going wrong, likely using ksft_print_msg().
Can you send a patch with the logging that you think would be clear please? I dropped these because they just seemed to be reporting the> overall
point of the test, unlike the cases where we ran into some error
during the setup and didn't actually manage to perform the test we were trying to do. Perhaps the tests should be renamed.
ksft_print_msg("Leak from parent into child");
Can you send a patch showing when/where you want this printing please? Like I said I suspect the test name is just unclear here...
On 03.06.25 16:58, Mark Brown wrote:
On Tue, Jun 03, 2025 at 04:15:42PM +0200, David Hildenbrand wrote:
On 03.06.25 15:21, Mark Brown wrote:
} else {
ksft_test_result_fail("Leak from parent into child\n");Same here and in other cases below (I probably didn't catch all).
We should log that somehow to indicate what exactly is going wrong, likely using ksft_print_msg().
Can you send a patch with the logging that you think would be clear please? I dropped these because they just seemed to be reporting the> overall
point of the test, unlike the cases where we ran into some error
during the setup and didn't actually manage to perform the test we were trying to do. Perhaps the tests should be renamed.
ksft_print_msg("Leak from parent into child");
Can you send a patch showing when/where you want this printing please?
I'm really busy right now, unfortunately.
Like I said I suspect the test name is just unclear here...
I would hope we find some mechanical replacement.
E.g.,
ksft_test_result_pass("No leak from parent into child\n");
becomes
ksft_print_msg("No leak from parent into child\n"); log_test_result(KSFT_PASS);
and
ksft_test_result_xfail("Leak from parent into child\n");
becomes
ksft_print_msg("Leak from parent into child\n"); log_test_result(KSFT_FAIL);
On Tue, Jun 03, 2025 at 05:06:17PM +0200, David Hildenbrand wrote:
On 03.06.25 16:58, Mark Brown wrote:
Like I said I suspect the test name is just unclear here...
I would hope we find some mechanical replacement.
E.g.,
ksft_test_result_pass("No leak from parent into child\n");
becomes
ksft_print_msg("No leak from parent into child\n"); log_test_result(KSFT_PASS);
Like I've been saying this is just the final test result, in this case I would expect that for the actual thing we're trying to test any confusion would be addressed in the name of the test so that it's clear what it was trying to test. So adding "Leak from parent to child" to the name of all the tests?
On 03.06.25 17:22, Mark Brown wrote:
On Tue, Jun 03, 2025 at 05:06:17PM +0200, David Hildenbrand wrote:
On 03.06.25 16:58, Mark Brown wrote:
Like I said I suspect the test name is just unclear here...
I would hope we find some mechanical replacement.
E.g.,
ksft_test_result_pass("No leak from parent into child\n");
becomes
ksft_print_msg("No leak from parent into child\n"); log_test_result(KSFT_PASS);
Like I've been saying this is just the final test result, in this case I would expect that for the actual thing we're trying to test any confusion would be addressed in the name of the test so that it's clear what it was trying to test. So adding "Leak from parent to child" to the name of all the tests?
I agree that printing something in case KSFT_PASS does not make sense indeed.
But if something goes wrong (KSFT_FAIL/KSFT_SKIP) I would expect a reason in all cases.
IIRC kselftest_harness.h behaves that way:
$ ./pfnmap TAP version 13 1..6 # Starting 6 tests from 1 test cases. # RUN pfnmap.madvise_disallowed ... # SKIP Cannot open '/dev/mem'
Changing the tests names really sounds suboptimal, if all we want do indicate is that the final memcp revealed a leak (part of the cow test).
On Tue, Jun 03, 2025 at 06:57:38PM +0200, David Hildenbrand wrote:
On 03.06.25 17:22, Mark Brown wrote:
Like I've been saying this is just the final test result, in this case I would expect that for the actual thing we're trying to test any confusion would be addressed in the name of the test so that it's clear what it was trying to test. So adding "Leak from parent to child" to the name of all the tests?
I agree that printing something in case KSFT_PASS does not make sense indeed.
But if something goes wrong (KSFT_FAIL/KSFT_SKIP) I would expect a reason in all cases.
IIRC kselftest_harness.h behaves that way:
That's mostly just it being chatty because it uses an assert based idiom rather than explicit pass/fail reports, it's a lot less common for things written directly to kselftest.h where it's for example fairly common to see a result detected directly in a ksft_result() call. That does tend to be quite helpful when looking at the results, you don't need to dig out the logs so often.
On Tue, Jun 03, 2025 at 06:48:19PM +0100, Mark Brown wrote:
On Tue, Jun 03, 2025 at 06:57:38PM +0200, David Hildenbrand wrote:
I agree that printing something in case KSFT_PASS does not make sense indeed.
But if something goes wrong (KSFT_FAIL/KSFT_SKIP) I would expect a reason in all cases.
IIRC kselftest_harness.h behaves that way:
That's mostly just it being chatty because it uses an assert based idiom rather than explicit pass/fail reports, it's a lot less common for things written directly to kselftest.h where it's for example fairly common to see a result detected directly in a ksft_result() call. That does tend to be quite helpful when looking at the results, you don't need to dig out the logs so often.
As was the case with the prior:
/* Finally, check if we read what we expected. */ - ksft_test_result(!memcmp(mem, tmp, size), - "Longterm R/W pin is reliable\n"); + if (!memcmp(mem, tmp, size)) + log_test_result(KSFT_PASS); + else + log_test_result(KSFT_FAIL);
On 03.06.25 19:55, Mark Brown wrote:
On Tue, Jun 03, 2025 at 06:48:19PM +0100, Mark Brown wrote:
On Tue, Jun 03, 2025 at 06:57:38PM +0200, David Hildenbrand wrote:
I agree that printing something in case KSFT_PASS does not make sense indeed.
But if something goes wrong (KSFT_FAIL/KSFT_SKIP) I would expect a reason in all cases.
IIRC kselftest_harness.h behaves that way:
That's mostly just it being chatty because it uses an assert based idiom rather than explicit pass/fail reports, it's a lot less common for things written directly to kselftest.h where it's for example fairly common to see a result detected directly in a ksft_result() call. That does tend to be quite helpful when looking at the results, you don't need to dig out the logs so often.
Right, and if the test fails, you immediately know why. So I am a big fan of the test telling you why it failed, not assuming "it's the last check, so the user can go figure out that it's the last check in that file and we just don't tell him that".
In any case, I hoe this will be gone at some point, and kselftest_harness.h will provide that to us automatically.
The kselftest framework uses the string logged when a test result is reported as the unique identifier for a test, using it to track test results between runs. The gup_longterm test fails to follow this pattern, it runs a single test function repeatedly with various parameters but each result report is a string logging an error message which is fixed between runs.
Since the code already logs each test uniquely before it starts refactor to also print this to a buffer, then use that name as the test result. This isn't especially pretty but is relatively straightforward and is a great help to tooling.
Signed-off-by: Mark Brown broonie@kernel.org --- tools/testing/selftests/mm/gup_longterm.c | 150 +++++++++++++++++++----------- 1 file changed, 94 insertions(+), 56 deletions(-)
diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c index e60e62809186..f84ea97c2543 100644 --- a/tools/testing/selftests/mm/gup_longterm.c +++ b/tools/testing/selftests/mm/gup_longterm.c @@ -93,33 +93,48 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) __fsword_t fs_type = get_fs_type(fd); bool should_work; char *mem; + int result = KSFT_PASS; int ret;
+ if (fd < 0) { + result = KSFT_FAIL; + goto report; + } + if (ftruncate(fd, size)) { if (errno == ENOENT) { skip_test_dodgy_fs("ftruncate()"); } else { - ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno)); + ksft_print_msg("ftruncate() failed (%s)\n", + strerror(errno)); + result = KSFT_FAIL; + goto report; } return; }
if (fallocate(fd, 0, 0, size)) { - if (size == pagesize) - ksft_test_result_fail("fallocate() failed (%s)\n", strerror(errno)); - else - ksft_test_result_skip("need more free huge pages\n"); - return; + if (size == pagesize) { + ksft_print_msg("fallocate() failed (%s)\n", strerror(errno)); + result = KSFT_FAIL; + } else { + ksft_print_msg("need more free huge pages\n"); + result = KSFT_SKIP; + } + goto report; }
mem = mmap(NULL, size, PROT_READ | PROT_WRITE, shared ? MAP_SHARED : MAP_PRIVATE, fd, 0); if (mem == MAP_FAILED) { - if (size == pagesize || shared) - ksft_test_result_fail("mmap() failed (%s)\n", strerror(errno)); - else - ksft_test_result_skip("need more free huge pages\n"); - return; + if (size == pagesize || shared) { + ksft_print_msg("mmap() failed (%s)\n", strerror(errno)); + result = KSFT_FAIL; + } else { + ksft_print_msg("need more free huge pages\n"); + result = KSFT_SKIP; + } + goto report; }
/* Fault in the page such that GUP-fast can pin it directly. */ @@ -134,7 +149,8 @@ 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 (%s)\n", strerror(errno)); + ksft_print_msg("mprotect() failed (%s)\n", strerror(errno)); + result = KSFT_FAIL; goto munmap; } /* FALLTHROUGH */ @@ -147,12 +163,14 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) type == TEST_TYPE_RW_FAST;
if (gup_fd < 0) { - ksft_test_result_skip("gup_test not available\n"); + ksft_print_msg("gup_test not available\n"); + result = KSFT_SKIP; break; }
if (rw && shared && fs_is_unknown(fs_type)) { - ksft_test_result_skip("Unknown filesystem\n"); + ksft_print_msg("Unknown filesystem\n"); + result = KSFT_SKIP; return; } /* @@ -169,14 +187,19 @@ 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 (EINVAL)n"); + ksft_print_msg("PIN_LONGTERM_TEST_START failed (EINVAL)n"); + result = KSFT_SKIP; break; } else if (ret && errno == EFAULT) { - ksft_test_result(!should_work, "Should have failed\n"); + if (should_work) + result = KSFT_FAIL; + else + result = KSFT_PASS; break; } else if (ret) { - ksft_test_result_fail("PIN_LONGTERM_TEST_START failed (%s)\n", - strerror(errno)); + ksft_print_msg("PIN_LONGTERM_TEST_START failed (%s)\n", + strerror(errno)); + result = KSFT_FAIL; break; }
@@ -189,7 +212,10 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) * some previously unsupported filesystems, we might want to * perform some additional tests for possible data corruptions. */ - ksft_test_result(should_work, "Should have worked\n"); + if (should_work) + result = KSFT_PASS; + else + result = KSFT_FAIL; break; } #ifdef LOCAL_CONFIG_HAVE_LIBURING @@ -199,8 +225,9 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
/* io_uring always pins pages writable. */ if (shared && fs_is_unknown(fs_type)) { - ksft_test_result_skip("Unknown filesystem\n"); - return; + ksft_print_msg("Unknown filesystem\n"); + result = KSFT_SKIP; + goto report; } should_work = !shared || fs_supports_writable_longterm_pinning(fs_type); @@ -208,8 +235,9 @@ 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 (%s)\n", - strerror(-ret)); + ksft_print_msg("io_uring_queue_init() failed (%s)\n", + strerror(-ret)); + result = KSFT_SKIP; break; } /* @@ -222,17 +250,28 @@ 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 (%s)\n", - strerror(errno)); + if (should_work) { + ksft_print_msg("Should have failed (%s)\n", + strerror(errno)); + result = KSFT_FAIL; + } else { + result = KSFT_PASS; + } } else if (ret) { /* * We might just lack support or have insufficient * MEMLOCK limits. */ - ksft_test_result_skip("io_uring_register_buffers() failed (%s)\n", - strerror(-ret)); + ksft_print_msg("io_uring_register_buffers() failed (%s)\n", + strerror(-ret)); + result = KSFT_SKIP; } else { - ksft_test_result(should_work, "Should have worked\n"); + if (should_work) { + result = KSFT_PASS; + } else { + ksft_print_msg("Should have worked\n"); + result = KSFT_FAIL; + } io_uring_unregister_buffers(&ring); }
@@ -246,6 +285,8 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
munmap: munmap(mem, size); +report: + log_test_result(result); }
typedef void (*test_fn)(int fd, size_t size); @@ -254,13 +295,11 @@ static void run_with_memfd(test_fn fn, const char *desc) { int fd;
- ksft_print_msg("[RUN] %s ... with memfd\n", desc); + log_test_start("%s ... with memfd", desc);
fd = memfd_create("test", 0); - if (fd < 0) { - ksft_test_result_fail("memfd_create() failed (%s)\n", strerror(errno)); - return; - } + if (fd < 0) + ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno));
fn(fd, pagesize); close(fd); @@ -271,23 +310,23 @@ static void run_with_tmpfile(test_fn fn, const char *desc) FILE *file; int fd;
- ksft_print_msg("[RUN] %s ... with tmpfile\n", desc); + log_test_start("%s ... with tmpfile", desc);
file = tmpfile(); if (!file) { - ksft_test_result_fail("tmpfile() failed (%s)\n", strerror(errno)); - return; - } - - fd = fileno(file); - if (fd < 0) { - ksft_test_result_fail("fileno() failed (%s)\n", strerror(errno)); - goto close; + ksft_print_msg("tmpfile() failed (%s)\n", strerror(errno)); + fd = -1; + } else { + fd = fileno(file); + if (fd < 0) { + ksft_print_msg("fileno() failed (%s)\n", strerror(errno)); + } }
fn(fd, pagesize); -close: - fclose(file); + + if (file) + fclose(file); }
static void run_with_local_tmpfile(test_fn fn, const char *desc) @@ -295,22 +334,22 @@ static void run_with_local_tmpfile(test_fn fn, const char *desc) char filename[] = __FILE__"_tmpfile_XXXXXX"; int fd;
- ksft_print_msg("[RUN] %s ... with local tmpfile\n", desc); + log_test_start("%s ... with local tmpfile", desc);
fd = mkstemp(filename); - if (fd < 0) { - ksft_test_result_fail("mkstemp() failed (%s)\n", strerror(errno)); - return; - } + if (fd < 0) + ksft_print_msg("mkstemp() failed (%s)\n", strerror(errno));
if (unlink(filename)) { - ksft_test_result_fail("unlink() failed (%s)\n", strerror(errno)); - goto close; + ksft_print_msg("unlink() failed (%s)\n", strerror(errno)); + close(fd); + fd = -1; }
fn(fd, pagesize); -close: - close(fd); + + if (fd >= 0) + close(fd); }
static void run_with_memfd_hugetlb(test_fn fn, const char *desc, @@ -319,15 +358,14 @@ static void run_with_memfd_hugetlb(test_fn fn, const char *desc, int flags = MFD_HUGETLB; int fd;
- ksft_print_msg("[RUN] %s ... with memfd hugetlb (%zu kB)\n", desc, + log_test_start("%s ... with memfd hugetlb (%zu kB)", desc, hugetlbsize / 1024);
flags |= __builtin_ctzll(hugetlbsize) << MFD_HUGE_SHIFT;
fd = memfd_create("test", flags); if (fd < 0) { - ksft_test_result_skip("memfd_create() failed (%s)\n", strerror(errno)); - return; + ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno)); }
fn(fd, hugetlbsize);
On 27.05.25 18:04, Mark Brown wrote:
The kselftest framework uses the string logged when a test result is reported as the unique identifier for a test, using it to track test results between runs. The gup_longterm test fails to follow this pattern, it runs a single test function repeatedly with various parameters but each result report is a string logging an error message which is fixed between runs.
Since the code already logs each test uniquely before it starts refactor to also print this to a buffer, then use that name as the test result. This isn't especially pretty but is relatively straightforward and is a great help to tooling.
Signed-off-by: Mark Brown broonie@kernel.org
tools/testing/selftests/mm/gup_longterm.c | 150 +++++++++++++++++++----------- 1 file changed, 94 insertions(+), 56 deletions(-)
diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c index e60e62809186..f84ea97c2543 100644 --- a/tools/testing/selftests/mm/gup_longterm.c +++ b/tools/testing/selftests/mm/gup_longterm.c @@ -93,33 +93,48 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) __fsword_t fs_type = get_fs_type(fd); bool should_work; char *mem;
- int result = KSFT_PASS; int ret;
- if (fd < 0) {
result = KSFT_FAIL;goto report;- }
Not a fan of that, especially as it suddenly converts ksft_test_result_skip() -- e.g., on the memfd path -- to KSFT_FAIL.
Can we just do the log_test_result(KSFT_FAIL/KSFT_SKIP) in the caller?
On Tue, Jun 03, 2025 at 02:36:07PM +0200, David Hildenbrand wrote:
On 27.05.25 18:04, Mark Brown wrote:
- int result = KSFT_PASS; int ret;
- if (fd < 0) {
result = KSFT_FAIL;goto report;- }
Not a fan of that, especially as it suddenly converts ksft_test_result_skip() -- e.g., on the memfd path -- to KSFT_FAIL.
It looks like the memfd path was an outlier here, the others all failed if they couldn't allocate the FD.
Can we just do the log_test_result(KSFT_FAIL/KSFT_SKIP) in the caller?
Nothing stopping that, or doing it for the cases where we want to skip. IIRC this simplified some of the other callers a little.
This seems to be causing tests to fail rather than be skipped if hugetlb isn't configured. I bisected the problem to this patch so it's definitely changed how things are handled (though of course it might just be _revealing_ some previously existing bug in this test...).
Using a couple of tests as an example:
Before this patch:
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) ok 39 # SKIP memfd_create() failed (Cannot allocate memory) # [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB) ok 40 # SKIP memfd_create() failed (Cannot allocate memory)
After:
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # memfd_create() failed (Cannot allocate memory) not ok 39 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB) # memfd_create() failed (Cannot allocate memory) not ok 40 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB)
See below, I think I've found where it happens...
On Tue, May 27, 2025 at 05:04:48PM +0100, Mark Brown wrote:
The kselftest framework uses the string logged when a test result is reported as the unique identifier for a test, using it to track test results between runs. The gup_longterm test fails to follow this pattern, it runs a single test function repeatedly with various parameters but each result report is a string logging an error message which is fixed between runs.
Since the code already logs each test uniquely before it starts refactor to also print this to a buffer, then use that name as the test result. This isn't especially pretty but is relatively straightforward and is a great help to tooling.
Signed-off-by: Mark Brown broonie@kernel.org
tools/testing/selftests/mm/gup_longterm.c | 150 +++++++++++++++++++----------- 1 file changed, 94 insertions(+), 56 deletions(-)
diff --git a/tools/testing/selftests/mm/gup_longterm.c b/tools/testing/selftests/mm/gup_longterm.c index e60e62809186..f84ea97c2543 100644 --- a/tools/testing/selftests/mm/gup_longterm.c +++ b/tools/testing/selftests/mm/gup_longterm.c @@ -93,33 +93,48 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) __fsword_t fs_type = get_fs_type(fd); bool should_work; char *mem;
int result = KSFT_PASS; int ret;
if (fd < 0) {
result = KSFT_FAIL;goto report;}
if (ftruncate(fd, size)) { if (errno == ENOENT) { skip_test_dodgy_fs("ftruncate()"); } else {
ksft_test_result_fail("ftruncate() failed (%s)\n", strerror(errno));
ksft_print_msg("ftruncate() failed (%s)\n",strerror(errno));result = KSFT_FAIL;goto report;} return; }
if (fallocate(fd, 0, 0, size)) {
if (size == pagesize)ksft_test_result_fail("fallocate() failed (%s)\n", strerror(errno));elseksft_test_result_skip("need more free huge pages\n");return;
if (size == pagesize) {ksft_print_msg("fallocate() failed (%s)\n", strerror(errno));result = KSFT_FAIL;} else {ksft_print_msg("need more free huge pages\n");result = KSFT_SKIP;}goto report;}
mem = mmap(NULL, size, PROT_READ | PROT_WRITE, shared ? MAP_SHARED : MAP_PRIVATE, fd, 0); if (mem == MAP_FAILED) {
if (size == pagesize || shared)ksft_test_result_fail("mmap() failed (%s)\n", strerror(errno));elseksft_test_result_skip("need more free huge pages\n");return;
if (size == pagesize || shared) {ksft_print_msg("mmap() failed (%s)\n", strerror(errno));result = KSFT_FAIL;} else {ksft_print_msg("need more free huge pages\n");result = KSFT_SKIP;}goto report;}
/* Fault in the page such that GUP-fast can pin it directly. */
@@ -134,7 +149,8 @@ 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 (%s)\n", strerror(errno));
ksft_print_msg("mprotect() failed (%s)\n", strerror(errno));result = KSFT_FAIL; goto munmap;@@ -147,12 +163,14 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) type == TEST_TYPE_RW_FAST;
if (gup_fd < 0) {
ksft_test_result_skip("gup_test not available\n");
ksft_print_msg("gup_test not available\n");result = KSFT_SKIP; break;}
if (rw && shared && fs_is_unknown(fs_type)) {
ksft_test_result_skip("Unknown filesystem\n");
ksft_print_msg("Unknown filesystem\n");result = KSFT_SKIP; return;@@ -169,14 +187,19 @@ 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 (EINVAL)n");
ksft_print_msg("PIN_LONGTERM_TEST_START failed (EINVAL)n");result = KSFT_SKIP; break;
ksft_test_result(!should_work, "Should have failed\n");
if (should_work)result = KSFT_FAIL;elseresult = KSFT_PASS; break;
ksft_test_result_fail("PIN_LONGTERM_TEST_START failed (%s)\n",strerror(errno));
ksft_print_msg("PIN_LONGTERM_TEST_START failed (%s)\n",strerror(errno));result = KSFT_FAIL; break;@@ -189,7 +212,10 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared) * some previously unsupported filesystems, we might want to * perform some additional tests for possible data corruptions. */
ksft_test_result(should_work, "Should have worked\n");
if (should_work)result = KSFT_PASS;elseresult = KSFT_FAIL;#ifdef LOCAL_CONFIG_HAVE_LIBURING @@ -199,8 +225,9 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
/* io_uring always pins pages writable. */ if (shared && fs_is_unknown(fs_type)) {
ksft_test_result_skip("Unknown filesystem\n");return;
ksft_print_msg("Unknown filesystem\n");result = KSFT_SKIP;goto report;@@ -208,8 +235,9 @@ 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 (%s)\n",strerror(-ret));
ksft_print_msg("io_uring_queue_init() failed (%s)\n",strerror(-ret));result = KSFT_SKIP; break;@@ -222,17 +250,28 @@ 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 (%s)\n",strerror(errno));
if (should_work) {ksft_print_msg("Should have failed (%s)\n",strerror(errno));result = KSFT_FAIL;} else {result = KSFT_PASS;}
ksft_test_result_skip("io_uring_register_buffers() failed (%s)\n",strerror(-ret));
ksft_print_msg("io_uring_register_buffers() failed (%s)\n",strerror(-ret));result = KSFT_SKIP;
ksft_test_result(should_work, "Should have worked\n");
if (should_work) {result = KSFT_PASS;} else {ksft_print_msg("Should have worked\n");result = KSFT_FAIL;} io_uring_unregister_buffers(&ring);@@ -246,6 +285,8 @@ static void do_test(int fd, size_t size, enum test_type type, bool shared)
munmap: munmap(mem, size); +report:
- log_test_result(result);
}
typedef void (*test_fn)(int fd, size_t size); @@ -254,13 +295,11 @@ static void run_with_memfd(test_fn fn, const char *desc) { int fd;
- ksft_print_msg("[RUN] %s ... with memfd\n", desc);
log_test_start("%s ... with memfd", desc);
fd = memfd_create("test", 0);
- if (fd < 0) {
ksft_test_result_fail("memfd_create() failed (%s)\n", strerror(errno));return;- }
if (fd < 0)
ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno));fn(fd, pagesize); close(fd);
@@ -271,23 +310,23 @@ static void run_with_tmpfile(test_fn fn, const char *desc) FILE *file; int fd;
- ksft_print_msg("[RUN] %s ... with tmpfile\n", desc);
log_test_start("%s ... with tmpfile", desc);
file = tmpfile(); if (!file) {
ksft_test_result_fail("tmpfile() failed (%s)\n", strerror(errno));return;- }
- fd = fileno(file);
- if (fd < 0) {
ksft_test_result_fail("fileno() failed (%s)\n", strerror(errno));goto close;
ksft_print_msg("tmpfile() failed (%s)\n", strerror(errno));fd = -1;} else {
fd = fileno(file);if (fd < 0) {ksft_print_msg("fileno() failed (%s)\n", strerror(errno));}}
fn(fd, pagesize);
-close:
- fclose(file);
- if (file)
fclose(file);}
static void run_with_local_tmpfile(test_fn fn, const char *desc) @@ -295,22 +334,22 @@ static void run_with_local_tmpfile(test_fn fn, const char *desc) char filename[] = __FILE__"_tmpfile_XXXXXX"; int fd;
- ksft_print_msg("[RUN] %s ... with local tmpfile\n", desc);
log_test_start("%s ... with local tmpfile", desc);
fd = mkstemp(filename);
- if (fd < 0) {
ksft_test_result_fail("mkstemp() failed (%s)\n", strerror(errno));return;- }
if (fd < 0)
ksft_print_msg("mkstemp() failed (%s)\n", strerror(errno));if (unlink(filename)) {
ksft_test_result_fail("unlink() failed (%s)\n", strerror(errno));goto close;
ksft_print_msg("unlink() failed (%s)\n", strerror(errno));close(fd);fd = -1;}
fn(fd, pagesize);
-close:
- close(fd);
- if (fd >= 0)
close(fd);}
static void run_with_memfd_hugetlb(test_fn fn, const char *desc, @@ -319,15 +358,14 @@ static void run_with_memfd_hugetlb(test_fn fn, const char *desc, int flags = MFD_HUGETLB; int fd;
- ksft_print_msg("[RUN] %s ... with memfd hugetlb (%zu kB)\n", desc,
log_test_start("%s ... with memfd hugetlb (%zu kB)", desc, hugetlbsize / 1024);
flags |= __builtin_ctzll(hugetlbsize) << MFD_HUGE_SHIFT;
fd = memfd_create("test", flags); if (fd < 0) {
ksft_test_result_skip("memfd_create() failed (%s)\n", strerror(errno));return;
ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno));
I think it's this change that does it (probably).
I wonder if it's worth checking for errno == ENOMEM and skipping in this case? Or is that too general?
}
fn(fd, hugetlbsize);
-- 2.39.5
On Thu, Jun 05, 2025 at 05:00:49PM +0100, Lorenzo Stoakes wrote:
This seems to be causing tests to fail rather than be skipped if hugetlb isn't configured. I bisected the problem to this patch so it's definitely changed how things are handled (though of course it might just be _revealing_ some previously existing bug in this test...).
Using a couple of tests as an example:
Before this patch:
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # memfd_create() failed (Cannot allocate memory) not ok 39 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB) # memfd_create() failed (Cannot allocate memory) not ok 40 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB)
That's the thing with memfd being special and skipping on setup failure that David mentioned, I've got a patch as part of the formatting series I was going to send after the merge window.
On Thu, Jun 05, 2025 at 05:15:51PM +0100, Mark Brown wrote:
On Thu, Jun 05, 2025 at 05:00:49PM +0100, Lorenzo Stoakes wrote:
This seems to be causing tests to fail rather than be skipped if hugetlb isn't configured. I bisected the problem to this patch so it's definitely changed how things are handled (though of course it might just be _revealing_ some previously existing bug in this test...).
Using a couple of tests as an example:
Before this patch:
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # memfd_create() failed (Cannot allocate memory) not ok 39 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB) # memfd_create() failed (Cannot allocate memory) not ok 40 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB)
That's the thing with memfd being special and skipping on setup failure that David mentioned, I've got a patch as part of the formatting series I was going to send after the merge window.
where did he mention this?
I mean I'd argue that making a test that previously worked now fail due to how somebody's set up their system is a reason not to merge that patch.
Better to do all of these formating fixes and maintain the _same behaviour_ then separately tackle whether or not we should skip.
Obviously the better option would be to somehow determine if hugetlb is available in advance (of course, theoretically somebody could come in and reserve pages but that's not veyr likely).
Anyway, mm-new accepts patches during the merge window, one of the advantages of having this separated out from mm-unstable, so there's no reason not to send something now.
On Thu, Jun 05, 2025 at 05:26:05PM +0100, Lorenzo Stoakes wrote:
On Thu, Jun 05, 2025 at 05:15:51PM +0100, Mark Brown wrote:
That's the thing with memfd being special and skipping on setup failure that David mentioned, I've got a patch as part of the formatting series I was going to send after the merge window.
where did he mention this?
I can't remember off hand, sorry.
I mean I'd argue that making a test that previously worked now fail due to how somebody's set up their system is a reason not to merge that patch.
Well, it's a bit late now given that this is in Linus' tree and actually it turns out this was the only update for gup_longterm so I just rebased it onto Linus' tree and kicked off my tests.
Better to do all of these formating fixes and maintain the _same behaviour_ then separately tackle whether or not we should skip.
I'm confused, that's generally the opposite of the standard advice for the kernel - usually it's fixes first, then deal with anything cosmetic or new?
Obviously the better option would be to somehow determine if hugetlb is available in advance (of course, theoretically somebody could come in and reserve pages but that's not veyr likely).
The tests do enumerate the set of available hugepage sizes at runtime (see the loop in run_test_case()) but detect_hugetlb_page_sizes() just looks in /sys/kernel/mm/hugepages/ for subdirectories and doesn't look inside those directories to see if there are actually any huge pages available for the huge page sizes advertised. There's probably utility in at least a version of that function that checks.
On 05.06.25 18:42, Mark Brown wrote:
On Thu, Jun 05, 2025 at 05:26:05PM +0100, Lorenzo Stoakes wrote:
On Thu, Jun 05, 2025 at 05:15:51PM +0100, Mark Brown wrote:
That's the thing with memfd being special and skipping on setup failure that David mentioned, I've got a patch as part of the formatting series I was going to send after the merge window.
where did he mention this?
I can't remember off hand, sorry.
I assume in ... my review to patch #4?
What an unpleasant upstream experience.
On Thu, Jun 05, 2025 at 06:55:53PM +0200, David Hildenbrand wrote:
On 05.06.25 18:42, Mark Brown wrote:
I can't remember off hand, sorry.
I assume in ... my review to patch #4?
Oh, yeah - it's there. I did look there but the "not a fan" bit made me think it was one of the stylistic things as I quickly scanned through.
What an unpleasant upstream experience.
TBH this has been a lot better than the more common failure mode with working on selftests where people just completely ignore or are openly dismissive about them :/ . Probably room for a middle ground though.
On 05.06.25 19:19, Mark Brown wrote:
On Thu, Jun 05, 2025 at 06:55:53PM +0200, David Hildenbrand wrote:
On 05.06.25 18:42, Mark Brown wrote:
I can't remember off hand, sorry.
I assume in ... my review to patch #4?
Oh, yeah - it's there. I did look there but the "not a fan" bit made me think it was one of the stylistic things as I quickly scanned through.
What an unpleasant upstream experience.
TBH this has been a lot better than the more common failure mode with working on selftests where people just completely ignore or are openly dismissive about them :/ . Probably room for a middle ground though.
Can we *please* limit such reworks to mechanical changes in the future?
It's just absolutely hard to spot these things during review (I did at least on patch #4).
And Andrew apparently just merges them -- and I am left with the feeling that we create more mess by "accident".
Anyhow, thanks for working on these tests ...
On Thu, Jun 05, 2025 at 07:34:28PM +0200, David Hildenbrand wrote:
On 05.06.25 19:19, Mark Brown wrote:
TBH this has been a lot better than the more common failure mode with working on selftests where people just completely ignore or are openly dismissive about them :/ . Probably room for a middle ground though.
Can we *please* limit such reworks to mechanical changes in the future?
Yes, that's better in general.
On Thu, Jun 05, 2025 at 05:42:55PM +0100, Mark Brown wrote:
On Thu, Jun 05, 2025 at 05:26:05PM +0100, Lorenzo Stoakes wrote:
On Thu, Jun 05, 2025 at 05:15:51PM +0100, Mark Brown wrote:
That's the thing with memfd being special and skipping on setup failure that David mentioned, I've got a patch as part of the formatting series I was going to send after the merge window.
where did he mention this?
I can't remember off hand, sorry.
I see his reply here and I guess it's because you now determine the result at the top level, I see you are doing this in do_test():
+ int result = KSFT_PASS; int ret;
+ if (fd < 0) { + result = KSFT_FAIL; + goto report; + } +
And in run_with_memfd_hugetlb():
fd = memfd_create("test", flags); if (fd < 0) { - ksft_test_result_skip("memfd_create() failed (%s)\n", strerror(errno)); - return; + ksft_print_msg("memfd_create() failed (%s)\n", strerror(errno)); }
So previously this would skip, now it fails.
I've not double, triple checked this is it, but seems likely!
You said you had a fix
I mean I'd argue that making a test that previously worked now fail due to how somebody's set up their system is a reason not to merge that patch.
Well, it's a bit late now given that this is in Linus' tree and actually it turns out this was the only update for gup_longterm so I just rebased it onto Linus' tree and kicked off my tests.
Ack.
Better to do all of these formating fixes and maintain the _same behaviour_ then separately tackle whether or not we should skip.
I'm confused, that's generally the opposite of the standard advice for the kernel - usually it's fixes first, then deal with anything cosmetic or new?
I mean the crux is that the 'cosmetic' changes also included a 'this might break things' change.
I'm saying do the cosmetic things in _isolation_, or fix the brokenness before doing the whole lot.
Anyway there's no point dwelling on this, let's just get a fix sorted.
Obviously the better option would be to somehow determine if hugetlb is available in advance (of course, theoretically somebody could come in and reserve pages but that's not veyr likely).
The tests do enumerate the set of available hugepage sizes at runtime (see the loop in run_test_case()) but detect_hugetlb_page_sizes() just looks in /sys/kernel/mm/hugepages/ for subdirectories and doesn't look inside those directories to see if there are actually any huge pages available for the huge page sizes advertised. There's probably utility in at least a version of that function that checks.
Right yes, I mean obviously this whole thing is a mess already that's not your fault, and ideally we'd have some general way of looking this up across _all_ tests and just switch things on/off accordingly.
There's a whole Pandora's box about what the tests should assume/not and yeah. Anyway. Maybe leave it closed for now :)
Cheers, Lorenzo
On Thu, Jun 05, 2025 at 06:09:09PM +0100, Lorenzo Stoakes wrote:
On Thu, Jun 05, 2025 at 05:42:55PM +0100, Mark Brown wrote:
Better to do all of these formating fixes and maintain the _same behaviour_ then separately tackle whether or not we should skip.
I'm confused, that's generally the opposite of the standard advice for the kernel - usually it's fixes first, then deal with anything cosmetic or new?
I mean the crux is that the 'cosmetic' changes also included a 'this might break things' change.
No, the cosmetic changes are separate. I'm just saying I have a small bunch of stuff based on David's feedback to send out after the merge window.
I'm saying do the cosmetic things in _isolation_, or fix the brokenness before doing the whole lot.
Some subsystems will complain if you send anything that isn't urgent during the merge window, this looked more like an "I suppose you could configure the kernel that way" problem than a "people will routinely run into this" one, I was expecting it (or something) to go in as a fix but that it was safer to wait for -rc1 to send.
Obviously the better option would be to somehow determine if hugetlb is available in advance (of course, theoretically somebody could come in and reserve pages but that's not veyr likely).
The tests do enumerate the set of available hugepage sizes at runtime (see the loop in run_test_case()) but detect_hugetlb_page_sizes() just looks in /sys/kernel/mm/hugepages/ for subdirectories and doesn't look inside those directories to see if there are actually any huge pages available for the huge page sizes advertised. There's probably utility in at least a version of that function that checks.
Right yes, I mean obviously this whole thing is a mess already that's not your fault, and ideally we'd have some general way of looking this up across _all_ tests and just switch things on/off accordingly.
That is at least library code so it'd get the three tests that use it, though possibly one of them actually wants the current behaviour for some reason?
There's a whole Pandora's box about what the tests should assume/not and yeah. Anyway. Maybe leave it closed for now :)
It's separate, yeah. It'd also be good to document what you need to enable all the tests somewhere as well - there's the config fragment already which is good, but you also at least need a bunch of command line options to set up huge pages and enable secretmem.
Mark, I'm not finding this productive.
Bottom line is you've broken the tests, please fix them or if you're not willing to I'll send a fix.
Thanks.
On Thu, Jun 05, 2025 at 06:38:36PM +0100, Mark Brown wrote:
On Thu, Jun 05, 2025 at 06:09:09PM +0100, Lorenzo Stoakes wrote:
On Thu, Jun 05, 2025 at 05:42:55PM +0100, Mark Brown wrote:
Better to do all of these formating fixes and maintain the _same behaviour_ then separately tackle whether or not we should skip.
I'm confused, that's generally the opposite of the standard advice for the kernel - usually it's fixes first, then deal with anything cosmetic or new?
I mean the crux is that the 'cosmetic' changes also included a 'this might break things' change.
No, the cosmetic changes are separate. I'm just saying I have a small bunch of stuff based on David's feedback to send out after the merge window.
I'm saying do the cosmetic things in _isolation_, or fix the brokenness before doing the whole lot.
Some subsystems will complain if you send anything that isn't urgent during the merge window, this looked more like an "I suppose you could configure the kernel that way" problem than a "people will routinely run into this" one, I was expecting it (or something) to go in as a fix but that it was safer to wait for -rc1 to send.
Obviously the better option would be to somehow determine if hugetlb is available in advance (of course, theoretically somebody could come in and reserve pages but that's not veyr likely).
The tests do enumerate the set of available hugepage sizes at runtime (see the loop in run_test_case()) but detect_hugetlb_page_sizes() just looks in /sys/kernel/mm/hugepages/ for subdirectories and doesn't look inside those directories to see if there are actually any huge pages available for the huge page sizes advertised. There's probably utility in at least a version of that function that checks.
Right yes, I mean obviously this whole thing is a mess already that's not your fault, and ideally we'd have some general way of looking this up across _all_ tests and just switch things on/off accordingly.
That is at least library code so it'd get the three tests that use it, though possibly one of them actually wants the current behaviour for some reason?
There's a whole Pandora's box about what the tests should assume/not and yeah. Anyway. Maybe leave it closed for now :)
It's separate, yeah. It'd also be good to document what you need to enable all the tests somewhere as well - there's the config fragment already which is good, but you also at least need a bunch of command line options to set up huge pages and enable secretmem.
On Thu, Jun 05, 2025 at 06:47:28PM +0100, Lorenzo Stoakes wrote:
Mark, I'm not finding this productive.
Bottom line is you've broken the tests, please fix them or if you're not willing to I'll send a fix.
Sure, like I said further up I'm just running my patch through testing at the minute.
On Thu, Jun 05, 2025 at 07:29:58PM +0100, Mark Brown wrote:
On Thu, Jun 05, 2025 at 06:47:28PM +0100, Lorenzo Stoakes wrote:
Mark, I'm not finding this productive.
Bottom line is you've broken the tests, please fix them or if you're not willing to I'll send a fix.
Sure, like I said further up I'm just running my patch through testing at the minute.
Awesome thanks, appreciate it!
On 05.06.25 18:15, Mark Brown wrote:
On Thu, Jun 05, 2025 at 05:00:49PM +0100, Lorenzo Stoakes wrote:
This seems to be causing tests to fail rather than be skipped if hugetlb isn't configured. I bisected the problem to this patch so it's definitely changed how things are handled (though of course it might just be _revealing_ some previously existing bug in this test...).
Using a couple of tests as an example:
Before this patch:
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # memfd_create() failed (Cannot allocate memory) not ok 39 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB) # memfd_create() failed (Cannot allocate memory) not ok 40 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB)
That's the thing with memfd being special and skipping on setup failure that David mentioned, I've got a patch as part of the formatting series I was going to send after the merge window.
@Andew, why did this series get merged already?
On Thu, 5 Jun 2025 18:48:49 +0200 David Hildenbrand david@redhat.com wrote:
On 05.06.25 18:15, Mark Brown wrote:
On Thu, Jun 05, 2025 at 05:00:49PM +0100, Lorenzo Stoakes wrote:
This seems to be causing tests to fail rather than be skipped if hugetlb isn't configured. I bisected the problem to this patch so it's definitely changed how things are handled (though of course it might just be _revealing_ some previously existing bug in this test...).
Using a couple of tests as an example:
Before this patch:
# [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # memfd_create() failed (Cannot allocate memory) not ok 39 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (2048 kB) # [RUN] R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB) # memfd_create() failed (Cannot allocate memory) not ok 40 R/O longterm GUP-fast pin in MAP_PRIVATE file mapping ... with memfd hugetlb (1048576 kB)
That's the thing with memfd being special and skipping on setup failure that David mentioned, I've got a patch as part of the formatting series I was going to send after the merge window.
@Andew, why did this series get merged already?
Late merge window hastiness :( And I saw nothing worrisome in the review.
linux-kselftest-mirror@lists.linaro.org
-
Andrew Morton -
David Hildenbrand -
Lorenzo Stoakes -
Mark Brown