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.
