On Mon, Oct 30, 2023 at 11:58:20AM +0100, Marco Pagani wrote:

On 2023-10-25 10:43, Maxime Ripard wrote:

...

On Tue, Oct 24, 2023 at 07:14:25PM +0200, Marco Pagani wrote:

...

...

...

+static void drm_gem_shmem_test_obj_create_private(struct kunit *test) +{

• struct fake_dev *fdev = test->priv;
• struct drm_gem_shmem_object *shmem;
• struct drm_gem_object *gem_obj;
• struct dma_buf buf_mock;
• struct dma_buf_attachment attach_mock;
• struct sg_table *sgt;
• char *buf;
• int ret;
• /* Create a mock scatter/gather table */
• buf = kunit_kzalloc(test, TEST_SIZE, GFP_KERNEL);
• KUNIT_ASSERT_NOT_NULL(test, buf);
• sgt = kzalloc(sizeof(*sgt), GFP_KERNEL);
• KUNIT_ASSERT_NOT_NULL(test, sgt);
• ret = sg_alloc_table(sgt, 1, GFP_KERNEL);
• KUNIT_ASSERT_EQ(test, ret, 0);
• sg_init_one(sgt->sgl, buf, TEST_SIZE);
• /* Init a mock DMA-BUF */
• buf_mock.size = TEST_SIZE;
• attach_mock.dmabuf = &buf_mock;
• gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt);
• KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);
• KUNIT_ASSERT_EQ(test, gem_obj->size, TEST_SIZE);
• KUNIT_ASSERT_NULL(test, gem_obj->filp);
• KUNIT_ASSERT_NOT_NULL(test, gem_obj->funcs);
• shmem = to_drm_gem_shmem_obj(gem_obj);
• KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);
• /* The scatter/gather table is freed by drm_gem_shmem_free */
• drm_gem_shmem_free(shmem);

+}

KUNIT_ASSERT_* will stop the execution of the test on failure, you should probably use a bit more of KUNIT_EXPECT_* calls otherwise you'll leak resources.

You also probably want to use a kunit_action to clean up and avoid that whole discussion

You are right. I slightly prefer using KUnit expectations (unless actions are strictly necessary) since I feel using actions makes test cases a bit less straightforward to understand. Is this okay for you?

I disagree. Actions make it easier to reason about, even when comparing assertion vs expectation

Like, for the call to sg_alloc_table and drm_gem_shmem_prime_import_sg_table(), the reasonable use of assert vs expect would be something like:

sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); KUNIT_ASSERT_NOT_NULL(test, sgt);

ret = sg_alloc_table(sgt, 1, GFP_KERNEL); KUNIT_ASSERT_EQ(test, ret, 0);

/*

• Here, it's already not super clear whether you want to expect vs
• assert. expect will make you handle the failure case later, assert will
• force you to call kfree on sgt. Both kind of suck in their own ways.

*/

sg_init_one(sgt->sgl, buf, TEST_SIZE);

gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj);

/*

• If the assert fails, we forgot to call sg_free_table(sgt) and kfree(sgt).

*/

KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE); KUNIT_EXPECT_NULL(test, gem_obj->filp); KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);

/*

• And here we have to handle the case where the expectation was wrong,
• but the test still continued.

*/

But if you're not using an action, you still have to call kfree(sgt), which means that you might still

shmem = to_drm_gem_shmem_obj(gem_obj); KUNIT_ASSERT_PTR_EQ(test, shmem->sgt, sgt);

/*

• If the assertion fails, we now have to call drm_gem_shmem_free(shmem)

*/

/* The scatter/gather table is freed by drm_gem_shmem_free */ drm_gem_shmem_free(shmem);

/* everything's fine now */

The semantics around drm_gem_shmem_free make it a bit convoluted, but doing it using goto/labels, plus handling the assertions and error reporting would be difficult.

Using actions, we have:

sgt = kzalloc(sizeof(*sgt), GFP_KERNEL); KUNIT_ASSERT_NOT_NULL(test, sgt);

ret = kunit_add_action_or_reset(test, kfree_wrapper, sgt); KUNIT_ASSERT_EQ(test, ret, 0);

ret = sg_alloc_table(sgt, 1, GFP_KERNEL); KUNIT_ASSERT_EQ(test, ret, 0);

ret = kunit_add_action_or_reset(test, sg_free_table_wrapper, sgt); KUNIT_ASSERT_EQ(test, ret, 0);

sg_init_one(sgt->sgl, buf, TEST_SIZE);

gem_obj = drm_gem_shmem_prime_import_sg_table(&fdev->drm_dev, &attach_mock, sgt); KUNIT_ASSERT_NOT_ERR_OR_NULL(test, gem_obj); KUNIT_EXPECT_EQ(test, gem_obj->size, TEST_SIZE); KUNIT_EXPECT_NULL(test, gem_obj->filp); KUNIT_EXPECT_NOT_NULL(test, gem_obj->funcs);

/* drm_gem_shmem_free will free the struct sg_table itself */ kunit_remove_action(test, sg_free_table_wrapper, sgt); kunit_remove_action(test, kfree_wrapper, sgt);

I agree that using actions makes error handling cleaner. However, I still have some concerns about the additional complexity that actions introduce. For instance, I feel these two lines make the testing harness more complex without asserting any additional property of the component under test.

If anything, the API makes it more difficult to deal with. It would actually be harder/messier to handle without an action.

In some sense, I wonder if it is worth worrying about memory leaks when a test case fails. At that point, the system is already in an inconsistent state due to a bug in the component under test, so it is unsafe to continue anyway.

I guess the larger issue is: once that code will be merged, we're going to have patches to convert to actions because they make it nicer and fix a couple of issues anyway.

So, if it's still the state we're going to end up in, why not doing it right from the beginning?

Maxime