Received-SPF: pass (google.com: domain of linux-kernel+bounces-87187-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240227121920.1905095-1-usama.anjum@collabora.com>
 <CABdmKX3_+G20TB5HJLLLMQQ1-i9g=RV1QU_A00Knd08pyiJWgw@mail.gmail.com>
 <a90a91bd-6182-412f-bfd9-fa3e3f9cc93e@collabora.com> <CABdmKX0d_Q2ishxS_bS7siEOmwDOobnTOyXK-QbdKsn_0Jv1VQ@mail.gmail.com>
 <202d7b66-edc4-4416-a32f-4d702742f44b@collabora.com>
In-Reply-To: <202d7b66-edc4-4416-a32f-4d702742f44b@collabora.com>
From: "T.J. Mercier" <tjmercier@google.com>
Date: Thu, 29 Feb 2024 09:36:39 -0800
Message-ID: <CABdmKX1jruzSn0E883xfo_pYOimpTTC2HHw=3JG6NYHAhXZrsw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] selftests/dmabuf-heap: conform test to TAP format output
To: Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc: Shuah Khan <shuah@kernel.org>, kernel@collabora.com, linux-kselftest@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 29, 2024 at 1:14=E2=80=AFAM 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=E2=80=AFAM 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=E2=80=AFAM 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 =3D -1, dmabuf_fd[32];
> >>>>         int i, j, ret;
> >>>>         void *p =3D 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 =3D dmabuf_heap_open(heap_name);
> >>>> -       if (heap_fd < 0)
> >>>> -               return -1;
> >>>>
> >>>>         /* Allocate and fill a bunch of buffers */
> >>>>         for (i =3D 0; i < 32; i++) {
> >>>>                 ret =3D 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) fa=
iled)\n", i);
> >>>> +
> >>>>                 /* mmap and fill with simple pattern */
> >>>>                 p =3D mmap(NULL, size, PROT_READ | PROT_WRITE, MAP_S=
HARED, dmabuf_fd[i], 0);
> >>>> -               if (p =3D=3D MAP_FAILED) {
> >>>> -                       printf("FAIL (mmap() failed!)\n");
> >>>> -                       ret =3D -1;
> >>>> -                       goto out;
> >>>> -               }
> >>>> +               if (p =3D=3D 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 switchin=
g
> >>> 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 whe=
n
> >>> 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_ms=
g
> >>> 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 a=
ren'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 put=
ting
> >> multiple ksft_test_result_fail() and later ksft_test_result_pass.
> >>
> >> ksft_exit_fail_msg() like behaviour was being followed before this pat=
ch.
> >> 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 ru=
n
> >> 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=3Dv6.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 =3D 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 =3D mmap(NULL,
@@ -162,8 +164,10 @@ static void test_alloc_and_import(char *heap_name)
                 MAP_SHARED,
                 dmabuf_fd,
                 0);
-       if (p =3D=3D MAP_FAILED)
-               ksft_exit_fail_msg("FAIL (mmap() failed)\n");
+       if (p =3D=3D 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 =3D 0; i < 32; i++) {
                ret =3D 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 =3D mmap(NULL, size, PROT_READ | PROT_WRITE,
MAP_SHARED, dmabuf_fd[i], 0);
-               if (p =3D=3D MAP_FAILED)
-                       ksft_exit_fail_msg("FAIL (mmap() failed!)\n");
+               if (p =3D=3D 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 =3D 0; i < 32; i++) {
                ret =3D 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 =3D mmap(NULL, size, PROT_READ | PROT_WRITE,
MAP_SHARED, dmabuf_fd[i], 0);
-               if (p =3D=3D MAP_FAILED)
-                       ksft_exit_fail_msg("FAIL (mmap() failed!)\n");
+               if (p =3D=3D 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
# =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
# Testing allocation and importing:
Bail out! FAIL (Allocation Failed!)
# Planned tests !=3D run tests (176 !=3D 0)
# Totals: pass:0 fail:0 xfail:0 xpass:0 skip:0 error:0

and none of the other 15 heaps are ever tested.