Re: [PATCH v5 06/12] drm/ttm: Use the LRU walker helper for swapping

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

>  
> -	/*
> -	 * 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?

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
> 



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux