[PATCH] zram: introduce per-device debug_stat sysfs node

[PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

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,
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.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.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@gmail.com>
+Description:
+		The debug_stat file is read-only and represents various
+		device's debugging info.
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);
+
 		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

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Minchan Kim]

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@gmail.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@gmail.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@kernel.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
> 

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

Hello Minchan,

On (05/13/16 08:41), Minchan Kim wrote:
[..]

will fix and update, thanks!


> > @@ -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".

I moved it out of goto intentionally. this second zs_malloc()

		handle = zs_malloc(meta->mem_pool, clen,
				GFP_NOIO | __GFP_HIGHMEM |
				__GFP_MOVABLE);

can take some time to complete, which will slow down zram for a bit,
and _theoretically_ this second zs_malloc() still can fail. yes, we
would do the error print out pr_err("Error allocating memory ... ")
and inc the `failed_writes' in zram_bvec_rw(), but zram_bvec_write()
has several more error return paths that can inc the `failed_writes'.
so by just looking at the stats we won't be able to tell that we had
failed fast path allocation combined with failed slow path allocation
(IOW, `goto recompress' never happened).

so I'm thinking about changing its name to num_failed_fast_compress
or num_failed_fast_write, or something similar and thus count the number
of times we fell to "!handle" branch, not the number of goto-s.
what do you think? or do you want it to be num_recompress specifically?

> Other than that,
> 
> Acked-by: Minchan Kim <minchan@kernel.org>
> 

thanks.

	-ss

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Minchan Kim]

On Fri, May 13, 2016 at 10:09:29AM +0900, Sergey Senozhatsky wrote:
> Hello Minchan,
> 
> On (05/13/16 08:41), Minchan Kim wrote:
> [..]
> 
> will fix and update, thanks!
> 
> 
> > > @@ -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".
> 
> I moved it out of goto intentionally. this second zs_malloc()
> 
> 		handle = zs_malloc(meta->mem_pool, clen,
> 				GFP_NOIO | __GFP_HIGHMEM |
> 				__GFP_MOVABLE);
> 
> can take some time to complete, which will slow down zram for a bit,
> and _theoretically_ this second zs_malloc() still can fail. yes, we
> would do the error print out pr_err("Error allocating memory ... ")
> and inc the `failed_writes' in zram_bvec_rw(), but zram_bvec_write()
> has several more error return paths that can inc the `failed_writes'.
> so by just looking at the stats we won't be able to tell that we had
> failed fast path allocation combined with failed slow path allocation
> (IOW, `goto recompress' never happened).
> 
> so I'm thinking about changing its name to num_failed_fast_compress
> or num_failed_fast_write, or something similar and thus count the number
> of times we fell to "!handle" branch, not the number of goto-s.
> what do you think? or do you want it to be num_recompress specifically?

Sorry, I don't get your point.
What's the problem with below?

        goto compress_again
        so
        atomic_inc(num_recompress)

My concern isn't a performance or something but just want to be more
readable and not error-prone which can increase num_compress although
second zs_malloc could be failed. When I tested with heavy workload,
I saw second zs_malloc can be fail but not frequently so it's not
theoretical issue.

What's the your concern with below?
Sorry if I don't get your point. Please elaborate it more.

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index a629bd8d452b..8bdcc4b2b9b8 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -737,12 +737,12 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		zcomp_strm_release(zram->comp, zstrm);
 		zstrm = NULL;
 
-		atomic64_inc(&zram->stats.num_recompress);
-
 		handle = zs_malloc(meta->mem_pool, clen,
 				GFP_NOIO | __GFP_HIGHMEM);
-		if (handle)
+		if (handle) {
+			atomic64_inc(&zram->stats.num_recompress);
 			goto compress_again;
+		}
 
 		pr_err("Error allocating memory for compressed page: %u, size=%zu\n",
 			index, clen);

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

On (05/13/16 15:23), Minchan Kim wrote:
[..]
> @@ -737,12 +737,12 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  		zcomp_strm_release(zram->comp, zstrm);
>  		zstrm = NULL;
>  
> -		atomic64_inc(&zram->stats.num_recompress);
> -
>  		handle = zs_malloc(meta->mem_pool, clen,
>  				GFP_NOIO | __GFP_HIGHMEM);
> -		if (handle)
> +		if (handle) {
> +			atomic64_inc(&zram->stats.num_recompress);
>  			goto compress_again;
> +		}

not like a real concern...

the main (and only) purpose of num_recompress is to match performance
slowdowns and failed fast write paths (when the first zs_malloc() fails).
this matching is depending on successful second zs_malloc(), but if it's
also unsuccessful we would only increase failed_writes; w/o increasing
the failed fast write counter, while we actually would have failed fast
write and extra zs_malloc() [unaccounted in this case]. yet it's probably
a bit unlikely to happen, but still. well, just saying.

	-ss

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

On (05/13/16 15:58), Sergey Senozhatsky wrote:
> On (05/13/16 15:23), Minchan Kim wrote:
> [..]
> > @@ -737,12 +737,12 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> >  		zcomp_strm_release(zram->comp, zstrm);
> >  		zstrm = NULL;
> >  
> > -		atomic64_inc(&zram->stats.num_recompress);
> > -
> >  		handle = zs_malloc(meta->mem_pool, clen,
> >  				GFP_NOIO | __GFP_HIGHMEM);
> > -		if (handle)
> > +		if (handle) {
> > +			atomic64_inc(&zram->stats.num_recompress);
> >  			goto compress_again;
> > +		}
> 
> not like a real concern...
> 
> the main (and only) purpose of num_recompress is to match performance
> slowdowns and failed fast write paths (when the first zs_malloc() fails).
> this matching is depending on successful second zs_malloc(), but if it's
> also unsuccessful we would only increase failed_writes; w/o increasing
> the failed fast write counter, while we actually would have failed fast
> write and extra zs_malloc() [unaccounted in this case]. yet it's probably
> a bit unlikely to happen, but still. well, just saying.

here I assume that the biggest contributor to re-compress latency is
enabled preemption after zcomp_strm_release() and this second zs_malloc().
the compression itself of a PAGE_SIZE buffer should be fast enough. so IOW
we would pass down the slow path, but would not account it.

	-ss

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Minchan Kim]

On Fri, May 13, 2016 at 04:05:53PM +0900, Sergey Senozhatsky wrote:
> On (05/13/16 15:58), Sergey Senozhatsky wrote:
> > On (05/13/16 15:23), Minchan Kim wrote:
> > [..]
> > > @@ -737,12 +737,12 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > >  		zcomp_strm_release(zram->comp, zstrm);
> > >  		zstrm = NULL;
> > >  
> > > -		atomic64_inc(&zram->stats.num_recompress);
> > > -
> > >  		handle = zs_malloc(meta->mem_pool, clen,
> > >  				GFP_NOIO | __GFP_HIGHMEM);
> > > -		if (handle)
> > > +		if (handle) {
> > > +			atomic64_inc(&zram->stats.num_recompress);
> > >  			goto compress_again;
> > > +		}
> > 
> > not like a real concern...
> > 
> > the main (and only) purpose of num_recompress is to match performance
> > slowdowns and failed fast write paths (when the first zs_malloc() fails).
> > this matching is depending on successful second zs_malloc(), but if it's
> > also unsuccessful we would only increase failed_writes; w/o increasing
> > the failed fast write counter, while we actually would have failed fast
> > write and extra zs_malloc() [unaccounted in this case]. yet it's probably
> > a bit unlikely to happen, but still. well, just saying.
> 
> here I assume that the biggest contributor to re-compress latency is
> enabled preemption after zcomp_strm_release() and this second zs_malloc().
> the compression itself of a PAGE_SIZE buffer should be fast enough. so IOW
> we would pass down the slow path, but would not account it.

biggest contributors are 1. direct reclaim by second zsmalloc call +
                         2. recompression overhead.

If zram start to support high comp ratio but slow speed algorithm like zlib
2 might be higher than 1 in the future so let's not ignore 2 overhead.

Although 2 is smaller, your patch just accounts only direct reclaim but my
suggestion can count both 1 and 2 so isn't it better?

I don't know why it's arguable here. :)

> 
> 	-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>

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

On (05/13/16 16:20), Minchan Kim wrote:
[..]
> > here I assume that the biggest contributor to re-compress latency is
> > enabled preemption after zcomp_strm_release() and this second zs_malloc().
> > the compression itself of a PAGE_SIZE buffer should be fast enough. so IOW
> > we would pass down the slow path, but would not account it.
> 
> biggest contributors are 1. direct reclaim by second zsmalloc call +
>                          2. recompression overhead.

			   3. enabled preemption after zcomp_strm_release()
			      we can be scheduled out for a long time.

> If zram start to support high comp ratio but slow speed algorithm like zlib
> 2 might be higher than 1 in the future so let's not ignore 2 overhead.

hm, yes, good point. not arguing, just for notice -- 2) has an upper limit
on its complexity, because we basically just do a number of arithmetical
operations on a buffer that has upper size limit -- PAGE_SIZE; while reclaim
in zsmalloc() can last an arbitrary amount of time. that's why I tend to
think of a PAGE_SIZE compression contribution as of constant, that can be
ignored.


