On Wed, 2024-06-19 at 04:23 +0000, Matthew Brost wrote:
> On Tue, Jun 18, 2024 at 09:18:14AM +0200, Thomas Hellström wrote:
> > Rework the TTM swapping to use the LRU walker helper.
> > This helps fixing up the ttm_bo_swapout() interface
> > to be consistent about not requiring any locking.
> >
> > For now mimic the current behaviour of using trylock
> > only. We could be using ticket-locks here but defer
> > that until it's deemed necessary. The TTM swapout
> > functionality is a bit weird anyway since it
> > alternates between memory types without exhausting
> > TTM_PL_SYSTEM first.
> >
> > 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 | 112 +++++++++++++++++++++------
> > ----
> > drivers/gpu/drm/ttm/ttm_device.c | 30 ++-------
> > include/drm/ttm/ttm_bo.h | 5 +-
> > 3 files changed, 83 insertions(+), 64 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> > b/drivers/gpu/drm/ttm/ttm_bo.c
> > index 43eda720657f..63a91b77f7da 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > @@ -1118,11 +1118,23 @@ int ttm_bo_wait_ctx(struct
> > ttm_buffer_object *bo, struct ttm_operation_ctx *ctx)
> > }
> > EXPORT_SYMBOL(ttm_bo_wait_ctx);
> >
> > -int ttm_bo_swapout(struct ttm_buffer_object *bo, struct
> > ttm_operation_ctx *ctx,
> > - gfp_t gfp_flags)
> > +/**
> > + * struct ttm_bo_swapout_walk - Parameters for the swapout walk
> > + */
> > +struct ttm_bo_swapout_walk {
> > + /** @walk: The walk base parameters. */
> > + struct ttm_lru_walk walk;
> > + /** @gfp_flags: The gfp flags to use for ttm_tt_swapout()
> > */
> > + gfp_t gfp_flags;
> > +};
> > +
> > +static long
> > +ttm_bo_swapout_cb(struct ttm_lru_walk *walk, struct
> > ttm_buffer_object *bo)
> > {
> > - struct ttm_place place;
> > - bool locked;
> > + struct ttm_place place = {.mem_type = bo->resource-
> > >mem_type};
> > + struct ttm_bo_swapout_walk *swapout_walk =
> > + container_of(walk, typeof(*swapout_walk), walk);
> > + struct ttm_operation_ctx *ctx = walk->ctx;
> > long ret;
> >
> > /*
> > @@ -1131,28 +1143,29 @@ int ttm_bo_swapout(struct ttm_buffer_object
> > *bo, struct ttm_operation_ctx *ctx,
> > * The driver may use the fact that we're moving from
> > SYSTEM
> > * as an indication that we're about to swap out.
> > */
> > - memset(&place, 0, sizeof(place));
> > - place.mem_type = bo->resource->mem_type;
> > - if (!ttm_bo_evict_swapout_allowable(bo, ctx, &place,
> > &locked, NULL))
> > - return -EBUSY;
> > + if (!bo->bdev->funcs->eviction_valuable(bo, &place)) {
> > + ret = -EBUSY;
> > + goto out;
> > + }
> >
> > if (!bo->ttm || !ttm_tt_is_populated(bo->ttm) ||
> > bo->ttm->page_flags & TTM_TT_FLAG_EXTERNAL ||
> > - bo->ttm->page_flags & TTM_TT_FLAG_SWAPPED ||
> > - !ttm_bo_get_unless_zero(bo)) {
> > - if (locked)
> > - dma_resv_unlock(bo->base.resv);
> > - return -EBUSY;
> > + bo->ttm->page_flags & TTM_TT_FLAG_SWAPPED) {
> > + ret = -EBUSY;
>
> I think answers my -EBUSY question from here [1]. In these cases we
> continue LRU walk as eviction of the BO is not valuable.
>
> [1]
> https://patchwork.freedesktop.org/patch/599606/?series=131815&rev=5#comment_1091419
>
> > + goto out;
> > }
> >
> > if (bo->deleted) {
> > - ret = ttm_bo_cleanup_refs(bo, false, false,
> > locked);
> > - ttm_bo_put(bo);
> > - return ret == -EBUSY ? -ENOSPC : ret;
> > - }
> > + pgoff_t num_pages = bo->ttm->num_pages;
> >
> > - /* TODO: Cleanup the locking */
> > - spin_unlock(&bo->bdev->lru_lock);
> > + ret = ttm_bo_wait_ctx(bo, ctx);
> > + if (ret)
> > + goto out;
> > +
> > + ttm_bo_cleanup_memtype_use(bo);
> > + ret = num_pages;
> > + goto out;
> > + }
> >
> > /*
> > * Move to system cached
> > @@ -1164,12 +1177,13 @@ int ttm_bo_swapout(struct ttm_buffer_object
> > *bo, struct ttm_operation_ctx *ctx,
> > memset(&hop, 0, sizeof(hop));
> > place.mem_type = TTM_PL_SYSTEM;
> > ret = ttm_resource_alloc(bo, &place, &evict_mem);
> > - if (unlikely(ret))
> > + if (ret)
> > goto out;
> >
> > ret = ttm_bo_handle_move_mem(bo, evict_mem, true,
> > ctx, &hop);
> > - if (unlikely(ret != 0)) {
> > - WARN(ret == -EMULTIHOP, "Unexpected
> > multihop in swaput - likely driver bug.\n");
> > + if (ret) {
> > + WARN(ret == -EMULTIHOP,
> > + "Unexpected multihop in swapout -
> > likely driver bug.\n");
> > ttm_resource_free(bo, &evict_mem);
> > goto out;
> > }
> > @@ -1179,30 +1193,54 @@ int ttm_bo_swapout(struct ttm_buffer_object
> > *bo, struct ttm_operation_ctx *ctx,
> > * Make sure BO is idle.
> > */
> > ret = ttm_bo_wait_ctx(bo, ctx);
> > - if (unlikely(ret != 0))
> > + if (ret)
> > goto out;
> >
> > ttm_bo_unmap_virtual(bo);
> > -
> > - /*
> > - * Swap out. Buffer will be swapped in again as soon as
> > - * anyone tries to access a ttm page.
> > - */
> > if (bo->bdev->funcs->swap_notify)
> > bo->bdev->funcs->swap_notify(bo);
> >
> > if (ttm_tt_is_populated(bo->ttm))
> > - ret = ttm_tt_swapout(bo->bdev, bo->ttm,
> > gfp_flags);
> > + ret = ttm_tt_swapout(bo->bdev, bo->ttm,
> > swapout_walk->gfp_flags);
> > out:
> > + /* Consider some error codes fatal. Others may continue
> > the walk. */
> > + if (ret == -ENOMEM || ret == -EINTR || ret == -ERESTARTSYS
> > ||
> > + ret == -EAGAIN || ret > 0)
> > + return ret;
>
> Would it be more robust / clear to do the inverse of this? i.e.
> Return 0
> on non-fatal error codes?
Hm. I'll check how that will turn out.
>
> >
> > - /*
> > - * Unreserve without putting on LRU to avoid swapping out
> > an
> > - * already swapped buffer.
> > - */
> > - if (locked)
> > - dma_resv_unlock(bo->base.resv);
> > - ttm_bo_put(bo);
> > - return ret == -EBUSY ? -ENOSPC : ret;
> > + return 0;
> > +}
> > +
> > +const struct ttm_lru_walk_ops ttm_swap_ops = {
> > + .process_bo = ttm_bo_swapout_cb,
> > +};
> > +
> > +/**
> > + * ttm_bo_swapout() - Swap out buffer objects on the LRU list to
> > shmem.
> > + * @bdev: The ttm device.
> > + * @ctx: The ttm_operation_ctx governing the swapout operation.
> > + * @man: The resource manager whose resources / buffer objects are
> > + * goint to be swapped out.
> > + * @gfp_flags: The gfp flags used for shmem page allocations.
> > + * @target: The desired number of pages to swap out.
> > + *
> > + * Return: The number of pages actually swapped out, or negative
> > error code
> > + * on error.
> > + */
> > +long ttm_bo_swapout(struct ttm_device *bdev, struct
> > ttm_operation_ctx *ctx,
> > + struct ttm_resource_manager *man, gfp_t
> > gfp_flags,
> > + pgoff_t target)
> > +{
> > + struct ttm_bo_swapout_walk swapout_walk = {
> > + .walk = {
> > + .ops = &ttm_swap_ops,
> > + .ctx = ctx,
> > + .trylock_only = true,
> > + },
> > + .gfp_flags = gfp_flags,
> > + };
> > +
> > + return ttm_lru_walk_for_evict(&swapout_walk.walk, bdev,
> > man, target);
> > }
> >
> > void ttm_bo_tt_destroy(struct ttm_buffer_object *bo)
> > diff --git a/drivers/gpu/drm/ttm/ttm_device.c
> > b/drivers/gpu/drm/ttm/ttm_device.c
> > index f9e9b1ec8c8a..ee575d8a54c0 100644
> > --- a/drivers/gpu/drm/ttm/ttm_device.c
> > +++ b/drivers/gpu/drm/ttm/ttm_device.c
> > @@ -148,40 +148,20 @@ int ttm_global_swapout(struct
> > ttm_operation_ctx *ctx, gfp_t gfp_flags)
> > int ttm_device_swapout(struct ttm_device *bdev, struct
> > ttm_operation_ctx *ctx,
> > gfp_t gfp_flags)
> > {
> > - struct ttm_resource_cursor cursor;
> > struct ttm_resource_manager *man;
> > - struct ttm_resource *res;
> > unsigned i;
> > - int ret;
> > + long lret;
> >
> > - spin_lock(&bdev->lru_lock);
> > for (i = TTM_PL_SYSTEM; i < TTM_NUM_MEM_TYPES; ++i) {
> > man = ttm_manager_type(bdev, i);
> > if (!man || !man->use_tt)
> > continue;
> >
> > - ttm_resource_manager_for_each_res(man, &cursor,
> > res) {
> > - struct ttm_buffer_object *bo = res->bo;
> > - uint32_t num_pages;
> > -
> > - if (!bo || bo->resource != res)
> > - continue;
> > -
> > - num_pages = PFN_UP(bo->base.size);
> > - ret = ttm_bo_swapout(bo, ctx, gfp_flags);
> > - /* ttm_bo_swapout has dropped the lru_lock
> > */
> > - if (!ret) {
> > - ttm_resource_cursor_fini(&cursor);
> > - return num_pages;
> > - }
> > - if (ret != -EBUSY) {
> > - ttm_resource_cursor_fini(&cursor);
> > - return ret;
> > - }
> > - }
> > + lret = ttm_bo_swapout(bdev, ctx, man, gfp_flags,
> > 1);
>
> With a target of 1 this will evict exactly 1 various sized BO which
> seems to match the current behavior.
>
> Just curious what is the usage of this function which evicts 1 BO
> from
> the device?
I think it evicts one bo at a time until the 50% system target is met,
alternating between TTM devices and, unfortunately, memory regions.
That could probably be improved upon, but beyond the scope of this
series since with the shrinker, we can't really exercise that on xe.
/Thomas
>
> Matt
>
> > + /* Can be both positive (num_pages) and negative
> > (error) */
> > + if (lret)
> > + return lret;
> > }
> > - ttm_resource_cursor_fini_locked(&cursor);
> > - spin_unlock(&bdev->lru_lock);
> > return 0;
> > }
> > EXPORT_SYMBOL(ttm_device_swapout);
> > diff --git a/include/drm/ttm/ttm_bo.h b/include/drm/ttm/ttm_bo.h
> > index 8b032298d66e..472a55b69afb 100644
> > --- a/include/drm/ttm/ttm_bo.h
> > +++ b/include/drm/ttm/ttm_bo.h
> > @@ -410,8 +410,9 @@ void ttm_bo_kunmap(struct ttm_bo_kmap_obj
> > *map);
> > int ttm_bo_vmap(struct ttm_buffer_object *bo, struct iosys_map
> > *map);
> > void ttm_bo_vunmap(struct ttm_buffer_object *bo, struct iosys_map
> > *map);
> > int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct
> > ttm_buffer_object *bo);
> > -int ttm_bo_swapout(struct ttm_buffer_object *bo, struct
> > ttm_operation_ctx *ctx,
> > - gfp_t gfp_flags);
> > +long ttm_bo_swapout(struct ttm_device *bdev, struct
> > ttm_operation_ctx *ctx,
> > + struct ttm_resource_manager *man, gfp_t
> > gfp_flags,
> > + 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,
> > --
> > 2.44.0
> >
