Hi, Matthew
On Wed, 2024-06-19 at 22:52 +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.
> >
> I'm inclined to RB this as I think I've made sense of it all but just
> have a few questions / nits first + one small bug.
>
> > 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;
>
> Nit, this assignment is not needed a out_no_ref just returns -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)
>
> So 'bo->resource != res' is checking between dropping of the LRU lock
> and grabbing the dma-resv lock in ttm_bo_reserve, the BO's backing
> resource has changed (i.e. someone else could've evicted to BO). Is
> that correct? Also in this case 'res' could be stale memory and not
> safe
> to dereference, right?
Yes, that's correct. Although if bo->resource == res, it is safe to
dereference res.
>
> I'm confused about 'res->mem_type != mem_type' though. Looking
> through
> the code is res->mem_type not immutable? I think I only see
> res->mem_type assigned in ttm_resource_init.
It's if someone freed res and then allocated a new resource that ended
up as bo->resource, but with the same virtual address as res. Not very
likely but definitely possible. Then if the new resource has the same
memory type we can go ahead and evict anyway, If not, we skip eviction.
>
> > + goto out_bad_res;
>
> s/out_bad_res/out_bo_moved
>
> Would that be more accurate?
Yes, I could change that.
>
> >
> > - 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)
>
> This should be 'if (!ret)', right? We should cleanup once the BO is
> idle.
Right. Will fix.
>
> > + 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. */
>
> s/pages/BOs or resources
>
> Another option would be 'forward_progess' or something like that
> given
> the usage in this patch.
OK, yes, will take a look and fix.
>
> > + 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;
>
> Same comment as the previous patch, the inverse of this might be more
> clear. Also if the condition is the same, a helper may make sense.
Will fix.
>
> >
> > - 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;
> > }
> > -
>
> Nit, seems unrelated.
Yup. Will fix.
>
> Matt
>
> > 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);
> > + } 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_firevictedst(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
> >
