[PATCH v2 0/4] selftests/mm: cow and gup_longterm cleanups

[PATCH v2 0/4] selftests/mm: cow and gup_longterm cleanups

[Mark Brown]

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@kernel.org

---
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,
-- 
Mark Brown <broonie@kernel.org>

[PATCH v2 1/4] selftests/mm: Use standard ksft_finished() in cow and gup_longterm

[Mark Brown]

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();
 }

-- 
2.39.5

[PATCH v2 2/4] selftests/mm: Add helper for logging test start and results

[Mark Brown]

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
  */

-- 
2.39.5

[PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[Mark Brown]

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;
 	}
 

-- 
2.39.5

[PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Mark Brown]

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);

-- 
2.39.5

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[David Hildenbrand]

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?


-- 
Cheers,

David / dhildenb

Re: [PATCH v2 2/4] selftests/mm: Add helper for logging test start and results

[David Hildenbrand]

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.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[David Hildenbrand]

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);
>   		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.

> -			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);
>   	} 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().

> +		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");

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);
>   		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);

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);
>   		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");

perror?

> +			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");

perror?

Probably worth going over all your changes and double checking them, I'm 
sure I missed some.

> +		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;

[...]

>   	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);

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);
>   		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");

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);
>   	free(old);

[...]

>   
> @@ -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);
>   		goto munmap;
-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 666 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

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.

> >   	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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[David Hildenbrand]

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");

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

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...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[David Hildenbrand]

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);

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[David Hildenbrand]

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).

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1069 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

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);
 


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] selftests/mm: Add helper for logging test start and results

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1208 bytes --]

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 :(

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/4] selftests/mm: Add helper for logging test start and results

[David Hildenbrand]

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 ...

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test

[David Hildenbrand]

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.


-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Lorenzo Stoakes]

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));
> -		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));

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
>

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1123 bytes --]

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.  

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Lorenzo Stoakes]

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.

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1626 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[David Hildenbrand]

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?

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[David Hildenbrand]

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.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Lorenzo Stoakes]

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

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[David Hildenbrand]

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 ...

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2514 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Lorenzo Stoakes]

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.

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 437 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 300 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Lorenzo Stoakes]

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!

Re: [PATCH v2 4/4] selftests/mm: Fix test result reporting in gup_longterm

[Andrew Morton]

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.