On Tue, Jun 18, 2024 at 09:18:13AM +0200, Thomas Hellström wrote:
Replying to correct version...
> Provide a generic LRU walker in TTM, in the spirit of drm_gem_lru_scan()
> but building on the restartable TTM LRU functionality.
>
> The LRU walker optionally supports locking objects as part of
> a ww mutex locking transaction, to mimic to some extent the
> current functionality in ttm. However any -EDEADLK return
> is converted to -ENOMEM, so that the driver will need to back
> off and possibly retry without being able to keep the
> ticket.
>
Wouldn't the backoff be unlock everything but keep the ticket?
> v3:
> - Move the helper to core ttm.
> - Remove the drm_exec usage from it for now, it will be
> reintroduced later in the series.
> v4:
> - Handle the -EALREADY case if ticketlocking.
>
> 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_util.c | 145 ++++++++++++++++++++++++++++++
> include/drm/ttm/ttm_bo.h | 32 +++++++
> 2 files changed, 177 insertions(+)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
> index 0b3f4267130c..45fcaf6f8644 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
> @@ -768,3 +768,148 @@ int ttm_bo_pipeline_gutting(struct ttm_buffer_object *bo)
> ttm_tt_destroy(bo->bdev, ttm);
> return ret;
> }
> +
> +static bool ttm_lru_walk_trylock(struct ttm_lru_walk *walk,
> + struct ttm_buffer_object *bo,
> + bool *needs_unlock)
> +{
> + struct ttm_operation_ctx *ctx = walk->ctx;
> +
> + *needs_unlock = false;
> +
> + if (dma_resv_trylock(bo->base.resv)) {
> + *needs_unlock = true;
> + return true;
> + }
> +
> + if (bo->base.resv == ctx->resv && ctx->allow_res_evict) {
> + dma_resv_assert_held(bo->base.resv);
> + return true;
> + }
> +i
Any reason this is done after the try lock? Just kinda goofy as if this
statement is true the dma_resv_trylock will always fail.
> + return false;
> +}
> +
> +static int ttm_lru_walk_ticketlock(struct ttm_lru_walk *walk,
> + struct ttm_buffer_object *bo,
> + bool *needs_unlock)
> +{
> + struct dma_resv *resv = bo->base.resv;
> + int ret;
> +
I suppose we don't have asserts here like in Xe but if we did,
assert(walk->ticket)?
> + if (walk->ctx->interruptible)
> + ret = dma_resv_lock_interruptible(resv, walk->ticket);
> + else
> + ret = dma_resv_lock(resv, walk->ticket);
> +
> + if (!ret) {
> + *needs_unlock = true;
> + /* Only a single ticketlock per loop. */
> + walk->ticket = NULL;
Can you explain this a bit more? I see that once the walk->ticket is set
to NULL this function will not be called again (i.e. only try locking
will be used). I want to understand the reasoning for this.
It might be helpful for a more lengthly explaination in the comments of
the code too.
> + } else if (ret == -EDEADLK) {
> + /* Caller needs to exit the ww transaction. */
> + ret = -ENOSPC;
The commit message says -ENOMEM.
> + }
> +
> + 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.
> + * @walk: describe the walks and actions taken
> + * @bdev: The TTM device.
> + * @man: The struct ttm_resource manager whose LRU lists we're walking.
> + * @target: The end condition for the walk.
> + *
> + * The LRU lists of @man are walk, and for each struct ttm_resource encountered,
> + * the corresponding ttm_buffer_object is locked and taken a reference on, and
> + * the LRU lock is dropped. the LRU lock may be dropped before locking and, in
> + * that case, it's verified that the item actually remains on the LRU list after
> + * the lock, and that the buffer object didn't switch resource in between.
> + *
> + * With a locked object, the actions indicated by @walk->process_bo are
> + * performed, and after that, the bo is unlocked, the refcount dropped and the
> + * next struct ttm_resource is processed. Here, the walker relies on
> + * TTM's restartable LRU list implementation.
> + *
> + * Typically @walk->process_bo() would return the number of pages evicted,
> + * swapped or shrunken, so that when the total exceeds @target, or when the
> + * LRU list has been walked in full, iteration is terminated. It's also terminated
> + * on error. Note that the definition of @target is done by the caller, it
> + * could have a different meaning than the number of pages.
> + *
> + * Note that the way dma_resv individualization is done, locking needs to be done
> + * either with the LRU lock held (trylocking only) or with a reference on the
> + * object.
> + *
> + * Return: The progress made towards target or negative error code on error.
> + */
> +long ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev,
> + struct ttm_resource_manager *man, long target)
> +{
> + struct ttm_resource_cursor cursor;
> + struct ttm_resource *res;
> + long sofar = 0;
s/sofar/evicted?
> + long lret;
> +
> + spin_lock(&bdev->lru_lock);
> + ttm_resource_manager_for_each_res(man, &cursor, res) {
> + struct ttm_buffer_object *bo = res->bo;
> + bool bo_needs_unlock = false;
> + bool bo_locked = false;
> + int mem_type;
> +
> + if (!bo || bo->resource != res)
> + continue;
> +
> + if (ttm_lru_walk_trylock(walk, bo, &bo_needs_unlock))
> + bo_locked = true;
> + else if ((!walk->ticket) || walk->ctx->no_wait_gpu ||
Nit - (!walk->ticket) could just be !walk->ticket.
> + walk->trylock_only)
> + continue;
> +
> + if (!ttm_bo_get_unless_zero(bo)) {
> + ttm_lru_walk_unlock(bo, bo_needs_unlock);
> + continue;
> + }
> +
This kinda goofy pattern too, typically in code a get_unless_zero is
done before trying to lock the object not after. Even odder here, the
could or could not be locked depending on the outcome of
ttm_lru_walk_trylock. This is covering individualization case? Would it
make more sense to move ttm_bo_get_unless_zero before the try lock or is
that to avoid a put on try lock failure + continue?
> + mem_type = res->mem_type;
> + spin_unlock(&bdev->lru_lock);
> +
> + lret = 0;
> + if (!bo_locked && walk->ticket)
As above could you explain the ticket usage a bit more?
Also you shouldn't need to check the ticket here there is !walk->ticket
above which triggers a continue.
> + lret = ttm_lru_walk_ticketlock(walk, 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 == res && res->mem_type == mem_type)
> + lret = walk->ops->process_bo(walk, bo);
> +
> + ttm_lru_walk_unlock(bo, bo_needs_unlock);
> + ttm_bo_put(bo);
> + if (lret == -EBUSY || lret == -EALREADY)
> + lret = 0;
What is usage of these error codes?
-EALREADY means the resv is locked with the current ticket, right?
Wouldn't we want to call process_bo in this case too?
-EBUSY I need some help figuring out.
> + sofar = (lret < 0) ? lret : sofar + lret;
> + if (sofar < 0 || sofar >= target)
> + goto out;
> +
Here we have dropped the BO unlock. What prevents the BO from being
moved back to the resource we just evicted it from resulting in sofar
not being accurate?
Matt
> + cond_resched();
> + spin_lock(&bdev->lru_lock);
> + }
> + spin_unlock(&bdev->lru_lock);
> +out:
> + ttm_resource_cursor_fini(&cursor);
> + return sofar;
> +}
> diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> index 6ccf96c91f3a..8b032298d66e 100644
> --- a/include/drm/ttm/ttm_bo.h
> +++ b/include/drm/ttm/ttm_bo.h
> @@ -190,6 +190,38 @@ struct ttm_operation_ctx {
> uint64_t bytes_moved;
> };
>
> +struct ttm_lru_walk;
> +
> +/** struct ttm_lru_walk_ops - Operations for a LRU walk. */
> +struct ttm_lru_walk_ops {
> + /**
> + * process_bo - Process this bo.
> + * @walk: struct ttm_lru_walk describing the walk.
> + * @bo: A locked and referenced buffer object.
> + *
> + * Return: Negative error code on error, Number of processed pages on
> + * success. 0 also indicates success.
> + */
> + long (*process_bo)(struct ttm_lru_walk *walk, struct ttm_buffer_object *bo);
> +};
> +
> +/**
> + * struct ttm_lru_walk - Structure describing a LRU walk.
> + */
> +struct ttm_lru_walk {
> + /** @ops: Pointer to the ops structure. */
> + const struct ttm_lru_walk_ops *ops;
> + /** @ctx: Pointer to the struct ttm_operation_ctx. */
> + struct ttm_operation_ctx *ctx;
> + /** @ticket: The struct ww_acquire_ctx if any. */
> + struct ww_acquire_ctx *ticket;
> + /** @tryock_only: Only use trylock for locking. */
> + bool trylock_only;
> +};
> +
> +long ttm_lru_walk_for_evict(struct ttm_lru_walk *walk, struct ttm_device *bdev,
> + struct ttm_resource_manager *man, long target);
> +
> /**
> * ttm_bo_get - reference a struct ttm_buffer_object
> *
> --
> 2.44.0
>
