[PATCH 3/3] drm/ttm, drm_xe, Implement ttm_lru_walk_for_evict() using the guarded LRU iteration
Thomas Hellström
thomas.hellstrom at linux.intel.com
Mon Jun 16 15:29:51 UTC 2025
On Mon, 2025-06-16 at 15:23 +0200, Christian König wrote:
> On 6/13/25 17:18, Thomas Hellström wrote:
> > To avoid duplicating the tricky bo locking implementation,
> > Implement ttm_lru_walk_for_evict() using the guarded bo LRU
> > iteration.
> >
> > To facilitate this, support ticketlocking from the guarded bo LRU
> > iteration.
>
> That looks mostly identical to a patch I have in my drm_exec branch.
>
> A few questions below.
>
> >
> > Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> > ---
> > drivers/gpu/drm/ttm/ttm_bo_util.c | 166 ++++++++++++--------------
> > ----
> > drivers/gpu/drm/xe/xe_shrinker.c | 7 +-
> > include/drm/ttm/ttm_bo.h | 9 +-
> > 3 files changed, 76 insertions(+), 106 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > index 62b76abac578..9bc17ea1adb2 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> > @@ -821,12 +821,6 @@ static int ttm_lru_walk_ticketlock(struct
> > ttm_lru_walk_arg *arg,
> > return ret;
> > }
> >
> > -static void ttm_lru_walk_unlock(struct ttm_buffer_object *bo, bool
> > locked)
> > -{
> > - if (locked)
> > - dma_resv_unlock(bo->base.resv);
> > -}
> > -
> > /**
> > * ttm_lru_walk_for_evict() - Perform a LRU list walk, with
> > actions taken on
> > * valid items.
> > @@ -861,64 +855,21 @@ static void ttm_lru_walk_unlock(struct
> > ttm_buffer_object *bo, bool locked)
> > s64 ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct
> > ttm_device *bdev,
> > struct ttm_resource_manager *man, s64
> > target)
> > {
> > - struct ttm_resource_cursor cursor;
> > - struct ttm_resource *res;
> > + struct ttm_bo_lru_cursor cursor;
> > + struct ttm_buffer_object *bo;
> > s64 progress = 0;
> > s64 lret;
> >
> > - spin_lock(&bdev->lru_lock);
> > - ttm_resource_cursor_init(&cursor, man);
> > - ttm_resource_manager_for_each_res(&cursor, res) {
> > - struct ttm_buffer_object *bo = res->bo;
> > - bool bo_needs_unlock = false;
> > - bool bo_locked = false;
> > - int mem_type;
> > -
> > - /*
> > - * Attempt a trylock before taking a reference on
> > the bo,
> > - * since if we do it the other way around, and the
> > trylock fails,
> > - * we need to drop the lru lock to put the bo.
> > - */
> > - if (ttm_lru_walk_trylock(&walk->arg, bo,
> > &bo_needs_unlock))
> > - bo_locked = true;
> > - else if (!walk->arg.ticket || walk->arg.ctx-
> > >no_wait_gpu ||
> > - walk->arg.trylock_only)
> > - continue;
> > -
> > - if (!ttm_bo_get_unless_zero(bo)) {
> > - ttm_lru_walk_unlock(bo, bo_needs_unlock);
> > - continue;
> > - }
> > -
> > - mem_type = res->mem_type;
> > - spin_unlock(&bdev->lru_lock);
> > -
> > - lret = 0;
> > - if (!bo_locked)
> > - lret = ttm_lru_walk_ticketlock(&walk->arg,
> > bo, &bo_needs_unlock);
> > -
> > - /*
> > - * Note that in between the release of the lru
> > lock and the
> > - * ticketlock, the bo may have switched resource,
> > - * and also memory type, since the resource may
> > have been
> > - * freed and allocated again with a different
> > memory type.
> > - * In that case, just skip it.
> > - */
> > - if (!lret && bo->resource && bo->resource-
> > >mem_type == mem_type)
> > - lret = walk->ops->process_bo(walk, bo);
> > -
> > - ttm_lru_walk_unlock(bo, bo_needs_unlock);
> > - ttm_bo_put(bo);
> > + ttm_bo_lru_for_each_reserved_guarded(&cursor, man, &walk-
> > >arg, bo) {
> > + lret = walk->ops->process_bo(walk, bo);
> > if (lret == -EBUSY || lret == -EALREADY)
> > lret = 0;
> > progress = (lret < 0) ? lret : progress + lret;
> > -
> > - spin_lock(&bdev->lru_lock);
> > if (progress < 0 || progress >= target)
> > break;
> > }
> > - ttm_resource_cursor_fini(&cursor);
> > - spin_unlock(&bdev->lru_lock);
> > + if (IS_ERR(bo))
> > + return PTR_ERR(bo);
> >
> > return progress;
> > }
> > @@ -958,10 +909,7 @@ EXPORT_SYMBOL(ttm_bo_lru_cursor_fini);
> > * @man: The ttm resource_manager whose LRU lists to iterate over.
> > * @arg: The ttm_lru_walk_arg to govern the walk.
> > *
> > - * Initialize a struct ttm_bo_lru_cursor. Currently only
> > trylocking
> > - * or prelocked buffer objects are available as detailed by
> > - * @arg->ctx.resv and @arg->ctx.allow_res_evict. Ticketlocking is
> > not
> > - * supported.
> > + * Initialize a struct ttm_bo_lru_cursor.
> > *
> > * Return: Pointer to @curs. The function does not fail.
> > */
> > @@ -979,21 +927,65 @@ ttm_bo_lru_cursor_init(struct
> > ttm_bo_lru_cursor *curs,
> > EXPORT_SYMBOL(ttm_bo_lru_cursor_init);
> >
> > static struct ttm_buffer_object *
> > -ttm_bo_from_res_reserved(struct ttm_resource *res, struct
> > ttm_bo_lru_cursor *curs)
> > +__ttm_bo_lru_cursor_next(struct ttm_bo_lru_cursor *curs, bool
> > first)
>
> Giving first as bool parameter here looks really ugly. Isn't there
> any other way to do this?
I agree. The previous way of doing this with a bo_from_res() isn't a
good fit since if ticketlocking, we'd like to return without the
spinlock held, and be called with the spinlock held. That's really
confusing both to readers and static analyzers so rather avoided.
We could look at curs->bo and see if it's NULL, but then I think the
bool argument is more readable. But if you have a better idea, I'm of
course open to changing this.
>
> > {
> > - struct ttm_buffer_object *bo = res->bo;
> > + spinlock_t *lru_lock = &curs->res_curs.man->bdev-
> > >lru_lock;
> > + struct ttm_resource *res = NULL;
> > + struct ttm_buffer_object *bo;
> > + struct ttm_lru_walk_arg *arg = curs->arg;
> >
> > - if (!ttm_lru_walk_trylock(curs->arg, bo, &curs-
> > >needs_unlock))
> > - return NULL;
> > + ttm_bo_lru_cursor_cleanup_bo(curs);
> >
> > - if (!ttm_bo_get_unless_zero(bo)) {
> > - if (curs->needs_unlock)
> > - dma_resv_unlock(bo->base.resv);
> > - return NULL;
> > + spin_lock(lru_lock);
> > + for (;;) {
> > + int mem_type, ret;
> > + bool bo_locked = false;
> > +
> > + if (first) {
> > + res = ttm_resource_manager_first(&curs-
> > >res_curs);
> > + first = false;
> > + } else {
> > + res = ttm_resource_manager_next(&curs-
> > >res_curs);
> > + }
> > + if (!res)
> > + break;
> > +
> > + bo = res->bo;
> > + if (ttm_lru_walk_trylock(arg, bo, &curs-
> > >needs_unlock))
>
> Could/should we move needs_unlock into arg as well?
They are different in that arg is state that governs the whole loop but
needs_unlock is local to a single iteration. We could of course pass
the cursor to the various locking functions to eliminate one argument
now that walk_for_evict() also uses the cursor.
Thoughts?
/Thomas
>
> Apart from that looks good to me.
>
> Regards,
> Christian.
>
> > + bo_locked = true;
> > + else if (!arg->ticket || arg->ctx->no_wait_gpu ||
> > arg->trylock_only)
> > + continue;
> > +
> > + if (!ttm_bo_get_unless_zero(bo)) {
> > + if (curs->needs_unlock)
> > + dma_resv_unlock(bo->base.resv);
> > + continue;
> > + }
> > +
> > + mem_type = res->mem_type;
> > + spin_unlock(lru_lock);
> > + if (!bo_locked)
> > + ret = ttm_lru_walk_ticketlock(arg, bo,
> > &curs->needs_unlock);
> > + /*
> > + * Note that in between the release of the lru
> > lock and the
> > + * ticketlock, the bo may have switched resource,
> > + * and also memory type, since the resource may
> > have been
> > + * freed and allocated again with a different
> > memory type.
> > + * In that case, just skip it.
> > + */
> > + curs->bo = bo;
> > + if (!ret && bo->resource && bo->resource->mem_type
> > == mem_type)
> > + return bo;
> > +
> > + ttm_bo_lru_cursor_cleanup_bo(curs);
> > + if (ret)
> > + return ERR_PTR(ret);
> > +
> > + spin_lock(lru_lock);
> > }
> >
> > - curs->bo = bo;
> > - return bo;
> > + spin_unlock(lru_lock);
> > + return res ? bo : NULL;
> > }
> >
> > /**
> > @@ -1007,25 +999,7 @@ ttm_bo_from_res_reserved(struct ttm_resource
> > *res, struct ttm_bo_lru_cursor *cur
> > */
> > struct ttm_buffer_object *ttm_bo_lru_cursor_next(struct
> > ttm_bo_lru_cursor *curs)
> > {
> > - spinlock_t *lru_lock = &curs->res_curs.man->bdev-
> > >lru_lock;
> > - struct ttm_resource *res = NULL;
> > - struct ttm_buffer_object *bo;
> > -
> > - ttm_bo_lru_cursor_cleanup_bo(curs);
> > -
> > - spin_lock(lru_lock);
> > - for (;;) {
> > - res = ttm_resource_manager_next(&curs->res_curs);
> > - if (!res)
> > - break;
> > -
> > - bo = ttm_bo_from_res_reserved(res, curs);
> > - if (bo)
> > - break;
> > - }
> > -
> > - spin_unlock(lru_lock);
> > - return res ? bo : NULL;
> > + return __ttm_bo_lru_cursor_next(curs, false);
> > }
> > EXPORT_SYMBOL(ttm_bo_lru_cursor_next);
> >
> > @@ -1039,21 +1013,7 @@ EXPORT_SYMBOL(ttm_bo_lru_cursor_next);
> > */
> > struct ttm_buffer_object *ttm_bo_lru_cursor_first(struct
> > ttm_bo_lru_cursor *curs)
> > {
> > - spinlock_t *lru_lock = &curs->res_curs.man->bdev-
> > >lru_lock;
> > - struct ttm_buffer_object *bo;
> > - struct ttm_resource *res;
> > -
> > - spin_lock(lru_lock);
> > - res = ttm_resource_manager_first(&curs->res_curs);
> > - if (!res) {
> > - spin_unlock(lru_lock);
> > - return NULL;
> > - }
> > -
> > - bo = ttm_bo_from_res_reserved(res, curs);
> > - spin_unlock(lru_lock);
> > -
> > - return bo ? bo : ttm_bo_lru_cursor_next(curs);
> > + return __ttm_bo_lru_cursor_next(curs, true);
> > }
> > EXPORT_SYMBOL(ttm_bo_lru_cursor_first);
> >
> > diff --git a/drivers/gpu/drm/xe/xe_shrinker.c
> > b/drivers/gpu/drm/xe/xe_shrinker.c
> > index f8a1129da2c3..1c3c04d52f55 100644
> > --- a/drivers/gpu/drm/xe/xe_shrinker.c
> > +++ b/drivers/gpu/drm/xe/xe_shrinker.c
> > @@ -66,7 +66,10 @@ static s64 xe_shrinker_walk(struct xe_device
> > *xe,
> > struct ttm_resource_manager *man =
> > ttm_manager_type(&xe->ttm, mem_type);
> > struct ttm_bo_lru_cursor curs;
> > struct ttm_buffer_object *ttm_bo;
> > - struct ttm_lru_walk_arg arg = {.ctx = ctx};
> > + struct ttm_lru_walk_arg arg = {
> > + .ctx = ctx,
> > + .trylock_only = true,
> > + };
> >
> > if (!man || !man->use_tt)
> > continue;
> > @@ -83,6 +86,8 @@ static s64 xe_shrinker_walk(struct xe_device *xe,
> > if (*scanned >= to_scan)
> > break;
> > }
> > + /* Trylocks should never error, just fail. */
> > + xe_assert(xe, !IS_ERR(ttm_bo));
> > }
> >
> > return freed;
> > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> > index 8f04fa48b332..d3a85d76aaff 100644
> > --- a/include/drm/ttm/ttm_bo.h
> > +++ b/include/drm/ttm/ttm_bo.h
> > @@ -529,10 +529,15 @@
> > class_ttm_bo_lru_cursor_lock_ptr(class_ttm_bo_lru_cursor_t *_T)
> > * up at looping termination, even if terminated prematurely by,
> > for
> > * example a return or break statement. Exiting the loop will also
> > unlock
> > * (if needed) and unreference @_bo.
> > + *
> > + * Return: If locking of a bo returns an error, then iteration is
> > terminated
> > + * and @_bo is set to a corresponding error pointer. It's illegal
> > to
> > + * dereference @_bo after loop exit.
> > */
> > #define ttm_bo_lru_for_each_reserved_guarded(_cursor, _man, _arg,
> > _bo) \
> > scoped_guard(ttm_bo_lru_cursor, _cursor, _man,
> > _arg) \
> > - for ((_bo) = ttm_bo_lru_cursor_first(_cursor);
> > (_bo); \
> > - (_bo) = ttm_bo_lru_cursor_next(_cursor))
> > + for ((_bo) =
> > ttm_bo_lru_cursor_first(_cursor); \
> > +
> > !IS_ERR_OR_NULL(_bo); \
> > + (_bo) = ttm_bo_lru_cursor_next(_cursor))
> >
> > #endif
>
More information about the Intel-xe
mailing list