On Tue, Sep 05, 2023 at 03:42:58PM +0200, Thomas Hellström wrote:
> 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@xxxxxxx>
> > > > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > > > > Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > > > > ---
> > > > > 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.
I'm not aware of something to check on lockdep's status when running a
kunit test, but I'm not sure anyone is expected to look at the dmesg
trace when running kunit to find out whether the test succeeded or not.
It looks like there was an attempt at some point to fail the test if
there was a lockdep error:
https://lore.kernel.org/all/20200814205527.1833459-1-urielguajardojr@xxxxxxxxx/
It doesn't look like it's been merged though. David, Brendan, do you
know why it wasn't merged or if there is a good option for us there?
At the very least, I think a comment after the call to drm_exec_fini to
make it clear that the error would be in the kernel logs, and a better
one on the test definition to explicitly say what you want to make sure
of, and how one can check it's been done would be great.
Maxime
Attachment:
signature.asc
Description: PGP signature
