On 08/13/2014 12:42 PM, Daniel Vetter wrote:
> On Wed, Aug 13, 2014 at 11:06:25AM +0200, Thomas Hellstrom wrote:
>> On 08/13/2014 05:52 AM, Jérôme Glisse wrote:
>>> From: Jérôme Glisse <jglisse@xxxxxxxxxx>
>>>
>>> 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.
> Yeah, artificially limiting your cache from growing when your shrinker
> gets called will just break the equal-memory pressure the core mm uses to
> rebalance between all caches when workload changes. In i915 we let
> everything grow without artificial bounds and only rely upon the shrinker
> callbacks to ensure we don't consume more than our fair share of available
> memory overall.
> -Daniel
Now when you bring i915 memory usage up, Daniel,
I can't refrain from bringing up the old user-space unreclaimable kernel
memory issue, for which gem open is a good example ;) Each time
user-space opens a gem handle, some un-reclaimable kernel memory is
allocated, for which there is no accounting, so theoretically I think a
user can bring a system to unusability this way.
Typically there are various limits on unreclaimable objects like this,
like open file descriptors, and IIRC the kernel even has an internal
limit on the number of struct files you initialize, based on the
available system memory, so dma-buf / prime should already have some
sort of protection.
/Thomas
>> /Thomas
>>> Signed-off-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
>>> Cc: Mario Kleiner <mario.kleiner.de@xxxxxxxxx>
>>> Cc: Michel Dänzer <michel@xxxxxxxxxxx>
>>> Cc: Thomas Hellstrom <thellstrom@xxxxxxxxxx>
>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
>> https://urldefense.proofpoint.com/v1/url?u=http://lists.freedesktop.org/mailman/listinfo/dri-devel&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=O3%2ForLs9%2FKcgsMAkAfqRcjXW1tOmP7AUEsPkaztSSrE%3D%0A&s=a7ca6074f66b92976893c6c6e18d96b96f905d4fc8ec0b8503614728fc387dc5
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
