Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1356963 > unrolled thread

Re: [RFC][PATCH v3 2/5] mm/zsmalloc: remove shrinker compaction callbacks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC][PATCH v3 2/5] mm/zsmalloc: remove shrinker compaction  callbacks Minchan Kim <minchan@kernel.org> - 2016-03-14 07:40 +0100
    Re: [RFC][PATCH v3 2/5] mm/zsmalloc: remove shrinker compaction  callbacks Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-03-14 08:50 +0100
      Re: [RFC][PATCH v3 2/5] mm/zsmalloc: remove shrinker compaction  callbacks Minchan Kim <minchan@kernel.org> - 2016-03-15 02:00 +0100
        Re: [RFC][PATCH v3 2/5] mm/zsmalloc: remove shrinker compaction  callbacks Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-03-15 02:10 +0100
          Re: [RFC][PATCH v3 2/5] mm/zsmalloc: remove shrinker compaction  callbacks Minchan Kim <minchan@kernel.org> - 2016-03-15 03:20 +0100

#1356963 — Re: [RFC][PATCH v3 2/5] mm/zsmalloc: remove shrinker compaction callbacks

On Thu, Mar 03, 2016 at 11:46:00PM +0900, Sergey Senozhatsky wrote:
> Do not register shrinker compaction callbacks anymore, since
> now we shedule class compaction work each time its fragmentation
> value goes above the watermark.

I suggested to remove shrinker compaction but while I review your
first patch in this thread, I thought we need upper-bound to
compact zspage so background work can bail out for latency easily.
IOW, the work could give up the job. In such case, we might need
fall-back scheme to continue the job. And I think that could be
a shrinker.

What do you think?

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---
>  mm/zsmalloc.c | 72 -----------------------------------------------------------
>  1 file changed, 72 deletions(-)
> 
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index a4ef7e7..0bb060f 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -256,13 +256,6 @@ struct zs_pool {
>  
>  	struct zs_pool_stats stats;
>  
> -	/* Compact classes */
> -	struct shrinker shrinker;
> -	/*
> -	 * To signify that register_shrinker() was successful
> -	 * and unregister_shrinker() will not Oops.
> -	 */
> -	bool shrinker_enabled;
>  #ifdef CONFIG_ZSMALLOC_STAT
>  	struct dentry *stat_dentry;
>  #endif
> @@ -1848,64 +1841,6 @@ void zs_pool_stats(struct zs_pool *pool, struct zs_pool_stats *stats)
>  }
>  EXPORT_SYMBOL_GPL(zs_pool_stats);
>  
> -static unsigned long zs_shrinker_scan(struct shrinker *shrinker,
> -		struct shrink_control *sc)
> -{
> -	unsigned long pages_freed;
> -	struct zs_pool *pool = container_of(shrinker, struct zs_pool,
> -			shrinker);
> -
> -	pages_freed = pool->stats.pages_compacted;
> -	/*
> -	 * Compact classes and calculate compaction delta.
> -	 * Can run concurrently with a manually triggered
> -	 * (by user) compaction.
> -	 */
> -	pages_freed = zs_compact(pool) - pages_freed;
> -
> -	return pages_freed ? pages_freed : SHRINK_STOP;
> -}
> -
> -static unsigned long zs_shrinker_count(struct shrinker *shrinker,
> -		struct shrink_control *sc)
> -{
> -	int i;
> -	struct size_class *class;
> -	unsigned long pages_to_free = 0;
> -	struct zs_pool *pool = container_of(shrinker, struct zs_pool,
> -			shrinker);
> -
> -	for (i = zs_size_classes - 1; i >= 0; i--) {
> -		class = pool->size_class[i];
> -		if (!class)
> -			continue;
> -		if (class->index != i)
> -			continue;
> -
> -		pages_to_free += zs_can_compact(class);
> -	}
> -
> -	return pages_to_free;
> -}
> -
> -static void zs_unregister_shrinker(struct zs_pool *pool)
> -{
> -	if (pool->shrinker_enabled) {
> -		unregister_shrinker(&pool->shrinker);
> -		pool->shrinker_enabled = false;
> -	}
> -}
> -
> -static int zs_register_shrinker(struct zs_pool *pool)
> -{
> -	pool->shrinker.scan_objects = zs_shrinker_scan;
> -	pool->shrinker.count_objects = zs_shrinker_count;
> -	pool->shrinker.batch = 0;
> -	pool->shrinker.seeks = DEFAULT_SEEKS;
> -
> -	return register_shrinker(&pool->shrinker);
> -}
> -
>  /**
>   * zs_create_pool - Creates an allocation pool to work from.
>   * @flags: allocation flags used to allocate pool metadata
> @@ -1994,12 +1929,6 @@ struct zs_pool *zs_create_pool(const char *name, gfp_t flags)
>  	if (zs_pool_stat_create(name, pool))
>  		goto err;
>  
> -	/*
> -	 * Not critical, we still can use the pool
> -	 * and user can trigger compaction manually.
> -	 */
> -	if (zs_register_shrinker(pool) == 0)
> -		pool->shrinker_enabled = true;
>  	return pool;
>  
>  err:
> @@ -2012,7 +1941,6 @@ void zs_destroy_pool(struct zs_pool *pool)
>  {
>  	int i;
>  
> -	zs_unregister_shrinker(pool);
>  	zs_pool_stat_destroy(pool);
>  
>  	for (i = 0; i < zs_size_classes; i++) {
> -- 
> 2.8.0.rc0
> 

[toc] | [next] | [standalone]

#1357017

On (03/14/16 15:32), Minchan Kim wrote:
> On Thu, Mar 03, 2016 at 11:46:00PM +0900, Sergey Senozhatsky wrote:
> > Do not register shrinker compaction callbacks anymore, since
> > now we shedule class compaction work each time its fragmentation
> > value goes above the watermark.
> 
> I suggested to remove shrinker compaction but while I review your
> first patch in this thread, I thought we need upper-bound to
> compact zspage so background work can bail out for latency easily.
> IOW, the work could give up the job. In such case, we might need
> fall-back scheme to continue the job. And I think that could be
> a shrinker.
> 
> What do you think?

wouldn't this unnecessarily complicate the whole thing? we would
have
 a) a compaction that can be triggered by used space
 b) a compaction from zs_free() that can bail out
 c) a compaction triggered by the shrinker.

