Re: [PATCH 3/3] drm/ttm: under memory pressure minimize the size of memory pool

[Thomas Hellstrom <thellstrom@vmware.com>]

On 08/13/2014 05:52 AM, Jérôme Glisse wrote:
> From: Jérôme Glisse <jglisse@redhat.com>
>
> When experiencing memory pressure we want to minimize pool size so that
> memory we just shrinked is not added back again just as the next thing.
>
> This will divide by 2 the maximum pool size for each device each time
> the pool have to shrink. The limit is bumped again is next allocation
> happen after one second since the last shrink. The one second delay is
> obviously an arbitrary choice.

Jérôme,

I don't like this patch. It adds extra complexity and its usefulness is
highly questionable.
There are a number of caches in the system, and if all of them added
some sort of voluntary shrink heuristics like this, we'd end up with
impossible-to-debug unpredictable performance issues.

We should let the memory subsystem decide when to reclaim pages from
caches and what caches to reclaim them from.

/Thomas
>
> Signed-off-by: Jérôme Glisse <jglisse@redhat.com>
> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Thomas Hellstrom <thellstrom@vmware.com>
> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> ---
>  drivers/gpu/drm/ttm/ttm_page_alloc.c     | 35 +++++++++++++++++++++++++-------
>  drivers/gpu/drm/ttm/ttm_page_alloc_dma.c | 27 ++++++++++++++++++++++--
>  2 files changed, 53 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index 09874d6..ab41adf 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -68,6 +68,8 @@
>   * @list: Pool of free uc/wc pages for fast reuse.
>   * @gfp_flags: Flags to pass for alloc_page.
>   * @npages: Number of pages in pool.
> + * @cur_max_size: Current maximum size for the pool.
> + * @shrink_timeout: Timeout for pool maximum size restriction.
>   */
>  struct ttm_page_pool {
>  	spinlock_t		lock;
> @@ -76,6 +78,8 @@ struct ttm_page_pool {
>  	gfp_t			gfp_flags;
>  	unsigned		npages;
>  	char			*name;
> +	unsigned		cur_max_size;
> +	unsigned long		last_shrink;
>  	unsigned long		nfrees;
>  	unsigned long		nrefills;
>  };
> @@ -289,6 +293,16 @@ static void ttm_pool_update_free_locked(struct ttm_page_pool *pool,
>  	pool->nfrees += freed_pages;
>  }
>  
> +static inline void ttm_pool_update_max_size(struct ttm_page_pool *pool)
> +{
> +	if (time_before(jiffies, pool->shrink_timeout))
> +		return;
> +	/* In case we reached zero bounce back to 512 pages. */
> +	pool->cur_max_size = max(pool->cur_max_size << 1, 512);
> +	pool->cur_max_size = min(pool->cur_max_size,
> +				 _manager->options.max_size);
> +}
> +
>  /**
>   * Free pages from pool.
>   *
> @@ -407,6 +421,9 @@ ttm_pool_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
>  		if (shrink_pages == 0)
>  			break;
>  		pool = &_manager->pools[(i + pool_offset)%NUM_POOLS];
> +		/* No matter what make sure the pool do not grow in next second. */
> +		pool->cur_max_size = pool->cur_max_size >> 1;
> +		pool->shrink_timeout = jiffies + HZ;
>  		shrink_pages = ttm_page_pool_free(pool, nr_free,
>  						  sc->gfp_mask);
>  		freed += nr_free - shrink_pages;
> @@ -701,13 +718,12 @@ static void ttm_put_pages(struct page **pages, unsigned npages, int flags,
>  	}
>  	/* Check that we don't go over the pool limit */
>  	npages = 0;
> -	if (pool->npages > _manager->options.max_size) {
> -		npages = pool->npages - _manager->options.max_size;
> -		/* free at least NUM_PAGES_TO_ALLOC number of pages
> -		 * to reduce calls to set_memory_wb */
> -		if (npages < NUM_PAGES_TO_ALLOC)
> -			npages = NUM_PAGES_TO_ALLOC;
> -	}
> +	/*
> +	 * Free at least NUM_PAGES_TO_ALLOC number of pages to reduce calls to
> +	 * set_memory_wb.
> +	 */
> +	if (pool->npages > (pool->cur_max_size + NUM_PAGES_TO_ALLOC))
> +		npages = pool->npages - pool->cur_max_size;
>  	spin_unlock_irqrestore(&pool->lock, irq_flags);
>  	if (npages)
>  		ttm_page_pool_free(pool, npages, GFP_KERNEL);
> @@ -751,6 +767,9 @@ static int ttm_get_pages(struct page **pages, unsigned npages, int flags,
>  		return 0;
>  	}
>  
> +	/* Update pool size in case shrinker limited it. */
> +	ttm_pool_update_max_size(pool);
> +
>  	/* combine zero flag to pool flags */
>  	gfp_flags |= pool->gfp_flags;
>  
> @@ -803,6 +822,8 @@ static void ttm_page_pool_init_locked(struct ttm_page_pool *pool, gfp_t flags,
>  	pool->npages = pool->nfrees = 0;
>  	pool->gfp_flags = flags;
>  	pool->name = name;
> +	pool->cur_max_size = _manager->options.max_size;
> +	pool->shrink_timeout = jiffies;
>  }
>  
>  int ttm_page_alloc_init(struct ttm_mem_global *glob, unsigned max_pages)
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index a076ff3..80b10aa 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -93,6 +93,8 @@ enum pool_type {
>   * @size: Size used during DMA allocation.
>   * @npages_free: Count of available pages for re-use.
>   * @npages_in_use: Count of pages that are in use.
> + * @cur_max_size: Current maximum size for the pool.
> + * @shrink_timeout: Timeout for pool maximum size restriction.
>   * @nfrees: Stats when pool is shrinking.
>   * @nrefills: Stats when the pool is grown.
>   * @gfp_flags: Flags to pass for alloc_page.
> @@ -110,6 +112,8 @@ struct dma_pool {
>  	unsigned size;
>  	unsigned npages_free;
>  	unsigned npages_in_use;
> +	unsigned cur_max_size;
> +	unsigned long last_shrink;
>  	unsigned long nfrees; /* Stats when shrunk. */
>  	unsigned long nrefills; /* Stats when grown. */
>  	gfp_t gfp_flags;
> @@ -331,6 +335,17 @@ static void __ttm_dma_free_page(struct dma_pool *pool, struct dma_page *d_page)
>  	kfree(d_page);
>  	d_page = NULL;
>  }
> +
> +static inline void ttm_dma_pool_update_max_size(struct dma_pool *pool)
> +{
> +	if (time_before(jiffies, pool->shrink_timeout))
> +		return;
> +	/* In case we reached zero bounce back to 512 pages. */
> +	pool->cur_max_size = max(pool->cur_max_size << 1, 512);
> +	pool->cur_max_size = min(pool->cur_max_size,
> +				 _manager->options.max_size);
> +}
> +
>  static struct dma_page *__ttm_dma_alloc_page(struct dma_pool *pool)
>  {
>  	struct dma_page *d_page;
> @@ -606,6 +621,8 @@ static struct dma_pool *ttm_dma_pool_init(struct device *dev, gfp_t flags,
>  	pool->size = PAGE_SIZE;
>  	pool->type = type;
>  	pool->nrefills = 0;
> +	pool->cur_max_size = _manager->options.max_size;
> +	pool->shrink_timeout = jiffies;
>  	p = pool->name;
>  	for (i = 0; i < 5; i++) {
>  		if (type & t[i]) {
> @@ -892,6 +909,9 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev)
>  		}
>  	}
>  
> +	/* Update pool size in case shrinker limited it. */
> +	ttm_dma_pool_update_max_size(pool);
> +
>  	INIT_LIST_HEAD(&ttm_dma->pages_list);
>  	for (i = 0; i < ttm->num_pages; ++i) {
>  		ret = ttm_dma_pool_get_pages(pool, ttm_dma, i);
> @@ -953,9 +973,9 @@ void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, struct device *dev)
>  	} else {
>  		pool->npages_free += count;
>  		list_splice(&ttm_dma->pages_list, &pool->free_list);
> -		if (pool->npages_free >= (_manager->options.max_size +
> +		if (pool->npages_free >= (pool->cur_max_size +
>  					  NUM_PAGES_TO_ALLOC))
> -			npages = pool->npages_free - _manager->options.max_size;
> +			npages = pool->npages_free - pool->cur_max_size;
>  	}
>  	spin_unlock_irqrestore(&pool->lock, irq_flags);
>  
> @@ -1024,6 +1044,9 @@ ttm_dma_pool_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
>  		/* Do it in round-robin fashion. */
>  		if (++idx < pool_offset)
>  			continue;
> +		/* No matter what make sure the pool do not grow in next second. */
> +		p->pool->cur_max_size = p->pool->cur_max_size >> 1;
> +		p->pool->shrink_timeout = jiffies + HZ;
>  		nr_free = shrink_pages;
>  		shrink_pages = ttm_dma_page_pool_free(p->pool, nr_free,
>  						      sc->gfp_mask);

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel