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

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;