Re: [PATCH] drm/ttm: switch back to static allocation limits for now

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

 



On Wed, Mar 24, 2021 at 02:48:45PM +0100, Christian König wrote:
> The shrinker based approach still has some flaws. Especially that we need
> temporary pages to free up the pages allocated to the driver is problematic
> in a shrinker.
> 
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
>  drivers/gpu/drm/ttm/ttm_device.c |  14 ++--
>  drivers/gpu/drm/ttm/ttm_tt.c     | 112 ++++++++++++-------------------
>  include/drm/ttm/ttm_tt.h         |   3 +-
>  3 files changed, 53 insertions(+), 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_device.c b/drivers/gpu/drm/ttm/ttm_device.c
> index 95e1b7b1f2e6..388da2a7f0bb 100644
> --- a/drivers/gpu/drm/ttm/ttm_device.c
> +++ b/drivers/gpu/drm/ttm/ttm_device.c
> @@ -53,7 +53,6 @@ static void ttm_global_release(void)
>  		goto out;
>  
>  	ttm_pool_mgr_fini();
> -	ttm_tt_mgr_fini();
>  
>  	__free_page(glob->dummy_read_page);
>  	memset(glob, 0, sizeof(*glob));
> @@ -64,7 +63,7 @@ static void ttm_global_release(void)
>  static int ttm_global_init(void)
>  {
>  	struct ttm_global *glob = &ttm_glob;
> -	unsigned long num_pages;
> +	unsigned long num_pages, num_dma32;
>  	struct sysinfo si;
>  	int ret = 0;
>  	unsigned i;
> @@ -79,8 +78,15 @@ static int ttm_global_init(void)
>  	 * system memory.
>  	 */
>  	num_pages = ((u64)si.totalram * si.mem_unit) >> PAGE_SHIFT;
> -	ttm_pool_mgr_init(num_pages * 50 / 100);
> -	ttm_tt_mgr_init();
> +	num_pages /= 2;
> +
> +	/* But for DMA32 we limit ourself to only use 2GiB maximum. */
> +	num_dma32 = (u64)(si.totalram - si.totalhigh) * si.mem_unit
> +		>> PAGE_SHIFT;
> +	num_dma32 = min(num_dma32, 2UL << (30 - PAGE_SHIFT));
> +
> +	ttm_pool_mgr_init(num_pages);
> +	ttm_tt_mgr_init(num_pages, num_dma32);
>  
>  	spin_lock_init(&glob->lru_lock);
>  	glob->dummy_read_page = alloc_page(__GFP_ZERO | GFP_DMA32);
> diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> index 2f0833c98d2c..5d8820725b75 100644
> --- a/drivers/gpu/drm/ttm/ttm_tt.c
> +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> @@ -40,8 +40,18 @@
>  
>  #include "ttm_module.h"
>  
> -static struct shrinker mm_shrinker;
> -static atomic_long_t swapable_pages;
> +static unsigned long ttm_pages_limit;
> +
> +MODULE_PARM_DESC(pages_limit, "Limit for the allocated pages");
> +module_param_named(pages_limit, ttm_pages_limit, ulong, 0644);
> +
> +static unsigned long ttm_dma32_pages_limit;
> +
> +MODULE_PARM_DESC(dma32_pages_limit, "Limit for the allocated DMA32 pages");
> +module_param_named(dma32_pages_limit, ttm_dma32_pages_limit, ulong, 0644);
> +
> +static atomic_long_t ttm_pages_allocated;
> +static atomic_long_t ttm_dma32_pages_allocated;

Making this configurable looks an awful lot like "job done, move on". Just
the revert to hardcoded 50% (or I guess just revert the shrinker patch at
that point) for -fixes is imo better.

Then I guess retry again for 5.14 or so.
-Daniel

>  
>  /*
>   * Allocates a ttm structure for the given BO.
> @@ -294,8 +304,6 @@ static void ttm_tt_add_mapping(struct ttm_device *bdev, struct ttm_tt *ttm)
>  
>  	for (i = 0; i < ttm->num_pages; ++i)
>  		ttm->pages[i]->mapping = bdev->dev_mapping;
> -
> -	atomic_long_add(ttm->num_pages, &swapable_pages);
>  }
>  
>  int ttm_tt_populate(struct ttm_device *bdev,
> @@ -309,12 +317,25 @@ int ttm_tt_populate(struct ttm_device *bdev,
>  	if (ttm_tt_is_populated(ttm))
>  		return 0;
>  
> +	atomic_long_add(ttm->num_pages, &ttm_pages_allocated);
> +	if (bdev->pool.use_dma32)
> +		atomic_long_add(ttm->num_pages, &ttm_dma32_pages_allocated);
> +
> +	while (atomic_long_read(&ttm_pages_allocated) > ttm_pages_limit ||
> +	       atomic_long_read(&ttm_dma32_pages_allocated) >
> +	       ttm_dma32_pages_limit) {
> +
> +		ret = ttm_bo_swapout(ctx, GFP_KERNEL);
> +		if (ret)
> +			goto error;
> +	}
> +
>  	if (bdev->funcs->ttm_tt_populate)
>  		ret = bdev->funcs->ttm_tt_populate(bdev, ttm, ctx);
>  	else
>  		ret = ttm_pool_alloc(&bdev->pool, ttm, ctx);
>  	if (ret)
> -		return ret;
> +		goto error;
>  
>  	ttm_tt_add_mapping(bdev, ttm);
>  	ttm->page_flags |= TTM_PAGE_FLAG_PRIV_POPULATED;
> @@ -327,6 +348,12 @@ int ttm_tt_populate(struct ttm_device *bdev,
>  	}
>  
>  	return 0;
> +
> +error:
> +	atomic_long_sub(ttm->num_pages, &ttm_pages_allocated);
> +	if (bdev->pool.use_dma32)
> +		atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated);
> +	return ret;
>  }
>  EXPORT_SYMBOL(ttm_tt_populate);
>  
> @@ -342,12 +369,9 @@ static void ttm_tt_clear_mapping(struct ttm_tt *ttm)
>  		(*page)->mapping = NULL;
>  		(*page++)->index = 0;
>  	}
> -
> -	atomic_long_sub(ttm->num_pages, &swapable_pages);
>  }
>  
> -void ttm_tt_unpopulate(struct ttm_device *bdev,
> -		       struct ttm_tt *ttm)
> +void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm)
>  {
>  	if (!ttm_tt_is_populated(ttm))
>  		return;
> @@ -357,76 +381,24 @@ void ttm_tt_unpopulate(struct ttm_device *bdev,
>  		bdev->funcs->ttm_tt_unpopulate(bdev, ttm);
>  	else
>  		ttm_pool_free(&bdev->pool, ttm);
> -	ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED;
> -}
> -
> -/* As long as pages are available make sure to release at least one */
> -static unsigned long ttm_tt_shrinker_scan(struct shrinker *shrink,
> -					  struct shrink_control *sc)
> -{
> -	struct ttm_operation_ctx ctx = {
> -		.no_wait_gpu = false
> -	};
> -	int ret;
> -
> -	ret = ttm_bo_swapout(&ctx, GFP_NOFS);
> -	return ret < 0 ? SHRINK_EMPTY : ret;
> -}
> -
> -/* Return the number of pages available or SHRINK_EMPTY if we have none */
> -static unsigned long ttm_tt_shrinker_count(struct shrinker *shrink,
> -					   struct shrink_control *sc)
> -{
> -	unsigned long num_pages;
> -
> -	num_pages = atomic_long_read(&swapable_pages);
> -	return num_pages ? num_pages : SHRINK_EMPTY;
> -}
>  
> -#ifdef CONFIG_DEBUG_FS
> +	atomic_long_sub(ttm->num_pages, &ttm_pages_allocated);
> +	if (bdev->pool.use_dma32)
> +		atomic_long_sub(ttm->num_pages, &ttm_dma32_pages_allocated);
>  
> -/* Test the shrinker functions and dump the result */
> -static int ttm_tt_debugfs_shrink_show(struct seq_file *m, void *data)
> -{
> -	struct shrink_control sc = { .gfp_mask = GFP_KERNEL };
> -
> -	fs_reclaim_acquire(GFP_KERNEL);
> -	seq_printf(m, "%lu/%lu\n", ttm_tt_shrinker_count(&mm_shrinker, &sc),
> -		   ttm_tt_shrinker_scan(&mm_shrinker, &sc));
> -	fs_reclaim_release(GFP_KERNEL);
> -
> -	return 0;
> +	ttm->page_flags &= ~TTM_PAGE_FLAG_PRIV_POPULATED;
>  }
> -DEFINE_SHOW_ATTRIBUTE(ttm_tt_debugfs_shrink);
> -
> -#endif
> -
> -
>  
>  /**
>   * ttm_tt_mgr_init - register with the MM shrinker
>   *
>   * Register with the MM shrinker for swapping out BOs.
>   */
> -int ttm_tt_mgr_init(void)
> +void ttm_tt_mgr_init(unsigned long num_pages, unsigned long num_dma32_pages)
>  {
> -#ifdef CONFIG_DEBUG_FS
> -	debugfs_create_file("tt_shrink", 0400, ttm_debugfs_root, NULL,
> -			    &ttm_tt_debugfs_shrink_fops);
> -#endif
> -
> -	mm_shrinker.count_objects = ttm_tt_shrinker_count;
> -	mm_shrinker.scan_objects = ttm_tt_shrinker_scan;
> -	mm_shrinker.seeks = 1;
> -	return register_shrinker(&mm_shrinker);
> -}
> +	if (!ttm_pages_limit)
> +		ttm_pages_limit = num_pages;
>  
> -/**
> - * ttm_tt_mgr_fini - unregister our MM shrinker
> - *
> - * Unregisters the MM shrinker.
> - */
> -void ttm_tt_mgr_fini(void)
> -{
> -	unregister_shrinker(&mm_shrinker);
> +	if (!ttm_dma32_pages_limit)
> +		ttm_dma32_pages_limit = num_dma32_pages;
>  }
> diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
> index 069f8130241a..134d09ef7766 100644
> --- a/include/drm/ttm/ttm_tt.h
> +++ b/include/drm/ttm/ttm_tt.h
> @@ -157,8 +157,7 @@ int ttm_tt_populate(struct ttm_device *bdev, struct ttm_tt *ttm, struct ttm_oper
>   */
>  void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm);
>  
> -int ttm_tt_mgr_init(void);
> -void ttm_tt_mgr_fini(void);
> +void ttm_tt_mgr_init(unsigned long num_pages, unsigned long num_dma32_pages);
>  
>  #if IS_ENABLED(CONFIG_AGP)
>  #include <linux/agp_backend.h>
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[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