From: Matthew Brost <matthew.brost@intel.com>
To: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
Cc: intel-xe@lists.freedesktop.org,
"Christian König" <christian.koenig@amd.com>,
"Somalapuram Amaranath" <Amaranath.Somalapuram@amd.com>,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v2 2/9] drm/ttm: Use LRU hitches
Date: Fri, 26 Apr 2024 01:23:36 +0000 [thread overview]
Message-ID: <ZisCGEsqlypxJgfG@DUT025-TGLU.fm.intel.com> (raw)
In-Reply-To: <20240416100730.6666-3-thomas.hellstrom@linux.intel.com>
On Tue, Apr 16, 2024 at 12:07:23PM +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)
>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.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 | 94 ++++++++++++++++++++----------
> include/drm/ttm/ttm_resource.h | 16 +++--
> 4 files changed, 82 insertions(+), 38 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 f27406e851e5..e8a6a1dab669 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
> @@ -169,12 +169,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 7aa5ca5c0e33..22f8ae4ff4c0 100644
> --- a/drivers/gpu/drm/ttm/ttm_resource.c
> +++ b/drivers/gpu/drm/ttm/ttm_resource.c
> @@ -32,6 +32,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
> @@ -484,61 +515,62 @@ 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,
> struct ttm_resource_cursor *cursor)
> {
> - struct ttm_lru_item *lru;
> -
> lockdep_assert_held(&man->bdev->lru_lock);
>
> - for (cursor->priority = 0; cursor->priority < TTM_MAX_BO_PRIORITY;
> - ++cursor->priority)
> - list_for_each_entry(lru, &man->lru[cursor->priority], link) {
> - if (ttm_lru_item_is_res(lru))
> - return ttm_lru_item_to_res(lru);
> - }
> + cursor->priority = 0;
> + cursor->man = man;
> + ttm_lru_item_init(&cursor->hitch, TTM_LRU_HITCH);
> + list_add(&cursor->hitch.link, &man->lru[cursor->priority]);
>
> - return NULL;
> + return ttm_resource_manager_next(cursor);
> }
>
> /**
> - * ttm_resource_manager_next
> - *
> - * @man: resource manager to iterate over
> + * ttm_resource_manager_next() - Continue iterating over the resource manager
> + * resources
> * @cursor: cursor to record the position
> - * @res: the current resource pointer
> *
> - * Returns the next resource from the resource manager.
> + * Return: The next resource from the resource manager.
> */
> struct ttm_resource *
> -ttm_resource_manager_next(struct ttm_resource_manager *man,
> - struct ttm_resource_cursor *cursor,
> - struct ttm_resource *res)
> +ttm_resource_manager_next(struct ttm_resource_cursor *cursor)
> {
> - struct ttm_lru_item *lru = &res->lru;
> + struct ttm_resource_manager *man = cursor->man;
> + struct ttm_lru_item *lru;
>
> lockdep_assert_held(&man->bdev->lru_lock);
>
> - list_for_each_entry_continue(lru, &man->lru[cursor->priority], link) {
> - if (ttm_lru_item_is_res(lru))
> - return ttm_lru_item_to_res(lru);
> - }
> -
> - for (++cursor->priority; cursor->priority < TTM_MAX_BO_PRIORITY;
> - ++cursor->priority)
> - list_for_each_entry(lru, &man->lru[cursor->priority], link) {
> - if (ttm_lru_item_is_res(lru))
> - ttm_lru_item_to_res(lru);
> + do {
> + lru = &cursor->hitch;
> + list_for_each_entry_continue(lru, &man->lru[cursor->priority], link) {
> + if (ttm_lru_item_is_res(lru)) {
> + list_move(&cursor->hitch.link, &lru->link);
> + return ttm_lru_item_to_res(lru);
> + }
> }
>
> + if (++cursor->priority >= TTM_MAX_BO_PRIORITY)
> + break;
> +
> + list_move(&cursor->hitch.link, &man->lru[cursor->priority]);
> + } while (true);
> +
> + list_del_init(&cursor->hitch.link);
As mentioned in patch #3, probably use ttm_resource_cursor_fini_locked here.
Have an open pacth #1 too related this patch but this does look sane.
Matt
> +
> return NULL;
> }
>
> diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
> index 4babc4ff10b0..dfc01258c981 100644
> --- a/include/drm/ttm/ttm_resource.h
> +++ b/include/drm/ttm/ttm_resource.h
> @@ -271,15 +271,23 @@ ttm_lru_item_to_res(struct ttm_lru_item *item)
>
> /**
> * struct ttm_resource_cursor
> - *
> + * @man: The resource manager currently being iterated over
> + * @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 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
> *
> @@ -438,9 +446,7 @@ struct ttm_resource *
> ttm_resource_manager_first(struct ttm_resource_manager *man,
> struct ttm_resource_cursor *cursor);
> struct ttm_resource *
> -ttm_resource_manager_next(struct ttm_resource_manager *man,
> - struct ttm_resource_cursor *cursor,
> - struct ttm_resource *res);
> +ttm_resource_manager_next(struct ttm_resource_cursor *cursor);
>
> /**
> * ttm_resource_manager_for_each_res - iterate over all resources
> @@ -452,7 +458,7 @@ ttm_resource_manager_next(struct ttm_resource_manager *man,
> */
> #define ttm_resource_manager_for_each_res(man, cursor, res) \
> for (res = ttm_resource_manager_first(man, cursor); res; \
> - res = ttm_resource_manager_next(man, cursor, res))
> + res = ttm_resource_manager_next(cursor))
>
> struct ttm_kmap_iter *
> ttm_kmap_iter_iomap_init(struct ttm_kmap_iter_iomap *iter_io,
> --
> 2.44.0
>
next prev parent reply other threads:[~2024-04-26 1:23 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 10:07 [PATCH v2 0/9] TTM shrinker helpers and xe buffer object shrinker Thomas Hellström
2024-04-16 10:07 ` [PATCH v2 1/9] drm/ttm: Allow TTM LRU list nodes of different types Thomas Hellström
2024-04-17 1:15 ` Matthew Brost
2024-04-17 6:09 ` Christian König
2024-05-02 11:41 ` Thomas Hellström
2024-04-16 10:07 ` [PATCH v2 2/9] drm/ttm: Use LRU hitches Thomas Hellström
2024-04-26 1:23 ` Matthew Brost [this message]
2024-04-16 10:07 ` [PATCH v2 3/9] drm/ttm, drm/amdgpu, drm/xe: Consider hitch moves within bulk sublist moves Thomas Hellström
2024-04-26 1:20 ` Matthew Brost
2024-04-16 10:07 ` [PATCH v2 4/9] drm/ttm: Allow continued swapout after -ENOSPC falure Thomas Hellström
2024-04-17 1:44 ` Matthew Brost
2024-04-16 10:07 ` [PATCH v2 5/9] drm/ttm: Add a virtual base class for graphics memory backup Thomas Hellström
2024-04-16 10:07 ` [PATCH v2 6/9] drm/ttm/pool: Provide a helper to shrink pages Thomas Hellström
2024-04-16 10:07 ` [PATCH v2 7/9] drm/ttm: Use fault-injection to test error paths Thomas Hellström
2024-04-16 10:07 ` [PATCH v2 8/9] drm/xe, drm/ttm: Provide a generic LRU walker helper Thomas Hellström
2024-04-16 10:07 ` [PATCH v2 9/9] drm/xe: Add a shrinker for xe bos Thomas Hellström
2024-04-16 21:32 ` kernel test robot
2024-04-17 13:45 ` kernel test robot
2024-04-16 11:55 ` [PATCH v2 0/9] TTM shrinker helpers and xe buffer object shrinker Christian König
2024-04-16 13:08 ` Thomas Hellström
2024-04-16 13:24 ` Christian König
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZisCGEsqlypxJgfG@DUT025-TGLU.fm.intel.com \
--to=matthew.brost@intel.com \
--cc=Amaranath.Somalapuram@amd.com \
--cc=christian.koenig@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-xe@lists.freedesktop.org \
--cc=thomas.hellstrom@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).