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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> Signed-off-by: Minchan Kim <minchan@xxxxxxxxxx>
> ---
> 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.