[PATCH v3] zram: extend zero pages to same element pages

[PATCH v3] zram: extend zero pages to same element pages

[Minchan Kim]

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.

This patch extend zero_page to same_page so if there is any user to have
monitored zero_pages, he will be surprised if the number is increased
but it's no harmful, I believe.

Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
I removed zram_set_page_partial because I think block layer works with
IO size unit which would be aligned (unsigned long) at least, maybe
SECTOR or PAGE size. Then, we can merge both zram_set_page and
zram_set_page_partial.

 Documentation/blockdev/zram.txt |  6 ++--
 drivers/block/zram/zram_drv.c   | 77 ++++++++++++++++++++++++++++-------------
 drivers/block/zram/zram_drv.h   |  9 +++--
 3 files changed, 61 insertions(+), 31 deletions(-)

diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index 1c0c08d..4fced8a 100644
--- a/Documentation/blockdev/zram.txt
+++ b/Documentation/blockdev/zram.txt
@@ -201,8 +201,8 @@ File /sys/block/zram<id>/mm_stat
 The stat file represents device's mm statistics. It consists of a single
 line of text and contains the following stats separated by whitespace:
  orig_data_size   uncompressed size of data stored in this disk.
-                  This excludes zero-filled pages (zero_pages) since no
-                  memory is allocated for them.
+		  This excludes same-element-filled pages (same_pages) since
+		  no memory is allocated for them.
                   Unit: bytes
  compr_data_size  compressed size of data stored in this disk
  mem_used_total   the amount of memory allocated for this disk. This
@@ -214,7 +214,7 @@ The stat file represents device's mm statistics. It consists of a single
                   the compressed data
  mem_used_max     the maximum amount of memory zram have consumed to
                   store the data
- zero_pages       the number of zero filled pages written to this disk.
+ same_pages       the number of same element filled pages written to this disk.
                   No memory is allocated for such pages.
  pages_compacted  the number of pages freed during compaction
 
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 85737b6..46da1c4 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -74,6 +74,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);
@@ -146,31 +157,43 @@ 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 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 = 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);
-	else
-		clear_page(user_mem);
+	zram_fill_page(user_mem + bvec->bv_offset, bvec->bv_len, element);
 	kunmap_atomic(user_mem);
 
 	flush_dcache_page(page);
@@ -363,7 +386,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);
 
@@ -462,18 +485,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),
@@ -496,9 +521,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;
 	}
 
@@ -534,9 +559,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);
@@ -584,6 +609,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)) {
@@ -612,16 +638,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;
 	}
diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
index 74fcf10..5725d24 100644
--- a/drivers/block/zram/zram_drv.h
+++ b/drivers/block/zram/zram_drv.h
@@ -61,7 +61,7 @@ static const size_t max_zpage_size = PAGE_SIZE / 4 * 3;
 /* Flags for zram pages (table[page_no].value) */
 enum zram_pageflags {
 	/* Page consists entirely of zeros */
-	ZRAM_ZERO = ZRAM_FLAG_SHIFT,
+	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 */
-- 
2.7.4

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

[Joonsoo Kim]

On Mon, Jan 23, 2017 at 10:55:23AM +0900, Minchan Kim 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.
> 
> the defect of the patch is that when we recovery a page from
> non-zero element the operations are low efficient for partial
> read.
> 
> This patch extend zero_page to same_page so if there is any user to have
> monitored zero_pages, he will be surprised if the number is increased
> but it's no harmful, I believe.
> 
> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
> Signed-off-by: Minchan Kim <minchan@kernel.org>
> ---
> I removed zram_set_page_partial because I think block layer works with
> IO size unit which would be aligned (unsigned long) at least, maybe
> SECTOR or PAGE size. Then, we can merge both zram_set_page and
> zram_set_page_partial.
> 
>  Documentation/blockdev/zram.txt |  6 ++--
>  drivers/block/zram/zram_drv.c   | 77 ++++++++++++++++++++++++++++-------------
>  drivers/block/zram/zram_drv.h   |  9 +++--
>  3 files changed, 61 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index 1c0c08d..4fced8a 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -201,8 +201,8 @@ File /sys/block/zram<id>/mm_stat
>  The stat file represents device's mm statistics. It consists of a single
>  line of text and contains the following stats separated by whitespace:
>   orig_data_size   uncompressed size of data stored in this disk.
> -                  This excludes zero-filled pages (zero_pages) since no
> -                  memory is allocated for them.
> +		  This excludes same-element-filled pages (same_pages) since
> +		  no memory is allocated for them.
>                    Unit: bytes
>   compr_data_size  compressed size of data stored in this disk
>   mem_used_total   the amount of memory allocated for this disk. This
> @@ -214,7 +214,7 @@ The stat file represents device's mm statistics. It consists of a single
>                    the compressed data
>   mem_used_max     the maximum amount of memory zram have consumed to
>                    store the data
> - zero_pages       the number of zero filled pages written to this disk.
> + same_pages       the number of same element filled pages written to this disk.
>                    No memory is allocated for such pages.
>   pages_compacted  the number of pages freed during compaction
>  
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 85737b6..46da1c4 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -74,6 +74,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);
> @@ -146,31 +157,43 @@ 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;
> +	}
> +}

