Hi,

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);

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

ret = kunit_add_action_or_reset(test, drm_gem_shmem_free_wrapper, shmem); KUNIT_ASSERT_EQ(test, ret, 0);

The last one is arguable, but for the previous ones it makes error handling much more convenient and easy to reason about.

The wrappers are also a bit inconvenient to use, but it's mostly there to avoid a compiler warning at the moment.

This patch will help hopefully: https://lore.kernel.org/linux-kselftest/20230915050125.3609689-1-davidgow@go...

Maxime