Re: [PATCH v3 03/21] drm/ttm: Use LRU hitches

[Matthew Brost <matthew.brost@intel.com>]

On Tue, May 21, 2024 at 09:16:21AM +0200, Thomas Hellström wrote:
> Have iterators insert themselves into the list they are iterating
> over using hitch list nodes. Since only the iterator owner
> can remove these list nodes from the list, it's safe to unlock
> the list and when continuing, use them as a starting point. Due to
> the way LRU bumping works in TTM, newly added items will not be
> missed, and bumped items will be iterated over a second time before
> reaching the end of the list.
> 
> The exception is list with bulk move sublists. When bumping a
> sublist, a hitch that is part of that sublist will also be moved
> and we might miss items if restarting from it. This will be
> addressed in a later patch.
> 
> Changes in previous series:
> - Updated ttm_resource_cursor_fini() documentation.
> v2:
> - Don't reorder ttm_resource_manager_first() and _next().
>   (Christian König).
> - Use list_add instead of list_move
>   (Christian König)
> v3:
> - Split into two patches, one cleanup, one new functionality
>   (Christian König)
> - use ttm_resource_cursor_fini_locked() instead of open-coding
>   (Matthew Brost)
> 
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/ttm/ttm_bo.c       |  1 +
>  drivers/gpu/drm/ttm/ttm_device.c   |  9 +++--
>  drivers/gpu/drm/ttm/ttm_resource.c | 56 +++++++++++++++++++++++++-----
>  include/drm/ttm/ttm_resource.h     |  9 +++--
>  4 files changed, 62 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 6396dece0db1..43eda720657f 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -621,6 +621,7 @@ int ttm_mem_evict_first(struct ttm_device *bdev,
>  		if (locked)
>  			dma_resv_unlock(res->bo->base.resv);
>  	}
> +	ttm_resource_cursor_fini_locked(&cursor);
>  
>  	if (!bo) {
>  		if (busy_bo && !ttm_bo_get_unless_zero(busy_bo))
> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
> index 09411978a13a..f9e9b1ec8c8a 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
> @@ -170,12 +170,17 @@ int ttm_device_swapout(struct ttm_device *bdev, struct ttm_operation_ctx *ctx,
>  			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)
> +			if (!ret) {
> +				ttm_resource_cursor_fini(&cursor);
>  				return num_pages;
> -			if (ret != -EBUSY)
> +			}
> +			if (ret != -EBUSY) {
> +				ttm_resource_cursor_fini(&cursor);
>  				return ret;
> +			}
>  		}
>  	}
> +	ttm_resource_cursor_fini_locked(&cursor);
>  	spin_unlock(&bdev->lru_lock);
>  	return 0;
>  }
> diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
> index 8bfbddddc0e8..9c8b6499edfb 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -33,6 +33,37 @@
>  
>  #include <drm/drm_util.h>
>  
> +/**
> + * ttm_resource_cursor_fini_locked() - Finalize the LRU list cursor usage
> + * @cursor: The struct ttm_resource_cursor to finalize.
> + *
> + * The function pulls the LRU list cursor off any lists it was previusly
> + * attached to. Needs to be called with the LRU lock held. The function
> + * can be called multiple times after eachother.
> + */
> +void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor *cursor)
> +{
> +	lockdep_assert_held(&cursor->man->bdev->lru_lock);
> +	list_del_init(&cursor->hitch.link);
> +}
> +
> +/**
> + * ttm_resource_cursor_fini() - Finalize the LRU list cursor usage
> + * @cursor: The struct ttm_resource_cursor to finalize.
> + *
> + * The function pulls the LRU list cursor off any lists it was previusly
> + * attached to. Needs to be called without the LRU list lock held. The
> + * function can be called multiple times after eachother.
> + */
> +void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor)
> +{
> +	spinlock_t *lru_lock = &cursor->man->bdev->lru_lock;
> +
> +	spin_lock(lru_lock);
> +	ttm_resource_cursor_fini_locked(cursor);
> +	spin_unlock(lru_lock);
> +}
> +
>  /**
>   * ttm_lru_bulk_move_init - initialize a bulk move structure
>   * @bulk: the structure to init
> @@ -485,12 +516,15 @@ void ttm_resource_manager_debug(struct ttm_resource_manager *man,
>  EXPORT_SYMBOL(ttm_resource_manager_debug);
>  
>  /**
> - * ttm_resource_manager_first
> - *
> + * ttm_resource_manager_first() - Start iterating over the resources
> + * of a resource manager
>   * @man: resource manager to iterate over
>   * @cursor: cursor to record the position
>   *
> - * Returns the first resource from the resource manager.
> + * Initializes the cursor and starts iterating. When done iterating,
> + * the caller must explicitly call ttm_resource_cursor_fini().
> + *
> + * Return: The first resource from the resource manager.
>   */
>  struct ttm_resource *
>  ttm_resource_manager_first(struct ttm_resource_manager *man,
> @@ -500,13 +534,15 @@ ttm_resource_manager_first(struct ttm_resource_manager *man,
>  
>  	cursor->priority = 0;
>  	cursor->man = man;
> -	cursor->cur = &man->lru[cursor->priority];
> +	ttm_lru_item_init(&cursor->hitch, TTM_LRU_HITCH);
> +	list_add(&cursor->hitch.link, &man->lru[cursor->priority]);
> +
>  	return ttm_resource_manager_next(cursor);
>  }
>  
>  /**
> - * ttm_resource_manager_next
> - *
> + * ttm_resource_manager_next() - Continue iterating over the resource manager
> + * resources
>   * @cursor: cursor to record the position
>   *
>   * Return: the next resource from the resource manager.
> @@ -520,10 +556,10 @@ ttm_resource_manager_next(struct ttm_resource_cursor *cursor)
>  	lockdep_assert_held(&man->bdev->lru_lock);
>  
>  	for (;;) {
> -		lru = list_entry(cursor->cur, typeof(*lru), link);
> +		lru = &cursor->hitch;
>  		list_for_each_entry_continue(lru, &man->lru[cursor->priority], link) {
>  			if (ttm_lru_item_is_res(lru)) {
> -				cursor->cur = &lru->link;
> +				list_move(&cursor->hitch.link, &lru->link);
>  				return ttm_lru_item_to_res(lru);
>  			}
>  		}
> @@ -531,9 +567,11 @@ ttm_resource_manager_next(struct ttm_resource_cursor *cursor)
>  		if (++cursor->priority >= TTM_MAX_BO_PRIORITY)
>  			break;
>  
> -		cursor->cur = &man->lru[cursor->priority];
> +		list_move(&cursor->hitch.link, &man->lru[cursor->priority]);
>  	}
>  
> +	ttm_resource_cursor_fini_locked(cursor);
> +
>  	return NULL;
>  }
>  
> diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
> index 7d81fd5b5b83..8fac781f641e 100644
> --- a/include/drm/ttm/ttm_resource.h
> +++ b/include/drm/ttm/ttm_resource.h
> @@ -273,17 +273,22 @@ ttm_lru_item_to_res(struct ttm_lru_item *item)
>   * struct ttm_resource_cursor
>   *
>   * @man: The resource manager currently being iterated over.
> - * @cur: The list head the cursor currently points to.
> + * @hitch: A hitch list node inserted before the next resource
> + * to iterate over.
>   * @priority: the current priority
>   *
>   * Cursor to iterate over the resources in a manager.
>   */
>  struct ttm_resource_cursor {
>  	struct ttm_resource_manager *man;
> -	struct list_head *cur;
> +	struct ttm_lru_item hitch;
>  	unsigned int priority;
>  };
>  
> +void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor *cursor);
> +
> +void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor);
> +
>  /**
>   * struct ttm_lru_bulk_move_pos
>   *
> -- 
> 2.44.0
>