Hello,

we don't need to iterate reversely. It makes code less understandable
and possibly it would have negative impact on the performance.

> +
> +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 = page[pos];
> +

Ditto.

Thanks.

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

[zhouxianrong]

the purpose for reverse iteration is that i want compiler
compose decreasing instruction and compared instruction with zero
into one instruction which change cpu condition state.

yes, this maybe cause cache problem so need to be reviewed. thanks

On 2017/1/23 11:02, Joonsoo Kim wrote:
> On Mon, Jan 23, 2017 at 10:55:23AM +0900, Minchan Kim 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.
>>
>> the defect of the patch is that when we recovery a page from
>> non-zero element the operations are low efficient for partial
>> read.
>>
>> This patch extend zero_page to same_page so if there is any user to have
>> monitored zero_pages, he will be surprised if the number is increased
>> but it's no harmful, I believe.
>>
>> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
>> Signed-off-by: Minchan Kim <minchan@kernel.org>
>> ---
>> I removed zram_set_page_partial because I think block layer works with
>> IO size unit which would be aligned (unsigned long) at least, maybe
>> SECTOR or PAGE size. Then, we can merge both zram_set_page and
>> zram_set_page_partial.
>>
>>  Documentation/blockdev/zram.txt |  6 ++--
>>  drivers/block/zram/zram_drv.c   | 77 ++++++++++++++++++++++++++++-------------
>>  drivers/block/zram/zram_drv.h   |  9 +++--
>>  3 files changed, 61 insertions(+), 31 deletions(-)
>>
>> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
>> index 1c0c08d..4fced8a 100644
>> --- a/Documentation/blockdev/zram.txt
>> +++ b/Documentation/blockdev/zram.txt
>> @@ -201,8 +201,8 @@ File /sys/block/zram<id>/mm_stat
>>  The stat file represents device's mm statistics. It consists of a single
>>  line of text and contains the following stats separated by whitespace:
>>   orig_data_size   uncompressed size of data stored in this disk.
>> -                  This excludes zero-filled pages (zero_pages) since no
>> -                  memory is allocated for them.
>> +		  This excludes same-element-filled pages (same_pages) since
>> +		  no memory is allocated for them.
>>                    Unit: bytes
>>   compr_data_size  compressed size of data stored in this disk
>>   mem_used_total   the amount of memory allocated for this disk. This
>> @@ -214,7 +214,7 @@ The stat file represents device's mm statistics. It consists of a single
>>                    the compressed data
>>   mem_used_max     the maximum amount of memory zram have consumed to
>>                    store the data
>> - zero_pages       the number of zero filled pages written to this disk.
>> + same_pages       the number of same element filled pages written to this disk.
>>                    No memory is allocated for such pages.
>>   pages_compacted  the number of pages freed during compaction
>>
>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>> index 85737b6..46da1c4 100644
>> --- a/drivers/block/zram/zram_drv.c
>> +++ b/drivers/block/zram/zram_drv.c
>> @@ -74,6 +74,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);
>> @@ -146,31 +157,43 @@ 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;
>> +	}
>> +}
>
> Hello,
>
> we don't need to iterate reversely. It makes code less understandable
> and possibly it would have negative impact on the performance.
>
>> +
>> +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 = page[pos];
>> +
>
> Ditto.
>
> Thanks.
>
>
> .
>

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

