On Fri, Jun 07, 2013 at 12:34:53AM +0400, Glauber Costa wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Convert the driver shrinkers to the new API. Most changes are
> compile tested only because I either don't have the hardware or it's
> staging stuff.
>
> FWIW, the md and android code is pretty good, but the rest of it
> makes me want to claw my eyes out. The amount of broken code I just
> encountered is mind boggling. I've added comments explaining what
> is broken, but I fear that some of the code would be best dealt with
> by being dragged behind the bike shed, burying in mud up to it's
> neck and then run over repeatedly with a blunt lawn mower.
The rest being i915, ttm, bcache- etc ?
>
> Special mention goes to the zcache/zcache2 drivers. They can't
> co-exist in the build at the same time, they are under different
> menu options in menuconfig, they only show up when you've got the
> right set of mm subsystem options configured and so even compile
> testing is an exercise in pulling teeth. And that doesn't even take
> into account the horrible, broken code...
Now that you have rebased it, did you still see issues here.
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc.c b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> index bd2a3b4..1746f30 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc.c
> @@ -377,28 +377,26 @@ out:
> return nr_free;
> }
>
> -/* Get good estimation how many pages are free in pools */
> -static int ttm_pool_get_num_unused_pages(void)
> -{
> - unsigned i;
> - int total = 0;
> - for (i = 0; i < NUM_POOLS; ++i)
> - total += _manager->pools[i].npages;
> -
> - return total;
> -}
> -
I am unclear as of why you move this.
> /**
> * Callback for mm to request pool to reduce number of page held.
> + *
> + * XXX: (dchinner) Deadlock warning!
> + *
> + * ttm_page_pool_free() does memory allocation using GFP_KERNEL. that means
That
> + * this can deadlock when called a sc->gfp_mask that is not equal to
> + * GFP_KERNEL.
> + *
> + * This code is crying out for a shrinker per pool....
It iterates over different pools.
The ttm_page_pool_free() could use GFP_ATOMIC to guard against the dead-lock
I think?
> */
> -static int ttm_pool_mm_shrink(struct shrinker *shrink,
> - struct shrink_control *sc)
> +static long
> +ttm_pool_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
> {
> static atomic_t start_pool = ATOMIC_INIT(0);
> unsigned i;
> unsigned pool_offset = atomic_add_return(1, &start_pool);
> struct ttm_page_pool *pool;
> int shrink_pages = sc->nr_to_scan;
> + long freed = 0;
>
> pool_offset = pool_offset % NUM_POOLS;
> /* select start pool in round robin fashion */
> @@ -408,14 +406,28 @@ static int ttm_pool_mm_shrink(struct shrinker *shrink,
> break;
> pool = &_manager->pools[(i + pool_offset)%NUM_POOLS];
> shrink_pages = ttm_page_pool_free(pool, nr_free);
> + freed += nr_free - shrink_pages;
> }
> - /* return estimated number of unused pages in pool */
> - return ttm_pool_get_num_unused_pages();
> + return freed;
> +}
> +
> +
> +static long
> +ttm_pool_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
> +{
> + unsigned i;
> + long count = 0;
> +
> + for (i = 0; i < NUM_POOLS; ++i)
> + count += _manager->pools[i].npages;
> +
> + return count;
> }
>
> static void ttm_pool_mm_shrink_init(struct ttm_pool_manager *manager)
> {
> - manager->mm_shrink.shrink = &ttm_pool_mm_shrink;
> + manager->mm_shrink.count_objects = &ttm_pool_shrink_count;
> + manager->mm_shrink.scan_objects = &ttm_pool_shrink_scan;
> manager->mm_shrink.seeks = 1;
> register_shrinker(&manager->mm_shrink);
> }
> diff --git a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> index b8b3943..dc009f1 100644
> --- a/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> +++ b/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c
> @@ -918,19 +918,6 @@ int ttm_dma_populate(struct ttm_dma_tt *ttm_dma, struct device *dev)
> }
> EXPORT_SYMBOL_GPL(ttm_dma_populate);
>
> -/* Get good estimation how many pages are free in pools */
> -static int ttm_dma_pool_get_num_unused_pages(void)
> -{
> - struct device_pools *p;
> - unsigned total = 0;
> -
> - mutex_lock(&_manager->lock);
> - list_for_each_entry(p, &_manager->pools, pools)
> - total += p->pool->npages_free;
> - mutex_unlock(&_manager->lock);
> - return total;
> -}
> -
> /* Put all pages in pages list to correct pool to wait for reuse */
> void ttm_dma_unpopulate(struct ttm_dma_tt *ttm_dma, struct device *dev)
> {
> @@ -1002,18 +989,29 @@ EXPORT_SYMBOL_GPL(ttm_dma_unpopulate);
>
> /**
> * Callback for mm to request pool to reduce number of page held.
> + *
> + * XXX: (dchinner) Deadlock warning!
> + *
> + * ttm_dma_page_pool_free() does GFP_KERNEL memory allocation, and so attention
> + * needs to be paid to sc->gfp_mask to determine if this can be done or not.
> + * GFP_KERNEL memory allocation in a GFP_ATOMIC reclaim context woul dbe really
> + * bad.
would be.
> + *
> + * I'm getting sadder as I hear more pathetical whimpers about needing per-pool
> + * shrinkers
Were are these whimpers coming from?
> */
> -static int ttm_dma_pool_mm_shrink(struct shrinker *shrink,
> - struct shrink_control *sc)
> +static long
> +ttm_dma_pool_shrink_scan(struct shrinker *shrink, struct shrink_control *sc)
> {
> static atomic_t start_pool = ATOMIC_INIT(0);
> unsigned idx = 0;
> unsigned pool_offset = atomic_add_return(1, &start_pool);
> unsigned shrink_pages = sc->nr_to_scan;
> struct device_pools *p;
> + long freed = 0;
>
> if (list_empty(&_manager->pools))
> - return 0;
> + return SHRINK_STOP;
>
> mutex_lock(&_manager->lock);
> pool_offset = pool_offset % _manager->npools;
> @@ -1029,18 +1027,33 @@ static int ttm_dma_pool_mm_shrink(struct shrinker *shrink,
> continue;
> nr_free = shrink_pages;
> shrink_pages = ttm_dma_page_pool_free(p->pool, nr_free);
> + freed += nr_free - shrink_pages;
> +
> pr_debug("%s: (%s:%d) Asked to shrink %d, have %d more to go\n",
> p->pool->dev_name, p->pool->name, current->pid,
> nr_free, shrink_pages);
> }
> mutex_unlock(&_manager->lock);
> - /* return estimated number of unused pages in pool */
> - return ttm_dma_pool_get_num_unused_pages();
> + return freed;
> +}
That code looks good.
> +
> +static long
> +ttm_dma_pool_shrink_count(struct shrinker *shrink, struct shrink_control *sc)
> +{
> + struct device_pools *p;
> + long count = 0;
> +
> + mutex_lock(&_manager->lock);
> + list_for_each_entry(p, &_manager->pools, pools)
> + count += p->pool->npages_free;
> + mutex_unlock(&_manager->lock);
> + return count;
> }
But this needn't to be moved? Or is it b/c you would like the code to
be in "one section" ?
If so, please use the same style for functions as the rest of the file
has.
>
> static void ttm_dma_pool_mm_shrink_init(struct ttm_pool_manager *manager)
> {
> - manager->mm_shrink.shrink = &ttm_dma_pool_mm_shrink;
> + manager->mm_shrink.count_objects = &ttm_dma_pool_shrink_count;
> + manager->mm_shrink.scan_objects = &ttm_dma_pool_shrink_scan;
> manager->mm_shrink.seeks = 1;
> register_shrinker(&manager->mm_shrink);
> }
.. snip..
> diff --git a/drivers/staging/zcache/zcache-main.c b/drivers/staging/zcache/zcache-main.c
> index dcceed2..4ade8e3 100644
> --- a/drivers/staging/zcache/zcache-main.c
> +++ b/drivers/staging/zcache/zcache-main.c
> @@ -1140,23 +1140,19 @@ static bool zcache_freeze;
> * pageframes in use. FIXME POLICY: Probably the writeback should only occur
> * if the eviction doesn't free enough pages.
> */
> -static int shrink_zcache_memory(struct shrinker *shrink,
> - struct shrink_control *sc)
> +static long scan_zcache_memory(struct shrinker *shrink,
> + struct shrink_control *sc)
> {
> static bool in_progress;
> - int ret = -1;
> - int nr = sc->nr_to_scan;
> int nr_evict = 0;
> int nr_writeback = 0;
> struct page *page;
> int file_pageframes_inuse, anon_pageframes_inuse;
> -
> - if (nr <= 0)
> - goto skip_evict;
> + long freed = 0;
>
> /* don't allow more than one eviction thread at a time */
> if (in_progress)
> - goto skip_evict;
> + return 0;
>
> in_progress = true;
>
> @@ -1176,6 +1172,7 @@ static int shrink_zcache_memory(struct shrinker *shrink,
> if (page == NULL)
> break;
> zcache_free_page(page);
> + freed++;
> }
>
> zcache_last_active_anon_pageframes =
> @@ -1192,13 +1189,22 @@ static int shrink_zcache_memory(struct shrinker *shrink,
> #ifdef CONFIG_ZCACHE_WRITEBACK
> int writeback_ret;
> writeback_ret = zcache_frontswap_writeback();
> - if (writeback_ret == -ENOMEM)
> + if (writeback_ret != -ENOMEM)
> + freed++;
> + else
> #endif
> break;
> }
> in_progress = false;
>
> -skip_evict:
> + return freed;
> +}
> +
> +static long count_zcache_memory(struct shrinker *shrink,
> + struct shrink_control *sc)
> +{
> + int ret = -1;
> +
> /* resample: has changed, but maybe not all the way yet */
> zcache_last_active_file_pageframes =
> global_page_state(NR_LRU_BASE + LRU_ACTIVE_FILE);
> @@ -1212,7 +1218,8 @@ skip_evict:
> }
>
> static struct shrinker zcache_shrinker = {
> - .shrink = shrink_zcache_memory,
> + .scan_objects = scan_zcache_memory,
> + .count_objects = count_zcache_memory,
> .seeks = DEFAULT_SEEKS,
> };
>
That looks OK, but I think it needs an Ack from Greg KH as well?
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
