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

[Maxime Ripard]

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

Attachment: signature.asc
Description: PGP signature