[Minchan Kim]

Hello,

When I look at first patch, I wanted to use increment loop but didn't
tell to you because that small piece of code is no harmful for readbility
to me so I want to keep author's code rather than pointing the trivial
which is just matter of preference out.

Rather than readiblity, I suspect it might hurt performance and talked
with Namhyung but we cannot find anything decremental loop is bad
compared to incremental. Rather than, many articles have been said
decrement loop is faster like zhouxianrong's mentiond although I don't
think it makes marginal difference.

Joonsoo, why do you think incremental is faster?
zhouxianrong, why do you think decrement loops makes cache problem?

I'm okay either way. Just want to know why you guys think about it.

Thanks.

On Mon, Jan 23, 2017 at 11:46:27AM +0800, zhouxianrong wrote:
> the purpose for reverse iteration is that i want compiler
> compose decreasing instruction and compared instruction with zero
> into one instruction which change cpu condition state.
> 
> yes, this maybe cause cache problem so need to be reviewed. thanks
> 
> On 2017/1/23 11:02, Joonsoo Kim wrote:
> >On Mon, Jan 23, 2017 at 10:55:23AM +0900, Minchan Kim 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.
> >>
> >>the defect of the patch is that when we recovery a page from
> >>non-zero element the operations are low efficient for partial
> >>read.
> >>
> >>This patch extend zero_page to same_page so if there is any user to have
> >>monitored zero_pages, he will be surprised if the number is increased
> >>but it's no harmful, I believe.
> >>
> >>Signed-off-by: zhouxianrong <zhouxianrong@huawei.com>
> >>Signed-off-by: Minchan Kim <minchan@kernel.org>
> >>---
> >>I removed zram_set_page_partial because I think block layer works with
> >>IO size unit which would be aligned (unsigned long) at least, maybe
> >>SECTOR or PAGE size. Then, we can merge both zram_set_page and
> >>zram_set_page_partial.
> >>
> >> Documentation/blockdev/zram.txt |  6 ++--
> >> drivers/block/zram/zram_drv.c   | 77 ++++++++++++++++++++++++++++-------------
> >> drivers/block/zram/zram_drv.h   |  9 +++--
> >> 3 files changed, 61 insertions(+), 31 deletions(-)
> >>
> >>diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> >>index 1c0c08d..4fced8a 100644
> >>--- a/Documentation/blockdev/zram.txt
> >>+++ b/Documentation/blockdev/zram.txt
> >>@@ -201,8 +201,8 @@ File /sys/block/zram<id>/mm_stat
> >> The stat file represents device's mm statistics. It consists of a single
> >> line of text and contains the following stats separated by whitespace:
> >>  orig_data_size   uncompressed size of data stored in this disk.
> >>-                  This excludes zero-filled pages (zero_pages) since no
> >>-                  memory is allocated for them.
> >>+		  This excludes same-element-filled pages (same_pages) since
> >>+		  no memory is allocated for them.
> >>                   Unit: bytes
> >>  compr_data_size  compressed size of data stored in this disk
> >>  mem_used_total   the amount of memory allocated for this disk. This
> >>@@ -214,7 +214,7 @@ The stat file represents device's mm statistics. It consists of a single
> >>                   the compressed data
> >>  mem_used_max     the maximum amount of memory zram have consumed to
> >>                   store the data
> >>- zero_pages       the number of zero filled pages written to this disk.
> >>+ same_pages       the number of same element filled pages written to this disk.
> >>                   No memory is allocated for such pages.
> >>  pages_compacted  the number of pages freed during compaction
> >>
> >>diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> >>index 85737b6..46da1c4 100644
> >>--- a/drivers/block/zram/zram_drv.c
> >>+++ b/drivers/block/zram/zram_drv.c
> >>@@ -74,6 +74,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);
> >>@@ -146,31 +157,43 @@ 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;
> >>+	}
> >>+}
> >
> >Hello,
> >
> >we don't need to iterate reversely. It makes code less understandable
> >and possibly it would have negative impact on the performance.
> >
> >>+
> >>+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 = page[pos];
> >>+
> >
> >Ditto.
> >
> >Thanks.
> >
> >
> >.
> >
> 

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

