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

[David Hildenbrand <david@redhat.com>]

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