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

Groups > linux.kernel > #1601893 > unrolled thread

[PATCH 4/4] zram: make deduplication feature optional

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

  [PATCH 4/4] zram: make deduplication feature optional js1304@gmail.com - 2017-03-16 03:50 +0100
    Re: [PATCH 4/4] zram: make deduplication feature optional Minchan Kim <minchan@kernel.org> - 2017-03-22 01:10 +0100
      Re: [PATCH 4/4] zram: make deduplication feature optional Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2017-03-23 04:10 +0100
        Re: [PATCH 4/4] zram: make deduplication feature optional Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-27 10:20 +0200
          Re: [PATCH 4/4] zram: make deduplication feature optional Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2017-03-28 03:10 +0200
            Re: [PATCH 4/4] zram: make deduplication feature optional Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-28 04:40 +0200
              Re: [PATCH 4/4] zram: make deduplication feature optional Minchan Kim <minchan@kernel.org> - 2017-03-28 05:00 +0200
                Re: [PATCH 4/4] zram: make deduplication feature optional Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-03-28 07:20 +0200
                  Re: [PATCH 4/4] zram: make deduplication feature optional Minchan Kim <minchan@kernel.org> - 2017-03-28 08:10 +0200

#1601893 — [PATCH 4/4] zram: make deduplication feature optional

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Benefit of deduplication is dependent on the workload so it's not
preferable to always enable. Therefore, make it optional.

Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 drivers/block/zram/zram_drv.c | 80 ++++++++++++++++++++++++++++++++++++++-----
 drivers/block/zram/zram_drv.h |  1 +
 2 files changed, 73 insertions(+), 8 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 012425f..e45aa9f 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -328,6 +328,39 @@ static ssize_t comp_algorithm_store(struct device *dev,
 	return len;
 }
 
