[PATCH v2] zram: better utilization of zram swap space

[PATCH v2] zram: better utilization of zram swap space

[Gopi Sai Teja]

75% of the PAGE_SIZE is not a correct threshold to store uncompressed
pages in zs_page as this must be changed if the maximum pages stored
in zspage changes. Instead using zs classes, we can set the correct
threshold irrespective of the maximum pages stored in zspage.

Tested on ARM:

Before Patch:
class  size  obj_allocated   obj_used pages_used
....
  190  3072           6744       6724       5058
  202  3264             90         87         72
  254  4096          11886      11886      11886

Total               123251     120511      55076

After Patch:
class  size  obj_allocated   obj_used pages_used
...
  190  3072           6368       6326       4776
  202  3264           2205       2197       1764
  254  4096          12624      12624      12624

Total               125655     122045      56541

Signed-off-by: Gopi Sai Teja <gopi.st@samsung.com>
---
v1 -> v2: Earlier, threshold to store uncompressed page is set
to 80% of PAGE_SIZE and now zsmalloc classes is used to set the
threshold.

 drivers/block/zram/zram_drv.c |  2 +-
 include/linux/zsmalloc.h      |  1 +
 mm/zsmalloc.c                 | 13 +++++++++++++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index d70eba3..dda0ef8 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -965,7 +965,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
 		return ret;
 	}
 
-	if (unlikely(comp_len > max_zpage_size)) {
+	if (unlikely(comp_len > zs_max_zpage_size(zram->mem_pool))) {
 		if (zram_wb_enabled(zram) && allow_wb) {
 			zcomp_stream_put(zram->comp);
 			ret = write_to_bdev(zram, bvec, index, bio, &element);
diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h
index 57a8e98..0b09aa5 100644
--- a/include/linux/zsmalloc.h
+++ b/include/linux/zsmalloc.h
@@ -54,5 +54,6 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
 unsigned long zs_get_total_pages(struct zs_pool *pool);
 unsigned long zs_compact(struct zs_pool *pool);
 
+unsigned int zs_max_zpage_size(struct zs_pool *pool);
 void zs_pool_stats(struct zs_pool *pool, struct zs_pool_stats *stats);
 #endif
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 685049a..5b434ab 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -261,6 +261,7 @@ struct zs_pool {
 	 * and unregister_shrinker() will not Oops.
 	 */
 	bool shrinker_enabled;
+	unsigned short max_zpage_size;
 #ifdef CONFIG_ZSMALLOC_STAT
 	struct dentry *stat_dentry;
 #endif
@@ -318,6 +319,11 @@ static void init_deferred_free(struct zs_pool *pool) {}
 static void SetZsPageMovable(struct zs_pool *pool, struct zspage *zspage) {}
 #endif
 
+unsigned int zs_max_zpage_size(struct zs_pool *pool)
+{
+	return pool->max_zpage_size;
+}
+
 static int create_cache(struct zs_pool *pool)
 {
 	pool->handle_cachep = kmem_cache_create("zs_handle", ZS_HANDLE_SIZE,
@@ -2368,6 +2374,8 @@ struct zs_pool *zs_create_pool(const char *name)
 	if (create_cache(pool))
 		goto err;
 
+	pool->max_zpage_size = 0;
+
 	/*
 	 * Iterate reversely, because, size of size_class that we want to use
 	 * for merging should be larger or equal to current size.
@@ -2411,6 +2419,11 @@ struct zs_pool *zs_create_pool(const char *name)
 		class->objs_per_zspage = objs_per_zspage;
 		spin_lock_init(&class->lock);
 		pool->size_class[i] = class;
+
+		if (!pool->max_zpage_size &&
+				pages_per_zspage < objs_per_zspage)
+			pool->max_zpage_size = class->size - ZS_HANDLE_SIZE;
+
 		for (fullness = ZS_EMPTY; fullness < NR_ZS_FULLNESS;
 							fullness++)
 			INIT_LIST_HEAD(&class->fullness_list[fullness]);
-- 
1.9.1

Re: [PATCH v2] zram: better utilization of zram swap space

[Minchan Kim]

Hello,

On Fri, Dec 22, 2017 at 04:00:06PM +0530, Gopi Sai Teja wrote:
> 75% of the PAGE_SIZE is not a correct threshold to store uncompressed

Please describe it in detail that why current threshold is bad in that
memory efficiency point of view.

> pages in zs_page as this must be changed if the maximum pages stored
> in zspage changes. Instead using zs classes, we can set the correct

Also, let's include the pharase Sergey pointed out in this description.

It's not a good idea that zram need to know allocator's implementation
with harded value like 75%.

> threshold irrespective of the maximum pages stored in zspage.
> 
> Tested on ARM:
> 
> Before Patch:
> class  size  obj_allocated   obj_used pages_used
> ....
>   190  3072           6744       6724       5058
>   202  3264             90         87         72
>   254  4096          11886      11886      11886
> 
> Total               123251     120511      55076
> 
> After Patch:
> class  size  obj_allocated   obj_used pages_used
> ...
>   190  3072           6368       6326       4776
>   202  3264           2205       2197       1764
>   254  4096          12624      12624      12624
> 
> Total               125655     122045      56541
> 
> Signed-off-by: Gopi Sai Teja <gopi.st@samsung.com>
> ---
> v1 -> v2: Earlier, threshold to store uncompressed page is set
> to 80% of PAGE_SIZE and now zsmalloc classes is used to set the
> threshold.
> 
>  drivers/block/zram/zram_drv.c |  2 +-
>  include/linux/zsmalloc.h      |  1 +
>  mm/zsmalloc.c                 | 13 +++++++++++++
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index d70eba3..dda0ef8 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -965,7 +965,7 @@ static int __zram_bvec_write(struct zram *zram, struct bio_vec *bvec,
>  		return ret;
>  	}
>  
> -	if (unlikely(comp_len > max_zpage_size)) {
> +	if (unlikely(comp_len > zs_max_zpage_size(zram->mem_pool))) {
>  		if (zram_wb_enabled(zram) && allow_wb) {
>  			zcomp_stream_put(zram->comp);
>  			ret = write_to_bdev(zram, bvec, index, bio, &element);
> diff --git a/include/linux/zsmalloc.h b/include/linux/zsmalloc.h
> index 57a8e98..0b09aa5 100644
> --- a/include/linux/zsmalloc.h
> +++ b/include/linux/zsmalloc.h
> @@ -54,5 +54,6 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,
>  unsigned long zs_get_total_pages(struct zs_pool *pool);
>  unsigned long zs_compact(struct zs_pool *pool);
>  
> +unsigned int zs_max_zpage_size(struct zs_pool *pool);
>  void zs_pool_stats(struct zs_pool *pool, struct zs_pool_stats *stats);
>  #endif
> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
> index 685049a..5b434ab 100644
> --- a/mm/zsmalloc.c
> +++ b/mm/zsmalloc.c
> @@ -261,6 +261,7 @@ struct zs_pool {
>  	 * and unregister_shrinker() will not Oops.
>  	 */
>  	bool shrinker_enabled;
> +	unsigned short max_zpage_size;
>  #ifdef CONFIG_ZSMALLOC_STAT
>  	struct dentry *stat_dentry;
>  #endif
> @@ -318,6 +319,11 @@ static void init_deferred_free(struct zs_pool *pool) {}
>  static void SetZsPageMovable(struct zs_pool *pool, struct zspage *zspage) {}
>  #endif
>  

It seems zs_max_zpage_size is rather confusing although I suggested it.
I couldn't think better name at that time and it's true, still.
Why it's bad is that user can consider it as max size zsmalloc could
store although real meaning is it's the max size allocator can store
for saving memory.

If we cannot think of better name, we should add description in the
head of the function.

/*
 * It returns the max size allocator can store for saving memory.
 * In fact, zsmalloc can store up to ZS_MAX_ALLOC_SIZE but
 * [zs_max_zpage_size, ZA_MAX_ALLOC_SIZE] are pointless for
 * memory saving point of view due to implementation detail.
 */

> +unsigned int zs_max_zpage_size(struct zs_pool *pool)
> +{
> +	return pool->max_zpage_size;

In zsmalloc, we can return hard-coded value instead of variable of pool.
Every instance of zs_pool has same max_zpage_size at this moment so
I don't think we need to introduce new variable of zs_pool.

Thanks.

> +}
> +
>  static int create_cache(struct zs_pool *pool)
>  {
>  	pool->handle_cachep = kmem_cache_create("zs_handle", ZS_HANDLE_SIZE,
> @@ -2368,6 +2374,8 @@ struct zs_pool *zs_create_pool(const char *name)
>  	if (create_cache(pool))
>  		goto err;
>  
> +	pool->max_zpage_size = 0;
> +
>  	/*
>  	 * Iterate reversely, because, size of size_class that we want to use
>  	 * for merging should be larger or equal to current size.
> @@ -2411,6 +2419,11 @@ struct zs_pool *zs_create_pool(const char *name)
>  		class->objs_per_zspage = objs_per_zspage;
>  		spin_lock_init(&class->lock);
>  		pool->size_class[i] = class;
> +
> +		if (!pool->max_zpage_size &&
> +				pages_per_zspage < objs_per_zspage)
> +			pool->max_zpage_size = class->size - ZS_HANDLE_SIZE;
> +
>  		for (fullness = ZS_EMPTY; fullness < NR_ZS_FULLNESS;
>  							fullness++)
>  			INIT_LIST_HEAD(&class->fullness_list[fullness]);
> -- 
> 1.9.1
> 

Re: [PATCH v2] zram: better utilization of zram swap space

[Sergey Senozhatsky]

On (12/27/17 15:29), Minchan Kim wrote:
> On Fri, Dec 22, 2017 at 04:00:06PM +0530, Gopi Sai Teja wrote:
> > 75% of the PAGE_SIZE is not a correct threshold to store uncompressed
> 
> Please describe it in detail that why current threshold is bad in that
> memory efficiency point of view.
> 
> > pages in zs_page as this must be changed if the maximum pages stored
> > in zspage changes. Instead using zs classes, we can set the correct
> 
> Also, let's include the pharase Sergey pointed out in this description.
> 
> It's not a good idea that zram need to know allocator's implementation
> with harded value like 75%.

so I don't like that, basically, my work and my findings are
now submitted by someone else without even crediting my work.
not to mention that I like my commit message much better.

	-ss

Re: [PATCH v2] zram: better utilization of zram swap space

[Minchan Kim]

On Wed, Dec 27, 2017 at 04:10:56PM +0900, Sergey Senozhatsky wrote:
> On (12/27/17 15:29), Minchan Kim wrote:
> > On Fri, Dec 22, 2017 at 04:00:06PM +0530, Gopi Sai Teja wrote:
> > > 75% of the PAGE_SIZE is not a correct threshold to store uncompressed
> > 
> > Please describe it in detail that why current threshold is bad in that
> > memory efficiency point of view.
> > 
> > > pages in zs_page as this must be changed if the maximum pages stored
> > > in zspage changes. Instead using zs classes, we can set the correct
> > 
> > Also, let's include the pharase Sergey pointed out in this description.
> > 
> > It's not a good idea that zram need to know allocator's implementation
> > with harded value like 75%.
> 
> so I don't like that, basically, my work and my findings are
> now submitted by someone else without even crediting my work.
> not to mention that I like my commit message much better.
> 
> 	-ss

Gopi Sai Teja, please discuss with Sergey about patch credit.

Re: [PATCH v2] zram: better utilization of zram swap space

[Minchan Kim]

On Thu, Dec 28, 2017 at 09:00:04AM +0900, Minchan Kim wrote:
> On Wed, Dec 27, 2017 at 04:10:56PM +0900, Sergey Senozhatsky wrote:
> > On (12/27/17 15:29), Minchan Kim wrote:
> > > On Fri, Dec 22, 2017 at 04:00:06PM +0530, Gopi Sai Teja wrote:
> > > > 75% of the PAGE_SIZE is not a correct threshold to store uncompressed
> > > 
> > > Please describe it in detail that why current threshold is bad in that
> > > memory efficiency point of view.
> > > 
> > > > pages in zs_page as this must be changed if the maximum pages stored
> > > > in zspage changes. Instead using zs classes, we can set the correct
> > > 
> > > Also, let's include the pharase Sergey pointed out in this description.
> > > 
> > > It's not a good idea that zram need to know allocator's implementation
> > > with harded value like 75%.
> > 
> > so I don't like that, basically, my work and my findings are
> > now submitted by someone else without even crediting my work.
> > not to mention that I like my commit message much better.
> > 
> > 	-ss
> 
> Gopi Sai Teja, please discuss with Sergey about patch credit.

Hi Gopi Sai Teja,

Now I read previous thread at v1 carefully, I found Sergey already
sent a patch long time ago which is almost same one I suggested.
And he told he will send a patch soon so I want to wait his patch.
We are approaching rc6 now so it's not urgent.

Sergey, sorry for missing your patch at that time.
Could you resend your patch when you have a time? Please think
over better name of the function "zs_get_huge_class_size_watermark"

Thanks.

Re: [PATCH v2] zram: better utilization of zram swap space

[Sergey Senozhatsky]

Hello,

On (12/29/17 16:26), Minchan Kim wrote:
[..]
> > Gopi Sai Teja, please discuss with Sergey about patch credit.
> 
> Hi Gopi Sai Teja,
> 
> Now I read previous thread at v1 carefully, I found Sergey already
> sent a patch long time ago which is almost same one I suggested.
> And he told he will send a patch soon so I want to wait his patch.
> We are approaching rc6 now so it's not urgent.

Thanks, Minchan.

Sorry for the noise and confusion. I was going to send it out some
time ago, but got interrupted.

> Sergey, sorry for missing your patch at that time.
> Could you resend your patch when you have a time? Please think
> over better name of the function "zs_get_huge_class_size_watermark"

sure, will take a look.

thanks,

	-ss