> Although 2 is smaller, your patch just accounts only direct reclaim but my
> suggestion can count both 1 and 2 so isn't it better?
> 
> I don't know why it's arguable here. :)

no objections to put it next to goto. just making sure that we have
considered all the possibilities and cases.

will resend shortly, thanks!

	-ss

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

On (05/13/16 16:20), Minchan Kim wrote:
> > > > @@ -737,12 +737,12 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > > >  		zcomp_strm_release(zram->comp, zstrm);
> > > >  		zstrm = NULL;
> > > >  
> > > > -		atomic64_inc(&zram->stats.num_recompress);
> > > > -
> > > >  		handle = zs_malloc(meta->mem_pool, clen,
> > > >  				GFP_NOIO | __GFP_HIGHMEM);
> > > > -		if (handle)
> > > > +		if (handle) {
> > > > +			atomic64_inc(&zram->stats.num_recompress);
> > > >  			goto compress_again;
> > > > +		}


just a small note:

> Although 2 is smaller, your patch just accounts only direct reclaim but my
> suggestion can count both 1 and 2 so isn't it better?

no, my patch accounts 1) and 2) as well. the only difference is that my
patch accounts second zs_malloc() call _EVEN_ if it has failed and we
jumped to goto err (because we still could have done reclaim). the new
version would account second zs_malloc() _ONLY_ if it has succeeded, and
thus possibly reclaim would not be accounted.


recompress:
	compress
	handle = zs_malloc FAST PATH

	if (!handle) {
		release stream
		handle = zs_malloc SLOW PATH

		<< my patch accounts SLOW PATH here >>

		if (handle) {
			num_recompress++  << NEW version accounts it here, only it was OK >>
			goto recompress;
		}

		goto err;    << SLOW PATH is not accounted if SLOW PATH was unsuccessful
	}


	-ss

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Minchan Kim]

Hello Sergey,

On Fri, May 13, 2016 at 05:06:43PM +0900, Sergey Senozhatsky wrote:
> On (05/13/16 16:20), Minchan Kim wrote:
> > > > > @@ -737,12 +737,12 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
> > > > >  		zcomp_strm_release(zram->comp, zstrm);
> > > > >  		zstrm = NULL;
> > > > >  
> > > > > -		atomic64_inc(&zram->stats.num_recompress);
> > > > > -
> > > > >  		handle = zs_malloc(meta->mem_pool, clen,
> > > > >  				GFP_NOIO | __GFP_HIGHMEM);
> > > > > -		if (handle)
> > > > > +		if (handle) {
> > > > > +			atomic64_inc(&zram->stats.num_recompress);
> > > > >  			goto compress_again;
> > > > > +		}
> 
> 
> just a small note:
> 
> > Although 2 is smaller, your patch just accounts only direct reclaim but my
> > suggestion can count both 1 and 2 so isn't it better?
> 
> no, my patch accounts 1) and 2) as well. the only difference is that my
> patch accounts second zs_malloc() call _EVEN_ if it has failed and we
> jumped to goto err (because we still could have done reclaim). the new
> version would account second zs_malloc() _ONLY_ if it has succeeded, and
> thus possibly reclaim would not be accounted.
> 
> 
> recompress:
> 	compress
> 	handle = zs_malloc FAST PATH
> 
> 	if (!handle) {
> 		release stream
> 		handle = zs_malloc SLOW PATH
> 
> 		<< my patch accounts SLOW PATH here >>
> 
> 		if (handle) {
> 			num_recompress++  << NEW version accounts it here, only it was OK >>
> 			goto recompress;
> 		}
> 
> 		goto err;    << SLOW PATH is not accounted if SLOW PATH was unsuccessful
> 	}
> 

I got your point. You want to account every slow path and change
the naming from num_recompress to something to show that slow path.
Sorry for catching your point too late. And I absolutely agree with you.
I want to name it with 'writestall' like MM's allocstall. :)
Now I saw you sent new version but I like your suggestion more.

I will send new verion by hand :)
Thanks for the arguing. It was worth!

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

Hello Minchan,

On (05/14/16 08:05), Minchan Kim wrote:
[..]
> > recompress:
> > 	compress
> > 	handle = zs_malloc FAST PATH
> > 
> > 	if (!handle) {
> > 		release stream
> > 		handle = zs_malloc SLOW PATH
> > 
> > 		<< my patch accounts SLOW PATH here >>
> > 
> > 		if (handle) {
> > 			num_recompress++  << NEW version accounts it here, only it was OK >>
> > 			goto recompress;
> > 		}
> > 
> > 		goto err;    << SLOW PATH is not accounted if SLOW PATH was unsuccessful
> > 	}
> > 
> 
> I got your point. You want to account every slow path and change
> the naming from num_recompress to something to show that slow path.

