On 10/2/26 02:38, Matthew Brost wrote: > drm_exec_init() always allocates the table of locked objects, a whole > page of it unless the caller asks for a specific size. Many users only > ever lock a handful of objects, often just a VM's common dma-resv and > one BO, yet still pay for an allocation, and any of them can fail with > -ENOMEM. That is awkward on paths which can't fail, such as dropping > what may be the last reference of a drm_gpuvm_bo, which needs both the > VM's and the BO's dma-resv held; drivers end up open coding the ww_mutex > dance instead. > > Keep room for DRM_EXEC_INLINE_OBJECTS (8) objects within struct drm_exec > itself and track the first locked objects there. Only once more are > locked does the table move to an allocation, which then grows as before. > A caller passing nr larger than that still gets an allocated table of > that size up front, falling back to the inline one should that fail. > > As a result, locking up to DRM_EXEC_INLINE_OBJECTS objects never > allocates memory and cannot fail with -ENOMEM, and callers passing > nr == 0 no longer allocate a page they mostly don't need. The cost is > 64 bytes more of struct drm_exec, which typically lives on the stack. > > Since objects may now point into the struct itself, a struct drm_exec > must not be moved or copied after drm_exec_init(); no user does so. > > Add a KUnit test which locks one object more than fits inline, checking > that the table moves out of the struct only then, that every object is > tracked across the move, and that a large nr skips the inline table. > > Cc: Christian König <[email protected]> > Cc: Thomas Hellström <[email protected]> > Cc: Maarten Lankhorst <[email protected]> > Cc: Maxime Ripard <[email protected]> > Cc: Thomas Zimmermann <[email protected]> > Cc: David Airlie <[email protected]> > Cc: Simona Vetter <[email protected]> > Cc: [email protected] > Cc: [email protected] > Signed-off-by: Matthew Brost <[email protected]> > Assisted-by: LLM
I Was considering that as well at some point when I found that using an xarray is actually quite a bit slower than just pre-allocating using kmalloc. No time to review that in deep, but feel free to add Acked-by: Christian König <[email protected]>. Regards, Christian. > > --- > This came out of a Sashiko review of the two-pass GPUVM series [1], > which flagged a nouveau cleanup path dropping a drm_gpuvm_bo with > drm_exec and silently carrying on if locking failed with -ENOMEM. > Grabbing a small number of locks on paths which cannot fail is quite > common, and drm_exec should provide a ww transaction wrapper for that > which can't fail, rather than drivers open coding the ww_mutex dance. > > [1] > https://sashiko.dev/#/patchset/20261001220632.3190896-1-matthew.brost%40intel.com > --- > drivers/gpu/drm/drm_exec.c | 39 +++++++++++++------ > drivers/gpu/drm/tests/drm_exec_test.c | 55 +++++++++++++++++++++++++++ > include/drm/drm_exec.h | 18 ++++++++- > 3 files changed, 99 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_exec.c b/drivers/gpu/drm/drm_exec.c > index 2453ec41360f..bdc9c79359b3 100644 > --- a/drivers/gpu/drm/drm_exec.c > +++ b/drivers/gpu/drm/drm_exec.c > @@ -70,19 +70,26 @@ static void drm_exec_unlock_all(struct drm_exec *exec) > * > * Initialize the object and make sure that we can track locked objects. > * > - * If nr is non-zero then it is used as the initial objects table size. > - * In either case, the table will grow (be re-allocated) on demand. > + * The first %DRM_EXEC_INLINE_OBJECTS locked objects are tracked within @exec > + * itself, so locking no more than that never allocates memory. If nr is > larger > + * than that, it is used as the initial size of an allocated objects table > + * instead. In either case, the table will grow (be re-allocated) on demand. > */ > void drm_exec_init(struct drm_exec *exec, u32 flags, unsigned nr) > { > - if (!nr) > - nr = PAGE_SIZE / sizeof(void *); > + exec->objects = NULL; > + if (nr > DRM_EXEC_INLINE_OBJECTS) > + exec->objects = kvmalloc_objs(*exec->objects, nr); > > - exec->flags = flags; > - exec->objects = kvmalloc_objs(*exec->objects, nr); > + /* If allocation here fails, just delay that till it is needed */ > + if (exec->objects) { > + exec->max_objects = nr; > + } else { > + exec->objects = exec->inline_objects; > + exec->max_objects = DRM_EXEC_INLINE_OBJECTS; > + } > > - /* If allocation here fails, just delay that till the first use */ > - exec->max_objects = exec->objects ? nr : 0; > + exec->flags = flags; > exec->num_objects = 0; > exec->contended = DRM_EXEC_DUMMY; > exec->prelocked = NULL; > @@ -99,7 +106,8 @@ EXPORT_SYMBOL(drm_exec_init); > void drm_exec_fini(struct drm_exec *exec) > { > drm_exec_unlock_all(exec); > - kvfree(exec->objects); > + if (exec->objects != exec->inline_objects) > + kvfree(exec->objects); > if (exec->contended != DRM_EXEC_DUMMY) { > drm_gem_object_put(exec->contended); > ww_acquire_fini(&exec->ticket); > @@ -140,9 +148,16 @@ static int drm_exec_obj_locked(struct drm_exec *exec, > { > if (unlikely(exec->num_objects == exec->max_objects)) { > size_t size = exec->max_objects * sizeof(void *); > - void *tmp; > - > - tmp = kvrealloc(exec->objects, size + PAGE_SIZE, GFP_KERNEL); > + struct drm_gem_object **tmp; > + > + if (exec->objects == exec->inline_objects) { > + tmp = kvmalloc(size + PAGE_SIZE, GFP_KERNEL); > + if (tmp) > + memcpy(tmp, exec->objects, size); > + } else { > + tmp = kvrealloc(exec->objects, size + PAGE_SIZE, > + GFP_KERNEL); > + } > if (!tmp) > return -ENOMEM; > > diff --git a/drivers/gpu/drm/tests/drm_exec_test.c > b/drivers/gpu/drm/tests/drm_exec_test.c > index 7a374e462348..434ab9c59878 100644 > --- a/drivers/gpu/drm/tests/drm_exec_test.c > +++ b/drivers/gpu/drm/tests/drm_exec_test.c > @@ -204,6 +204,60 @@ static void test_multiple_loops(struct kunit *test) > KUNIT_SUCCEED(test); > } > > +static void test_inline_objects(struct kunit *test) > +{ > + struct drm_exec_priv *priv = test->priv; > + const unsigned int count = DRM_EXEC_INLINE_OBJECTS + 1; > + DECLARE_BITMAP(seen, DRM_EXEC_INLINE_OBJECTS + 1) = {}; > + struct drm_gem_object *gobj, *obj; > + struct drm_exec exec; > + unsigned int i; > + int ret = 0; > + > + gobj = kunit_kcalloc(test, count, sizeof(*gobj), GFP_KERNEL); > + KUNIT_ASSERT_NOT_NULL(test, gobj); > + > + for (i = 0; i < count; i++) > + drm_gem_private_object_init(priv->drm, &gobj[i], PAGE_SIZE); > + > + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, 0); > + KUNIT_EXPECT_PTR_EQ(test, exec.objects, &exec.inline_objects[0]); > + drm_exec_until_all_locked(&exec) { > + for (i = 0; i < count; i++) { > + ret = drm_exec_lock_obj(&exec, &gobj[i]); > + drm_exec_retry_on_contention(&exec); > + if (ret) > + break; > + > + /* Only spills out of the struct past the inline ones */ > + if (exec.num_objects <= DRM_EXEC_INLINE_OBJECTS) > + KUNIT_EXPECT_PTR_EQ(test, exec.objects, > + &exec.inline_objects[0]); > + else > + KUNIT_EXPECT_PTR_NE(test, exec.objects, > + &exec.inline_objects[0]); > + } > + } > + KUNIT_EXPECT_EQ(test, ret, 0); > + KUNIT_EXPECT_EQ(test, exec.num_objects, count); > + /* Contention may reorder them, but each must be tracked once */ > + drm_exec_for_each_locked_object(&exec, obj) { > + KUNIT_ASSERT_TRUE(test, obj >= gobj && obj < gobj + count); > + KUNIT_EXPECT_FALSE(test, test_and_set_bit(obj - gobj, seen)); > + } > + KUNIT_EXPECT_TRUE(test, bitmap_full(seen, count)); > + drm_exec_fini(&exec); > + > + /* A larger initial size skips the inline objects altogether */ > + drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT, count); > + KUNIT_EXPECT_PTR_NE(test, exec.objects, &exec.inline_objects[0]); > + KUNIT_EXPECT_EQ(test, exec.max_objects, count); > + drm_exec_fini(&exec); > + > + for (i = 0; i < count; i++) > + drm_gem_private_object_fini(&gobj[i]); > +} > + > static struct kunit_case drm_exec_tests[] = { > KUNIT_CASE(sanitycheck), > KUNIT_CASE(test_lock), > @@ -212,6 +266,7 @@ static struct kunit_case drm_exec_tests[] = { > KUNIT_CASE(test_prepare), > KUNIT_CASE(test_prepare_array), > KUNIT_CASE(test_multiple_loops), > + KUNIT_CASE(test_inline_objects), > {} > }; > > diff --git a/include/drm/drm_exec.h b/include/drm/drm_exec.h > index cc2937185a9f..ddbdd745efd9 100644 > --- a/include/drm/drm_exec.h > +++ b/include/drm/drm_exec.h > @@ -15,6 +15,12 @@ > */ > #define DRM_EXEC_DUMMY ((void *)~0) > > +/* > + * Number of locked objects tracked inside &struct drm_exec itself. Locking > at > + * most this many objects never allocates memory for tracking them. > + */ > +#define DRM_EXEC_INLINE_OBJECTS 8 > + > struct drm_gem_object; > > /** > @@ -42,7 +48,8 @@ struct drm_exec { > unsigned int max_objects; > > /** > - * @objects: array of the locked objects > + * @objects: array of the locked objects, either @inline_objects or > + * allocated once more objects need tracking than it can hold > */ > struct drm_gem_object **objects; > > @@ -55,6 +62,15 @@ struct drm_exec { > * @prelocked: already locked GEM object due to contention > */ > struct drm_gem_object *prelocked; > + > + /** > + * @inline_objects: storage for the first %DRM_EXEC_INLINE_OBJECTS > + * locked objects, so that locking only a few objects does not need to > + * allocate memory, and therefore cannot fail with -ENOMEM. As @objects > + * may point here, a &struct drm_exec must not be moved or copied after > + * drm_exec_init(). > + */ > + struct drm_gem_object *inline_objects[DRM_EXEC_INLINE_OBJECTS]; > }; > > /**
