[PATCH] mm: extend zero pages to same element pages for zram

[PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

From: zhouxianrong <zhouxianrong@huawei.com>

the idea is that without doing more calculations we extend zero pages
to same element pages for zram. zero page is special case of
same element page with zero element.

1. the test is done under android 7.0
2. startup too many applications circularly
3. sample the zero pages, same pages (none-zero element) 
   and total pages in function page_zero_filled

the result is listed as below:

ZERO	SAME	TOTAL
36214	17842	598196

		ZERO/TOTAL	 SAME/TOTAL	  (ZERO+SAME)/TOTAL ZERO/SAME
AVERAGE	0.060631909	 0.024990816  0.085622726		2.663825038
STDEV	0.00674612	 0.005887625  0.009707034		2.115881328
MAX		0.069698422	 0.030046087  0.094975336		7.56043956
MIN		0.03959586	 0.007332205  0.056055193		1.928985507

from above data, the benefit is about 2.5% and up to 3% of total 
swapout pages.

the defect of the patch is that when we recovery a page from 
non-zero element the operations are low efficient for partial
read.

Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
---
 drivers/block/zram/zram_drv.c |  102 +++++++++++++++++++++++++++++++----------
 drivers/block/zram/zram_drv.h |   11 +++--
 2 files changed, 84 insertions(+), 29 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 15f58ab..3250a8b 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -94,6 +94,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
 	meta->table[index].value &= ~BIT(flag);
 }
 
+static inline void zram_set_element(struct zram_meta *meta, u32 index,
+			unsigned long element)
+{
+	meta->table[index].element = element;
+}
+
+static inline void zram_clear_element(struct zram_meta *meta, u32 index)
+{
+	meta->table[index].element = 0;
+}
+
 static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
 {
 	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
@@ -158,31 +169,68 @@ static inline void update_used_max(struct zram *zram,
 	} while (old_max != cur_max);
 }
 
-static bool page_zero_filled(void *ptr)
+static inline void zram_set_page(char *ptr, unsigned long value)
+{
+	int i;
+	unsigned long *page = (unsigned long *)ptr;
+
+	for (i = PAGE_SIZE / sizeof(unsigned long) - 1; i >= 0; i--)
+		page[i] = value;
+}
+
+static inline void zram_set_page_partial(char *ptr, unsigned int size,
+		unsigned long value)
+{
+	int i;
+
+	i = ((unsigned long)ptr) % sizeof(unsigned long);
+	if (i) {
+		while (i < sizeof(unsigned long)) {
+			*ptr++ = (value >> (i * 8)) & 0xff;
+			--size;
+			++i;
+		}
+	}
+
+	for (i = size / sizeof(unsigned long); i > 0; --i) {
+		*(unsigned long *)ptr = value;
+		ptr += sizeof(unsigned long);
+		size -= sizeof(unsigned long);
+	}
+
+	for (i = 0; i < size; ++i)
+		*ptr++ = (value >> (i * 8)) & 0xff;
+}
+
+static bool page_same_filled(void *ptr, unsigned long *element)
 {
 	unsigned int pos;
 	unsigned long *page;
 
 	page = (unsigned long *)ptr;
 
-	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
-		if (page[pos])
+	for (pos = PAGE_SIZE / sizeof(unsigned long) - 1; pos > 0; pos--) {
+		if (page[pos] != page[pos - 1])
 			return false;
 	}
 
+	if (element)
+		element[0] = page[pos];
+
 	return true;
 }
 
