On 2/29/24 10:36 PM, T.J. Mercier wrote:
> On Thu, Feb 29, 2024 at 1:14 AM Muhammad Usama Anjum
> <usama.anjum@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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/tools/testing/selftests/kselftest.h?h=v6.8-rc6#n29
>>>
>>> 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.
>
I'll send the updated patch.
--
BR,
Muhammad Usama Anjum