[Joonsoo Kim]

On Mon, Jan 23, 2017 at 01:47:20PM +0900, Minchan Kim wrote:
> Hello,
> 
> When I look at first patch, I wanted to use increment loop but didn't
> tell to you because that small piece of code is no harmful for readbility
> to me so I want to keep author's code rather than pointing the trivial
> which is just matter of preference out.
> 
> Rather than readiblity, I suspect it might hurt performance and talked
> with Namhyung but we cannot find anything decremental loop is bad
> compared to incremental. Rather than, many articles have been said
> decrement loop is faster like zhouxianrong's mentiond although I don't
> think it makes marginal difference.
> 
> Joonsoo, why do you think incremental is faster?
> zhouxianrong, why do you think decrement loops makes cache problem?
> 
> I'm okay either way. Just want to know why you guys think about it.

Hmm... I guess that cache prefetcher works better for forward access
but I'm not sure.

Thanks.

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

[zhouxianrong]

i am not sure as well about reverse hurting cache.

On 2017/1/23 14:13, Joonsoo Kim wrote:
> On Mon, Jan 23, 2017 at 01:47:20PM +0900, Minchan Kim wrote:
>> Hello,
>>
>> When I look at first patch, I wanted to use increment loop but didn't
>> tell to you because that small piece of code is no harmful for readbility
>> to me so I want to keep author's code rather than pointing the trivial
>> which is just matter of preference out.
>>
>> Rather than readiblity, I suspect it might hurt performance and talked
>> with Namhyung but we cannot find anything decremental loop is bad
>> compared to incremental. Rather than, many articles have been said
>> decrement loop is faster like zhouxianrong's mentiond although I don't
>> think it makes marginal difference.
>>
>> Joonsoo, why do you think incremental is faster?
>> zhouxianrong, why do you think decrement loops makes cache problem?
>>
>> I'm okay either way. Just want to know why you guys think about it.
>
> Hmm... I guess that cache prefetcher works better for forward access
> but I'm not sure.
>
> Thanks.
>
>
> .
>

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

[Minchan Kim]

On Mon, Jan 23, 2017 at 02:17:23PM +0800, zhouxianrong wrote:
> i am not sure as well about reverse hurting cache.
> 
> On 2017/1/23 14:13, Joonsoo Kim wrote:
> >On Mon, Jan 23, 2017 at 01:47:20PM +0900, Minchan Kim wrote:
> >>Hello,
> >>
> >>When I look at first patch, I wanted to use increment loop but didn't
> >>tell to you because that small piece of code is no harmful for readbility
> >>to me so I want to keep author's code rather than pointing the trivial
> >>which is just matter of preference out.
> >>
> >>Rather than readiblity, I suspect it might hurt performance and talked
> >>with Namhyung but we cannot find anything decremental loop is bad
> >>compared to incremental. Rather than, many articles have been said
> >>decrement loop is faster like zhouxianrong's mentiond although I don't
> >>think it makes marginal difference.
> >>
> >>Joonsoo, why do you think incremental is faster?
> >>zhouxianrong, why do you think decrement loops makes cache problem?
> >>
> >>I'm okay either way. Just want to know why you guys think about it.
> >
> >Hmm... I guess that cache prefetcher works better for forward access
> >but I'm not sure.
 
If there is no one to ask changing it to incremental, I wanted to go
with decremental you wrote firstly but now there is one more people
who likes incremental loop as well as me. And he spent a time to his
precious time for review so we should take care of his credit.

So, please go with incremental approach if you are not against.
Please resend the patch by yourself. With patch I sent today, I guess
you got to know my intention about semantic change of documentation
, description change to notice zero page change risk and remoe
partial handling function. :)

Thanks.