On 2023-10-25 10:43, Maxime Ripard wrote:
> 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);
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.
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.
>
> 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@xxxxxxxxxx/
>
> Maxime
Thanks,
Marco
