On Thu, Feb 29, 2024 at 1:14 AM Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 2/28/24 11:47 PM, T.J. Mercier wrote:
On Wed, Feb 28, 2024 at 3:46 AM Muhammad Usama Anjum usama.anjum@collabora.com wrote:
On 2/27/24 10:18 PM, T.J. Mercier wrote:
On Tue, Feb 27, 2024 at 4:21 AM Muhammad Usama Anjum usama.anjum@collabora.com wrote:
...
-static int test_alloc_zeroed(char *heap_name, size_t size) +static void test_alloc_zeroed(char *heap_name, size_t size) { int heap_fd = -1, dmabuf_fd[32]; int i, j, ret; void *p = NULL; char *c;
printf(" Testing alloced %ldk buffers are zeroed: ", size / 1024);
ksft_print_msg("Testing alloced %ldk buffers are zeroed:\n", size / 1024); heap_fd = dmabuf_heap_open(heap_name);
if (heap_fd < 0)return -1; /* Allocate and fill a bunch of buffers */ for (i = 0; i < 32; i++) { ret = dmabuf_heap_alloc(heap_fd, size, 0, &dmabuf_fd[i]);if (ret < 0) {printf("FAIL (Allocation (%i) failed)\n", i);goto out;}
if (ret)ksft_exit_fail_msg("FAIL (Allocation (%i) failed)\n", i);/* mmap and fill with simple pattern */ p = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, dmabuf_fd[i], 0);
if (p == MAP_FAILED) {printf("FAIL (mmap() failed!)\n");ret = -1;goto out;}
if (p == MAP_FAILED)ksft_exit_fail_msg("FAIL (mmap() failed!)\n");So based on the previous ksft_exit_fail_msg calls I thought your intention was to exit the program and never run subsequent tests when errors occurred. That's what led to my initial comment about switching to ksft_exit_fail_msg from ksft_print_msg here, and I expected to see only ksft_exit_fail_msg for error cases afterwards. But you're still mixing ksft_exit_fail_msg and (ksft_print_msg + ksft_test_result{_pass,_fail,_skip}) so we've got a mix of behaviors where some errors lead to complete program exits and different errors lead to skipped/failed tests followed by further progress.
It seems most useful and predictable to me to have all tests run even after encountering an error for a single test, which we don't get when ksft_exit_fail_msg is called from the individual tests. I was fine with switching all error handling to ksft_exit_fail_msg to eliminate cleanup code and reduce maintenance, but I think we should be consistent with the behavior for dealing with errors which this doesn't currently have. So let's either always call ksft_exit_fail_msg for errors, or never call it (my preference).
The following rules are being used:
- If a fetal error occurs where initial conditions to perform a test aren't
fulfilled, we exit the entire test by ksft_exit_fail_msg().
But this doesn't exit just the test, it exits the entire program.
- If some test fails after fulfilling of initial conditions,
ksft_print_msg() + ksft_test_result{_pass,_fail} are used to avoid putting multiple ksft_test_result_fail() and later ksft_test_result_pass.
ksft_exit_fail_msg() like behaviour was being followed before this patch. On non-zero return value, all of following test weren't being run. ksft_exit_fail_msg() cannot be used on every failure as it wouldn't run following test cases.
Yeah this is what I'm saying. I'd prefer to always run remaining test cases for the current heap, and all test cases for subsequent heaps following an error so you can see all the passes/fails at once. (like continue in the while loop in main instead of break w/the current implementation) ksft_exit_fail_msg ends the whole program and that's what was happening before, but that means the number of test results that gets reported is inconsistent (unless everything always passes for all heaps). Failures from one heap mask passes/fails in failures from other heaps, and that's inconvenient for CI which expects to see the same set of reported test results across runs, but will have nothing to report for tests skipped due to premature program exit from ksft_exit_fail_msg that could have been a single test failure. Like you mentioned this would be a behavior change, but IDK if it's worth the churn to exactly duplicate the existing behavior and then go back to retouch many of the same spots in a later patch to get (what I consider) better behavior from the program.
The docs mention about calling ksft_exit_* only once after all tests are finished: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tool...
But actual usage seems to be split between ksft_exit_fail_msg for all the things (e.g. fchmodat2_test.c), and ksft_exit_skip/fail for prerequisites + ksft_test_result_skip/pass/fail for individual tests followed by ksft_exit_fail_msg once at the end (e.g. ksm_functional_tests.c).
So what you have is fine based on the fact that nobody has fixed it yet, but I think we could do better for not a lot of work here.
I'll send a v3 by fixing only the other thing you caught.
Ok, but this is all that's needed:
@@ -152,8 +152,10 @@ static void test_alloc_and_import(char *heap_name)
ksft_print_msg("Testing allocation and importing:\n"); ret = dmabuf_heap_alloc(heap_fd, ONE_MEG, 0, &dmabuf_fd); - if (ret) - ksft_exit_fail_msg("FAIL (Allocation Failed!)\n"); + if (ret) { + ksft_test_result_fail("FAIL (Allocation Failed!)\n"); + return; + }
/* mmap and write a simple pattern */ p = mmap(NULL, @@ -162,8 +164,10 @@ static void test_alloc_and_import(char *heap_name) MAP_SHARED, dmabuf_fd, 0); - if (p == MAP_FAILED) - ksft_exit_fail_msg("FAIL (mmap() failed)\n"); + if (p == MAP_FAILED) { + ksft_test_result_fail("FAIL (mmap() failed)\n"); + return; + }
dmabuf_sync(dmabuf_fd, DMA_BUF_SYNC_START); memset(p, 1, ONE_MEG / 2); @@ -217,13 +221,17 @@ static void test_alloc_zeroed(char *heap_name, size_t size) /* Allocate and fill a bunch of buffers */ for (i = 0; i < 32; i++) { ret = dmabuf_heap_alloc(heap_fd, size, 0, &dmabuf_fd[i]); - if (ret) - ksft_exit_fail_msg("FAIL (Allocation (%i) failed)\n", i); + if (ret) { + ksft_test_result_fail("FAIL (Allocation (%i) failed)\n", i); + return; + }
/* mmap and fill with simple pattern */ p = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, dmabuf_fd[i], 0); - if (p == MAP_FAILED) - ksft_exit_fail_msg("FAIL (mmap() failed!)\n"); + if (p == MAP_FAILED) { + ksft_test_result_fail("FAIL (mmap() failed!)\n"); + return; + }
dmabuf_sync(dmabuf_fd[i], DMA_BUF_SYNC_START); memset(p, 0xff, size); @@ -238,13 +246,17 @@ static void test_alloc_zeroed(char *heap_name, size_t size) /* Allocate and validate all buffers are zeroed */ for (i = 0; i < 32; i++) { ret = dmabuf_heap_alloc(heap_fd, size, 0, &dmabuf_fd[i]); - if (ret < 0) - ksft_exit_fail_msg("FAIL (Allocation (%i) failed)\n", i); + if (ret < 0) { + ksft_test_result_fail("FAIL (Allocation (%i) failed)\n", i); + return; + }
/* mmap and validate everything is zero */ p = mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, dmabuf_fd[i], 0); - if (p == MAP_FAILED) - ksft_exit_fail_msg("FAIL (mmap() failed!)\n"); + if (p == MAP_FAILED) { + ksft_test_result_fail("FAIL (mmap() failed!)\n"); + return; + }
Otherwise, on a Pixel 6 I get just:
TAP version 13 1..176 # Testing heap: aaudio_capture_heap # ======================================= # Testing allocation and importing: Bail out! FAIL (Allocation Failed!) # Planned tests != run tests (176 != 0) # Totals: pass:0 fail:0 xfail:0 xpass:0 skip:0 error:0
and none of the other 15 heaps are ever tested.