linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: "Christian König" <ckoenig.leichtzumerken@gmail.com>
To: dri-devel@lists.freedesktop.org, linux-mm@kvack.org,
	Liang.Liang@amd.com, thomas_os@shipmail.org
Subject: Re: [PATCH] drm/ttm: switch back to static allocation limits for now
Date: Thu, 25 Mar 2021 09:07:51 +0100	[thread overview]
Message-ID: <0f2308b4-f36e-ab02-c26d-4065e9972b50@gmail.com> (raw)
In-Reply-To: <YFuTEl/R73Fvet/y@phenom.ffwll.local>

Am 24.03.21 um 20:29 schrieb Daniel Vetter:
> 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.

Well this is the revert to hardcoded 50%. We had that configurable 
before and have it configurable now.

Reverting everything back would mean that we also need to revert the 
sysfs changes and move all the memory accounting code from VMWGFX back 
into TTM.

Christian.

>
> 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
>>



  reply	other threads:[~2021-03-25  8:07 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
2021-03-25  8:07   ` Christian König [this message]
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=0f2308b4-f36e-ab02-c26d-4065e9972b50@gmail.com \
    --to=ckoenig.leichtzumerken@gmail.com \
    --cc=Liang.Liang@amd.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).