Re: [Intel-xe] [PATCH 2/3] drm/tests/drm_exec: Add a test for object freeing within drm_exec_fini()

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Maxime

On 9/5/23 15:16, Maxime Ripard wrote:
> On Tue, Sep 05, 2023 at 02:32:38PM +0200, Thomas Hellström wrote:
>> Hi,
>>
>> On 9/5/23 14:05, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, Sep 05, 2023 at 10:58:31AM +0200, Thomas Hellström wrote:
>>>> Check that object freeing from within drm_exec_fini() works as expected
>>>> and doesn't generate any warnings.
>>>>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/tests/drm_exec_test.c | 47 +++++++++++++++++++++++++++
>>>>    1 file changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
>>>> index 563949d777dd..294c25f49cc7 100644
>>>> --- a/drivers/gpu/drm/tests/drm_exec_test.c
>>>> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
>>>> @@ -170,6 +170,52 @@ static void test_prepare_array(struct kunit *test)
>>>>    	drm_gem_private_object_fini(&gobj2);
>>>>    }
>>>> +static const struct drm_gem_object_funcs put_funcs = {
>>>> +	.free = (void *)kfree,
>>>> +};
>>>> +
>>>> +/*
>>>> + * Check that freeing objects from within drm_exec_fini()
>>>> + * behaves as expected.
>>>> + */
>>>> +static void test_early_put(struct kunit *test)
>>>> +{
>>>> +	struct drm_exec_priv *priv = test->priv;
>>>> +	struct drm_gem_object *gobj1;
>>>> +	struct drm_gem_object *gobj2;
>>>> +	struct drm_gem_object *array[2];
>>>> +	struct drm_exec exec;
>>>> +	int ret;
>>>> +
>>>> +	gobj1 = kzalloc(sizeof(*gobj1), GFP_KERNEL);
>>>> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj1);
>>>> +	if (!gobj1)
>>>> +		return;
>>>> +
>>>> +	gobj2 = kzalloc(sizeof(*gobj2), GFP_KERNEL);
>>>> +	KUNIT_EXPECT_NOT_ERR_OR_NULL(test, gobj2);
>>>> +	if (!gobj2) {
>>>> +		kfree(gobj1);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	gobj1->funcs = &put_funcs;
>>>> +	gobj2->funcs = &put_funcs;
>>>> +	array[0] = gobj1;
>>>> +	array[1] = gobj2;
>>>> +	drm_gem_private_object_init(priv->drm, gobj1, PAGE_SIZE);
>>>> +	drm_gem_private_object_init(priv->drm, gobj2, PAGE_SIZE);
>>>> +
>>>> +	drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT);
>>>> +	drm_exec_until_all_locked(&exec)
>>>> +		ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array),
>>>> +					     1);
>>>> +	KUNIT_EXPECT_EQ(test, ret, 0);
>>>> +	drm_gem_object_put(gobj1);
>>>> +	drm_gem_object_put(gobj2);
>>>> +	drm_exec_fini(&exec);
>>> It doesn't look like you actually check that "freeing objects from
>>> within drm_exec_fini() behaves as expected." What is the expectation
>>> here, and how is it checked?
>> Hm. Good question, I've been manually checking dmesg for lockdep splats. Is
>> there a way to automate that?
> I'm not familiar with the drm_exec API, but judging by the code I'd
> assume you want to check that gobj1 and gobj2 are actually freed using
> kfree?

Actually not. What's important here is that the call to drm_exec_fini(), 
which puts the last references to gobj1 and gobj2 doesn't trigger any 
lockdep splats, like the one in the commit message of patch 3/3. So to 
make more sense, the test could perhaps be conditioned on 
CONFIG_DEBUG_LOCK_ALLOC. Still it would require manual checking of 
dmesg() after being run.

/Thomas