Re: [RFC PATCH] drm/test: add a test suite for GEM objects backed by shmem

[Marco Pagani <marpagan@redhat.com>]

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@google.com/
> 
> Maxime

Thanks,
Marco