-static void handle_zero_page(struct bio_vec *bvec)
+static void handle_same_page(struct bio_vec *bvec, unsigned long element)
 {
 	struct page *page = bvec->bv_page;
 	void *user_mem;
 
 	user_mem = kmap_atomic(page);
 	if (is_partial_io(bvec))
-		memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
+		zram_set_page_partial(user_mem + bvec->bv_offset, bvec->bv_len,
+			element);
 	else
-		clear_page(user_mem);
+		zram_set_page(user_mem, element);
 	kunmap_atomic(user_mem);
 
 	flush_dcache_page(page);
@@ -431,7 +479,7 @@ static ssize_t mm_stat_show(struct device *dev,
 			mem_used << PAGE_SHIFT,
 			zram->limit_pages << PAGE_SHIFT,
 			max_used << PAGE_SHIFT,
-			(u64)atomic64_read(&zram->stats.zero_pages),
+			(u64)atomic64_read(&zram->stats.same_pages),
 			pool_stats.pages_compacted);
 	up_read(&zram->init_lock);
 
@@ -464,7 +512,7 @@ static ssize_t debug_stat_show(struct device *dev,
 ZRAM_ATTR_RO(failed_writes);
 ZRAM_ATTR_RO(invalid_io);
 ZRAM_ATTR_RO(notify_free);
-ZRAM_ATTR_RO(zero_pages);
+ZRAM_ATTR_RO(same_pages);
 ZRAM_ATTR_RO(compr_data_size);
 
 static inline bool zram_meta_get(struct zram *zram)
@@ -538,18 +586,20 @@ static void zram_free_page(struct zram *zram, size_t index)
 	struct zram_meta *meta = zram->meta;
 	unsigned long handle = meta->table[index].handle;
 
-	if (unlikely(!handle)) {
-		/*
-		 * No memory is allocated for zero filled pages.
-		 * Simply clear zero page flag.
-		 */
-		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
-			zram_clear_flag(meta, index, ZRAM_ZERO);
-			atomic64_dec(&zram->stats.zero_pages);
-		}
+	/*
+	 * No memory is allocated for same element filled pages.
+	 * Simply clear same page flag.
+	 */
+	if (zram_test_flag(meta, index, ZRAM_SAME)) {
+		zram_clear_flag(meta, index, ZRAM_SAME);
+		zram_clear_element(meta, index);
+		atomic64_dec(&zram->stats.same_pages);
 		return;
 	}
 
+	if (!handle)
+		return;
+
 	zs_free(meta->mem_pool, handle);
 
 	atomic64_sub(zram_get_obj_size(meta, index),
@@ -572,9 +622,9 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	handle = meta->table[index].handle;
 	size = zram_get_obj_size(meta, index);
 
-	if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) {
+	if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		clear_page(mem);
+		zram_set_page(mem, meta->table[index].element);
 		return 0;
 	}
 
@@ -610,9 +660,9 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 
 	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
 	if (unlikely(!meta->table[index].handle) ||
-			zram_test_flag(meta, index, ZRAM_ZERO)) {
+			zram_test_flag(meta, index, ZRAM_SAME)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		handle_zero_page(bvec);
+		handle_same_page(bvec, meta->table[index].element);
 		return 0;
 	}
 	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
@@ -660,6 +710,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	struct zram_meta *meta = zram->meta;
 	struct zcomp_strm *zstrm = NULL;
 	unsigned long alloced_pages;
+	unsigned long element;
 
 	page = bvec->bv_page;
 	if (is_partial_io(bvec)) {
@@ -688,16 +739,17 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		uncmem = user_mem;
 	}
 
-	if (page_zero_filled(uncmem)) {
+	if (page_same_filled(uncmem, &element)) {
 		if (user_mem)
 			kunmap_atomic(user_mem);
 		/* Free memory associated with this sector now. */
 		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
 		zram_free_page(zram, index);
-		zram_set_flag(meta, index, ZRAM_ZERO);
+		zram_set_flag(meta, index, ZRAM_SAME);
+		zram_set_element(meta, index, element);
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 
-		atomic64_inc(&zram->stats.zero_pages);
+		atomic64_inc(&zram->stats.same_pages);
 		ret = 0;
 		goto out;
 	}
@@ -1203,7 +1255,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 	&dev_attr_compact.attr,
 	&dev_attr_invalid_io.attr,
 	&dev_attr_notify_free.attr,
-	&dev_attr_zero_pages.attr,
+	&dev_attr_same_pages.attr,
 	&dev_attr_orig_data_size.attr,
 	&dev_attr_compr_data_size.attr,
 	&dev_attr_mem_used_total.attr,
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 74fcf10..4bb92e1 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -60,8 +60,8 @@
 
 /* Flags for zram pages (table[page_no].value) */
 enum zram_pageflags {
-	/* Page consists entirely of zeros */
-	ZRAM_ZERO = ZRAM_FLAG_SHIFT,
+	/* Page consists entirely of same elements */
+	ZRAM_SAME = ZRAM_FLAG_SHIFT,
 	ZRAM_ACCESS,	/* page is now accessed */
 
 	__NR_ZRAM_PAGEFLAGS,
@@ -71,7 +71,10 @@ enum zram_pageflags {
 
 /* Allocated for each disk page */
 struct zram_table_entry {
-	unsigned long handle;
+	union {
+		unsigned long handle;
+		unsigned long element;
+	};
 	unsigned long value;
 };
 
@@ -83,7 +86,7 @@ struct zram_stats {
 	atomic64_t failed_writes;	/* can happen when memory is too low */
 	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
 	atomic64_t notify_free;	/* no. of swap slot free notifications */
-	atomic64_t zero_pages;		/* no. of zero filled pages */
+	atomic64_t same_pages;		/* no. of same element filled pages */
 	atomic64_t pages_stored;	/* no. of pages currently stored */
 	atomic_long_t max_used_pages;	/* no. of maximum pages stored */
 	atomic64_t writestall;		/* no. of write slow paths */
-- 
1.7.9.5

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

Hello,

On Fri, Jan 06, 2017 at 04:42:25PM +0800, zhouxianrong@huawei.com wrote:
> From: zhouxianrong <zhouxianrong@huawei.com>
> 
> the idea is that without doing more calculations we extend zero pages
> to same element pages for zram. zero page is special case of
> same element page with zero element.
> 
> 1. the test is done under android 7.0
> 2. startup too many applications circularly
> 3. sample the zero pages, same pages (none-zero element) 
>    and total pages in function page_zero_filled
> 
> the result is listed as below:
> 
> ZERO	SAME	TOTAL
> 36214	17842	598196
> 
> 		ZERO/TOTAL	 SAME/TOTAL	  (ZERO+SAME)/TOTAL ZERO/SAME
> AVERAGE	0.060631909	 0.024990816  0.085622726		2.663825038
> STDEV	0.00674612	 0.005887625  0.009707034		2.115881328
> MAX		0.069698422	 0.030046087  0.094975336		7.56043956
> MIN		0.03959586	 0.007332205  0.056055193		1.928985507
> 
> from above data, the benefit is about 2.5% and up to 3% of total 
> swapout pages.

It looks great if we considers the amount of diff.
Thanks!

> 
> the defect of the patch is that when we recovery a page from 
> non-zero element the operations are low efficient for partial
> read.

Nice description. I love to see pros/cons. :)
If it hurts partial-read little bit, it would be not critical problem.

> 
> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
> ---
>  drivers/block/zram/zram_drv.c |  102 +++++++++++++++++++++++++++++++----------
>  drivers/block/zram/zram_drv.h |   11 +++--
>  2 files changed, 84 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 15f58ab..3250a8b 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -94,6 +94,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
>  	meta->table[index].value &= ~BIT(flag);
>  }
>  
> +static inline void zram_set_element(struct zram_meta *meta, u32 index,
> +			unsigned long element)
> +{
> +	meta->table[index].element = element;
> +}
> +
> +static inline void zram_clear_element(struct zram_meta *meta, u32 index)
> +{
> +	meta->table[index].element = 0;
> +}
> +
>  static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
>  {
>  	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
> @@ -158,31 +169,68 @@ static inline void update_used_max(struct zram *zram,
>  	} while (old_max != cur_max);
>  }
>  
> -static bool page_zero_filled(void *ptr)
> +static inline void zram_set_page(char *ptr, unsigned long value)

Nit:
I prefer fill to set in this case.
How about zram_fill_page?
It is a just nit so if you want to set, I'm not against.

> +{
> +	int i;
> +	unsigned long *page = (unsigned long *)ptr;
> +
> +	for (i = PAGE_SIZE / sizeof(unsigned long) - 1; i >= 0; i--)
> +		page[i] = value;
> +}
> +
> +static inline void zram_set_page_partial(char *ptr, unsigned int size,
> +		unsigned long value)
> +{
> +	int i;
> +
> +	i = ((unsigned long)ptr) % sizeof(unsigned long);
> +	if (i) {

Hmm, Is this case really happens? I guess block layer works with IO size unit
which would be aligned (unsigned long) at least, maybe SECTOR or PAGE size.

Then, I think we can merge both zram_set_page and zram_set_page_partial
like this.

static inline void zram_fill_page(char *ptr, unsigned int size, unsigned long elelment)
{
        int i;
        unsigned long *page = (unsigned long *)ptr;

        WARN_ON_ONCE((unsigned long)ptr % sizeof(unsigned long));
        for (i = size / sizeof(unsigned long) - 1; i >= 0; i--)
                page[i] = element
}

> +		while (i < sizeof(unsigned long)) {
> +			*ptr++ = (value >> (i * 8)) & 0xff;
> +			--size;
> +			++i;
> +		}
> +	}
> +
> +	for (i = size / sizeof(unsigned long); i > 0; --i) {
> +		*(unsigned long *)ptr = value;
> +		ptr += sizeof(unsigned long);
> +		size -= sizeof(unsigned long);
> +	}
> +
> +	for (i = 0; i < size; ++i)
> +		*ptr++ = (value >> (i * 8)) & 0xff;
> +}
> +
> +static bool page_same_filled(void *ptr, unsigned long *element)
>  {
>  	unsigned int pos;
>  	unsigned long *page;
>  
>  	page = (unsigned long *)ptr;
>  
> -	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
> -		if (page[pos])
> +	for (pos = PAGE_SIZE / sizeof(unsigned long) - 1; pos > 0; pos--) {
> +		if (page[pos] != page[pos - 1])
>  			return false;
>  	}
>  
> +	if (element)

Maybe, we don't need this condition check?

> +		element[0] = page[pos];
> +
>  	return true;
>  }
>  
> -static void handle_zero_page(struct bio_vec *bvec)
> +static void handle_same_page(struct bio_vec *bvec, unsigned long element)
>  {
>  	struct page *page = bvec->bv_page;
>  	void *user_mem;
>  
>  	user_mem = kmap_atomic(page);
>  	if (is_partial_io(bvec))
> -		memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
> +		zram_set_page_partial(user_mem + bvec->bv_offset, bvec->bv_len,
> +			element);
>  	else
> -		clear_page(user_mem);
> +		zram_set_page(user_mem, element);
>  	kunmap_atomic(user_mem);
>  
>  	flush_dcache_page(page);
> @@ -431,7 +479,7 @@ static ssize_t mm_stat_show(struct device *dev,
>  			mem_used << PAGE_SHIFT,
>  			zram->limit_pages << PAGE_SHIFT,
>  			max_used << PAGE_SHIFT,
> -			(u64)atomic64_read(&zram->stats.zero_pages),
> +			(u64)atomic64_read(&zram->stats.same_pages),

Unfortunately, we cannot replace zero pages stat with same pages's one right
now due to compatibility problem. Please add same_pages to tail of the stat
and we should warn deprecated zero_pages stat so we finally will remove it
two year later. Please reference Documentation/ABI/obsolete/sysfs-block-zram
And add zero-pages to the document.

For example,

... mm_stat_show()
{
        pr_warn_once("zero pages was deprecated so it will be removed at 2019 Jan");
}

Sergey, what's your opinion?

>  			pool_stats.pages_compacted);
>  	up_read(&zram->init_lock);
>  
> @@ -464,7 +512,7 @@ static ssize_t debug_stat_show(struct device *dev,
>  ZRAM_ATTR_RO(failed_writes);
>  ZRAM_ATTR_RO(invalid_io);
>  ZRAM_ATTR_RO(notify_free);
> -ZRAM_ATTR_RO(zero_pages);
> +ZRAM_ATTR_RO(same_pages);
>  ZRAM_ATTR_RO(compr_data_size);
>  
>  static inline bool zram_meta_get(struct zram *zram)
> @@ -538,18 +586,20 @@ static void zram_free_page(struct zram *zram, size_t index)
>  	struct zram_meta *meta = zram->meta;
>  	unsigned long handle = meta->table[index].handle;
>  
> -	if (unlikely(!handle)) {
> -		/*
> -		 * No memory is allocated for zero filled pages.
> -		 * Simply clear zero page flag.
> -		 */
> -		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
> -			zram_clear_flag(meta, index, ZRAM_ZERO);
> -			atomic64_dec(&zram->stats.zero_pages);
> -		}
> +	/*
> +	 * No memory is allocated for same element filled pages.
> +	 * Simply clear same page flag.
> +	 */
> +	if (zram_test_flag(meta, index, ZRAM_SAME)) {
> +		zram_clear_flag(meta, index, ZRAM_SAME);
> +		zram_clear_element(meta, index);
> +		atomic64_dec(&zram->stats.same_pages);
>  		return;
>  	}
>  
> +	if (!handle)
> +		return;
> +
>  	zs_free(meta->mem_pool, handle);
>  
>  	atomic64_sub(zram_get_obj_size(meta, index),
> @@ -572,9 +622,9 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  	handle = meta->table[index].handle;
>  	size = zram_get_obj_size(meta, index);
>  
> -	if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) {
> +	if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
>  		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> -		clear_page(mem);
> +		zram_set_page(mem, meta->table[index].element);

>  		return 0;
>  	}
>  
> @@ -610,9 +660,9 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
>  
>  	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
>  	if (unlikely(!meta->table[index].handle) ||
> -			zram_test_flag(meta, index, ZRAM_ZERO)) {
> +			zram_test_flag(meta, index, ZRAM_SAME)) {
>  		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> -		handle_zero_page(bvec);
> +		handle_same_page(bvec, meta->table[index].element);
>  		return 0;
>  	}
>  	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> @@ -660,6 +710,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  	struct zram_meta *meta = zram->meta;
>  	struct zcomp_strm *zstrm = NULL;
>  	unsigned long alloced_pages;
> +	unsigned long element;
>  
>  	page = bvec->bv_page;
>  	if (is_partial_io(bvec)) {
> @@ -688,16 +739,17 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		uncmem = user_mem;
>  	}
>  
> -	if (page_zero_filled(uncmem)) {
> +	if (page_same_filled(uncmem, &element)) {
>  		if (user_mem)
>  			kunmap_atomic(user_mem);
>  		/* Free memory associated with this sector now. */
>  		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
>  		zram_free_page(zram, index);
> -		zram_set_flag(meta, index, ZRAM_ZERO);
> +		zram_set_flag(meta, index, ZRAM_SAME);
> +		zram_set_element(meta, index, element);
>  		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>  
> -		atomic64_inc(&zram->stats.zero_pages);
> +		atomic64_inc(&zram->stats.same_pages);
>  		ret = 0;
>  		goto out;
>  	}
> @@ -1203,7 +1255,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
>  	&dev_attr_compact.attr,
>  	&dev_attr_invalid_io.attr,
>  	&dev_attr_notify_free.attr,
> -	&dev_attr_zero_pages.attr,
> +	&dev_attr_same_pages.attr,
>  	&dev_attr_orig_data_size.attr,
>  	&dev_attr_compr_data_size.attr,
>  	&dev_attr_mem_used_total.attr,
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 74fcf10..4bb92e1 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -60,8 +60,8 @@
>  
>  /* Flags for zram pages (table[page_no].value) */
>  enum zram_pageflags {
> -	/* Page consists entirely of zeros */
> -	ZRAM_ZERO = ZRAM_FLAG_SHIFT,
> +	/* Page consists entirely of same elements */
> +	ZRAM_SAME = ZRAM_FLAG_SHIFT,
>  	ZRAM_ACCESS,	/* page is now accessed */
>  
>  	__NR_ZRAM_PAGEFLAGS,
> @@ -71,7 +71,10 @@ enum zram_pageflags {
>  
>  /* Allocated for each disk page */
>  struct zram_table_entry {
> -	unsigned long handle;
> +	union {
> +		unsigned long handle;
> +		unsigned long element;
> +	};
>  	unsigned long value;
>  };
>  
> @@ -83,7 +86,7 @@ struct zram_stats {
>  	atomic64_t failed_writes;	/* can happen when memory is too low */
>  	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
>  	atomic64_t notify_free;	/* no. of swap slot free notifications */
> -	atomic64_t zero_pages;		/* no. of zero filled pages */
> +	atomic64_t same_pages;		/* no. of same element filled pages */
>  	atomic64_t pages_stored;	/* no. of pages currently stored */
>  	atomic_long_t max_used_pages;	/* no. of maximum pages stored */
>  	atomic64_t writestall;		/* no. of write slow paths */
> -- 
> 1.7.9.5
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

Hello,

sorry, was mostly offline for the past few days, now catching up.

On (01/10/17 08:41), Minchan Kim wrote:
> > the idea is that without doing more calculations we extend zero pages
> > to same element pages for zram. zero page is special case of
> > same element page with zero element.
> > 

interesting idea.

[..]
> >  	flush_dcache_page(page);
> > @@ -431,7 +479,7 @@ static ssize_t mm_stat_show(struct device *dev,
> >  			mem_used << PAGE_SHIFT,
> >  			zram->limit_pages << PAGE_SHIFT,
> >  			max_used << PAGE_SHIFT,
> > -			(u64)atomic64_read(&zram->stats.zero_pages),
> > +			(u64)atomic64_read(&zram->stats.same_pages),
> 
> Unfortunately, we cannot replace zero pages stat with same pages's one right
> now due to compatibility problem. Please add same_pages to tail of the stat
> and we should warn deprecated zero_pages stat so we finally will remove it
> two year later. Please reference Documentation/ABI/obsolete/sysfs-block-zram
> And add zero-pages to the document.
> 
> For example,
> 
> ... mm_stat_show()
> {
>         pr_warn_once("zero pages was deprecated so it will be removed at 2019 Jan");
> }
> 
> Sergey, what's your opinion?

oh, I was going to ask you whether you have any work in progress at
the moment or not. because deprecated attrs are scheduled to be removed
in 4.11. IOW, we must send the clean up patch, well, right now. so I can
prepare the patch, but it can conflict with someone's 'more serious/relevant'
work.

we also have zram hot/addd sysfs attr, which must be deprecated and
converted to a char device. per Greg KH.

> Please add same_pages to tail of the stat

sounds ok to me. and yes, can deprecate zero_pages.

seems that with that patch the concept of ZRAM_ZERO disappears. both
ZERO and SAME_ELEMENT pages are considered to be the same thing now.
which is fine and makes sense to me, I think. and if ->.same_pages will
replace ->.zero_pages in mm_stat() then I'm also OK. yes, we will see
increased number in the last column of mm_stat file, but I don't tend
to see any issues here. Minchan, what do you think?


> -ZRAM_ATTR_RO(zero_pages);
> +ZRAM_ATTR_RO(same_pages);

this part is a no-no-no-no :)  we can't simply rename the user space
visible attrs.

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

Hi Sergey,

On Fri, Jan 13, 2017 at 01:24:44PM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> sorry, was mostly offline for the past few days, now catching up.
> 
> On (01/10/17 08:41), Minchan Kim wrote:
> > > the idea is that without doing more calculations we extend zero pages
> > > to same element pages for zram. zero page is special case of
> > > same element page with zero element.
> > > 
> 
> interesting idea.
> 
> [..]
> > >  	flush_dcache_page(page);
> > > @@ -431,7 +479,7 @@ static ssize_t mm_stat_show(struct device *dev,
> > >  			mem_used << PAGE_SHIFT,
> > >  			zram->limit_pages << PAGE_SHIFT,
> > >  			max_used << PAGE_SHIFT,
> > > -			(u64)atomic64_read(&zram->stats.zero_pages),
> > > +			(u64)atomic64_read(&zram->stats.same_pages),
> > 
> > Unfortunately, we cannot replace zero pages stat with same pages's one right
> > now due to compatibility problem. Please add same_pages to tail of the stat
> > and we should warn deprecated zero_pages stat so we finally will remove it
> > two year later. Please reference Documentation/ABI/obsolete/sysfs-block-zram
> > And add zero-pages to the document.
> > 
> > For example,
> > 
> > ... mm_stat_show()
> > {
> >         pr_warn_once("zero pages was deprecated so it will be removed at 2019 Jan");
> > }
> > 
> > Sergey, what's your opinion?
> 
> oh, I was going to ask you whether you have any work in progress at
> the moment or not. because deprecated attrs are scheduled to be removed
> in 4.11. IOW, we must send the clean up patch, well, right now. so I can
> prepare the patch, but it can conflict with someone's 'more serious/relevant'
> work.

I think deprecating attrs is top priority to me so go ahead. :)

> 
> we also have zram hot/addd sysfs attr, which must be deprecated and
> converted to a char device. per Greg KH.
> 
> > Please add same_pages to tail of the stat
> 
> sounds ok to me. and yes, can deprecate zero_pages.
> 
> seems that with that patch the concept of ZRAM_ZERO disappears. both
> ZERO and SAME_ELEMENT pages are considered to be the same thing now.

Right.

> which is fine and makes sense to me, I think. and if ->.same_pages will
> replace ->.zero_pages in mm_stat() then I'm also OK. yes, we will see
> increased number in the last column of mm_stat file, but I don't tend
> to see any issues here. Minchan, what do you think?

Could you elaborate a bit? Do you mean this?

        ret = scnprintf(buf, PAGE_SIZE,
                        "%8llu %8llu %8llu %8lu %8ld %8llu %8lu\n",
                        orig_size << PAGE_SHIFT,
                        (u64)atomic64_read(&zram->stats.compr_data_size),
                        mem_used << PAGE_SHIFT,
                        zram->limit_pages << PAGE_SHIFT,
                        max_used << PAGE_SHIFT,
                        // (u64)atomic64_read(&zram->stats.zero_pages),
                        (u64)atomic64_read(&zram->stats.same_pages),
                        pool_stats.pages_compacted);

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

On (01/13/17 15:23), Minchan Kim wrote:
[..]
> > > Please add same_pages to tail of the stat
> > 
> > sounds ok to me. and yes, can deprecate zero_pages.
> > 
> > seems that with that patch the concept of ZRAM_ZERO disappears. both
> > ZERO and SAME_ELEMENT pages are considered to be the same thing now.
> 
> Right.
> 
> > which is fine and makes sense to me, I think. and if ->.same_pages will
> > replace ->.zero_pages in mm_stat() then I'm also OK. yes, we will see
> > increased number in the last column of mm_stat file, but I don't tend
> > to see any issues here. Minchan, what do you think?
> 
> Could you elaborate a bit? Do you mean this?
> 
>         ret = scnprintf(buf, PAGE_SIZE,
>                         "%8llu %8llu %8llu %8lu %8ld %8llu %8lu\n",
>                         orig_size << PAGE_SHIFT,
>                         (u64)atomic64_read(&zram->stats.compr_data_size),
>                         mem_used << PAGE_SHIFT,
>                         zram->limit_pages << PAGE_SHIFT,
>                         max_used << PAGE_SHIFT,
>                         // (u64)atomic64_read(&zram->stats.zero_pages),
>                         (u64)atomic64_read(&zram->stats.same_pages),
>                         pool_stats.pages_compacted);

yes, correct.

do we need to export it as two different stats (zero_pages and
same_pages), if those are basically same thing internally?

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

On Fri, Jan 13, 2017 at 03:36:14PM +0900, Sergey Senozhatsky wrote:
> On (01/13/17 15:23), Minchan Kim wrote:
> [..]
> > > > Please add same_pages to tail of the stat
> > > 
> > > sounds ok to me. and yes, can deprecate zero_pages.
> > > 
> > > seems that with that patch the concept of ZRAM_ZERO disappears. both
> > > ZERO and SAME_ELEMENT pages are considered to be the same thing now.
> > 
> > Right.
> > 
> > > which is fine and makes sense to me, I think. and if ->.same_pages will
> > > replace ->.zero_pages in mm_stat() then I'm also OK. yes, we will see
> > > increased number in the last column of mm_stat file, but I don't tend
> > > to see any issues here. Minchan, what do you think?
> > 
> > Could you elaborate a bit? Do you mean this?
> > 
> >         ret = scnprintf(buf, PAGE_SIZE,
> >                         "%8llu %8llu %8llu %8lu %8ld %8llu %8lu\n",
> >                         orig_size << PAGE_SHIFT,
> >                         (u64)atomic64_read(&zram->stats.compr_data_size),
> >                         mem_used << PAGE_SHIFT,
> >                         zram->limit_pages << PAGE_SHIFT,
> >                         max_used << PAGE_SHIFT,
> >                         // (u64)atomic64_read(&zram->stats.zero_pages),
> >                         (u64)atomic64_read(&zram->stats.same_pages),
> >                         pool_stats.pages_compacted);
> 
> yes, correct.
> 
> do we need to export it as two different stats (zero_pages and
> same_pages), if those are basically same thing internally?

So, let summary up.

1. replace zero_page stat into same page stat in mm_stat
2. s/zero_pages/same_pages/Documentation/blockdev/zram.txt
3. No need to warn to "cat /sys/block/zram0/mm_stat" user to see zero_pages
   about semantic change

?

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

On (01/13/17 15:47), Minchan Kim wrote:
[..]
> > > Could you elaborate a bit? Do you mean this?
> > > 
> > >         ret = scnprintf(buf, PAGE_SIZE,
> > >                         "%8llu %8llu %8llu %8lu %8ld %8llu %8lu\n",
> > >                         orig_size << PAGE_SHIFT,
> > >                         (u64)atomic64_read(&zram->stats.compr_data_size),
> > >                         mem_used << PAGE_SHIFT,
> > >                         zram->limit_pages << PAGE_SHIFT,
> > >                         max_used << PAGE_SHIFT,
> > >                         // (u64)atomic64_read(&zram->stats.zero_pages),
> > >                         (u64)atomic64_read(&zram->stats.same_pages),
> > >                         pool_stats.pages_compacted);
> > 
> > yes, correct.
> > 
> > do we need to export it as two different stats (zero_pages and
> > same_pages), if those are basically same thing internally?
> 
> So, let summary up.
> 
> 1. replace zero_page stat into same page stat in mm_stat
> 2. s/zero_pages/same_pages/Documentation/blockdev/zram.txt
> 3. No need to warn to "cat /sys/block/zram0/mm_stat" user to see zero_pages
>    about semantic change

1) account zero_page and same_pages in one attr.

	this already is in the patch.

2) do not rename zero_pages attr.

	we can't do this so fast, I think.


> 3. No need to warn to "cat /sys/block/zram0/mm_stat" user to see zero_pages
>    about semantic change

yes. we just _may_ have more pages (depending on data pattern) which we treat
as "zero" pages internally. this results in lower memory consumption. I don't
think warn users about this change is necessary; they won't be able to do
anything about it anyway. zero_pages stat is pretty much just a fun number to
know. isn't it?

or do you think that we should account it in separate stats?

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

On Fri, Jan 13, 2017 at 04:02:45PM +0900, Sergey Senozhatsky wrote:
> On (01/13/17 15:47), Minchan Kim wrote:
> [..]
> > > > Could you elaborate a bit? Do you mean this?
> > > > 
> > > >         ret = scnprintf(buf, PAGE_SIZE,
> > > >                         "%8llu %8llu %8llu %8lu %8ld %8llu %8lu\n",
> > > >                         orig_size << PAGE_SHIFT,
> > > >                         (u64)atomic64_read(&zram->stats.compr_data_size),
> > > >                         mem_used << PAGE_SHIFT,
> > > >                         zram->limit_pages << PAGE_SHIFT,
> > > >                         max_used << PAGE_SHIFT,
> > > >                         // (u64)atomic64_read(&zram->stats.zero_pages),
> > > >                         (u64)atomic64_read(&zram->stats.same_pages),
> > > >                         pool_stats.pages_compacted);
> > > 
> > > yes, correct.
> > > 
> > > do we need to export it as two different stats (zero_pages and
> > > same_pages), if those are basically same thing internally?
> > 
> > So, let summary up.
> > 
> > 1. replace zero_page stat into same page stat in mm_stat
> > 2. s/zero_pages/same_pages/Documentation/blockdev/zram.txt
> > 3. No need to warn to "cat /sys/block/zram0/mm_stat" user to see zero_pages
> >    about semantic change
> 
> 1) account zero_page and same_pages in one attr.
> 
> 	this already is in the patch.
> 
> 2) do not rename zero_pages attr.
> 
> 	we can't do this so fast, I think.
> 
> 
> > 3. No need to warn to "cat /sys/block/zram0/mm_stat" user to see zero_pages
> >    about semantic change
> 
> yes. we just _may_ have more pages (depending on data pattern) which we treat
> as "zero" pages internally. this results in lower memory consumption. I don't
> think warn users about this change is necessary; they won't be able to do
> anything about it anyway. zero_pages stat is pretty much just a fun number to
> know. isn't it?
> 

I just wanted to confirm your thought.
I hope anyone doesn't is sensitive on that number. ;)
Let's replace zero_page stat field in mm_stat.

Thanks.

[PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

From: zhouxianrong <zhouxianrong@huawei.com>

the idea is that without doing more calculations we extend zero pages
to same element pages for zram. zero page is special case of
same element page with zero element.

1. the test is done under android 7.0
2. startup too many applications circularly
3. sample the zero pages, same pages (none-zero element) 
   and total pages in function page_zero_filled

the result is listed as below:

ZERO	SAME	TOTAL
36214	17842	598196

		ZERO/TOTAL	 SAME/TOTAL	  (ZERO+SAME)/TOTAL ZERO/SAME
AVERAGE	0.060631909	 0.024990816  0.085622726		2.663825038
STDEV	0.00674612	 0.005887625  0.009707034		2.115881328
MAX		0.069698422	 0.030046087  0.094975336		7.56043956
MIN		0.03959586	 0.007332205  0.056055193		1.928985507

from above data, the benefit is about 2.5% and up to 3% of total 
swapout pages.

the defect of the patch is that when we recovery a page from 
non-zero element the operations are low efficient for partial
read.

Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
---
 Documentation/ABI/testing/sysfs-block-zram |   12 +--
 Documentation/blockdev/zram.txt            |    4 +-
 drivers/block/zram/zram_drv.c              |  110 +++++++++++++++++++++-------
 drivers/block/zram/zram_drv.h              |   11 ++-
 4 files changed, 100 insertions(+), 37 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-block-zram b/Documentation/ABI/testing/sysfs-block-zram
index 4518d30..1a759fa 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -86,21 +86,21 @@ Description:
 		ones are sent by filesystem mounted with discard option,
 		whenever some data blocks are getting discarded.
 
-What:		/sys/block/zram<id>/zero_pages
+What:		/sys/block/zram<id>/same_pages
 Date:		August 2010
 Contact:	Nitin Gupta <ngupta@vflare.org>
 Description:
-		The zero_pages file is read-only and specifies number of zero
-		filled pages written to this disk. No memory is allocated for
-		such pages.
+		The same_pages file is read-only and specifies number of same
+		element filled pages written to this disk. No memory is allocated
+		for such pages.
 
 What:		/sys/block/zram<id>/orig_data_size
 Date:		August 2010
 Contact:	Nitin Gupta <ngupta@vflare.org>
 Description:
 		The orig_data_size file is read-only and specifies uncompressed
-		size of data stored in this disk. This excludes zero-filled
-		pages (zero_pages) since no memory is allocated for them.
+		size of data stored in this disk. This excludes same-element-filled
+		pages (same_pages) since no memory is allocated for them.
 		Unit: bytes
 
 What:		/sys/block/zram<id>/compr_data_size
diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index 0535ae1..e93a7ff 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -170,7 +170,7 @@ max_comp_streams  RW    the number of possible concurrent compress operations
 comp_algorithm    RW    show and change the compression algorithm
 notify_free       RO    the number of notifications to free pages (either
                         slot free notifications or REQ_DISCARD requests)
-zero_pages        RO    the number of zero filled pages written to this disk
+same_pages        RO    the number of same element filled pages written to this disk
 orig_data_size    RO    uncompressed size of data stored in this disk
 compr_data_size   RO    compressed size of data stored in this disk
 mem_used_total    RO    the amount of memory allocated for this disk
@@ -225,7 +225,7 @@ line of text and contains the following stats separated by whitespace:
 	mem_used_total
 	mem_limit
 	mem_used_max
-	zero_pages
+	same_pages
 	num_migrated
 
 9) Deactivate:
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 15f58ab..a72ecf1 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -94,6 +94,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
 	meta->table[index].value &= ~BIT(flag);
 }
 
+static inline void zram_set_element(struct zram_meta *meta, u32 index,
+			unsigned long element)
+{
+	meta->table[index].element = element;
+}
+
+static inline void zram_clear_element(struct zram_meta *meta, u32 index)
+{
+	meta->table[index].element = 0;
+}
+
 static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
 {
 	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
@@ -158,31 +169,76 @@ static inline void update_used_max(struct zram *zram,
 	} while (old_max != cur_max);
 }
 
-static bool page_zero_filled(void *ptr)
+static inline void zram_fill_page(char *ptr, unsigned long value)
+{
+	int i;
+	unsigned long *page = (unsigned long *)ptr;
+
+	if (likely(value == 0)) {
+		clear_page(ptr);
+	} else {
+		for (i = PAGE_SIZE / sizeof(unsigned long) - 1; i >= 0; i--)
+			page[i] = value;
+	}
+}
+
+static inline void zram_set_page_partial(char *ptr, unsigned int size,
+		unsigned long value)
+{
+	int i;
+
+	if (likely(value == 0)) {
+		memset(ptr, 0, size);
+		return;
+	}
+
+	i = ((unsigned long)ptr) % sizeof(unsigned long);
+	if (i) {
+		while (i < sizeof(unsigned long)) {
+			*ptr++ = (value >> (i * 8)) & 0xff;
+			--size;
+			++i;
+		}
+	}
+
+	for (i = size / sizeof(unsigned long); i > 0; --i) {
+		*(unsigned long *)ptr = value;
+		ptr += sizeof(unsigned long);
+		size -= sizeof(unsigned long);
+	}
+
+	for (i = 0; i < size; ++i)
+		*ptr++ = (value >> (i * 8)) & 0xff;
+}
+
+static bool page_same_filled(void *ptr, unsigned long *element)
 {
 	unsigned int pos;
 	unsigned long *page;
 
 	page = (unsigned long *)ptr;
 
-	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
-		if (page[pos])
+	for (pos = PAGE_SIZE / sizeof(unsigned long) - 1; pos > 0; pos--) {
+		if (page[pos] != page[pos - 1])
 			return false;
 	}
 
+	element[0] = page[pos];
+
 	return true;
 }
 
-static void handle_zero_page(struct bio_vec *bvec)
+static void handle_same_page(struct bio_vec *bvec, unsigned long element)
 {
 	struct page *page = bvec->bv_page;
 	void *user_mem;
 
 	user_mem = kmap_atomic(page);
 	if (is_partial_io(bvec))
-		memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
+		zram_set_page_partial(user_mem + bvec->bv_offset, bvec->bv_len,
+			element);
 	else
-		clear_page(user_mem);
+		zram_fill_page(user_mem, element);
 	kunmap_atomic(user_mem);
 
 	flush_dcache_page(page);
@@ -431,7 +487,7 @@ static ssize_t mm_stat_show(struct device *dev,
 			mem_used << PAGE_SHIFT,
 			zram->limit_pages << PAGE_SHIFT,
 			max_used << PAGE_SHIFT,
-			(u64)atomic64_read(&zram->stats.zero_pages),
+			(u64)atomic64_read(&zram->stats.same_pages),
 			pool_stats.pages_compacted);
 	up_read(&zram->init_lock);
 
@@ -464,7 +520,7 @@ static ssize_t debug_stat_show(struct device *dev,
 ZRAM_ATTR_RO(failed_writes);
 ZRAM_ATTR_RO(invalid_io);
 ZRAM_ATTR_RO(notify_free);
-ZRAM_ATTR_RO(zero_pages);
+ZRAM_ATTR_RO(same_pages);
 ZRAM_ATTR_RO(compr_data_size);
 
 static inline bool zram_meta_get(struct zram *zram)
@@ -538,18 +594,20 @@ static void zram_free_page(struct zram *zram, size_t index)
 	struct zram_meta *meta = zram->meta;
 	unsigned long handle = meta->table[index].handle;
 
-	if (unlikely(!handle)) {
-		/*
-		 * No memory is allocated for zero filled pages.
-		 * Simply clear zero page flag.
-		 */
-		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
-			zram_clear_flag(meta, index, ZRAM_ZERO);
-			atomic64_dec(&zram->stats.zero_pages);
-		}
+	/*
+	 * No memory is allocated for same element filled pages.
+	 * Simply clear same page flag.
+	 */
+	if (zram_test_flag(meta, index, ZRAM_SAME)) {
+		zram_clear_flag(meta, index, ZRAM_SAME);
+		zram_clear_element(meta, index);
+		atomic64_dec(&zram->stats.same_pages);
 		return;
 	}
 
+	if (!handle)
+		return;
+
 	zs_free(meta->mem_pool, handle);
 
 	atomic64_sub(zram_get_obj_size(meta, index),
@@ -572,9 +630,9 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	handle = meta->table[index].handle;
 	size = zram_get_obj_size(meta, index);
 
-	if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) {
+	if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		clear_page(mem);
+		zram_fill_page(mem, meta->table[index].element);
 		return 0;
 	}
 
@@ -610,9 +668,9 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 
 	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
 	if (unlikely(!meta->table[index].handle) ||
-			zram_test_flag(meta, index, ZRAM_ZERO)) {
+			zram_test_flag(meta, index, ZRAM_SAME)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		handle_zero_page(bvec);
+		handle_same_page(bvec, meta->table[index].element);
 		return 0;
 	}
 	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
@@ -660,6 +718,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	struct zram_meta *meta = zram->meta;
 	struct zcomp_strm *zstrm = NULL;
 	unsigned long alloced_pages;
+	unsigned long element;
 
 	page = bvec->bv_page;
 	if (is_partial_io(bvec)) {
@@ -688,16 +747,17 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		uncmem = user_mem;
 	}
 
-	if (page_zero_filled(uncmem)) {
+	if (page_same_filled(uncmem, &element)) {
 		if (user_mem)
 			kunmap_atomic(user_mem);
 		/* Free memory associated with this sector now. */
 		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
 		zram_free_page(zram, index);
-		zram_set_flag(meta, index, ZRAM_ZERO);
+		zram_set_flag(meta, index, ZRAM_SAME);
+		zram_set_element(meta, index, element);
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 
-		atomic64_inc(&zram->stats.zero_pages);
+		atomic64_inc(&zram->stats.same_pages);
 		ret = 0;
 		goto out;
 	}
@@ -1203,7 +1263,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 	&dev_attr_compact.attr,
 	&dev_attr_invalid_io.attr,
 	&dev_attr_notify_free.attr,
-	&dev_attr_zero_pages.attr,
+	&dev_attr_same_pages.attr,
 	&dev_attr_orig_data_size.attr,
 	&dev_attr_compr_data_size.attr,
 	&dev_attr_mem_used_total.attr,
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 74fcf10..4bb92e1 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -60,8 +60,8 @@
 
 /* Flags for zram pages (table[page_no].value) */
 enum zram_pageflags {
-	/* Page consists entirely of zeros */
-	ZRAM_ZERO = ZRAM_FLAG_SHIFT,
+	/* Page consists entirely of same elements */
+	ZRAM_SAME = ZRAM_FLAG_SHIFT,
 	ZRAM_ACCESS,	/* page is now accessed */
 
 	__NR_ZRAM_PAGEFLAGS,
@@ -71,7 +71,10 @@ enum zram_pageflags {
 
 /* Allocated for each disk page */
 struct zram_table_entry {
-	unsigned long handle;
+	union {
+		unsigned long handle;
+		unsigned long element;
+	};
 	unsigned long value;
 };
 
@@ -83,7 +86,7 @@ struct zram_stats {
 	atomic64_t failed_writes;	/* can happen when memory is too low */
 	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
 	atomic64_t notify_free;	/* no. of swap slot free notifications */
-	atomic64_t zero_pages;		/* no. of zero filled pages */
+	atomic64_t same_pages;		/* no. of same element filled pages */
 	atomic64_t pages_stored;	/* no. of pages currently stored */
 	atomic_long_t max_used_pages;	/* no. of maximum pages stored */
 	atomic64_t writestall;		/* no. of write slow paths */
-- 
1.7.9.5

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

Hello,

On (01/13/17 16:29), zhouxianrong@huawei.com wrote:
[..]
> --- a/Documentation/ABI/testing/sysfs-block-zram
> +++ b/Documentation/ABI/testing/sysfs-block-zram
> @@ -86,21 +86,21 @@ Description:
>  		ones are sent by filesystem mounted with discard option,
>  		whenever some data blocks are getting discarded.
>  
> -What:		/sys/block/zram<id>/zero_pages
> +What:		/sys/block/zram<id>/same_pages
[..]
> -zero_pages        RO    the number of zero filled pages written to this disk
> +same_pages        RO    the number of same element filled pages written to this disk
[..]
> -	zero_pages
> +	same_pages
>  	num_migrated
> +}

we removed deprecated sysfs attrs. zero_pages does not exist anymore.

>  static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
>  {
>  	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
> @@ -158,31 +169,76 @@ static inline void update_used_max(struct zram *zram,
>  	} while (old_max != cur_max);
>  }
>  
> -static bool page_zero_filled(void *ptr)
> +static inline void zram_fill_page(char *ptr, unsigned long value)
> +{
> +	int i;
> +	unsigned long *page = (unsigned long *)ptr;
> +
> +	if (likely(value == 0)) {
> +		clear_page(ptr);
> +	} else {
> +		for (i = PAGE_SIZE / sizeof(unsigned long) - 1; i >= 0; i--)
> +			page[i] = value;
> +	}

any particular reason not to use memset() here?
memset() can be faster that that, right?


[..]
>  /* Flags for zram pages (table[page_no].value) */
>  enum zram_pageflags {
> -	/* Page consists entirely of zeros */
> -	ZRAM_ZERO = ZRAM_FLAG_SHIFT,
> +	/* Page consists entirely of same elements */
> +	ZRAM_SAME = ZRAM_FLAG_SHIFT,
>  	ZRAM_ACCESS,	/* page is now accessed */
[..]
> @@ -83,7 +86,7 @@ struct zram_stats {
>  	atomic64_t failed_writes;	/* can happen when memory is too low */
>  	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
>  	atomic64_t notify_free;	/* no. of swap slot free notifications */
> -	atomic64_t zero_pages;		/* no. of zero filled pages */
> +	atomic64_t same_pages;		/* no. of same element filled pages */

not like this rename is particularity important, but ok. works for me.

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

1. memset is just set a int value but i want to set a long value.
2. using clear_page rather than memset MAYBE due to in arm64 arch
    it is a 64-bytes operations.

6.6.4. Data Cache Zero

The ARMv8-A architecture introduces a Data Cache Zero by Virtual Address (DC ZVA) instruction. This enables a block of 64
bytes in memory, aligned to 64 bytes in size, to be set to zero. If the DC ZVA instruction misses in the cache, it clears main
memory, without causing an L1 or L2 cache allocation.

but i only consider the arm64 arch, other archs need to be reviewed.

On 2017/1/21 16:43, Sergey Senozhatsky wrote:
> Hello,
>
> On (01/13/17 16:29), zhouxianrong@huawei.com wrote:
> [..]
>> --- a/Documentation/ABI/testing/sysfs-block-zram
>> +++ b/Documentation/ABI/testing/sysfs-block-zram
>> @@ -86,21 +86,21 @@ Description:
>>  		ones are sent by filesystem mounted with discard option,
>>  		whenever some data blocks are getting discarded.
>>
>> -What:		/sys/block/zram<id>/zero_pages
>> +What:		/sys/block/zram<id>/same_pages
> [..]
>> -zero_pages        RO    the number of zero filled pages written to this disk
>> +same_pages        RO    the number of same element filled pages written to this disk
> [..]
>> -	zero_pages
>> +	same_pages
>>  	num_migrated
>> +}
>
> we removed deprecated sysfs attrs. zero_pages does not exist anymore.
>
>>  static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
>>  {
>>  	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
>> @@ -158,31 +169,76 @@ static inline void update_used_max(struct zram *zram,
>>  	} while (old_max != cur_max);
>>  }
>>
>> -static bool page_zero_filled(void *ptr)
>> +static inline void zram_fill_page(char *ptr, unsigned long value)
>> +{
>> +	int i;
>> +	unsigned long *page = (unsigned long *)ptr;
>> +
>> +	if (likely(value == 0)) {
>> +		clear_page(ptr);
>> +	} else {
>> +		for (i = PAGE_SIZE / sizeof(unsigned long) - 1; i >= 0; i--)
>> +			page[i] = value;
>> +	}
>
> any particular reason not to use memset() here?
> memset() can be faster that that, right?
>
>
> [..]
>>  /* Flags for zram pages (table[page_no].value) */
>>  enum zram_pageflags {
>> -	/* Page consists entirely of zeros */
>> -	ZRAM_ZERO = ZRAM_FLAG_SHIFT,
>> +	/* Page consists entirely of same elements */
>> +	ZRAM_SAME = ZRAM_FLAG_SHIFT,
>>  	ZRAM_ACCESS,	/* page is now accessed */
> [..]
>> @@ -83,7 +86,7 @@ struct zram_stats {
>>  	atomic64_t failed_writes;	/* can happen when memory is too low */
>>  	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
>>  	atomic64_t notify_free;	/* no. of swap slot free notifications */
>> -	atomic64_t zero_pages;		/* no. of zero filled pages */
>> +	atomic64_t same_pages;		/* no. of same element filled pages */
>
> not like this rename is particularity important, but ok. works for me.
>
> 	-ss
>
> .
>

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

On (01/22/17 10:58), zhouxianrong wrote:
> 1. memset is just set a int value but i want to set a long value.

ah... ok. because you union it with the handle.

> 2. using clear_page rather than memset MAYBE due to in arm64 arch
>    it is a 64-bytes operations.

clear_page() basically does memset(), which is quite well optimized.
except for arm64, yes.


> 6.6.4. Data Cache Zero
> 
> The ARMv8-A architecture introduces a Data Cache Zero by Virtual Address (DC ZVA) instruction. This enables a block of 64
> bytes in memory, aligned to 64 bytes in size, to be set to zero. If the DC ZVA instruction misses in the cache, it clears main
> memory, without causing an L1 or L2 cache allocation.
> 
> but i only consider the arm64 arch, other archs need to be reviewed.

thaks for the reply.

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Joonsoo Kim]

Hello,

On Sun, Jan 22, 2017 at 10:58:38AM +0800, zhouxianrong wrote:
> 1. memset is just set a int value but i want to set a long value.

Sorry for late review.

Do we really need to set a long value? I cannot believe that
long value is repeated in the page. Value repeatition is
usually done by value 0 or 1 and it's enough to use int. And, I heard
that value 0 or 1 is repeated in Android. Could you check the distribution
of the value in the same page?

Thanks.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

hey Joonsoo:
	i would test and give the same element type later.

On 2017/1/23 10:58, Joonsoo Kim wrote:
> Hello,
>
> On Sun, Jan 22, 2017 at 10:58:38AM +0800, zhouxianrong wrote:
>> 1. memset is just set a int value but i want to set a long value.
>
> Sorry for late review.
>
> Do we really need to set a long value? I cannot believe that
> long value is repeated in the page. Value repeatition is
> usually done by value 0 or 1 and it's enough to use int. And, I heard
> that value 0 or 1 is repeated in Android. Could you check the distribution
> of the value in the same page?
>
> Thanks.
>
>
> .
>

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

On (01/23/17 11:58), Joonsoo Kim wrote:
> Hello,
> 
> On Sun, Jan 22, 2017 at 10:58:38AM +0800, zhouxianrong wrote:
> > 1. memset is just set a int value but i want to set a long value.
> 
> Sorry for late review.
> 
> Do we really need to set a long value? I cannot believe that
> long value is repeated in the page. Value repeatition is
> usually done by value 0 or 1 and it's enough to use int. And, I heard
> that value 0 or 1 is repeated in Android. Could you check the distribution
> of the value in the same page?

Hello Joonsoo,

thanks for taking a look and for bringing this question up.
so I kinda wanted to propose union of `ulong handle' with `uint element'
and switching to memset(), but I couldn't figure out if that change would
break detection of some patterns.

 /* Allocated for each disk page */
 struct zram_table_entry {
-       unsigned long handle;
+       union {
+               unsigned long handle;
+               unsigned int element;
+       };
        unsigned long value;
 };

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Matthew Wilcox]

On Sun, Jan 22, 2017 at 10:58:38AM +0800, zhouxianrong wrote:
> 1. memset is just set a int value but i want to set a long value.

memset doesn't set an int value.

DESCRIPTION
       The  memset()  function  fills  the  first  n  bytes of the memory area
       pointed to by s with the constant byte c.

It sets a byte value.  K&R just happened to choose 'int' as the type
to store that "unsigned char" in.  Probably for very good reasons which
make absolutely no sense today.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Joonsoo Kim]

On Mon, Jan 23, 2017 at 01:03:47PM +0900, Sergey Senozhatsky wrote:
> On (01/23/17 11:58), Joonsoo Kim wrote:
> > Hello,
> > 
> > On Sun, Jan 22, 2017 at 10:58:38AM +0800, zhouxianrong wrote:
> > > 1. memset is just set a int value but i want to set a long value.
> > 
> > Sorry for late review.
> > 
> > Do we really need to set a long value? I cannot believe that
> > long value is repeated in the page. Value repeatition is
> > usually done by value 0 or 1 and it's enough to use int. And, I heard
> > that value 0 or 1 is repeated in Android. Could you check the distribution
> > of the value in the same page?
> 
> Hello Joonsoo,
> 
> thanks for taking a look and for bringing this question up.
> so I kinda wanted to propose union of `ulong handle' with `uint element'
> and switching to memset(), but I couldn't figure out if that change would
> break detection of some patterns.
> 
>  /* Allocated for each disk page */
>  struct zram_table_entry {
> -       unsigned long handle;
> +       union {
> +               unsigned long handle;
> +               unsigned int element;
> +       };
>         unsigned long value;
>  };

Hello,

Think about following case in 64 bits kernel.

If value pattern in the page is like as following, we cannot detect
the same page with 'unsigned int' element.

AAAAAAAABBBBBBBBAAAAAAAABBBBBBBB...

4 bytes is 0xAAAAAAAA and next 4 bytes is 0xBBBBBBBB and so on.

However, as I said before, I think that it is uncommon case.

Thanks.

答复: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

Yes, memset's prototype is int but
the implement of arch is unsigned char; for example, in arm64

	.weak memset
ENTRY(__memset)
ENTRY(memset)
	mov	dst, dstin	/* Preserve return value.  */
	and	A_lw, val, #255
	orr	A_lw, A_lw, A_lw, lsl #8
	orr	A_lw, A_lw, A_lw, lsl #16
	orr	A_l, A_l, A_l, lsl #32

-----邮件原件-----
发件人: Matthew Wilcox [mailto:willy@infradead.org] 
发送时间: 2017年1月23日 14:26
收件人: zhouxianrong
抄送: Sergey Senozhatsky; linux-mm@kvack.org; linux-kernel@vger.kernel.org; akpm@linux-foundation.org; sergey.senozhatsky@gmail.com; minchan@kernel.org; ngupta@vflare.org; Mi Sophia Wang; Zhouxiyu; Duwei (Device OS); Zhangshiming (Simon, Device OS); Won Ho Park
主题: Re: [PATCH] mm: extend zero pages to same element pages for zram

On Sun, Jan 22, 2017 at 10:58:38AM +0800, zhouxianrong wrote:
> 1. memset is just set a int value but i want to set a long value.

memset doesn't set an int value.

DESCRIPTION
       The  memset()  function  fills  the  first  n  bytes of the memory area
       pointed to by s with the constant byte c.

It sets a byte value.  K&R just happened to choose 'int' as the type to store that "unsigned char" in.  Probably for very good reasons which make absolutely no sense today.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

On (01/23/17 15:27), Joonsoo Kim wrote:
> Hello,
> 
> Think about following case in 64 bits kernel.
> 
> If value pattern in the page is like as following, we cannot detect
> the same page with 'unsigned int' element.
> 
> AAAAAAAABBBBBBBBAAAAAAAABBBBBBBB...
> 
> 4 bytes is 0xAAAAAAAA and next 4 bytes is 0xBBBBBBBB and so on.

yep, that's exactly the case that I though would be broken
with a 4-bytes pattern matching. so my conlusion was that
for 4 byte pattern we would have working detection anyway,
for 8 bytes patterns we might have some extra matching.
not sure if it matters that much though.

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

On Mon, Jan 23, 2017 at 04:13:39PM +0900, Sergey Senozhatsky wrote:
> On (01/23/17 15:27), Joonsoo Kim wrote:
> > Hello,
> > 
> > Think about following case in 64 bits kernel.
> > 
> > If value pattern in the page is like as following, we cannot detect
> > the same page with 'unsigned int' element.
> > 
> > AAAAAAAABBBBBBBBAAAAAAAABBBBBBBB...
> > 
> > 4 bytes is 0xAAAAAAAA and next 4 bytes is 0xBBBBBBBB and so on.
> 
> yep, that's exactly the case that I though would be broken
> with a 4-bytes pattern matching. so my conlusion was that
> for 4 byte pattern we would have working detection anyway,
> for 8 bytes patterns we might have some extra matching.
> not sure if it matters that much though.

It would be better for deduplication as pattern coverage is bigger
and we cannot guess all of patterns now so it would be never ending
story(i.e., someone claims 16bytes pattern matching would be better).
So, I want to make that path fast rather than increasing dedup ratio
if memset is really fast rather than open-looping. So in future,
if we can prove bigger pattern can increase dedup ratio a lot, then,
we could consider to extend it at the cost of make that path slow.

In summary, zhouxianrong, please test pattern as Joonsoo asked.
So if there are not much benefit with 'long', let's go to the
'int' with memset. And Please resend patch if anyone dosn't oppose
strongly by the time.

Thanks.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

@@ -161,15 +161,55 @@ static bool page_zero_filled(void *ptr)
  {
  	unsigned int pos;
  	unsigned long *page;
+	static unsigned long total;
+	static unsigned long zero;
+	static unsigned long pattern_char;
+	static unsigned long pattern_short;
+	static unsigned long pattern_int;
+	static unsigned long pattern_long;
+	unsigned char *p_char;
+	unsigned short *p_short;
+	unsigned int *p_int;
+	bool retval = false;
+
+	++total;

  	page = (unsigned long *)ptr;

-	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
-		if (page[pos])
-			return false;
+	for (pos = 0; pos < PAGE_SIZE / sizeof(unsigned long) - 1; ++pos) {
+	       if (page[pos] != page[pos + 1])
+	                return false;
  	}

-	return true;
+	p_char = (unsigned char *)ptr;
+	p_short = (unsigned short *)ptr;
+	p_int = (unsigned int *)ptr;
+
+	if (page[0] == 0) {
+		++zero;
+		retval = true;
+	} else if (p_char[0] == p_char[1] &&
+		       p_char[1] == p_char[2] &&
+		       p_char[2] == p_char[3] &&
+		       p_char[3] == p_char[4] &&
+		       p_char[4] == p_char[5] &&
+		       p_char[5] == p_char[6] &&
+		       p_char[6] == p_char[7])
+		++pattern_char;
+	else if (p_short[0] == p_short[1] &&
+		       p_short[1] == p_short[2] &&
+		       p_short[2] == p_short[3])
+		++pattern_short;
+	else if (p_int[0] == p_int[1] &&
+		       p_int[1] == p_int[2])
+		++pattern_int;
+	else {
+		++pattern_long;
+	}
+
+	pr_err("%lld %lld %lld %lld %lld %lld\n", zero, pattern_char, pattern_short, pattern_int, pattern_long, total);
+
+	return retval;
  }

the result as listed below:

zero    pattern_char   pattern_short   pattern_int   pattern_long   total      (unit)
162989  14454          3534            23516         2769           3294399    (page)

statistics for the result:

          pattern zero  pattern char  pattern short  pattern int  pattern long
AVERAGE  0.745696298   0.085937175   0.015957701    0.131874915  0.020533911
STDEV    0.035623777   0.016892402   0.004454534    0.021657123  0.019420072
MAX      0.973813421   0.222222222   0.021409518    0.211812245  0.176512625
MIN      0.645431905   0.004634398   0              0            0


On 2017/1/23 15:40, Minchan Kim wrote:
> On Mon, Jan 23, 2017 at 04:13:39PM +0900, Sergey Senozhatsky wrote:
>> On (01/23/17 15:27), Joonsoo Kim wrote:
>>> Hello,
>>>
>>> Think about following case in 64 bits kernel.
>>>
>>> If value pattern in the page is like as following, we cannot detect
>>> the same page with 'unsigned int' element.
>>>
>>> AAAAAAAABBBBBBBBAAAAAAAABBBBBBBB...
>>>
>>> 4 bytes is 0xAAAAAAAA and next 4 bytes is 0xBBBBBBBB and so on.
>>
>> yep, that's exactly the case that I though would be broken
>> with a 4-bytes pattern matching. so my conlusion was that
>> for 4 byte pattern we would have working detection anyway,
>> for 8 bytes patterns we might have some extra matching.
>> not sure if it matters that much though.
>
> It would be better for deduplication as pattern coverage is bigger
> and we cannot guess all of patterns now so it would be never ending
> story(i.e., someone claims 16bytes pattern matching would be better).
> So, I want to make that path fast rather than increasing dedup ratio
> if memset is really fast rather than open-looping. So in future,
> if we can prove bigger pattern can increase dedup ratio a lot, then,
> we could consider to extend it at the cost of make that path slow.
>
> In summary, zhouxianrong, please test pattern as Joonsoo asked.
> So if there are not much benefit with 'long', let's go to the
> 'int' with memset. And Please resend patch if anyone dosn't oppose
> strongly by the time.
>
> Thanks.
>
>
> .
>

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

Hi zhouxianrong,

On Tue, Jan 24, 2017 at 03:58:02PM +0800, zhouxianrong wrote:
> @@ -161,15 +161,55 @@ static bool page_zero_filled(void *ptr)
>  {
>  	unsigned int pos;
>  	unsigned long *page;
> +	static unsigned long total;
> +	static unsigned long zero;
> +	static unsigned long pattern_char;
> +	static unsigned long pattern_short;
> +	static unsigned long pattern_int;
> +	static unsigned long pattern_long;
> +	unsigned char *p_char;
> +	unsigned short *p_short;
> +	unsigned int *p_int;
> +	bool retval = false;
> +
> +	++total;
> 
>  	page = (unsigned long *)ptr;
> 
> -	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
> -		if (page[pos])
> -			return false;
> +	for (pos = 0; pos < PAGE_SIZE / sizeof(unsigned long) - 1; ++pos) {
> +	       if (page[pos] != page[pos + 1])
> +	                return false;
>  	}
> 
> -	return true;
> +	p_char = (unsigned char *)ptr;
> +	p_short = (unsigned short *)ptr;
> +	p_int = (unsigned int *)ptr;
> +
> +	if (page[0] == 0) {
> +		++zero;
> +		retval = true;
> +	} else if (p_char[0] == p_char[1] &&
> +		       p_char[1] == p_char[2] &&
> +		       p_char[2] == p_char[3] &&
> +		       p_char[3] == p_char[4] &&
> +		       p_char[4] == p_char[5] &&
> +		       p_char[5] == p_char[6] &&
> +		       p_char[6] == p_char[7])
> +		++pattern_char;
> +	else if (p_short[0] == p_short[1] &&
> +		       p_short[1] == p_short[2] &&
> +		       p_short[2] == p_short[3])
> +		++pattern_short;
> +	else if (p_int[0] == p_int[1] &&
> +		       p_int[1] == p_int[2])
> +		++pattern_int;
> +	else {
> +		++pattern_long;
> +	}
> +
> +	pr_err("%lld %lld %lld %lld %lld %lld\n", zero, pattern_char, pattern_short, pattern_int, pattern_long, total);
> +
> +	return retval;
>  }
> 
> the result as listed below:
> 
> zero    pattern_char   pattern_short   pattern_int   pattern_long   total      (unit)
> 162989  14454          3534            23516         2769           3294399    (page)
> 

so, int covers 93%. As considering non-zero dedup hit ratio is low, I think *int* is
enough if memset is really fast. So, I'd like to go with 'int' if Sergey doesn't mind.

Please include the number in description and resend patch, zhouxianrong. :)

Thanks.

> statistics for the result:
> 
>          pattern zero  pattern char  pattern short  pattern int  pattern long
> AVERAGE  0.745696298   0.085937175   0.015957701    0.131874915  0.020533911
> STDEV    0.035623777   0.016892402   0.004454534    0.021657123  0.019420072
> MAX      0.973813421   0.222222222   0.021409518    0.211812245  0.176512625
> MIN      0.645431905   0.004634398   0              0            0
> 
> 
> On 2017/1/23 15:40, Minchan Kim wrote:
> >On Mon, Jan 23, 2017 at 04:13:39PM +0900, Sergey Senozhatsky wrote:
> >>On (01/23/17 15:27), Joonsoo Kim wrote:
> >>>Hello,
> >>>
> >>>Think about following case in 64 bits kernel.
> >>>
> >>>If value pattern in the page is like as following, we cannot detect
> >>>the same page with 'unsigned int' element.
> >>>
> >>>AAAAAAAABBBBBBBBAAAAAAAABBBBBBBB...
> >>>
> >>>4 bytes is 0xAAAAAAAA and next 4 bytes is 0xBBBBBBBB and so on.
> >>
> >>yep, that's exactly the case that I though would be broken
> >>with a 4-bytes pattern matching. so my conlusion was that
> >>for 4 byte pattern we would have working detection anyway,
> >>for 8 bytes patterns we might have some extra matching.
> >>not sure if it matters that much though.
> >
> >It would be better for deduplication as pattern coverage is bigger
> >and we cannot guess all of patterns now so it would be never ending
> >story(i.e., someone claims 16bytes pattern matching would be better).
> >So, I want to make that path fast rather than increasing dedup ratio
> >if memset is really fast rather than open-looping. So in future,
> >if we can prove bigger pattern can increase dedup ratio a lot, then,
> >we could consider to extend it at the cost of make that path slow.
> >
> >In summary, zhouxianrong, please test pattern as Joonsoo asked.
> >So if there are not much benefit with 'long', let's go to the
> >'int' with memset. And Please resend patch if anyone dosn't oppose
> >strongly by the time.
> >
> >Thanks.
> >
> >
> >.
> >
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

Hello,

On (01/25/17 10:29), Minchan Kim wrote:
[..]
> > the result as listed below:
> > 
> > zero    pattern_char   pattern_short   pattern_int   pattern_long   total      (unit)
> > 162989  14454          3534            23516         2769           3294399    (page)
> > 
>
> so, int covers 93%. As considering non-zero dedup hit ratio is low, I think *int* is
> enough if memset is really fast. So, I'd like to go with 'int' if Sergey doesn't mind.

yep, 4 byte pattern matching and memset() sounds like a good plan to me

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Matthew Wilcox]

On Wed, Jan 25, 2017 at 10:32:44AM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (01/25/17 10:29), Minchan Kim wrote:
> [..]
> > > the result as listed below:
> > > 
> > > zero    pattern_char   pattern_short   pattern_int   pattern_long   total      (unit)
> > > 162989  14454          3534            23516         2769           3294399    (page)
> > > 
> >
> > so, int covers 93%. As considering non-zero dedup hit ratio is low, I think *int* is
> > enough if memset is really fast. So, I'd like to go with 'int' if Sergey doesn't mind.
> 
> yep, 4 byte pattern matching and memset() sounds like a good plan to me

what?  memset ONLY HANDLES BYTES.

I pointed this out earlier, but you don't seem to be listening.  Let me
try it again.

MEMSET ONLY HANDLES BYTES.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

On (01/24/17 18:48), Matthew Wilcox wrote:
> On Wed, Jan 25, 2017 at 10:32:44AM +0900, Sergey Senozhatsky wrote:
> > Hello,
> > 
> > On (01/25/17 10:29), Minchan Kim wrote:
> > [..]
> > > > the result as listed below:
> > > > 
> > > > zero    pattern_char   pattern_short   pattern_int   pattern_long   total      (unit)
> > > > 162989  14454          3534            23516         2769           3294399    (page)
> > > > 
> > >
> > > so, int covers 93%. As considering non-zero dedup hit ratio is low, I think *int* is
> > > enough if memset is really fast. So, I'd like to go with 'int' if Sergey doesn't mind.
> > 
> > yep, 4 byte pattern matching and memset() sounds like a good plan to me
> 
> what?  memset ONLY HANDLES BYTES.
> 
> I pointed this out earlier, but you don't seem to be listening.  Let me
> try it again.
> 
> MEMSET ONLY HANDLES BYTES.

dammit... how did that happen...


Matthew, you are absolute right. and, yes, I missed out your previous
mail, indeed. sorry. and thanks for "re-pointing" that out.


Minchan, zhouxianrong, I was completely wrong. we can't
do memset(). d'oh, I did not know it truncates 4 bytes to
one byte only (doesn't make too much sense to me).

my apologies.

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

On Wed, Jan 25, 2017 at 01:18:58PM +0900, Sergey Senozhatsky wrote:
> On (01/24/17 18:48), Matthew Wilcox wrote:
> > On Wed, Jan 25, 2017 at 10:32:44AM +0900, Sergey Senozhatsky wrote:
> > > Hello,
> > > 
> > > On (01/25/17 10:29), Minchan Kim wrote:
> > > [..]
> > > > > the result as listed below:
> > > > > 
> > > > > zero    pattern_char   pattern_short   pattern_int   pattern_long   total      (unit)
> > > > > 162989  14454          3534            23516         2769           3294399    (page)
> > > > > 
> > > >
> > > > so, int covers 93%. As considering non-zero dedup hit ratio is low, I think *int* is
> > > > enough if memset is really fast. So, I'd like to go with 'int' if Sergey doesn't mind.
> > > 
> > > yep, 4 byte pattern matching and memset() sounds like a good plan to me
> > 
> > what?  memset ONLY HANDLES BYTES.
> > 
> > I pointed this out earlier, but you don't seem to be listening.  Let me
> > try it again.
> > 
> > MEMSET ONLY HANDLES BYTES.
> 
> dammit... how did that happen...
> 
> 
> Matthew, you are absolute right. and, yes, I missed out your previous
> mail, indeed. sorry. and thanks for "re-pointing" that out.
> 
> 
> Minchan, zhouxianrong, I was completely wrong. we can't
> do memset(). d'oh, I did not know it truncates 4 bytes to
> one byte only (doesn't make too much sense to me).

Now, I read Matthew's comment and understood. Thanks.
It means zhouxianrong's patch I sent recently is okay?

Thanks.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Sergey Senozhatsky]

On (01/25/17 13:51), Minchan Kim wrote:
[..]
> > Minchan, zhouxianrong, I was completely wrong. we can't
> > do memset(). d'oh, I did not know it truncates 4 bytes to
> > one byte only (doesn't make too much sense to me).
> 
> Now, I read Matthew's comment and understood. Thanks.
> It means zhouxianrong's patch I sent recently is okay?

this one looks OK to me
https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1316290.html


I'd agree with Joonsoo that doing forward prefetching is _probably_ better
than backwards prefetching. not that it necessarily should confuse the CPU
(need to google if ARM handles it normally), but still.

	-ss

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

On Wed, Jan 25, 2017 at 02:38:49PM +0900, Sergey Senozhatsky wrote:
> On (01/25/17 13:51), Minchan Kim wrote:
> [..]
> > > Minchan, zhouxianrong, I was completely wrong. we can't
> > > do memset(). d'oh, I did not know it truncates 4 bytes to
> > > one byte only (doesn't make too much sense to me).
> > 
> > Now, I read Matthew's comment and understood. Thanks.
> > It means zhouxianrong's patch I sent recently is okay?
> 
> this one looks OK to me
> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1316290.html
> 
> 
> I'd agree with Joonsoo that doing forward prefetching is _probably_ better
> than backwards prefetching. not that it necessarily should confuse the CPU
> (need to google if ARM handles it normally), but still.

Okay, let's settle down.

zhouxianrong, please resend one Sergey pointed out with changing to
forward loop. though, sorry for a lot confusion!

> 
> 	-ss
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

From: zhouxianrong <zhouxianrong@huawei.com>

test result as listed below:

zero   pattern_char pattern_short pattern_int pattern_long total   (unit)
162989 14454        3534          23516       2769         3294399 (page)

statistics for the result:

        zero  pattern_char  pattern_short  pattern_int  pattern_long
AVERAGE 0.745696298 0.085937175 0.015957701 0.131874915 0.020533911
STDEV   0.035623777 0.016892402 0.004454534 0.021657123 0.019420072
MAX     0.973813421 0.222222222 0.021409518 0.211812245 0.176512625
MIN     0.645431905 0.004634398 0           0           0

Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
---
 drivers/block/zram/zram_drv.c |  122 ++++++++++++++++++++++++++++++++---------
 drivers/block/zram/zram_drv.h |   11 ++--
 2 files changed, 102 insertions(+), 31 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index e5ab7d9..e826d0d 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -95,6 +95,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
 	meta->table[index].value &= ~BIT(flag);
 }
 
+static inline void zram_set_element(struct zram_meta *meta, u32 index,
+			unsigned long element)
+{
+	meta->table[index].element = element;
+}
+
+static inline void zram_clear_element(struct zram_meta *meta, u32 index)
+{
+	meta->table[index].element = 0;
+}
+
 static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
 {
 	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
@@ -167,31 +178,78 @@ static inline void update_used_max(struct zram *zram,
 	} while (old_max != cur_max);
 }
 
-static bool page_zero_filled(void *ptr)
+static inline void zram_fill_page(char *ptr, unsigned long value)
+{
+	int i;
+	unsigned long *page = (unsigned long *)ptr;
+
+	if (likely(value == 0)) {
+		clear_page(ptr);
+	} else {
+		for (i = 0; i < PAGE_SIZE / sizeof(*page); i++)
+			page[i] = value;
+	}
+}
+
+static inline void zram_fill_page_partial(char *ptr, unsigned int size,
+		unsigned long value)
+{
+	int i;
+	unsigned long *page;
+
+	if (likely(value == 0)) {
+		memset(ptr, 0, size);
+		return;
+	}
+
+	i = ((unsigned long)ptr) % sizeof(*page);
+	if (i) {
+		while (i < sizeof(*page)) {
+			*ptr++ = (value >> (i * 8)) & 0xff;
+			--size;
+			++i;
+		}
+	}
+
+	for (i = size / sizeof(*page); i > 0; --i) {
+		page = (unsigned long *)ptr;
+		*page = value;
+		ptr += sizeof(*page);
+		size -= sizeof(*page);
+	}
+
+	for (i = 0; i < size; ++i)
+		*ptr++ = (value >> (i * 8)) & 0xff;
+}
+
+static bool page_same_filled(void *ptr, unsigned long *element)
 {
 	unsigned int pos;
 	unsigned long *page;
 
 	page = (unsigned long *)ptr;
 
-	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
-		if (page[pos])
+	for (pos = 0; pos < PAGE_SIZE / sizeof(*page) - 1; pos++) {
+		if (page[pos] != page[pos + 1])
 			return false;
 	}
 
+	*element = page[pos];
+
 	return true;
 }
 
-static void handle_zero_page(struct bio_vec *bvec)
+static void handle_same_page(struct bio_vec *bvec, unsigned long element)
 {
 	struct page *page = bvec->bv_page;
 	void *user_mem;
 
 	user_mem = kmap_atomic(page);
 	if (is_partial_io(bvec))
-		memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
+		zram_fill_page_partial(user_mem + bvec->bv_offset, bvec->bv_len,
+			element);
 	else
-		clear_page(user_mem);
+		zram_fill_page(user_mem, element);
 	kunmap_atomic(user_mem);
 
 	flush_dcache_page(page);
@@ -440,7 +498,7 @@ static ssize_t mm_stat_show(struct device *dev,
 			mem_used << PAGE_SHIFT,
 			zram->limit_pages << PAGE_SHIFT,
 			max_used << PAGE_SHIFT,
-			(u64)atomic64_read(&zram->stats.zero_pages),
+			(u64)atomic64_read(&zram->stats.same_pages),
 			pool_stats.pages_compacted);
 	up_read(&zram->init_lock);
 
@@ -473,7 +531,7 @@ static ssize_t debug_stat_show(struct device *dev,
 ZRAM_ATTR_RO(failed_writes);
 ZRAM_ATTR_RO(invalid_io);
 ZRAM_ATTR_RO(notify_free);
-ZRAM_ATTR_RO(zero_pages);
+ZRAM_ATTR_RO(same_pages);
 ZRAM_ATTR_RO(compr_data_size);
 
 static inline bool zram_meta_get(struct zram *zram)
@@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
 
 	/* Free all pages that are still in this zram device */
 	for (index = 0; index < num_pages; index++) {
-		unsigned long handle = meta->table[index].handle;
+		unsigned long handle;
+
+		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+		handle = meta->table[index].handle;
 
-		if (!handle)
+		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
+			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 			continue;
+		}
 
+		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 		zs_free(meta->mem_pool, handle);
 	}
 
@@ -547,18 +611,20 @@ static void zram_free_page(struct zram *zram, size_t index)
 	struct zram_meta *meta = zram->meta;
 	unsigned long handle = meta->table[index].handle;
 
-	if (unlikely(!handle)) {
-		/*
-		 * No memory is allocated for zero filled pages.
-		 * Simply clear zero page flag.
-		 */
-		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
-			zram_clear_flag(meta, index, ZRAM_ZERO);
-			atomic64_dec(&zram->stats.zero_pages);
-		}
+	/*
+	 * No memory is allocated for same element filled pages.
+	 * Simply clear same page flag.
+	 */
+	if (zram_test_flag(meta, index, ZRAM_SAME)) {
+		zram_clear_flag(meta, index, ZRAM_SAME);
+		zram_clear_element(meta, index);
+		atomic64_dec(&zram->stats.same_pages);
 		return;
 	}
 
+	if (unlikely(!handle))
+		return;
+
 	zs_free(meta->mem_pool, handle);
 
 	atomic64_sub(zram_get_obj_size(meta, index),
@@ -581,9 +647,9 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	handle = meta->table[index].handle;
 	size = zram_get_obj_size(meta, index);
 
-	if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) {
+	if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		clear_page(mem);
+		zram_fill_page(mem, meta->table[index].element);
 		return 0;
 	}
 
@@ -619,9 +685,9 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 
 	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
 	if (unlikely(!meta->table[index].handle) ||
-			zram_test_flag(meta, index, ZRAM_ZERO)) {
+			zram_test_flag(meta, index, ZRAM_SAME)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		handle_zero_page(bvec);
+		handle_same_page(bvec, meta->table[index].element);
 		return 0;
 	}
 	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
@@ -669,6 +735,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	struct zram_meta *meta = zram->meta;
 	struct zcomp_strm *zstrm = NULL;
 	unsigned long alloced_pages;
+	unsigned long element;
 
 	page = bvec->bv_page;
 	if (is_partial_io(bvec)) {
@@ -697,16 +764,17 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		uncmem = user_mem;
 	}
 
-	if (page_zero_filled(uncmem)) {
+	if (page_same_filled(uncmem, &element)) {
 		if (user_mem)
 			kunmap_atomic(user_mem);
 		/* Free memory associated with this sector now. */
 		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
 		zram_free_page(zram, index);
-		zram_set_flag(meta, index, ZRAM_ZERO);
+		zram_set_flag(meta, index, ZRAM_SAME);
+		zram_set_element(meta, index, element);
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 
-		atomic64_inc(&zram->stats.zero_pages);
+		atomic64_inc(&zram->stats.same_pages);
 		ret = 0;
 		goto out;
 	}
@@ -1206,7 +1274,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 	&dev_attr_compact.attr,
 	&dev_attr_invalid_io.attr,
 	&dev_attr_notify_free.attr,
-	&dev_attr_zero_pages.attr,
+	&dev_attr_same_pages.attr,
 	&dev_attr_orig_data_size.attr,
 	&dev_attr_compr_data_size.attr,
 	&dev_attr_mem_used_total.attr,
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 74fcf10..4bb92e1 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -60,8 +60,8 @@
 
 /* Flags for zram pages (table[page_no].value) */
 enum zram_pageflags {
-	/* Page consists entirely of zeros */
-	ZRAM_ZERO = ZRAM_FLAG_SHIFT,
+	/* Page consists entirely of same elements */
+	ZRAM_SAME = ZRAM_FLAG_SHIFT,
 	ZRAM_ACCESS,	/* page is now accessed */
 
 	__NR_ZRAM_PAGEFLAGS,
@@ -71,7 +71,10 @@ enum zram_pageflags {
 
 /* Allocated for each disk page */
 struct zram_table_entry {
-	unsigned long handle;
+	union {
+		unsigned long handle;
+		unsigned long element;
+	};
 	unsigned long value;
 };
 
@@ -83,7 +86,7 @@ struct zram_stats {
 	atomic64_t failed_writes;	/* can happen when memory is too low */
 	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
 	atomic64_t notify_free;	/* no. of swap slot free notifications */
-	atomic64_t zero_pages;		/* no. of zero filled pages */
+	atomic64_t same_pages;		/* no. of same element filled pages */
 	atomic64_t pages_stored;	/* no. of pages currently stored */
 	atomic_long_t max_used_pages;	/* no. of maximum pages stored */
 	atomic64_t writestall;		/* no. of write slow paths */
-- 
1.7.9.5

[PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

From: zhouxianrong <zhouxianrong@huawei.com>

test result as listed below:

zero   pattern_char pattern_short pattern_int pattern_long total   (unit)
162989 14454        3534          23516       2769         3294399 (page)

statistics for the result:

        zero  pattern_char  pattern_short  pattern_int  pattern_long
AVERAGE 0.745696298 0.085937175 0.015957701 0.131874915 0.020533911
STDEV   0.035623777 0.016892402 0.004454534 0.021657123 0.019420072
MAX     0.973813421 0.222222222 0.021409518 0.211812245 0.176512625
MIN     0.645431905 0.004634398 0           0           0

Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
---
 drivers/block/zram/zram_drv.c |  124 +++++++++++++++++++++++++++++++----------
 drivers/block/zram/zram_drv.h |   11 ++--
 2 files changed, 103 insertions(+), 32 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index e5ab7d9..6a8c9c5 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -95,6 +95,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
 	meta->table[index].value &= ~BIT(flag);
 }
 
+static inline void zram_set_element(struct zram_meta *meta, u32 index,
+			unsigned long element)
+{
+	meta->table[index].element = element;
+}
+
+static inline void zram_clear_element(struct zram_meta *meta, u32 index)
+{
+	meta->table[index].element = 0;
+}
+
 static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
 {
 	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
@@ -167,31 +178,78 @@ static inline void update_used_max(struct zram *zram,
 	} while (old_max != cur_max);
 }
 
-static bool page_zero_filled(void *ptr)
+static inline void zram_fill_page(char *ptr, unsigned long value)
+{
+	int i;
+	unsigned long *page = (unsigned long *)ptr;
+
+	if (likely(value == 0)) {
+		clear_page(ptr);
+	} else {
+		for (i = 0; i < PAGE_SIZE / sizeof(*page); i++)
+			page[i] = value;
+	}
+}
+
+static inline void zram_fill_page_partial(char *ptr, unsigned int size,
+		unsigned long value)
+{
+	int i;
+	unsigned long *page;
+
+	if (likely(value == 0)) {
+		memset(ptr, 0, size);
+		return;
+	}
+
+	i = ((unsigned long)ptr) % sizeof(*page);
+	if (i) {
+		while (i < sizeof(*page)) {
+			*ptr++ = (value >> (i * 8)) & 0xff;
+			--size;
+			++i;
+		}
+	}
+
+	for (i = size / sizeof(*page); i > 0; --i) {
+		page = (unsigned long *)ptr;
+		*page = value;
+		ptr += sizeof(*page);
+		size -= sizeof(*page);
+	}
+
+	for (i = 0; i < size; ++i)
+		*ptr++ = (value >> (i * 8)) & 0xff;
+}
+
+static bool page_same_filled(void *ptr, unsigned long *element)
 {
 	unsigned int pos;
 	unsigned long *page;
 
 	page = (unsigned long *)ptr;
 
-	for (pos = 0; pos != PAGE_SIZE / sizeof(*page); pos++) {
-		if (page[pos])
+	for (pos = 0; pos < PAGE_SIZE / sizeof(*page) - 1; pos++) {
+		if (page[pos] != page[pos + 1])
 			return false;
 	}
 
+	*element = page[pos];
+
 	return true;
 }
 
-static void handle_zero_page(struct bio_vec *bvec)
+static void handle_same_page(struct bio_vec *bvec, unsigned long element)
 {
 	struct page *page = bvec->bv_page;
 	void *user_mem;
 
 	user_mem = kmap_atomic(page);
 	if (is_partial_io(bvec))
-		memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
+		zram_fill_page_partial(user_mem + bvec->bv_offset, bvec->bv_len,
+			element);
 	else
-		clear_page(user_mem);
+		zram_fill_page(user_mem, element);
 	kunmap_atomic(user_mem);
 
 	flush_dcache_page(page);
@@ -440,7 +498,7 @@ static ssize_t mm_stat_show(struct device *dev,
 			mem_used << PAGE_SHIFT,
 			zram->limit_pages << PAGE_SHIFT,
 			max_used << PAGE_SHIFT,
-			(u64)atomic64_read(&zram->stats.zero_pages),
+			(u64)atomic64_read(&zram->stats.same_pages),
 			pool_stats.pages_compacted);
 	up_read(&zram->init_lock);
 
@@ -473,7 +531,7 @@ static ssize_t debug_stat_show(struct device *dev,
 ZRAM_ATTR_RO(failed_writes);
 ZRAM_ATTR_RO(invalid_io);
 ZRAM_ATTR_RO(notify_free);
-ZRAM_ATTR_RO(zero_pages);
+ZRAM_ATTR_RO(same_pages);
 ZRAM_ATTR_RO(compr_data_size);
 
 static inline bool zram_meta_get(struct zram *zram)
@@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
 
 	/* Free all pages that are still in this zram device */
 	for (index = 0; index < num_pages; index++) {
-		unsigned long handle = meta->table[index].handle;
+		unsigned long handle;
+
+		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+		handle = meta->table[index].handle;
 
-		if (!handle)
+		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
+			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 			continue;
+		}
 
+		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 		zs_free(meta->mem_pool, handle);
 	}
 
@@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
 static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
 {
 	size_t num_pages;
-	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
+	struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL);
 
 	if (!meta)
 		return NULL;
@@ -547,18 +611,20 @@ static void zram_free_page(struct zram *zram, size_t index)
 	struct zram_meta *meta = zram->meta;
 	unsigned long handle = meta->table[index].handle;
 
-	if (unlikely(!handle)) {
-		/*
-		 * No memory is allocated for zero filled pages.
-		 * Simply clear zero page flag.
-		 */
-		if (zram_test_flag(meta, index, ZRAM_ZERO)) {
-			zram_clear_flag(meta, index, ZRAM_ZERO);
-			atomic64_dec(&zram->stats.zero_pages);
-		}
+	/*
+	 * No memory is allocated for same element filled pages.
+	 * Simply clear same page flag.
+	 */
+	if (zram_test_flag(meta, index, ZRAM_SAME)) {
+		zram_clear_flag(meta, index, ZRAM_SAME);
+		zram_clear_element(meta, index);
+		atomic64_dec(&zram->stats.same_pages);
 		return;
 	}
 
+	if (unlikely(!handle))
+		return;
+
 	zs_free(meta->mem_pool, handle);
 
 	atomic64_sub(zram_get_obj_size(meta, index),
@@ -581,9 +647,9 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	handle = meta->table[index].handle;
 	size = zram_get_obj_size(meta, index);
 
-	if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) {
+	if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		clear_page(mem);
+		zram_fill_page(mem, meta->table[index].element);
 		return 0;
 	}
 
@@ -619,9 +685,9 @@ static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
 
 	bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
 	if (unlikely(!meta->table[index].handle) ||
-			zram_test_flag(meta, index, ZRAM_ZERO)) {
+			zram_test_flag(meta, index, ZRAM_SAME)) {
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
-		handle_zero_page(bvec);
+		handle_same_page(bvec, meta->table[index].element);
 		return 0;
 	}
 	bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
@@ -669,6 +735,7 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	struct zram_meta *meta = zram->meta;
 	struct zcomp_strm *zstrm = NULL;
 	unsigned long alloced_pages;
+	unsigned long element;
 
 	page = bvec->bv_page;
 	if (is_partial_io(bvec)) {
@@ -697,16 +764,17 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		uncmem = user_mem;
 	}
 
-	if (page_zero_filled(uncmem)) {
+	if (page_same_filled(uncmem, &element)) {
 		if (user_mem)
 			kunmap_atomic(user_mem);
 		/* Free memory associated with this sector now. */
 		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
 		zram_free_page(zram, index);
-		zram_set_flag(meta, index, ZRAM_ZERO);
+		zram_set_flag(meta, index, ZRAM_SAME);
+		zram_set_element(meta, index, element);
 		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
 
-		atomic64_inc(&zram->stats.zero_pages);
+		atomic64_inc(&zram->stats.same_pages);
 		ret = 0;
 		goto out;
 	}
@@ -1206,7 +1274,7 @@ static int zram_open(struct block_device *bdev, fmode_t mode)
 	&dev_attr_compact.attr,
 	&dev_attr_invalid_io.attr,
 	&dev_attr_notify_free.attr,
-	&dev_attr_zero_pages.attr,
+	&dev_attr_same_pages.attr,
 	&dev_attr_orig_data_size.attr,
 	&dev_attr_compr_data_size.attr,
 	&dev_attr_mem_used_total.attr,
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 74fcf10..4bb92e1 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -60,8 +60,8 @@
 
 /* Flags for zram pages (table[page_no].value) */
 enum zram_pageflags {
-	/* Page consists entirely of zeros */
-	ZRAM_ZERO = ZRAM_FLAG_SHIFT,
+	/* Page consists entirely of same elements */
+	ZRAM_SAME = ZRAM_FLAG_SHIFT,
 	ZRAM_ACCESS,	/* page is now accessed */
 
 	__NR_ZRAM_PAGEFLAGS,
@@ -71,7 +71,10 @@ enum zram_pageflags {
 
 /* Allocated for each disk page */
 struct zram_table_entry {
-	unsigned long handle;
+	union {
+		unsigned long handle;
+		unsigned long element;
+	};
 	unsigned long value;
 };
 
@@ -83,7 +86,7 @@ struct zram_stats {
 	atomic64_t failed_writes;	/* can happen when memory is too low */
 	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
 	atomic64_t notify_free;	/* no. of swap slot free notifications */
-	atomic64_t zero_pages;		/* no. of zero filled pages */
+	atomic64_t same_pages;		/* no. of same element filled pages */
 	atomic64_t pages_stored;	/* no. of pages currently stored */
 	atomic_long_t max_used_pages;	/* no. of maximum pages stored */
 	atomic64_t writestall;		/* no. of write slow paths */
-- 
1.7.9.5

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Matthew Wilcox]

On Fri, Feb 03, 2017 at 04:42:27PM +0800, zhouxianrong@huawei.com wrote:
> +static inline void zram_fill_page_partial(char *ptr, unsigned int size,
> +		unsigned long value)
> +{
> +	int i;
> +	unsigned long *page;
> +
> +	if (likely(value == 0)) {
> +		memset(ptr, 0, size);
> +		return;
> +	}
> +
> +	i = ((unsigned long)ptr) % sizeof(*page);
> +	if (i) {
> +		while (i < sizeof(*page)) {
> +			*ptr++ = (value >> (i * 8)) & 0xff;
> +			--size;
> +			++i;
> +		}
> +	}
> +
> +	for (i = size / sizeof(*page); i > 0; --i) {
> +		page = (unsigned long *)ptr;
> +		*page = value;
> +		ptr += sizeof(*page);
> +		size -= sizeof(*page);
> +	}
> +
> +	for (i = 0; i < size; ++i)
> +		*ptr++ = (value >> (i * 8)) & 0xff;
> +}

You're assuming little-endian here.  I think you need to do a
cpu_to_le() here, but I don't think we have a cpu_to_leul, only
cpu_to_le64/cpu_to_le32.  So you may have some work to do ...

Re: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

right, thanks.

On 2017/2/3 23:33, Matthew Wilcox wrote:
> On Fri, Feb 03, 2017 at 04:42:27PM +0800, zhouxianrong@huawei.com wrote:
>> +static inline void zram_fill_page_partial(char *ptr, unsigned int size,
>> +		unsigned long value)
>> +{
>> +	int i;
>> +	unsigned long *page;
>> +
>> +	if (likely(value == 0)) {
>> +		memset(ptr, 0, size);
>> +		return;
>> +	}
>> +
>> +	i = ((unsigned long)ptr) % sizeof(*page);
>> +	if (i) {
>> +		while (i < sizeof(*page)) {
>> +			*ptr++ = (value >> (i * 8)) & 0xff;
>> +			--size;
>> +			++i;
>> +		}
>> +	}
>> +
>> +	for (i = size / sizeof(*page); i > 0; --i) {
>> +		page = (unsigned long *)ptr;
>> +		*page = value;
>> +		ptr += sizeof(*page);
>> +		size -= sizeof(*page);
>> +	}
>> +
>> +	for (i = 0; i < size; ++i)
>> +		*ptr++ = (value >> (i * 8)) & 0xff;
>> +}
>
> You're assuming little-endian here.  I think you need to do a
> cpu_to_le() here, but I don't think we have a cpu_to_leul, only
> cpu_to_le64/cpu_to_le32.  So you may have some work to do ...
>
>
> .
>

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

Hi zhouxianrong,

On Fri, Feb 03, 2017 at 04:42:27PM +0800, zhouxianrong@huawei.com wrote:
> From: zhouxianrong <zhouxianrong@huawei.com>
> 
> test result as listed below:
> 
> zero   pattern_char pattern_short pattern_int pattern_long total   (unit)
> 162989 14454        3534          23516       2769         3294399 (page)
> 
> statistics for the result:
> 
>         zero  pattern_char  pattern_short  pattern_int  pattern_long
> AVERAGE 0.745696298 0.085937175 0.015957701 0.131874915 0.020533911
> STDEV   0.035623777 0.016892402 0.004454534 0.021657123 0.019420072
> MAX     0.973813421 0.222222222 0.021409518 0.211812245 0.176512625
> MIN     0.645431905 0.004634398 0           0           0

The description in old version was better for justifying same page merging
feature.

> 
> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
> ---
>  drivers/block/zram/zram_drv.c |  124 +++++++++++++++++++++++++++++++----------
>  drivers/block/zram/zram_drv.h |   11 ++--
>  2 files changed, 103 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index e5ab7d9..6a8c9c5 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -95,6 +95,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
>  	meta->table[index].value &= ~BIT(flag);
>  }
>  
> +static inline void zram_set_element(struct zram_meta *meta, u32 index,
> +			unsigned long element)
> +{
> +	meta->table[index].element = element;
> +}
> +
> +static inline void zram_clear_element(struct zram_meta *meta, u32 index)
> +{
> +	meta->table[index].element = 0;
> +}
> +
>  static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
>  {
>  	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
> @@ -167,31 +178,78 @@ static inline void update_used_max(struct zram *zram,
>  	} while (old_max != cur_max);
>  }
>  
> -static bool page_zero_filled(void *ptr)
> +static inline void zram_fill_page(char *ptr, unsigned long value)
> +{
> +	int i;
> +	unsigned long *page = (unsigned long *)ptr;
> +
> +	if (likely(value == 0)) {
> +		clear_page(ptr);
> +	} else {
> +		for (i = 0; i < PAGE_SIZE / sizeof(*page); i++)
> +			page[i] = value;
> +	}
> +}
> +
> +static inline void zram_fill_page_partial(char *ptr, unsigned int size,
> +		unsigned long value)
> +{
> +	int i;
> +	unsigned long *page;
> +
> +	if (likely(value == 0)) {
> +		memset(ptr, 0, size);
> +		return;
> +	}
> +
> +	i = ((unsigned long)ptr) % sizeof(*page);
> +	if (i) {
> +		while (i < sizeof(*page)) {
> +			*ptr++ = (value >> (i * 8)) & 0xff;
> +			--size;
> +			++i;
> +		}
> +	}
> +

I don't think we need this part because block layer works with sector
size or multiple times of it so it must be aligned unsigned long.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

On 2017/2/5 22:21, Minchan Kim wrote:
> Hi zhouxianrong,
>
> On Fri, Feb 03, 2017 at 04:42:27PM +0800, zhouxianrong@huawei.com wrote:
>> From: zhouxianrong <zhouxianrong@huawei.com>
>>
>> test result as listed below:
>>
>> zero   pattern_char pattern_short pattern_int pattern_long total   (unit)
>> 162989 14454        3534          23516       2769         3294399 (page)
>>
>> statistics for the result:
>>
>>         zero  pattern_char  pattern_short  pattern_int  pattern_long
>> AVERAGE 0.745696298 0.085937175 0.015957701 0.131874915 0.020533911
>> STDEV   0.035623777 0.016892402 0.004454534 0.021657123 0.019420072
>> MAX     0.973813421 0.222222222 0.021409518 0.211812245 0.176512625
>> MIN     0.645431905 0.004634398 0           0           0
>
> The description in old version was better for justifying same page merging
> feature.
>
>>
>> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
>> ---
>>  drivers/block/zram/zram_drv.c |  124 +++++++++++++++++++++++++++++++----------
>>  drivers/block/zram/zram_drv.h |   11 ++--
>>  2 files changed, 103 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>> index e5ab7d9..6a8c9c5 100644
>> --- a/drivers/block/zram/zram_drv.c
>> +++ b/drivers/block/zram/zram_drv.c
>> @@ -95,6 +95,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
>>  	meta->table[index].value &= ~BIT(flag);
>>  }
>>
>> +static inline void zram_set_element(struct zram_meta *meta, u32 index,
>> +			unsigned long element)
>> +{
>> +	meta->table[index].element = element;
>> +}
>> +
>> +static inline void zram_clear_element(struct zram_meta *meta, u32 index)
>> +{
>> +	meta->table[index].element = 0;
>> +}
>> +
>>  static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
>>  {
>>  	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
>> @@ -167,31 +178,78 @@ static inline void update_used_max(struct zram *zram,
>>  	} while (old_max != cur_max);
>>  }
>>
>> -static bool page_zero_filled(void *ptr)
>> +static inline void zram_fill_page(char *ptr, unsigned long value)
>> +{
>> +	int i;
>> +	unsigned long *page = (unsigned long *)ptr;
>> +
>> +	if (likely(value == 0)) {
>> +		clear_page(ptr);
>> +	} else {
>> +		for (i = 0; i < PAGE_SIZE / sizeof(*page); i++)
>> +			page[i] = value;
>> +	}
>> +}
>> +
>> +static inline void zram_fill_page_partial(char *ptr, unsigned int size,
>> +		unsigned long value)
>> +{
>> +	int i;
>> +	unsigned long *page;
>> +
>> +	if (likely(value == 0)) {
>> +		memset(ptr, 0, size);
>> +		return;
>> +	}
>> +
>> +	i = ((unsigned long)ptr) % sizeof(*page);
>> +	if (i) {
>> +		while (i < sizeof(*page)) {
>> +			*ptr++ = (value >> (i * 8)) & 0xff;
>> +			--size;
>> +			++i;
>> +		}
>> +	}
>> +
>
> I don't think we need this part because block layer works with sector
> size or multiple times of it so it must be aligned unsigned long.
>
>
>
>
> .
>

Minchan and Matthew Wilcox:

1. right, but users could open /dev/block/zram0 file and do any read operations.

2. about endian operation for long, the modification is trivial and low efficient.
    i have not better method. do you have any good idea for this?

3. the below should be modified.

static inline bool zram_meta_get(struct zram *zram)
@@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)

  	/* Free all pages that are still in this zram device */
  	for (index = 0; index < num_pages; index++) {
-		unsigned long handle = meta->table[index].handle;
+		unsigned long handle;
+
+		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
+		handle = meta->table[index].handle;

-		if (!handle)
+		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
+			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
  			continue;
+		}

+		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
  		zs_free(meta->mem_pool, handle);
  	}

@@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
  static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
  {
  	size_t num_pages;
-	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
+	struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL);

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Matthew Wilcox]

On Mon, Feb 06, 2017 at 09:28:18AM +0800, zhouxianrong wrote:
> > > +static inline void zram_fill_page_partial(char *ptr, unsigned int size,
> > > +		unsigned long value)
> > > +{
> > > +	int i;
> > > +	unsigned long *page;
> > > +
> > > +	if (likely(value == 0)) {
> > > +		memset(ptr, 0, size);
> > > +		return;
> > > +	}
> > > +
> > > +	i = ((unsigned long)ptr) % sizeof(*page);
> > > +	if (i) {
> > > +		while (i < sizeof(*page)) {
> > > +			*ptr++ = (value >> (i * 8)) & 0xff;
> > > +			--size;
> > > +			++i;
> > > +		}
> > > +	}
> > > +
> > 
> > I don't think we need this part because block layer works with sector
> > size or multiple times of it so it must be aligned unsigned long.
> 
> Minchan and Matthew Wilcox:
> 
> 1. right, but users could open /dev/block/zram0 file and do any read operations.

But any such read operation would go through the page cache, so will
be page aligned.  Unless they do an O_DIRECT operation, in which case
it must be aligned to block size.  Please, try it.  I/Os which are not
aligned should be failed long before they reach your driver.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

Hi

On Mon, Feb 06, 2017 at 09:28:18AM +0800, zhouxianrong wrote:
> 
> 
> On 2017/2/5 22:21, Minchan Kim wrote:
> >Hi zhouxianrong,
> >
> >On Fri, Feb 03, 2017 at 04:42:27PM +0800, zhouxianrong@huawei.com wrote:
> >>From: zhouxianrong <zhouxianrong@huawei.com>
> >>
> >>test result as listed below:
> >>
> >>zero   pattern_char pattern_short pattern_int pattern_long total   (unit)
> >>162989 14454        3534          23516       2769         3294399 (page)
> >>
> >>statistics for the result:
> >>
> >>        zero  pattern_char  pattern_short  pattern_int  pattern_long
> >>AVERAGE 0.745696298 0.085937175 0.015957701 0.131874915 0.020533911
> >>STDEV   0.035623777 0.016892402 0.004454534 0.021657123 0.019420072
> >>MAX     0.973813421 0.222222222 0.021409518 0.211812245 0.176512625
> >>MIN     0.645431905 0.004634398 0           0           0
> >
> >The description in old version was better for justifying same page merging
> >feature.
> >
> >>
> >>Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
> >>---
> >> drivers/block/zram/zram_drv.c |  124 +++++++++++++++++++++++++++++++----------
> >> drivers/block/zram/zram_drv.h |   11 ++--
> >> 2 files changed, 103 insertions(+), 32 deletions(-)
> >>
> >>diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> >>index e5ab7d9..6a8c9c5 100644
> >>--- a/drivers/block/zram/zram_drv.c
> >>+++ b/drivers/block/zram/zram_drv.c
> >>@@ -95,6 +95,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
> >> 	meta->table[index].value &= ~BIT(flag);
> >> }
> >>
> >>+static inline void zram_set_element(struct zram_meta *meta, u32 index,
> >>+			unsigned long element)
> >>+{
> >>+	meta->table[index].element = element;
> >>+}
> >>+
> >>+static inline void zram_clear_element(struct zram_meta *meta, u32 index)
> >>+{
> >>+	meta->table[index].element = 0;
> >>+}
> >>+
> >> static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
> >> {
> >> 	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
> >>@@ -167,31 +178,78 @@ static inline void update_used_max(struct zram *zram,
> >> 	} while (old_max != cur_max);
> >> }
> >>
> >>-static bool page_zero_filled(void *ptr)
> >>+static inline void zram_fill_page(char *ptr, unsigned long value)
> >>+{
> >>+	int i;
> >>+	unsigned long *page = (unsigned long *)ptr;
> >>+
> >>+	if (likely(value == 0)) {
> >>+		clear_page(ptr);
> >>+	} else {
> >>+		for (i = 0; i < PAGE_SIZE / sizeof(*page); i++)
> >>+			page[i] = value;
> >>+	}
> >>+}
> >>+
> >>+static inline void zram_fill_page_partial(char *ptr, unsigned int size,
> >>+		unsigned long value)
> >>+{
> >>+	int i;
> >>+	unsigned long *page;
> >>+
> >>+	if (likely(value == 0)) {
> >>+		memset(ptr, 0, size);
> >>+		return;
> >>+	}
> >>+
> >>+	i = ((unsigned long)ptr) % sizeof(*page);
> >>+	if (i) {
> >>+		while (i < sizeof(*page)) {
> >>+			*ptr++ = (value >> (i * 8)) & 0xff;
> >>+			--size;
> >>+			++i;
> >>+		}
> >>+	}
> >>+
> >
> >I don't think we need this part because block layer works with sector
> >size or multiple times of it so it must be aligned unsigned long.
> >
> >
> >
> >
> >.
> >
> 
> Minchan and Matthew Wilcox:
> 
> 1. right, but users could open /dev/block/zram0 file and do any read operations.

Could you make that happen?
I don't think it's possible as Matthew already pointed out, too.

> 
> 2. about endian operation for long, the modification is trivial and low efficient.
>    i have not better method. do you have any good idea for this?

So if assumption 1 is wrong, we don't need 2, either.

> 
> 3. the below should be modified.
> 
> static inline bool zram_meta_get(struct zram *zram)
> @@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
> 
>  	/* Free all pages that are still in this zram device */
>  	for (index = 0; index < num_pages; index++) {
> -		unsigned long handle = meta->table[index].handle;
> +		unsigned long handle;
> +
> +		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
> +		handle = meta->table[index].handle;
> 
> -		if (!handle)
> +		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
> +			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>  			continue;
> +		}
> 
> +		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>  		zs_free(meta->mem_pool, handle);

Could you explain why we need this modification?

>  	}
> 
> @@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
>  static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
>  {
>  	size_t num_pages;
> -	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
> +	struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL);

Ditto

> 
> 

Re: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

On 2017/2/7 7:48, Minchan Kim wrote:
> Hi
>
> On Mon, Feb 06, 2017 at 09:28:18AM +0800, zhouxianrong wrote:
>>
>>
>> On 2017/2/5 22:21, Minchan Kim wrote:
>>> Hi zhouxianrong,
>>>
>>> On Fri, Feb 03, 2017 at 04:42:27PM +0800, zhouxianrong@huawei.com wrote:
>>>> From: zhouxianrong <zhouxianrong@huawei.com>
>>>>
>>>> test result as listed below:
>>>>
>>>> zero   pattern_char pattern_short pattern_int pattern_long total   (unit)
>>>> 162989 14454        3534          23516       2769         3294399 (page)
>>>>
>>>> statistics for the result:
>>>>
>>>>        zero  pattern_char  pattern_short  pattern_int  pattern_long
>>>> AVERAGE 0.745696298 0.085937175 0.015957701 0.131874915 0.020533911
>>>> STDEV   0.035623777 0.016892402 0.004454534 0.021657123 0.019420072
>>>> MAX     0.973813421 0.222222222 0.021409518 0.211812245 0.176512625
>>>> MIN     0.645431905 0.004634398 0           0           0
>>>
>>> The description in old version was better for justifying same page merging
>>> feature.
>>>
>>>>
>>>> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
>>>> ---
>>>> drivers/block/zram/zram_drv.c |  124 +++++++++++++++++++++++++++++++----------
>>>> drivers/block/zram/zram_drv.h |   11 ++--
>>>> 2 files changed, 103 insertions(+), 32 deletions(-)
>>>>
>>>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>>>> index e5ab7d9..6a8c9c5 100644
>>>> --- a/drivers/block/zram/zram_drv.c
>>>> +++ b/drivers/block/zram/zram_drv.c
>>>> @@ -95,6 +95,17 @@ static void zram_clear_flag(struct zram_meta *meta, u32 index,
>>>> 	meta->table[index].value &= ~BIT(flag);
>>>> }
>>>>
>>>> +static inline void zram_set_element(struct zram_meta *meta, u32 index,
>>>> +			unsigned long element)
>>>> +{
>>>> +	meta->table[index].element = element;
>>>> +}
>>>> +
>>>> +static inline void zram_clear_element(struct zram_meta *meta, u32 index)
>>>> +{
>>>> +	meta->table[index].element = 0;
>>>> +}
>>>> +
>>>> static size_t zram_get_obj_size(struct zram_meta *meta, u32 index)
>>>> {
>>>> 	return meta->table[index].value & (BIT(ZRAM_FLAG_SHIFT) - 1);
>>>> @@ -167,31 +178,78 @@ static inline void update_used_max(struct zram *zram,
>>>> 	} while (old_max != cur_max);
>>>> }
>>>>
>>>> -static bool page_zero_filled(void *ptr)
>>>> +static inline void zram_fill_page(char *ptr, unsigned long value)
>>>> +{
>>>> +	int i;
>>>> +	unsigned long *page = (unsigned long *)ptr;
>>>> +
>>>> +	if (likely(value == 0)) {
>>>> +		clear_page(ptr);
>>>> +	} else {
>>>> +		for (i = 0; i < PAGE_SIZE / sizeof(*page); i++)
>>>> +			page[i] = value;
>>>> +	}
>>>> +}
>>>> +
>>>> +static inline void zram_fill_page_partial(char *ptr, unsigned int size,
>>>> +		unsigned long value)
>>>> +{
>>>> +	int i;
>>>> +	unsigned long *page;
>>>> +
>>>> +	if (likely(value == 0)) {
>>>> +		memset(ptr, 0, size);
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	i = ((unsigned long)ptr) % sizeof(*page);
>>>> +	if (i) {
>>>> +		while (i < sizeof(*page)) {
>>>> +			*ptr++ = (value >> (i * 8)) & 0xff;
>>>> +			--size;
>>>> +			++i;
>>>> +		}
>>>> +	}
>>>> +
>>>
>>> I don't think we need this part because block layer works with sector
>>> size or multiple times of it so it must be aligned unsigned long.
>>>
>>>
>>>
>>>
>>> .
>>>
>>
>> Minchan and Matthew Wilcox:
>>
>> 1. right, but users could open /dev/block/zram0 file and do any read operations.
>
> Could you make that happen?
> I don't think it's possible as Matthew already pointed out, too.

yes, Matthew is right, thanks

>
>>
>> 2. about endian operation for long, the modification is trivial and low efficient.
>>    i have not better method. do you have any good idea for this?
>
> So if assumption 1 is wrong, we don't need 2, either.

yes

>
>>
>> 3. the below should be modified.
>>
>> static inline bool zram_meta_get(struct zram *zram)
>> @@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
>>
>>  	/* Free all pages that are still in this zram device */
>>  	for (index = 0; index < num_pages; index++) {
>> -		unsigned long handle = meta->table[index].handle;
>> +		unsigned long handle;
>> +
>> +		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
>> +		handle = meta->table[index].handle;
>>
>> -		if (!handle)
>> +		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
>> +			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>>  			continue;
>> +		}
>>
>> +		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>>  		zs_free(meta->mem_pool, handle);
>
> Could you explain why we need this modification?
>
>>  	}
>>
>> @@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
>>  static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
>>  {
>>  	size_t num_pages;
>> -	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
>> +	struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL);
>
> Ditto
>
>>
>>
>
> .
>

because of union of handle and element, i think a non-zero element (other than handle) is prevented from freeing.
if zram_meta_get was modified, zram_meta_alloc did so.

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

On Tue, Feb 07, 2017 at 10:20:57AM +0800, zhouxianrong wrote:

< snip >

> >>3. the below should be modified.
> >>
> >>static inline bool zram_meta_get(struct zram *zram)
> >>@@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
> >>
> >> 	/* Free all pages that are still in this zram device */
> >> 	for (index = 0; index < num_pages; index++) {
> >>-		unsigned long handle = meta->table[index].handle;
> >>+		unsigned long handle;
> >>+
> >>+		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
> >>+		handle = meta->table[index].handle;
> >>
> >>-		if (!handle)
> >>+		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
> >>+			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> >> 			continue;
> >>+		}
> >>
> >>+		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> >> 		zs_free(meta->mem_pool, handle);
> >
> >Could you explain why we need this modification?
> >
> >> 	}
> >>
> >>@@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
> >> static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
> >> {
> >> 	size_t num_pages;
> >>-	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
> >>+	struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL);
> >
> >Ditto
> >
> >>
> >>
> >
> >.
> >
> 
> because of union of handle and element, i think a non-zero element (other than handle) is prevented from freeing.
> if zram_meta_get was modified, zram_meta_alloc did so.

Right. Thanks but I don't see why we need the locking in there and modification of
zram_meta_alloc.

Isn't it enough with this?

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index c20b05a84f21..a25d34a8af19 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -425,8 +425,11 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
 	/* Free all pages that are still in this zram device */
 	for (index = 0; index < num_pages; index++) {
 		unsigned long handle = meta->table[index].handle;
-
-		if (!handle)
+		/*
+		 * No memory is allocated for same element filled pages.
+		 * Simply clear same page flag.
+		 */
+		if (!handle || zram_test_flag(meta, index, ZRAM_SAME))
 			continue;
 
 		zs_free(meta->mem_pool, handle);

Re: [PATCH] mm: extend zero pages to same element pages for zram

[zhouxianrong]

On 2017/2/7 10:54, Minchan Kim wrote:
> On Tue, Feb 07, 2017 at 10:20:57AM +0800, zhouxianrong wrote:
>
> < snip >
>
>>>> 3. the below should be modified.
>>>>
>>>> static inline bool zram_meta_get(struct zram *zram)
>>>> @@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
>>>>
>>>> 	/* Free all pages that are still in this zram device */
>>>> 	for (index = 0; index < num_pages; index++) {
>>>> -		unsigned long handle = meta->table[index].handle;
>>>> +		unsigned long handle;
>>>> +
>>>> +		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
>>>> +		handle = meta->table[index].handle;
>>>>
>>>> -		if (!handle)
>>>> +		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
>>>> +			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>>>> 			continue;
>>>> +		}
>>>>
>>>> +		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
>>>> 		zs_free(meta->mem_pool, handle);
>>>
>>> Could you explain why we need this modification?
>>>
>>>> 	}
>>>>
>>>> @@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
>>>> static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
>>>> {
>>>> 	size_t num_pages;
>>>> -	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
>>>> +	struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL);
>>>
>>> Ditto
>>>
>>>>
>>>>
>>>
>>> .
>>>
>>
>> because of union of handle and element, i think a non-zero element (other than handle) is prevented from freeing.
>> if zram_meta_get was modified, zram_meta_alloc did so.
>
> Right. Thanks but I don't see why we need the locking in there and modification of
> zram_meta_alloc.
>
> Isn't it enough with this?

i am afraid someone do reset_store, so did lock.

yes, i am wrong, zram_meta_alloc should not be modified here. because meta->table has already cleared

	meta->table = vzalloc(num_pages * sizeof(*meta->table));




>
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index c20b05a84f21..a25d34a8af19 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -425,8 +425,11 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
>  	/* Free all pages that are still in this zram device */
>  	for (index = 0; index < num_pages; index++) {
>  		unsigned long handle = meta->table[index].handle;
> -
> -		if (!handle)
> +		/*
> +		 * No memory is allocated for same element filled pages.
> +		 * Simply clear same page flag.
> +		 */
> +		if (!handle || zram_test_flag(meta, index, ZRAM_SAME))
>  			continue;
>
>  		zs_free(meta->mem_pool, handle);
>
> .
>

Re: [PATCH] mm: extend zero pages to same element pages for zram

[Minchan Kim]

On Tue, Feb 07, 2017 at 11:24:40AM +0800, zhouxianrong wrote:
> 
> 
> On 2017/2/7 10:54, Minchan Kim wrote:
> >On Tue, Feb 07, 2017 at 10:20:57AM +0800, zhouxianrong wrote:
> >
> >< snip >
> >
> >>>>3. the below should be modified.
> >>>>
> >>>>static inline bool zram_meta_get(struct zram *zram)
> >>>>@@ -495,11 +553,17 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
> >>>>
> >>>>	/* Free all pages that are still in this zram device */
> >>>>	for (index = 0; index < num_pages; index++) {
> >>>>-		unsigned long handle = meta->table[index].handle;
> >>>>+		unsigned long handle;
> >>>>+
> >>>>+		bit_spin_lock(ZRAM_ACCESS, &meta->table[index].value);
> >>>>+		handle = meta->table[index].handle;
> >>>>
> >>>>-		if (!handle)
> >>>>+		if (!handle || zram_test_flag(meta, index, ZRAM_SAME)) {
> >>>>+			bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> >>>>			continue;
> >>>>+		}
> >>>>
> >>>>+		bit_spin_unlock(ZRAM_ACCESS, &meta->table[index].value);
> >>>>		zs_free(meta->mem_pool, handle);
> >>>
> >>>Could you explain why we need this modification?
> >>>
> >>>>	}
> >>>>
> >>>>@@ -511,7 +575,7 @@ static void zram_meta_free(struct zram_meta *meta, u64 disksize)
> >>>>static struct zram_meta *zram_meta_alloc(char *pool_name, u64 disksize)
> >>>>{
> >>>>	size_t num_pages;
> >>>>-	struct zram_meta *meta = kmalloc(sizeof(*meta), GFP_KERNEL);
> >>>>+	struct zram_meta *meta = kzalloc(sizeof(*meta), GFP_KERNEL);
> >>>
> >>>Ditto
> >>>
> >>>>
> >>>>
> >>>
> >>>.
> >>>
> >>
> >>because of union of handle and element, i think a non-zero element (other than handle) is prevented from freeing.
> >>if zram_meta_get was modified, zram_meta_alloc did so.
> >
> >Right. Thanks but I don't see why we need the locking in there and modification of
> >zram_meta_alloc.
> >
> >Isn't it enough with this?
> 
> i am afraid someone do reset_store, so did lock.

reset_store is protected by zram->claim and and init_done so I don't
think so.