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