Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test
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 3/4] selftests/mm: Report unique test names for each cow test
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); signature.asc Description: PGP signature
Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test
On Tue, Jun 03, 2025 at 06:57:38PM +0200, David Hildenbrand wrote: > On 03.06.25 17:22, Mark Brown wrote: > > Like I've been saying this is just the final test result, in this case I > > would expect that for the actual thing we're trying to test any > > confusion would be addressed in the name of the test so that it's clear > > what it was trying to test. So adding "Leak from parent to child" to > > the name of all the tests? > > I agree that printing something in case KSFT_PASS does not make sense > indeed. > > But if something goes wrong (KSFT_FAIL/KSFT_SKIP) I would expect a reason in > all cases. > > IIRC kselftest_harness.h behaves that way: That's mostly just it being chatty because it uses an assert based idiom rather than explicit pass/fail reports, it's a lot less common for things written directly to kselftest.h where it's for example fairly common to see a result detected directly in a ksft_result() call. That does tend to be quite helpful when looking at the results, you don't need to dig out the logs so often. signature.asc Description: PGP signature
Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test
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
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?
signature.asc
Description: PGP signature
Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test
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
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...
signature.asc
Description: PGP signature
Re: [PATCH v2 3/4] selftests/mm: Report unique test names for each cow test
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
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.
signature.asc
Description: PGP signature
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
---
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);
