Message-ID: <CABdmKX1jruzSn0E883xfo_pYOimpTTC2HHw=3JG6NYHAhXZrsw@mail.gmail.com>
Date: Thu, 29 Feb 2024 09:36:39 -0800
From: "T.J. Mercier" <tjmercier@...gle.com>
To: Muhammad Usama Anjum <usama.anjum@...labora.com>
Cc: Shuah Khan <shuah@...nel.org>, kernel@...labora.com, linux-kselftest@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] selftests/dmabuf-heap: conform test to TAP format output

On Thu, Feb 29, 2024 at 1:14 AM Muhammad Usama Anjum
<usama.anjum@...labora.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@...labora.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@...labora.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/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.

Powered by blists - more mailing lists