On 5/15/25 22:57, Tvrtko Ursulin wrote:
> Currently the TTM pool shrinker ensures it frees at least something every
> time it is invoked, but it also lies to the core a bit on how hard it
> tried.
>
> For example core will ask it to free SHRINK_BATCH pages but the shrinker
> can, due how it walks the LRU list of pools, free just a single page and
> still leave the core thinking it expended the full SHRINK_BATCH effort.
>
> Apart from being inefficient in terms of number of calls to the TTM pool
> shrinker required, another consquence of this is that the core can abandon
> the shrinking attempt too early due thinking that the whole set of
> freeable pages has been scanned.
>
> We fix this last part by correctly reporting how many out of potentially
> freeable pages have been scanned.
>
> We also do the freeing in an aggressive manner, considering the scan
> target as a free target, to ensure walks over pools with unfreeable pages
> do not cause no-op calls into our callback.
>
> And finally we customise the shrinker batch size based on the median pool
> order and the total number of pools, with the aim of searching more
> possible pools on an average invocation.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursu...@igalia.com>
> Cc: Christian König <christian.koe...@amd.com>
> Cc: Thomas Hellström <thomas.hellst...@linux.intel.com>
> ---
> drivers/gpu/drm/ttm/ttm_pool.c | 39 +++++++++++++++++++++-------------
> 1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c
> index c2ea865be657..a76fe5f95781 100644
> --- a/drivers/gpu/drm/ttm/ttm_pool.c
> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
> @@ -368,7 +368,7 @@ static struct ttm_pool_type *ttm_pool_select_type(struct
> ttm_pool *pool,
> }
>
> /* Free pages using the global shrinker list */
> -static unsigned int ttm_pool_shrink(void)
> +static unsigned int ttm_pool_shrink(unsigned long *nr_scanned)
> {
> struct ttm_pool_type *pt;
> unsigned int num_pages;
> @@ -380,16 +380,15 @@ static unsigned int ttm_pool_shrink(void)
> list_move_tail(&pt->shrinker_list, &shrinker_list);
> spin_unlock(&shrinker_lock);
>
> + num_pages = 1 << pt->order;
> + (*nr_scanned) += num_pages;
> +
> p = ttm_pool_type_take(pt);
> - if (p) {
> + if (p)
> ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
> - num_pages = 1 << pt->order;
> - } else {
> - num_pages = 0;
> - }
> up_read(&pool_shrink_rwsem);
>
> - return num_pages;
> + return p ? num_pages : 0;
> }
That change here doesn't make any sense.
Scanning a pool type for pages and not finding anything doesn't mean we have
scanned for freeable pages and then not freed them.
>
> /* Return the allocation order based for a page */
> @@ -881,10 +880,12 @@ int ttm_pool_restore_and_alloc(struct ttm_pool *pool,
> struct ttm_tt *tt,
> */
> void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt)
> {
> + unsigned long nr_scanned = 0;
> +
> ttm_pool_free_range(pool, tt, tt->caching, 0, tt->num_pages);
>
> while (atomic_long_read(&allocated_pages) > page_pool_size)
> - ttm_pool_shrink();
> + ttm_pool_shrink(&nr_scanned);
> }
> EXPORT_SYMBOL(ttm_pool_free);
>
> @@ -1132,17 +1133,21 @@ void ttm_pool_fini(struct ttm_pool *pool)
> }
> EXPORT_SYMBOL(ttm_pool_fini);
>
> -/* As long as pages are available make sure to release at least one */
> static unsigned long ttm_pool_shrinker_scan(struct shrinker *shrink,
> struct shrink_control *sc)
> {
> - unsigned long num_freed = 0;
> + unsigned long to_scan, freed = 0;
>
> - do
> - num_freed += ttm_pool_shrink();
> - while (!num_freed && atomic_long_read(&allocated_pages));
> + sc->nr_scanned = 0;
> + to_scan = min_t(unsigned long,
> + sc->nr_to_scan, atomic_long_read(&allocated_pages));
> + while (freed < to_scan) {
> + freed += ttm_pool_shrink(&sc->nr_scanned);
> + to_scan = min_t(unsigned long,
> + to_scan, atomic_long_read(&allocated_pages));
> + }
>
> - return num_freed;
> + return sc->nr_scanned ? freed : SHRINK_STOP;
That again doesn't make sense. That we only find pool types which don't have
pages doesn't mean we have scanned them.
As far as I can see the existing code was correct after all.
> }
>
> /* Return the number of pages available or SHRINK_EMPTY if we have none */
> @@ -1266,7 +1271,10 @@ EXPORT_SYMBOL(ttm_pool_debugfs);
> /* Test the shrinker functions and dump the result */
> static int ttm_pool_debugfs_shrink_show(struct seq_file *m, void *data)
> {
> - struct shrink_control sc = { .gfp_mask = GFP_NOFS };
> + struct shrink_control sc = {
> + .gfp_mask = GFP_NOFS,
> + .nr_to_scan = 1,
> + };
>
> fs_reclaim_acquire(GFP_KERNEL);
> seq_printf(m, "%lu/%lu\n", ttm_pool_shrinker_count(mm_shrinker, &sc),
> @@ -1324,6 +1332,7 @@ int ttm_pool_mgr_init(unsigned long num_pages)
>
> mm_shrinker->count_objects = ttm_pool_shrinker_count;
> mm_shrinker->scan_objects = ttm_pool_shrinker_scan;
> + mm_shrinker->batch = (1 << (MAX_PAGE_ORDER / 2)) * NR_PAGE_ORDERS;
Since we install only one global shrinker for all pool types, which might
contain everything from 1 page till 512 pages, this seems to not make sense at
all either.
What exactly are you trying to do here?
Regards,
Christian.
> mm_shrinker->seeks = 1;
>
> shrinker_register(mm_shrinker);