+static ssize_t use_dedup_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	int val;
+	struct zram *zram = dev_to_zram(dev);
+
+	down_read(&zram->init_lock);
+	val = zram->use_dedup;
+	up_read(&zram->init_lock);
+
+	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
+}
+
+static ssize_t use_dedup_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t len)
+{
+	int val;
+	struct zram *zram = dev_to_zram(dev);
+
+	if (kstrtoint(buf, 10, &val) || (val != 0 && val != 1))
+		return -EINVAL;
+
+	down_write(&zram->init_lock);
+	if (init_done(zram)) {
+		up_write(&zram->init_lock);
+		pr_info("Can't change dedup usage for initialized device\n");
+		return -EBUSY;
+	}
+	zram->use_dedup = val;
+	up_write(&zram->init_lock);
+	return len;
+}
+
 static ssize_t compact_store(struct device *dev,
 		struct device_attribute *attr, const char *buf, size_t len)
 {
@@ -422,11 +455,23 @@ static ssize_t debug_stat_show(struct device *dev,
 static DEVICE_ATTR_RO(mm_stat);
 static DEVICE_ATTR_RO(debug_stat);
 
-static u32 zram_calc_checksum(unsigned char *mem)
+static u32 zram_calc_checksum(struct zram *zram, unsigned char *mem)
 {
+	if (!zram->use_dedup)
+		return 0;
+
 	return jhash(mem, PAGE_SIZE, 0);
 }
 
+static unsigned long zram_entry_handle(struct zram *zram,
+				struct zram_entry *entry)
+{
+	if (!zram->use_dedup)
+		return (unsigned long)entry;
+
+	return entry->handle;
+}
+
 static struct zram_entry *zram_entry_alloc(struct zram *zram,
 					unsigned int len, gfp_t flags)
 {
@@ -438,6 +483,9 @@ static struct zram_entry *zram_entry_alloc(struct zram *zram,
 	if (!handle)
 		return NULL;
 
+	if (!zram->use_dedup)
+		return (struct zram_entry *)handle;
+
 	entry = kzalloc(sizeof(*entry), flags);
 	if (!entry) {
 		zs_free(meta->mem_pool, handle);
@@ -462,6 +510,9 @@ static void zram_entry_insert(struct zram *zram, struct zram_entry *new,
 	struct rb_node **rb_node, *parent = NULL;
 	struct zram_entry *entry;
 
+	if (!zram->use_dedup)
+		return;
+
 	new->checksum = checksum;
 	hash = &meta->hash[checksum % meta->hash_size];
 	rb_root = &hash->rb_root;
@@ -492,7 +543,8 @@ static bool zram_entry_match(struct zram *zram, struct zram_entry *entry,
 	struct zram_meta *meta = zram->meta;
 	struct zcomp_strm *zstrm;
 
-	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_RO);
+	cmem = zs_map_object(meta->mem_pool,
+			zram_entry_handle(zram, entry), ZS_MM_RO);
 	if (entry->len == PAGE_SIZE) {
 		match = !memcmp(mem, cmem, PAGE_SIZE);
 	} else {
@@ -501,7 +553,7 @@ static bool zram_entry_match(struct zram *zram, struct zram_entry *entry,
 			match = !memcmp(mem, zstrm->buffer, PAGE_SIZE);
 		zcomp_stream_put(zram->comp);
 	}
-	zs_unmap_object(meta->mem_pool, entry->handle);
+	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
 
 	return match;
 }
@@ -521,6 +573,11 @@ static bool zram_entry_put(struct zram *zram, struct zram_meta *meta,
 	struct zram_hash *hash;
 	u32 checksum;
 
+	if (!zram->use_dedup) {
+		zs_free(meta->mem_pool, zram_entry_handle(zram, entry));
+		return false;
+	}
+
 	if (!populated)
 		goto free;
 
@@ -551,6 +608,9 @@ static struct zram_entry *zram_entry_get(struct zram *zram,
 	struct zram_entry *entry;
 	struct rb_node *rb_node;
 
+	if (!zram->use_dedup)
+		return NULL;
+
 	hash = &meta->hash[checksum % meta->hash_size];
 
 	spin_lock(&hash->lock);
@@ -713,7 +773,8 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 		return 0;
 	}
 
-	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_RO);
+	cmem = zs_map_object(meta->mem_pool,
+			zram_entry_handle(zram, entry), ZS_MM_RO);
 	if (size == PAGE_SIZE) {
 		copy_page(mem, cmem);
 	} else {
@@ -722,7 +783,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 		ret = zcomp_decompress(zstrm, cmem, size, mem);
 		zcomp_stream_put(zram->comp);
 	}
-	zs_unmap_object(meta->mem_pool, entry->handle);
+	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
 	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 
 	/* Should NEVER happen. Return bio error if it does. */
@@ -840,7 +901,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		goto out;
 	}
 
-	checksum = zram_calc_checksum(uncmem);
+	checksum = zram_calc_checksum(zram, uncmem);
 	if (!entry) {
 		entry = zram_entry_get(zram, uncmem, checksum);
 		if (entry) {
@@ -915,7 +976,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		goto out;
 	}
 
-	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_WO);
+	cmem = zs_map_object(meta->mem_pool,
+			zram_entry_handle(zram, entry), ZS_MM_WO);
 
 	if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) {
 		src = kmap_atomic(page);
@@ -927,7 +989,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 
 	zcomp_stream_put(zram->comp);
 	zstrm = NULL;
-	zs_unmap_object(meta->mem_pool, entry->handle);
+	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
 	zram_entry_insert(zram, entry, checksum);
 
 found_duplication:
@@ -1310,6 +1372,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 static DEVICE_ATTR_WO(mem_used_max);
 static DEVICE_ATTR_RW(max_comp_streams);
 static DEVICE_ATTR_RW(comp_algorithm);
+static DEVICE_ATTR_RW(use_dedup);
 
 static struct attribute *zram_disk_attrs[] = {
 	&dev_attr_disksize.attr,
@@ -1320,6 +1383,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 	&dev_attr_mem_used_max.attr,
 	&dev_attr_max_comp_streams.attr,
 	&dev_attr_comp_algorithm.attr,
+	&dev_attr_use_dedup.attr,
 	&dev_attr_io_stat.attr,
 	&dev_attr_mm_stat.attr,
 	&dev_attr_debug_stat.attr,
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 07d1f8d..b823555 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -141,5 +141,6 @@ struct zram {
 	 * zram is claimed so open request will be failed
 	 */
 	bool claim; /* Protected by bdev->bd_mutex */
+	int use_dedup;
 };
 #endif
-- 
1.9.1

[toc] | [next] | [standalone]

#1606108

On Thu, Mar 16, 2017 at 11:46:38AM +0900, js1304@gmail.com wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> 
> Benefit of deduplication is dependent on the workload so it's not
> preferable to always enable. Therefore, make it optional.

Please make it to Kconfig, too. And write down the description to impress
"help a lot for users who uses zram to build output directory"
And the feature should be disabled as default.

> 
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  drivers/block/zram/zram_drv.c | 80 ++++++++++++++++++++++++++++++++++++++-----
>  drivers/block/zram/zram_drv.h |  1 +
>  2 files changed, 73 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 012425f..e45aa9f 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -328,6 +328,39 @@ static ssize_t comp_algorithm_store(struct device *dev,
>  	return len;
>  }
>  
> +static ssize_t use_dedup_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	int val;
> +	struct zram *zram = dev_to_zram(dev);
> +
> +	down_read(&zram->init_lock);
> +	val = zram->use_dedup;
> +	up_read(&zram->init_lock);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
> +}
> +
> +static ssize_t use_dedup_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	int val;
> +	struct zram *zram = dev_to_zram(dev);
> +
> +	if (kstrtoint(buf, 10, &val) || (val != 0 && val != 1))
> +		return -EINVAL;
> +
> +	down_write(&zram->init_lock);
> +	if (init_done(zram)) {
> +		up_write(&zram->init_lock);
> +		pr_info("Can't change dedup usage for initialized device\n");
> +		return -EBUSY;
> +	}
> +	zram->use_dedup = val;
> +	up_write(&zram->init_lock);
> +	return len;
> +}
> +
>  static ssize_t compact_store(struct device *dev,
>  		struct device_attribute *attr, const char *buf, size_t len)
>  {
> @@ -422,11 +455,23 @@ static ssize_t debug_stat_show(struct device *dev,
>  static DEVICE_ATTR_RO(mm_stat);
>  static DEVICE_ATTR_RO(debug_stat);
>  
> -static u32 zram_calc_checksum(unsigned char *mem)
> +static u32 zram_calc_checksum(struct zram *zram, unsigned char *mem)
>  {
> +	if (!zram->use_dedup)
> +		return 0;
> +

Hmm, I don't like this style which every dedup functions have the check
"use_dedup".

Can't we abstract like this?

I want to find more simple way to no need to add the check when new dedup
functions pop up.

bool zram_dedup_check
        if (!zram->dedup)
                return false;

        zram_dedup_checksum
        entry = zram_dedup_get
        if (!entry) {
                zram_dedup_add
                return false;
        }
        ..
        return true;


zram_bvec_write:
        ...
        ...

        if (zram_dedup_match())
                goto found_dup;



>  	return jhash(mem, PAGE_SIZE, 0);
>  }
>  
> +static unsigned long zram_entry_handle(struct zram *zram,
> +				struct zram_entry *entry)
> +{
> +	if (!zram->use_dedup)
> +		return (unsigned long)entry;
> +
> +	return entry->handle;
> +}
> +
>  static struct zram_entry *zram_entry_alloc(struct zram *zram,
>  					unsigned int len, gfp_t flags)
>  {
> @@ -438,6 +483,9 @@ static struct zram_entry *zram_entry_alloc(struct zram *zram,
>  	if (!handle)
>  		return NULL;
>  
> +	if (!zram->use_dedup)
> +		return (struct zram_entry *)handle;
> +
>  	entry = kzalloc(sizeof(*entry), flags);
>  	if (!entry) {
>  		zs_free(meta->mem_pool, handle);
> @@ -462,6 +510,9 @@ static void zram_entry_insert(struct zram *zram, struct zram_entry *new,
>  	struct rb_node **rb_node, *parent = NULL;
>  	struct zram_entry *entry;
>  
> +	if (!zram->use_dedup)
> +		return;
> +
>  	new->checksum = checksum;
>  	hash = &meta->hash[checksum % meta->hash_size];
>  	rb_root = &hash->rb_root;
> @@ -492,7 +543,8 @@ static bool zram_entry_match(struct zram *zram, struct zram_entry *entry,
>  	struct zram_meta *meta = zram->meta;
>  	struct zcomp_strm *zstrm;
>  
> -	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_RO);
> +	cmem = zs_map_object(meta->mem_pool,
> +			zram_entry_handle(zram, entry), ZS_MM_RO);
>  	if (entry->len == PAGE_SIZE) {
>  		match = !memcmp(mem, cmem, PAGE_SIZE);
>  	} else {
> @@ -501,7 +553,7 @@ static bool zram_entry_match(struct zram *zram, struct zram_entry *entry,
>  			match = !memcmp(mem, zstrm->buffer, PAGE_SIZE);
>  		zcomp_stream_put(zram->comp);
>  	}
> -	zs_unmap_object(meta->mem_pool, entry->handle);
> +	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
>  
>  	return match;
>  }
> @@ -521,6 +573,11 @@ static bool zram_entry_put(struct zram *zram, struct zram_meta *meta,
>  	struct zram_hash *hash;
>  	u32 checksum;
>  
> +	if (!zram->use_dedup) {
> +		zs_free(meta->mem_pool, zram_entry_handle(zram, entry));
> +		return false;
> +	}
> +
>  	if (!populated)
>  		goto free;
>  
> @@ -551,6 +608,9 @@ static struct zram_entry *zram_entry_get(struct zram *zram,
>  	struct zram_entry *entry;
>  	struct rb_node *rb_node;
>  
> +	if (!zram->use_dedup)
> +		return NULL;
> +
>  	hash = &meta->hash[checksum % meta->hash_size];
>  
>  	spin_lock(&hash->lock);
> @@ -713,7 +773,8 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  		return 0;
>  	}
>  
> -	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_RO);
> +	cmem = zs_map_object(meta->mem_pool,
> +			zram_entry_handle(zram, entry), ZS_MM_RO);
>  	if (size == PAGE_SIZE) {
>  		copy_page(mem, cmem);
>  	} else {
> @@ -722,7 +783,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  		ret = zcomp_decompress(zstrm, cmem, size, mem);
>  		zcomp_stream_put(zram->comp);
>  	}
> -	zs_unmap_object(meta->mem_pool, entry->handle);
> +	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
>  	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>  
>  	/* Should NEVER happen. Return bio error if it does. */
> @@ -840,7 +901,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		goto out;
>  	}
>  
> -	checksum = zram_calc_checksum(uncmem);
> +	checksum = zram_calc_checksum(zram, uncmem);
>  	if (!entry) {
>  		entry = zram_entry_get(zram, uncmem, checksum);
>  		if (entry) {
> @@ -915,7 +976,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		goto out;
>  	}
>  
> -	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_WO);
> +	cmem = zs_map_object(meta->mem_pool,
> +			zram_entry_handle(zram, entry), ZS_MM_WO);
>  
>  	if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) {
>  		src = kmap_atomic(page);
> @@ -927,7 +989,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  
>  	zcomp_stream_put(zram->comp);
>  	zstrm = NULL;
> -	zs_unmap_object(meta->mem_pool, entry->handle);
> +	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
>  	zram_entry_insert(zram, entry, checksum);
>  
>  found_duplication:
> @@ -1310,6 +1372,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
>  static DEVICE_ATTR_WO(mem_used_max);
>  static DEVICE_ATTR_RW(max_comp_streams);
>  static DEVICE_ATTR_RW(comp_algorithm);
> +static DEVICE_ATTR_RW(use_dedup);
>  
>  static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_disksize.attr,
> @@ -1320,6 +1383,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
>  	&dev_attr_mem_used_max.attr,
>  	&dev_attr_max_comp_streams.attr,
>  	&dev_attr_comp_algorithm.attr,
> +	&dev_attr_use_dedup.attr,
>  	&dev_attr_io_stat.attr,
>  	&dev_attr_mm_stat.attr,
>  	&dev_attr_debug_stat.attr,
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 07d1f8d..b823555 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -141,5 +141,6 @@ struct zram {
>  	 * zram is claimed so open request will be failed
>  	 */
>  	bool claim; /* Protected by bdev->bd_mutex */
> +	int use_dedup;

For binary result, I want to use 'bool'

>  };
>  #endif
> -- 
> 1.9.1
> 

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

#1607146

On Wed, Mar 22, 2017 at 09:00:59AM +0900, Minchan Kim wrote:
> On Thu, Mar 16, 2017 at 11:46:38AM +0900, js1304@gmail.com wrote:
> > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > 
> > Benefit of deduplication is dependent on the workload so it's not
> > preferable to always enable. Therefore, make it optional.
> 
> Please make it to Kconfig, too. And write down the description to impress
> "help a lot for users who uses zram to build output directory"
> And the feature should be disabled as default.

Okay.

> 
> > 
> > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > ---
> >  drivers/block/zram/zram_drv.c | 80 ++++++++++++++++++++++++++++++++++++++-----
> >  drivers/block/zram/zram_drv.h |  1 +
> >  2 files changed, 73 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 012425f..e45aa9f 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -328,6 +328,39 @@ static ssize_t comp_algorithm_store(struct device *dev,
> >  	return len;
> >  }
> >  
> > +static ssize_t use_dedup_show(struct device *dev,
> > +		struct device_attribute *attr, char *buf)
> > +{
> > +	int val;
> > +	struct zram *zram = dev_to_zram(dev);
> > +
> > +	down_read(&zram->init_lock);
> > +	val = zram->use_dedup;
> > +	up_read(&zram->init_lock);
> > +
> > +	return scnprintf(buf, PAGE_SIZE, "%d\n", val);
> > +}
> > +
> > +static ssize_t use_dedup_store(struct device *dev,
> > +		struct device_attribute *attr, const char *buf, size_t len)
> > +{
> > +	int val;
> > +	struct zram *zram = dev_to_zram(dev);
> > +
> > +	if (kstrtoint(buf, 10, &val) || (val != 0 && val != 1))
> > +		return -EINVAL;
> > +
> > +	down_write(&zram->init_lock);
> > +	if (init_done(zram)) {
> > +		up_write(&zram->init_lock);
> > +		pr_info("Can't change dedup usage for initialized device\n");
> > +		return -EBUSY;
> > +	}
> > +	zram->use_dedup = val;
> > +	up_write(&zram->init_lock);
> > +	return len;
> > +}
> > +
> >  static ssize_t compact_store(struct device *dev,
> >  		struct device_attribute *attr, const char *buf, size_t len)
> >  {
> > @@ -422,11 +455,23 @@ static ssize_t debug_stat_show(struct device *dev,
> >  static DEVICE_ATTR_RO(mm_stat);
> >  static DEVICE_ATTR_RO(debug_stat);
> >  
> > -static u32 zram_calc_checksum(unsigned char *mem)
> > +static u32 zram_calc_checksum(struct zram *zram, unsigned char *mem)
> >  {
> > +	if (!zram->use_dedup)
> > +		return 0;
> > +
> 
> Hmm, I don't like this style which every dedup functions have the check
> "use_dedup".
> 
> Can't we abstract like this?

I will try but I'm not sure it can be.

> 
> I want to find more simple way to no need to add the check when new dedup
> functions pop up.
> 
> bool zram_dedup_check
>         if (!zram->dedup)
>                 return false;
> 
>         zram_dedup_checksum
>         entry = zram_dedup_get
>         if (!entry) {
>                 zram_dedup_add
>                 return false;
>         }
>         ..
>         return true;
> 
> 
> zram_bvec_write:
>         ...
>         ...
> 
>         if (zram_dedup_match())
>                 goto found_dup;
> 
> 
> 
> >  	return jhash(mem, PAGE_SIZE, 0);
> >  }
> >  
> > +static unsigned long zram_entry_handle(struct zram *zram,
> > +				struct zram_entry *entry)
> > +{
> > +	if (!zram->use_dedup)
> > +		return (unsigned long)entry;
> > +
> > +	return entry->handle;
> > +}
> > +
> >  static struct zram_entry *zram_entry_alloc(struct zram *zram,
> >  					unsigned int len, gfp_t flags)
> >  {
> > @@ -438,6 +483,9 @@ static struct zram_entry *zram_entry_alloc(struct zram *zram,
> >  	if (!handle)
> >  		return NULL;
> >  
> > +	if (!zram->use_dedup)
> > +		return (struct zram_entry *)handle;
> > +
> >  	entry = kzalloc(sizeof(*entry), flags);
> >  	if (!entry) {
> >  		zs_free(meta->mem_pool, handle);
> > @@ -462,6 +510,9 @@ static void zram_entry_insert(struct zram *zram, struct zram_entry *new,
> >  	struct rb_node **rb_node, *parent = NULL;
> >  	struct zram_entry *entry;
> >  
> > +	if (!zram->use_dedup)
> > +		return;
> > +
> >  	new->checksum = checksum;
> >  	hash = &meta->hash[checksum % meta->hash_size];
> >  	rb_root = &hash->rb_root;
> > @@ -492,7 +543,8 @@ static bool zram_entry_match(struct zram *zram, struct zram_entry *entry,
> >  	struct zram_meta *meta = zram->meta;
> >  	struct zcomp_strm *zstrm;
> >  
> > -	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_RO);
> > +	cmem = zs_map_object(meta->mem_pool,
> > +			zram_entry_handle(zram, entry), ZS_MM_RO);
> >  	if (entry->len == PAGE_SIZE) {
> >  		match = !memcmp(mem, cmem, PAGE_SIZE);
> >  	} else {
> > @@ -501,7 +553,7 @@ static bool zram_entry_match(struct zram *zram, struct zram_entry *entry,
> >  			match = !memcmp(mem, zstrm->buffer, PAGE_SIZE);
> >  		zcomp_stream_put(zram->comp);
> >  	}
> > -	zs_unmap_object(meta->mem_pool, entry->handle);
> > +	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
> >  
> >  	return match;
> >  }
> > @@ -521,6 +573,11 @@ static bool zram_entry_put(struct zram *zram, struct zram_meta *meta,
> >  	struct zram_hash *hash;
> >  	u32 checksum;
> >  
> > +	if (!zram->use_dedup) {
> > +		zs_free(meta->mem_pool, zram_entry_handle(zram, entry));
> > +		return false;
> > +	}
> > +
> >  	if (!populated)
> >  		goto free;
> >  
> > @@ -551,6 +608,9 @@ static struct zram_entry *zram_entry_get(struct zram *zram,
> >  	struct zram_entry *entry;
> >  	struct rb_node *rb_node;
> >  
> > +	if (!zram->use_dedup)
> > +		return NULL;
> > +
> >  	hash = &meta->hash[checksum % meta->hash_size];
> >  
> >  	spin_lock(&hash->lock);
> > @@ -713,7 +773,8 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
> >  		return 0;
> >  	}
> >  
> > -	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_RO);
> > +	cmem = zs_map_object(meta->mem_pool,
> > +			zram_entry_handle(zram, entry), ZS_MM_RO);
> >  	if (size == PAGE_SIZE) {
> >  		copy_page(mem, cmem);
> >  	} else {
> > @@ -722,7 +783,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
> >  		ret = zcomp_decompress(zstrm, cmem, size, mem);
> >  		zcomp_stream_put(zram->comp);
> >  	}
> > -	zs_unmap_object(meta->mem_pool, entry->handle);
> > +	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
> >  	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> >  
> >  	/* Should NEVER happen. Return bio error if it does. */
> > @@ -840,7 +901,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  		goto out;
> >  	}
> >  
> > -	checksum = zram_calc_checksum(uncmem);
> > +	checksum = zram_calc_checksum(zram, uncmem);
> >  	if (!entry) {
> >  		entry = zram_entry_get(zram, uncmem, checksum);
> >  		if (entry) {
> > @@ -915,7 +976,8 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  		goto out;
> >  	}
> >  
> > -	cmem = zs_map_object(meta->mem_pool, entry->handle, ZS_MM_WO);
> > +	cmem = zs_map_object(meta->mem_pool,
> > +			zram_entry_handle(zram, entry), ZS_MM_WO);
> >  
> >  	if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) {
> >  		src = kmap_atomic(page);
> > @@ -927,7 +989,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  
> >  	zcomp_stream_put(zram->comp);
> >  	zstrm = NULL;
> > -	zs_unmap_object(meta->mem_pool, entry->handle);
> > +	zs_unmap_object(meta->mem_pool, zram_entry_handle(zram, entry));
> >  	zram_entry_insert(zram, entry, checksum);
> >  
> >  found_duplication:
> > @@ -1310,6 +1372,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
> >  static DEVICE_ATTR_WO(mem_used_max);
> >  static DEVICE_ATTR_RW(max_comp_streams);
> >  static DEVICE_ATTR_RW(comp_algorithm);
> > +static DEVICE_ATTR_RW(use_dedup);
> >  
> >  static struct attribute *zram_disk_attrs[] = {
> >  	&dev_attr_disksize.attr,
> > @@ -1320,6 +1383,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
> >  	&dev_attr_mem_used_max.attr,
> >  	&dev_attr_max_comp_streams.attr,
> >  	&dev_attr_comp_algorithm.attr,
> > +	&dev_attr_use_dedup.attr,
> >  	&dev_attr_io_stat.attr,
> >  	&dev_attr_mm_stat.attr,
> >  	&dev_attr_debug_stat.attr,
> > diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> > index 07d1f8d..b823555 100644
> > --- a/drivers/block/zram/zram_drv.h
> > +++ b/drivers/block/zram/zram_drv.h
> > @@ -141,5 +141,6 @@ struct zram {
> >  	 * zram is claimed so open request will be failed
> >  	 */
> >  	bool claim; /* Protected by bdev->bd_mutex */
> > +	int use_dedup;
> 
> For binary result, I want to use 'bool'

Okay.

Thanks.

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

#1609561

On (03/23/17 12:05), Joonsoo Kim wrote:
> On Wed, Mar 22, 2017 at 09:00:59AM +0900, Minchan Kim wrote:
> > On Thu, Mar 16, 2017 at 11:46:38AM +0900, js1304@gmail.com wrote:
> > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > 
> > > Benefit of deduplication is dependent on the workload so it's not
> > > preferable to always enable. Therefore, make it optional.
> > 
> > Please make it to Kconfig, too. And write down the description to impress
> > "help a lot for users who uses zram to build output directory"
> > And the feature should be disabled as default.
> 
> Okay.

so I was thinking for a moment -- do we want to keep this
functionality in zram or may be it belongs to allocator (zsmalloc)?
what do you think? just a question.

	-ss

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

#1610221

On Mon, Mar 27, 2017 at 05:11:05PM +0900, Sergey Senozhatsky wrote:
> On (03/23/17 12:05), Joonsoo Kim wrote:
> > On Wed, Mar 22, 2017 at 09:00:59AM +0900, Minchan Kim wrote:
> > > On Thu, Mar 16, 2017 at 11:46:38AM +0900, js1304@gmail.com wrote:
> > > > From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> > > > 
> > > > Benefit of deduplication is dependent on the workload so it's not
> > > > preferable to always enable. Therefore, make it optional.
> > > 
> > > Please make it to Kconfig, too. And write down the description to impress
> > > "help a lot for users who uses zram to build output directory"
> > > And the feature should be disabled as default.
> > 
> > Okay.
> 
> so I was thinking for a moment -- do we want to keep this
> functionality in zram or may be it belongs to allocator (zsmalloc)?
> what do you think? just a question.

I think that zram is more appropriate layer to implement this feature.
I may be wrong so please let me know if I'm missing something.

First, I'd like to leave allocator to just allocator. If it awares the
contents, further improvement would be restricted. For example, we
should use map/unmap semantic to store contents, since, without them,
we can't know when the content is changed and when deduplication check
should be done. I know that zsmalloc is already implemented by that
way but I guess that similar issue could happen in the future.

Second, we always need to compress the page to check duplication
if it is implemented in zsmalloc since we store compressed page to
zsmalloc. I guess that less compression would be better in performance
wise.

Third, in case of zsmalloc dedup, we always need to allocate zs memory
before checking duplication and need to free it if duplication is
found. It's also undesirable.

If you are okay with above arguments, I will send v2 soon.

Thanks.

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

#1610266

Cc Seth and Dan, just in case


Hello Joonsoo,

On (03/28/17 10:02), Joonsoo Kim wrote:
[..]
> > so I was thinking for a moment -- do we want to keep this
> > functionality in zram or may be it belongs to allocator (zsmalloc)?
> > what do you think? just a question.
> 
> I think that zram is more appropriate layer to implement this feature.
> I may be wrong so please let me know if I'm missing something.
> 
> First, I'd like to leave allocator to just allocator. If it awares the
> contents, further improvement would be restricted. For example, we
> should use map/unmap semantic to store contents, since, without them,
> we can't know when the content is changed and when deduplication check
> should be done. I know that zsmalloc is already implemented by that
> way but I guess that similar issue could happen in the future.
> 
> Second, we always need to compress the page to check duplication
> if it is implemented in zsmalloc since we store compressed page to
> zsmalloc. I guess that less compression would be better in performance
> wise.
> 
> Third, in case of zsmalloc dedup, we always need to allocate zs memory
> before checking duplication and need to free it if duplication is
> found. It's also undesirable.
> 
> If you are okay with above arguments, I will send v2 soon.

thanks.
I'm OK with your arguments.


to explain my point a bit further (zsmalloc was a bad call,
I guess I meant zpool):

the reason I asked was that both zram and zswap sort of trying to
have same optimizations - zero filled pages handling, for example.
zram is a bit ahead now (to the best of my knowledge), because of
the recent 'same element' filled pages. zswap, probably, will have
something like this as well some day. or may be it won't, up to Seth
and Dan. de-duplication definitely can improve both zram and zswap,
which, once again, suggests that at some point zswap will have its
own implementation. well, or it won't.

so I though that may be we could have zero filled pages handling/same
element pages handling/de-duplication somewhere in the "middle" layer.
like zpool for instance (zram does not support zpool as of now) so we
could unify things.

just an idea. no pressure.

	-ss

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

#1610271

Hi Sergey,

On Tue, Mar 28, 2017 at 11:22:45AM +0900, Sergey Senozhatsky wrote:
> Cc Seth and Dan, just in case
> 
> 
> Hello Joonsoo,
> 
> On (03/28/17 10:02), Joonsoo Kim wrote:
> [..]
> > > so I was thinking for a moment -- do we want to keep this
> > > functionality in zram or may be it belongs to allocator (zsmalloc)?
> > > what do you think? just a question.
> > 
> > I think that zram is more appropriate layer to implement this feature.
> > I may be wrong so please let me know if I'm missing something.
> > 
> > First, I'd like to leave allocator to just allocator. If it awares the
> > contents, further improvement would be restricted. For example, we
> > should use map/unmap semantic to store contents, since, without them,
> > we can't know when the content is changed and when deduplication check
> > should be done. I know that zsmalloc is already implemented by that
> > way but I guess that similar issue could happen in the future.
> > 
> > Second, we always need to compress the page to check duplication
> > if it is implemented in zsmalloc since we store compressed page to
> > zsmalloc. I guess that less compression would be better in performance
> > wise.
> > 
> > Third, in case of zsmalloc dedup, we always need to allocate zs memory
> > before checking duplication and need to free it if duplication is
> > found. It's also undesirable.
> > 
> > If you are okay with above arguments, I will send v2 soon.
> 
> thanks.
> I'm OK with your arguments.
> 
> 
> to explain my point a bit further (zsmalloc was a bad call,
> I guess I meant zpool):
> 
> the reason I asked was that both zram and zswap sort of trying to
> have same optimizations - zero filled pages handling, for example.
> zram is a bit ahead now (to the best of my knowledge), because of
> the recent 'same element' filled pages. zswap, probably, will have
> something like this as well some day. or may be it won't, up to Seth
> and Dan. de-duplication definitely can improve both zram and zswap,
> which, once again, suggests that at some point zswap will have its
> own implementation. well, or it won't.

As I pointed out, at least, dedup was no benefit for the swap case.
I don't want to disrupt zsmalloc without any *proved* benefit.
Even though it *might* have benefit, it shouldn't be in allocator
layer unless it's really huge benefit like performance.
It makes hard zram's allocator change in future.
And please consider zswap is born for the latency in server workload
while zram is memory efficiency in embedded world.
dedup feature is trade-off for them and zram is perfectly matched.

> 
> so I though that may be we could have zero filled pages handling/same
> element pages handling/de-duplication somewhere in the "middle" layer.
> like zpool for instance (zram does not support zpool as of now) so we
> could unify things.
> 
> just an idea. no pressure.
> 
> 	-ss

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

#1610298

Hello Minchan,

On (03/28/17 11:50), Minchan Kim wrote:
[..]
> > the reason I asked was that both zram and zswap sort of trying to
> > have same optimizations - zero filled pages handling, for example.
> > zram is a bit ahead now (to the best of my knowledge), because of
> > the recent 'same element' filled pages. zswap, probably, will have
> > something like this as well some day. or may be it won't, up to Seth
> > and Dan. de-duplication definitely can improve both zram and zswap,
> > which, once again, suggests that at some point zswap will have its
> > own implementation. well, or it won't.
> 
> As I pointed out, at least, dedup was no benefit for the swap case.
> I don't want to disrupt zsmalloc without any *proved* benefit.
> Even though it *might* have benefit, it shouldn't be in allocator
> layer unless it's really huge benefit like performance.

sure.

zpool, I meant zpool. I mistakenly used the word 'allocator'.

I meant some intermediate layer between zram and actual memory allocator,
a common layer which both zram and zswap can use and which can have
common functionality. just an idea. haven't really thought about it yet.

> It makes hard zram's allocator change in future.
> And please consider zswap is born for the latency in server workload
> while zram is memory efficiency in embedded world.

may be. I do suspect zswap is used in embedded as well [1]. there is even
a brand new allocator that 'reportedly' uses less memory than zsmalloc
and outperforms zsmalloc in embedded setups [1] (once again, reportedly.
I haven't tried it).

if z3fold is actually this good (I'm not saying it is not, haven't
tested it), then it makes sense to switch to zpool API in zram and let
zram users to select the allocator that fits their setups better.

just saying.


[1] http://events.linuxfoundation.org/sites/events/files/slides/zram1.pdf

	-ss

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

#1610350

On Tue, Mar 28, 2017 at 02:12:04PM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> On (03/28/17 11:50), Minchan Kim wrote:
> [..]
> > > the reason I asked was that both zram and zswap sort of trying to
> > > have same optimizations - zero filled pages handling, for example.
> > > zram is a bit ahead now (to the best of my knowledge), because of
> > > the recent 'same element' filled pages. zswap, probably, will have
> > > something like this as well some day. or may be it won't, up to Seth
> > > and Dan. de-duplication definitely can improve both zram and zswap,
> > > which, once again, suggests that at some point zswap will have its
> > > own implementation. well, or it won't.
> > 
> > As I pointed out, at least, dedup was no benefit for the swap case.
> > I don't want to disrupt zsmalloc without any *proved* benefit.
> > Even though it *might* have benefit, it shouldn't be in allocator
> > layer unless it's really huge benefit like performance.
> 
> sure.
> 
> zpool, I meant zpool. I mistakenly used the word 'allocator'.
> 
> I meant some intermediate layer between zram and actual memory allocator,
> a common layer which both zram and zswap can use and which can have
> common functionality. just an idea. haven't really thought about it yet.
> 
> > It makes hard zram's allocator change in future.
> > And please consider zswap is born for the latency in server workload
> > while zram is memory efficiency in embedded world.
> 
> may be. I do suspect zswap is used in embedded as well [1]. there is even
> a brand new allocator that 'reportedly' uses less memory than zsmalloc
> and outperforms zsmalloc in embedded setups [1] (once again, reportedly.
> I haven't tried it).
> 
> if z3fold is actually this good (I'm not saying it is not, haven't
> tested it), then it makes sense to switch to zpool API in zram and let
> zram users to select the allocator that fits their setups better.
> 
> just saying.
> 
> 
> [1] http://events.linuxfoundation.org/sites/events/files/slides/zram1.pdf

I do not want to support multiple allocators in zram.
It's really maintainance headache as well as making zram's goal float.
If new allocator *saves* much memory compared to zsmalloc, it might be
good candidate for replacing zsmalloc.

If so, feel free to send patches with test workload without *any noise*.
Please, do not tell "it's good" with just simple test. What we need is
"why it's good" so that we can investigate what is current problem and
if it is caused by zsmalloc's design so it's hard to change, then
we might think of new allocator seriously.

Anyway, it's off-topic with Joonsoo's patch.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web