all 3 three can run simultaneously.


_if_ we can keep every class below its watermark, we can reduce the
need of "c)".

	-ss

[toc] | [prev] | [next] | [standalone]

#1357725

On Mon, Mar 14, 2016 at 04:45:23PM +0900, Sergey Senozhatsky wrote:
> On (03/14/16 15:32), Minchan Kim wrote:
> > On Thu, Mar 03, 2016 at 11:46:00PM +0900, Sergey Senozhatsky wrote:
> > > Do not register shrinker compaction callbacks anymore, since
> > > now we shedule class compaction work each time its fragmentation
> > > value goes above the watermark.
> > 
> > I suggested to remove shrinker compaction but while I review your
> > first patch in this thread, I thought we need upper-bound to
> > compact zspage so background work can bail out for latency easily.
> > IOW, the work could give up the job. In such case, we might need
> > fall-back scheme to continue the job. And I think that could be
> > a shrinker.
> > 
> > What do you think?
> 
> wouldn't this unnecessarily complicate the whole thing? we would
> have
>  a) a compaction that can be triggered by used space

Maybe, user space? :)

>  b) a compaction from zs_free() that can bail out
>  c) a compaction triggered by the shrinker.
> 
> all 3 three can run simultaneously.

Yeb.

> 
> 
> _if_ we can keep every class below its watermark, we can reduce the
> need of "c)".

But the problem is timing. We cannot guarantee when background
compaction triggers while shrinker is interop with VM so we should
do the job instantly for the system.


> 
> 	-ss

[toc] | [prev] | [next] | [standalone]

#1357729

On (03/15/16 09:52), Minchan Kim wrote:
[..]
> > > I suggested to remove shrinker compaction but while I review your
> > > first patch in this thread, I thought we need upper-bound to
> > > compact zspage so background work can bail out for latency easily.
> > > IOW, the work could give up the job. In such case, we might need
> > > fall-back scheme to continue the job. And I think that could be
> > > a shrinker.
> > > 
> > > What do you think?
> > 
> > wouldn't this unnecessarily complicate the whole thing? we would
> > have
> >  a) a compaction that can be triggered by used space
> 
> Maybe, user space? :)

haha, yes!  sorry, I do quite a lot of typos.

> >  b) a compaction from zs_free() that can bail out
> >  c) a compaction triggered by the shrinker.
> > 
> > all 3 three can run simultaneously.
> 
> Yeb.
> 
> > 
> > 
> > _if_ we can keep every class below its watermark, we can reduce the
> > need of "c)".
> 
> But the problem is timing. We cannot guarantee when background
> compaction triggers while shrinker is interop with VM so we should
> do the job instantly for the system.

we can have pool's compaction-kthread that we will wake_up()
every time we need a compaction, with no dependency on workqueue
or shrinker.

	-ss

[toc] | [prev] | [next] | [standalone]

#1357754

On Tue, Mar 15, 2016 at 10:05:42AM +0900, Sergey Senozhatsky wrote:
> On (03/15/16 09:52), Minchan Kim wrote:
> [..]
> > > > I suggested to remove shrinker compaction but while I review your
> > > > first patch in this thread, I thought we need upper-bound to
> > > > compact zspage so background work can bail out for latency easily.
> > > > IOW, the work could give up the job. In such case, we might need
> > > > fall-back scheme to continue the job. And I think that could be
> > > > a shrinker.
> > > > 
> > > > What do you think?
> > > 
> > > wouldn't this unnecessarily complicate the whole thing? we would
> > > have
> > >  a) a compaction that can be triggered by used space
> > 
> > Maybe, user space? :)
> 
> haha, yes!  sorry, I do quite a lot of typos.
> 
> > >  b) a compaction from zs_free() that can bail out
> > >  c) a compaction triggered by the shrinker.
> > > 
> > > all 3 three can run simultaneously.
> > 
> > Yeb.
> > 
> > > 
> > > 
> > > _if_ we can keep every class below its watermark, we can reduce the
> > > need of "c)".
> > 
> > But the problem is timing. We cannot guarantee when background
> > compaction triggers while shrinker is interop with VM so we should
> > do the job instantly for the system.
> 
> we can have pool's compaction-kthread that we will wake_up()
> every time we need a compaction, with no dependency on workqueue
> or shrinker.

Hmm, I don't think it can work either because wake_up doesn't
guarantee instant execution of the thread.
I think it would be better to have both direct/background
compaction.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web