linux-kernel - Re: [PATCH] zram: introduce per-device debug

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160512234143.GA27204@bbox>
Date:	Fri, 13 May 2016 08:41:43 +0900
From:	Minchan Kim <minchan@...nel.org>
To:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	<linux-kernel@...r.kernel.org>, <linux-mm@...ck.org>,
	Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>
Subject: Re: [PATCH] zram: introduce per-device debug_stat sysfs node

Hello Sergey,

On Wed, May 11, 2016 at 10:45:53PM +0900, Sergey Senozhatsky wrote:
> debug_stat sysfs is read-only and represents various debugging
> data that zram developers may need. This file is not meant to be
> used by anyone else: its content is not documented and will change
> any time w/o any notice. Therefore, the output of debug_stat file
> contains a version string. To reduce the possibility of contusion,

                                                          confusion

> etc. we would increase the version number every time we modify
> the output.
> 
> At the moment this file exports only one value -- the number of
> re-compressions, IOW, the number of times compression fast path
> has failed; which is a temporarily stats, that we will be using
> should any per-cpu regressions happen. It's excepted to go away.
 
                                              expected

> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> ---
>  Documentation/ABI/testing/sysfs-block-zram |  7 +++++++
>  Documentation/blockdev/zram.txt            |  1 +
>  drivers/block/zram/zram_drv.c              | 21 +++++++++++++++++++++
>  drivers/block/zram/zram_drv.h              |  1 +
>  4 files changed, 30 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-block-zram b/Documentation/ABI/testing/sysfs-block-zram
> index 2e69e83..740aada 100644
> --- a/Documentation/ABI/testing/sysfs-block-zram
> +++ b/Documentation/ABI/testing/sysfs-block-zram
> @@ -166,3 +166,10 @@ Description:
>  		The mm_stat file is read-only and represents device's mm
>  		statistics (orig_data_size, compr_data_size, etc.) in a format
>  		similar to block layer statistics file format.
> +
> +What:		/sys/block/zram<id>/debug_stat
> +Date:		July 2016
> +Contact:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>
> +Description:
> +		The debug_stat file is read-only and represents various
> +		device's debugging info.
                                        for zram kernel developer so

                content is not documented for userspace intentionally
                and will change anytime without any notice.

would be more clear to waste user's time to read this. :)


> diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
> index d88f0c7..13100fb 100644
> --- a/Documentation/blockdev/zram.txt
> +++ b/Documentation/blockdev/zram.txt
> @@ -172,6 +172,7 @@ mem_limit         RW    the maximum amount of memory ZRAM can use to store
>  pages_compacted   RO    the number of pages freed during compaction
>                          (available only via zram<id>/mm_stat node)
>  compact           WO    trigger memory compaction
> +debug_stat        RO    this file is used for zram debugging purposes
>  
>  WARNING
>  =======
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 8fcfbeb..a629bd8d 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -435,8 +435,26 @@ static ssize_t mm_stat_show(struct device *dev,
>  	return ret;
>  }
>  
> +static ssize_t debug_stat_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	unsigned int version = 1;
> +	struct zram *zram = dev_to_zram(dev);
> +	ssize_t ret;
> +
> +	down_read(&zram->init_lock);
> +	ret = scnprintf(buf, PAGE_SIZE,
> +			"version: %d\n%8llu\n",
> +			version,
> +			(u64)atomic64_read(&zram->stats.num_recompress));
> +	up_read(&zram->init_lock);
> +
> +	return ret;
> +}
> +
>  static DEVICE_ATTR_RO(io_stat);
>  static DEVICE_ATTR_RO(mm_stat);
> +static DEVICE_ATTR_RO(debug_stat);
>  ZRAM_ATTR_RO(num_reads);
>  ZRAM_ATTR_RO(num_writes);
>  ZRAM_ATTR_RO(failed_reads);
> @@ -719,6 +737,8 @@ compress_again:
>  		zcomp_strm_release(zram->comp, zstrm);
>  		zstrm = NULL;
>  
> +		atomic64_inc(&zram->stats.num_recompress);
> +

It should be below "goto compress_again".

Other than that,

Acked-by: Minchan Kim <minchan@...nel.org>

Thanks!


>  		handle = zs_malloc(meta->mem_pool, clen,
>  				GFP_NOIO | __GFP_HIGHMEM);
>  		if (handle)
> @@ -1181,6 +1201,7 @@ static struct attribute *zram_disk_attrs[] = {
>  	&dev_attr_comp_algorithm.attr,
>  	&dev_attr_io_stat.attr,
>  	&dev_attr_mm_stat.attr,
> +	&dev_attr_debug_stat.attr,
>  	NULL,
>  };
>  
> diff --git a/drivers/block/zram/zram_drv.h b/drivers/block/zram/zram_drv.h
> index 06b1636..1fb45f7 100644
> --- a/drivers/block/zram/zram_drv.h
> +++ b/drivers/block/zram/zram_drv.h
> @@ -85,6 +85,7 @@ struct zram_stats {
>  	atomic64_t zero_pages;		/* no. of zero filled pages */
>  	atomic64_t pages_stored;	/* no. of pages currently stored */
>  	atomic_long_t max_used_pages;	/* no. of maximum pages stored */
> +	atomic64_t num_recompress;	/* no. of compression slow paths */
>  };
>  
>  struct zram_meta {
> -- 
> 2.8.2.372.g63a3502
>