yes :)

> Sorry for catching your point too late. And I absolutely agree with you.
> I want to name it with 'writestall' like MM's allocstall. :)

no problem. 'writestall' is really good, that's the word I was looking
for.

> Now I saw you sent new version but I like your suggestion more.
> 
> I will send new verion by hand :)
> Thanks for the arguing. It was worth!

oh, thanks!

	-ss

[PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

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 avoid any confusion, we will 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. This stat is temporary any will be useful in case if
any per-cpu compression streams regressions will be reported.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/ABI/testing/sysfs-block-zram |  9 +++++++++
 Documentation/blockdev/zram.txt            |  1 +
 drivers/block/zram/zram_drv.c              | 23 ++++++++++++++++++++++-
 drivers/block/zram/zram_drv.h              |  1 +
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-block-zram b/Documentation/ABI/testing/sysfs-block-zram
index 2e69e83..4518d30 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -166,3 +166,12 @@ 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@gmail.com>
+Description:
+		The debug_stat file is read-only and represents various
+		device's debugging info useful for kernel developers. Its
+		format is not documented intentionally and may change
+		anytime without any notice.
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..19274d5 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)
+{
+	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);
@@ -721,8 +739,10 @@ compress_again:
 
 		handle = zs_malloc(meta->mem_pool, clen,
 				GFP_NOIO | __GFP_HIGHMEM);
-		if (handle)
+		if (handle) {
+			atomic64_inc(&zram->stats.num_recompress);
 			goto compress_again;
+		}
 
 		pr_err("Error allocating memory for compressed page: %u, size=%zu\n",
 			index, clen);
@@ -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

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Minchan Kim]

On Fri, May 13, 2016 at 10:03:58PM +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 avoid any confusion, we will 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. This stat is temporary any will be useful in case if
> any per-cpu compression streams regressions will be reported.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Acked-by: Minchan Kim <minchan@kernel.org>

Fortunately, Andrew isn't pick up this patch yet so I want to replace
it to below suggested by Sergey which is better.

>From ba2f6c37ceeb11c6afc0f078ccc2ed0ead5e1104 Mon Sep 17 00:00:00 2001
From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Date: Sat, 14 May 2016 07:55:31 +0900
Subject: [PATCH] zram: introduce per-device debug_stat sysfs node

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 avoid any confusion, we will 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. This stat is temporary any will be useful in case if
any per-cpu compression streams regressions will be reported.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 Documentation/ABI/testing/sysfs-block-zram |  9 +++++++++
 Documentation/blockdev/zram.txt            |  1 +
 drivers/block/zram/zram_drv.c              | 21 +++++++++++++++++++++
 drivers/block/zram/zram_drv.h              |  1 +
 4 files changed, 32 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-block-zram b/Documentation/ABI/testing/sysfs-block-zram
index 2e69e83bf510..4518d30b8c2e 100644
--- a/Documentation/ABI/testing/sysfs-block-zram
+++ b/Documentation/ABI/testing/sysfs-block-zram
@@ -166,3 +166,12 @@ Contact:	Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
 		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@gmail.com>
+Description:
+		The debug_stat file is read-only and represents various
+		device's debugging info useful for kernel developers. Its
+		format is not documented intentionally and may change
+		anytime without any notice.
diff --git a/Documentation/blockdev/zram.txt b/Documentation/blockdev/zram.txt
index d88f0c70cd7f..13100fb3c26d 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 8fcfbebe79cd..8fcad8b761f1 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)
+{
+	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.writestall));
+	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 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 		zcomp_strm_release(zram->comp, zstrm);
 		zstrm = NULL;
 
+		atomic64_inc(&zram->stats.writestall);
+
 		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 06b1636f4722..3f5bf66a27e4 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 writestall;		/* no. of write slow paths */
 };
 
 struct zram_meta {
-- 
1.9.1

Re: [PATCH] zram: introduce per-device debug_stat sysfs node

[Sergey Senozhatsky]

On (05/14/16 08:08), Minchan Kim wrote:
[..]
> Fortunately, Andrew isn't pick up this patch yet so I want to replace
> it to below suggested by Sergey which is better.
[..]
>  
> +		atomic64_inc(&zram->stats.writestall);

looks good to me. thanks!

	-ss