On Wed, 2024-06-19 at 23:33 +0000, Matthew Brost wrote:
> On Tue, Jun 18, 2024 at 09:18:15AM +0200, Thomas Hellström wrote:
> > Use the LRU walker for eviction. This helps
> > removing a lot of code with weird locking
> > semantics.
> >
> > The functionality is slightly changed so that
> > when trylocked buffer objects are exhausted, we
> > continue to interleave walks with ticket-locks while
> > there is still progress made. The list walks are
> > not restarted in-between evictions.
> >
> > Also provide a separate ttm_bo_evict_first()
> > function for its single user. The context of that
> > user allows sleeping dma_resv locks.
> >
> > Cc: Christian König <christian.koenig@xxxxxxx>
> > Cc: Somalapuram Amaranath <Amaranath.Somalapuram@xxxxxxx>
> > Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
> > Cc: <dri-devel@xxxxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/ttm/ttm_bo.c | 350 ++++++++++++-------------
> > ----
> > drivers/gpu/drm/ttm/ttm_resource.c | 20 +-
> > include/drm/ttm/ttm_bo.h | 8 +-
> > 3 files changed, 145 insertions(+), 233 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> > b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 63a91b77f7da..316afe19a325 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -224,80 +224,6 @@ static void ttm_bo_flush_all_fences(struct
> > ttm_buffer_object *bo)
> > dma_resv_iter_end(&cursor);
> > }
> >
> > -/**
> > - * ttm_bo_cleanup_refs
> > - * If bo idle, remove from lru lists, and unref.
> > - * If not idle, block if possible.
> > - *
> > - * Must be called with lru_lock and reservation held, this
> > function
> > - * will drop the lru lock and optionally the reservation lock
> > before returning.
> > - *
> > - * @bo: The buffer object to clean-up
> > - * @interruptible: Any sleeps should occur interruptibly.
> > - * @no_wait_gpu: Never wait for gpu. Return -EBUSY
> > instead.
> > - * @unlock_resv: Unlock the reservation lock as well.
> > - */
> > -
> > -static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
> > - bool interruptible, bool
> > no_wait_gpu,
> > - bool unlock_resv)
> > -{
> > - struct dma_resv *resv = &bo->base._resv;
> > - int ret;
> > -
> > - if (dma_resv_test_signaled(resv, DMA_RESV_USAGE_BOOKKEEP))
> > - ret = 0;
> > - else
> > - ret = -EBUSY;
> > -
> > - if (ret && !no_wait_gpu) {
> > - long lret;
> > -
> > - if (unlock_resv)
> > - dma_resv_unlock(bo->base.resv);
> > - spin_unlock(&bo->bdev->lru_lock);
> > -
> > - lret = dma_resv_wait_timeout(resv,
> > DMA_RESV_USAGE_BOOKKEEP,
> > - interruptible,
> > - 30 * HZ);
> > -
> > - if (lret < 0)
> > - return lret;
> > - else if (lret == 0)
> > - return -EBUSY;
> > -
> > - spin_lock(&bo->bdev->lru_lock);
> > - if (unlock_resv && !dma_resv_trylock(bo-
> > >base.resv)) {
> > - /*
> > - * We raced, and lost, someone else holds
> > the reservation now,
> > - * and is probably busy in
> > ttm_bo_cleanup_memtype_use.
> > - *
> > - * Even if it's not the case, because we
> > finished waiting any
> > - * delayed destruction would succeed, so
> > just return success
> > - * here.
> > - */
> > - spin_unlock(&bo->bdev->lru_lock);
> > - return 0;
> > - }
> > - ret = 0;
> > - }
> > -
> > - if (ret) {
> > - if (unlock_resv)
> > - dma_resv_unlock(bo->base.resv);
> > - spin_unlock(&bo->bdev->lru_lock);
> > - return ret;
> > - }
> > -
> > - spin_unlock(&bo->bdev->lru_lock);
> > - ttm_bo_cleanup_memtype_use(bo);
> > -
> > - if (unlock_resv)
> > - dma_resv_unlock(bo->base.resv);
> > -
> > - return 0;
> > -}
> > -
> > /*
> > * Block for the dma_resv object to become idle, lock the buffer
> > and clean up
> > * the resource and tt object.
> > @@ -505,151 +431,154 @@ bool ttm_bo_eviction_valuable(struct
> > ttm_buffer_object *bo,
> > }
> > EXPORT_SYMBOL(ttm_bo_eviction_valuable);
> >
> > -/*
> > - * Check the target bo is allowable to be evicted or swapout,
> > including cases:
> > - *
> > - * a. if share same reservation object with ctx->resv, have
> > assumption
> > - * reservation objects should already be locked, so not lock again
> > and
> > - * return true directly when either the opreation
> > allow_reserved_eviction
> > - * or the target bo already is in delayed free list;
> > +/**
> > + * ttm_bo_evict_first() - Evict the first bo on the manager's LRU
> > list.
> > + * @bdev: The ttm device.
> > + * @man: The manager whose bo to evict.
> > + * @ctx: The TTM operation ctx governing the eviction.
> > *
> > - * b. Otherwise, trylock it.
> > + * Return: 0 if successful or the resource disappeared. Negative
> > error code on error.
> > */
> > -static bool ttm_bo_evict_swapout_allowable(struct
> > ttm_buffer_object *bo,
> > - struct
> > ttm_operation_ctx *ctx,
> > - const struct ttm_place
> > *place,
> > - bool *locked, bool
> > *busy)
> > +int ttm_bo_evict_first(struct ttm_device *bdev, struct
> > ttm_resource_manager *man,
> > + struct ttm_operation_ctx *ctx)
> > {
> > - bool ret = false;
> > + struct ttm_resource_cursor cursor;
> > + struct ttm_buffer_object *bo;
> > + struct ttm_resource *res;
> > + unsigned int mem_type;
> > + int ret = 0;
> >
> > - if (bo->pin_count) {
> > - *locked = false;
> > - if (busy)
> > - *busy = false;
> > - return false;
> > + spin_lock(&bdev->lru_lock);
> > + res = ttm_resource_manager_first(man, &cursor);
> > + if (!res) {
> > + ret = -ENOENT;
> > + goto out_no_ref;
> > }
> > + bo = res->bo;
> > + if (!ttm_bo_get_unless_zero(bo))
> > + goto out_no_ref;
> > + mem_type = res->mem_type;
> > + spin_unlock(&bdev->lru_lock);
> > + ret = ttm_bo_reserve(bo, ctx->interruptible, ctx-
> > >no_wait_gpu, NULL);
> > + if (ret)
> > + goto out_no_lock;
> > + if (bo->resource != res || res->mem_type != mem_type)
> > + goto out_bad_res;
> >
> > - if (bo->base.resv == ctx->resv) {
> > - dma_resv_assert_held(bo->base.resv);
> > - if (ctx->allow_res_evict)
> > - ret = true;
> > - *locked = false;
> > - if (busy)
> > - *busy = false;
> > + if (bo->deleted) {
> > + ret = ttm_bo_wait_ctx(bo, ctx);
> > + if (ret)
> > + ttm_bo_cleanup_memtype_use(bo);
> > } else {
> > - ret = dma_resv_trylock(bo->base.resv);
> > - *locked = ret;
> > - if (busy)
> > - *busy = !ret;
> > - }
> > -
> > - if (ret && place && (bo->resource->mem_type != place-
> > >mem_type ||
> > - !bo->bdev->funcs->eviction_valuable(bo, place))) {
> > - ret = false;
> > - if (*locked) {
> > - dma_resv_unlock(bo->base.resv);
> > - *locked = false;
> > - }
> > + ret = ttm_bo_evict(bo, ctx);
> > }
> > -
> > +out_bad_res:
> > + dma_resv_unlock(bo->base.resv);
> > +out_no_lock:
> > + ttm_bo_put(bo);
> > + ttm_resource_cursor_fini(&cursor);
> > return ret;
> > +
> > +out_no_ref:
> > + ttm_resource_cursor_fini_locked(&cursor);
> > + spin_unlock(&bdev->lru_lock);
> > + return -ENOENT;
> > }
> >
> > /**
> > - * ttm_mem_evict_wait_busy - wait for a busy BO to become
> > available
> > - *
> > - * @busy_bo: BO which couldn't be locked with trylock
> > - * @ctx: operation context
> > - * @ticket: acquire ticket
> > - *
> > - * Try to lock a busy buffer object to avoid failing eviction.
> > + * struct ttm_bo_evict_walk - Parameters for the evict walk.
> > */
> > -static int ttm_mem_evict_wait_busy(struct ttm_buffer_object
> > *busy_bo,
> > - struct ttm_operation_ctx *ctx,
> > - struct ww_acquire_ctx *ticket)
> > -{
> > - int r;
> > -
> > - if (!busy_bo || !ticket)
> > - return -EBUSY;
> > -
> > - if (ctx->interruptible)
> > - r = dma_resv_lock_interruptible(busy_bo-
> > >base.resv,
> > - ticket);
> > - else
> > - r = dma_resv_lock(busy_bo->base.resv, ticket);
> > -
> > - /*
> > - * TODO: It would be better to keep the BO locked until
> > allocation is at
> > - * least tried one more time, but that would mean a much
> > larger rework
> > - * of TTM.
> > - */
> > - if (!r)
> > - dma_resv_unlock(busy_bo->base.resv);
> > -
> > - return r == -EDEADLK ? -EBUSY : r;
> > -}
> > +struct ttm_bo_evict_walk {
> > + /** @walk: The walk base parameters. */
> > + struct ttm_lru_walk walk;
> > + /** @place: The place passed to the resource allocation.
> > */
> > + const struct ttm_place *place;
> > + /** @evictor: The buffer object we're trying to make room
> > for. */
> > + struct ttm_buffer_object *evictor;
> > + /** @res: The allocated resource if any. */
> > + struct ttm_resource **res;
> > + /** @evicted: The number of evicted pages. */
> > + unsigned long evicted;
> > +};
> >
> > -int ttm_mem_evict_first(struct ttm_device *bdev,
> > - struct ttm_resource_manager *man,
> > - const struct ttm_place *place,
> > - struct ttm_operation_ctx *ctx,
> > - struct ww_acquire_ctx *ticket)
> > +static long ttm_bo_evict_cb(struct ttm_lru_walk *walk, struct
> > ttm_buffer_object *bo)
> > {
> > - struct ttm_buffer_object *bo = NULL, *busy_bo = NULL;
> > - struct ttm_resource_cursor cursor;
> > - struct ttm_resource *res;
> > - bool locked = false;
> > - int ret;
> > + struct ttm_bo_evict_walk *evict_walk =
> > + container_of(walk, typeof(*evict_walk), walk);
> > + long lret;
> >
> > - spin_lock(&bdev->lru_lock);
> > - ttm_resource_manager_for_each_res(man, &cursor, res) {
> > - bool busy;
> > -
> > - if (!ttm_bo_evict_swapout_allowable(res->bo, ctx,
> > place,
> > - &locked,
> > &busy)) {
> > - if (busy && !busy_bo && ticket !=
> > - dma_resv_locking_ctx(res->bo-
> > >base.resv))
> > - busy_bo = res->bo;
> > - continue;
> > - }
> > + if (!bo->bdev->funcs->eviction_valuable(bo, evict_walk-
> > >place))
> > + return 0;
> >
> > - if (ttm_bo_get_unless_zero(res->bo)) {
> > - bo = res->bo;
> > - break;
> > - }
> > - if (locked)
> > - dma_resv_unlock(res->bo->base.resv);
> > + if (bo->deleted) {
> > + lret = ttm_bo_wait_ctx(bo, walk->ctx);
> > + if (!lret)
> > + ttm_bo_cleanup_memtype_use(bo);
> > + } else {
> > + lret = ttm_bo_evict(bo, walk->ctx);
> > }
> > - ttm_resource_cursor_fini_locked(&cursor);
> >
> > - if (!bo) {
> > - if (busy_bo && !ttm_bo_get_unless_zero(busy_bo))
> > - busy_bo = NULL;
> > - spin_unlock(&bdev->lru_lock);
> > - ret = ttm_mem_evict_wait_busy(busy_bo, ctx,
> > ticket);
> > - if (busy_bo)
> > - ttm_bo_put(busy_bo);
> > - return ret;
> > - }
> > + if (lret)
> > + goto out;
> >
> > - if (bo->deleted) {
> > - ret = ttm_bo_cleanup_refs(bo, ctx->interruptible,
> > - ctx->no_wait_gpu,
> > locked);
> > - ttm_bo_put(bo);
> > - return ret;
> > - }
> > + evict_walk->evicted++;
> > + if (evict_walk->res)
> > + lret = ttm_resource_alloc(evict_walk->evictor,
> > evict_walk->place,
> > + evict_walk->res);
> > + if (lret == 0)
> > + return 1;
> > +out:
> > + /* Errors that should terminate the walk. */
> > + if (lret == -ENOMEM || lret == -EINTR || lret == -
> > ERESTARTSYS ||
> > + lret == -EAGAIN)
> > + return lret;
> >
> > - spin_unlock(&bdev->lru_lock);
> > + return 0;
> > +}
> >
> > - ret = ttm_bo_evict(bo, ctx);
> > - if (locked)
> > - ttm_bo_unreserve(bo);
> > - else
> > - ttm_bo_move_to_lru_tail_unlocked(bo);
> > +static const struct ttm_lru_walk_ops ttm_evict_walk_ops = {
> > + .process_bo = ttm_bo_evict_cb,
> > +};
> >
> > - ttm_bo_put(bo);
> > - return ret;
> > +static int ttm_bo_evict_alloc(struct ttm_device *bdev,
> > + struct ttm_resource_manager *man,
> > + const struct ttm_place *place,
> > + struct ttm_buffer_object *evictor,
> > + struct ttm_operation_ctx *ctx,
> > + struct ww_acquire_ctx *ticket,
> > + struct ttm_resource **res)
> > +{
> > + struct ttm_bo_evict_walk evict_walk = {
> > + .walk = {
> > + .ops = &ttm_evict_walk_ops,
> > + .ctx = ctx,
> > + .ticket = ticket,
> > + },
> > + .place = place,
> > + .evictor = evictor,
> > + .res = res,
> > + };
> > + long lret;
> > +
> > + evict_walk.walk.trylock_only = true;
> > + lret = ttm_lru_walk_for_evict(&evict_walk.walk, bdev, man,
> > 1);
> > + if (lret || !ticket)
> > + goto out;
> > +
> > + /* If ticket-locking, repeat while making progress. */
> > + evict_walk.walk.trylock_only = false;
> > + do {
> > + /* The walk may clear the evict_walk.walk.ticket
> > field */
> > + evict_walk.walk.ticket = ticket;
> > + evict_walk.evicted = 0;
> > + lret = ttm_lru_walk_for_evict(&evict_walk.walk,
> > bdev, man, 1);
> > + } while (!lret && evict_walk.evicted);
> > +out:
> > + if (lret < 0)
> > + return lret;
> > + if (lret == 0)
> > + return -EBUSY;
> > + return 0;
> > }
> >
> > /**
> > @@ -760,6 +689,7 @@ static int ttm_bo_alloc_resource(struct
> > ttm_buffer_object *bo,
> > for (i = 0; i < placement->num_placement; ++i) {
> > const struct ttm_place *place = &placement-
> > >placement[i];
> > struct ttm_resource_manager *man;
> > + bool may_evict;
> >
> > man = ttm_manager_type(bdev, place->mem_type);
> > if (!man || !ttm_resource_manager_used(man))
> > @@ -769,22 +699,21 @@ static int ttm_bo_alloc_resource(struct
> > ttm_buffer_object *bo,
> > TTM_PL_FLAG_FALLBACK))
> > continue;
> >
> > - do {
> > - ret = ttm_resource_alloc(bo, place, res);
> > - if (unlikely(ret && ret != -ENOSPC))
> > + may_evict = (force_space && place->mem_type !=
> > TTM_PL_SYSTEM);
> > + ret = ttm_resource_alloc(bo, place, res);
> > + if (ret) {
> > + if (ret != -ENOSPC)
> > return ret;
> > - if (likely(!ret) || !force_space)
> > - break;
> > -
> > - ret = ttm_mem_evict_first(bdev, man,
> > place, ctx,
> > - ticket);
> > - if (unlikely(ret == -EBUSY))
> > - break;
> > - if (unlikely(ret))
> > + if (!may_evict)
> > + continue;
> > +
> > + ret = ttm_bo_evict_alloc(bdev, man, place,
> > bo, ctx,
> > + ticket, res);
> > + if (ret == -EBUSY)
> > + continue;
> > + if (ret)
> > return ret;
> > - } while (1);
> > - if (ret)
> > - continue;
> > + }
> >
> > ret = ttm_bo_add_move_fence(bo, man, ctx-
> > >no_wait_gpu);
> > if (unlikely(ret)) {
> > @@ -796,7 +725,6 @@ static int ttm_bo_alloc_resource(struct
> > ttm_buffer_object *bo,
> > }
> > return 0;
> > }
> > -
> > return -ENOSPC;
> > }
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_resource.c
> > b/drivers/gpu/drm/ttm/ttm_resource.c
> > index a03090683e79..6d0c66fc36e3 100644
> > --- a/drivers/gpu/drm/ttm/ttm_resource.c
> > +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> > @@ -508,24 +508,10 @@ int ttm_resource_manager_evict_all(struct
> > ttm_device *bdev,
> > };
> > struct dma_fence *fence;
> > int ret;
> > - unsigned i;
> > -
> > - /*
> > - * Can't use standard list traversal since we're
> > unlocking.
> > - */
> >
> > - spin_lock(&bdev->lru_lock);
> > - for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
> > - while (!list_empty(&man->lru[i])) {
> > - spin_unlock(&bdev->lru_lock);
> > - ret = ttm_mem_evict_first(bdev, man, NULL,
> > &ctx,
> > - NULL);
> > - if (ret)
> > - return ret;
> > - spin_lock(&bdev->lru_lock);
> > - }
> > - }
> > - spin_unlock(&bdev->lru_lock);
> > + do {
> > + ret = ttm_bo_evict_first(bdev, man, &ctx);
>
> Ooo, just noticed this after my initial review.
>
> This function, ttm_bo_evict_first, will return -ENOENT if
> ttm_bo_get_unless_zero returns false breaking this loop. I don't
> think
> that is the correct behavior. If ttm_bo_get_unless_zero returns false
> on
> the head of LRU, that means this is getting destroyed. I don't think
> in
> that what we want do to here. Shouldn't continue the LRU walk
> evicting
> all other BOs on the LRU list?
OK, yes, I'll take a look to see if it's possible to make that more
robust.
/Thomas
>
> Matt
>
> > + } while (!ret);
> >
> > spin_lock(&man->move_lock);
> > fence = dma_fence_get(man->move);
> > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> > index 472a55b69afb..148f49f625e4 100644
> > --- a/include/drm/ttm/ttm_bo.h
> > +++ b/include/drm/ttm/ttm_bo.h
> > @@ -415,11 +415,9 @@ long ttm_bo_swapout(struct ttm_device *bdev,
> > struct ttm_operation_ctx *ctx,
> > pgoff_t target);
> > void ttm_bo_pin(struct ttm_buffer_object *bo);
> > void ttm_bo_unpin(struct ttm_buffer_object *bo);
> > -int ttm_mem_evict_first(struct ttm_device *bdev,
> > - struct ttm_resource_manager *man,
> > - const struct ttm_place *place,
> > - struct ttm_operation_ctx *ctx,
> > - struct ww_acquire_ctx *ticket);
> > +int ttm_bo_evict_first(struct ttm_device *bdev,
> > + struct ttm_resource_manager *man,
> > + struct ttm_operation_ctx *ctx);
> > vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
> > struct vm_fault *vmf);
> > vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> > --
> > 2.44.0
> >
