linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: "Christian König" <ckoenig.leichtzumerken@gmail.com>
Cc: dri-devel@lists.freedesktop.org, linux-mm@kvack.org,
	Liang.Liang@amd.com, daniel@ffwll.ch, thomas_os@shipmail.org
Subject: Re: [PATCH] drm/ttm: switch back to static allocation limits for now
Date: Wed, 24 Mar 2021 20:29:22 +0100	[thread overview]
Message-ID: <YFuTEl/R73Fvet/y@phenom.ffwll.local> (raw)
In-Reply-To: <20210324134845.2338-1-christian.koenig@amd.com>

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@amd.com>
> ---
>  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


  reply	other threads:[~2021-03-24 19:29 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-24 13:48 [PATCH] drm/ttm: switch back to static allocation limits for now Christian König
2021-03-24 19:29 ` Daniel Vetter [this message]
2021-03-25  8:07   ` Christian König
2021-03-25  8:31     ` Daniel Vetter
2021-03-25  9:27       ` Christian König
2021-03-25  9:38         ` Daniel Vetter

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=YFuTEl/R73Fvet/y@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=Liang.Liang@amd.com \
    --cc=ckoenig.leichtzumerken@gmail.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=linux-mm@kvack.org \
    --cc=thomas_os@shipmail.org \
    /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 NNTP newsgroup(s).