Re: + zram-support-compaction.patch added to -mm tree

Re: + zram-support-compaction.patch added to -mm tree

[Sergey Senozhatsky]

Hello,

On (03/04/15 14:02), akpm@linux-foundation.org wrote:
[..]
> +++ a/drivers/block/zram/zram_drv.c
> @@ -70,6 +70,27 @@ static inline struct zram *dev_to_zram(s
>  	return (struct zram *)dev_to_disk(dev)->private_data;
>  }
>  
> +static ssize_t compact_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	unsigned long nr_migrated;
> +	struct zram *zram = dev_to_zram(dev);
> +	struct zram_meta *meta;
> +
> +	down_read(&zram->init_lock);
> +	if (!init_done(zram)) {
> +		up_read(&zram->init_lock);
> +		return -EINVAL;
> +	}
> +
> +	meta = zram->meta;
> +	nr_migrated = zs_compact(meta->mem_pool);
> +	atomic64_add(nr_migrated, &zram->stats.num_migrated);
> +	up_read(&zram->init_lock);
> +
> +	return len;
> +}
> +
>  /* flag operations require table entry bit_spin_lock() being held */
>  static int zram_test_flag(struct zram_meta *meta, u32 index,
>  			enum zram_pageflags flag)


let's stick to "helpers, attrs show/store, mm (meta, page), IO, zram control"
function layout.

so can we please put compact_store() after, say,
	354 static ssize_t comp_algorithm_store(...)

function?

	-ss

> @@ -374,6 +395,7 @@ ZRAM_ATTR_RO(invalid_io);
>  ZRAM_ATTR_RO(notify_free);
>  ZRAM_ATTR_RO(zero_pages);
>  ZRAM_ATTR_RO(compr_data_size);
> +ZRAM_ATTR_RO(num_migrated);
>  
>  static inline bool zram_meta_get(struct zram *zram)
>  {
> @@ -1031,6 +1053,7 @@ static const struct block_device_operati
>  	.owner = THIS_MODULE
>  };
>  
> +static DEVICE_ATTR_WO(compact);
>  static DEVICE_ATTR_RW(disksize);
>  static DEVICE_ATTR_RO(initstate);
>  static DEVICE_ATTR_WO(reset);
> @@ -1049,6 +1072,8 @@ static struct attribute *zram_disk_attrs
>  	&dev_attr_num_writes.attr,
>  	&dev_attr_failed_reads.attr,
>  	&dev_attr_failed_writes.attr,
> +	&dev_attr_num_migrated.attr,
> +	&dev_attr_compact.attr,
>  	&dev_attr_invalid_io.attr,
>  	&dev_attr_notify_free.attr,
>  	&dev_attr_zero_pages.attr,
> diff -puN drivers/block/zram/zram_drv.h~zram-support-compaction drivers/block/zram/zram_drv.h
> --- a/drivers/block/zram/zram_drv.h~zram-support-compaction
> +++ a/drivers/block/zram/zram_drv.h
> @@ -78,6 +78,7 @@ struct zram_stats {
>  	atomic64_t compr_data_size;	/* compressed size of pages stored */
>  	atomic64_t num_reads;	/* failed + successful */
>  	atomic64_t num_writes;	/* --do-- */
> +	atomic64_t num_migrated;	/* no. of migrated object */
>  	atomic64_t failed_reads;	/* can happen when memory is too low */
>  	atomic64_t failed_writes;	/* can happen when memory is too low */
>  	atomic64_t invalid_io;	/* non-page-aligned I/O requests */
> _
> 
> Patches currently in -mm which might be from minchan@kernel.org are
> 
> mm-vmscan-fix-the-page-state-calculation-in-too_many_isolated.patch
> mm-page_isolation-check-pfn-validity-before-access.patch
> mm-support-madvisemadv_free.patch
> mm-support-madvisemadv_free-fix.patch
> x86-add-pmd_-for-thp.patch
> x86-add-pmd_-for-thp-fix.patch
> sparc-add-pmd_-for-thp.patch
> sparc-add-pmd_-for-thp-fix.patch
> powerpc-add-pmd_-for-thp.patch
> arm-add-pmd_mkclean-for-thp.patch
> arm64-add-pmd_-for-thp.patch
> mm-dont-split-thp-page-when-syscall-is-called.patch
> mm-dont-split-thp-page-when-syscall-is-called-fix.patch
> mm-dont-split-thp-page-when-syscall-is-called-fix-2.patch
> zram-cosmetic-zram_attr_ro-code-formatting-tweak.patch
> zram-use-idr-instead-of-zram_devices-array.patch
> zram-factor-out-device-reset-from-reset_store.patch
> zram-reorganize-code-layout.patch
> zram-add-dynamic-device-add-remove-functionality.patch
> zram-remove-max_num_devices-limitation.patch
> zram-report-every-added-and-removed-device.patch
> zram-trivial-correct-flag-operations-comment.patch
> zsmalloc-decouple-handle-and-object.patch
> zsmalloc-factor-out-obj_.patch
> zsmalloc-support-compaction.patch
> zsmalloc-adjust-zs_almost_full.patch
> zram-support-compaction.patch
> zsmalloc-record-handle-in-page-private-for-huge-object.patch
> zsmalloc-add-fullness-into-stat.patch
> 
> --
> To unsubscribe from this list: send the line "unsubscribe mm-commits" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: + zram-support-compaction.patch added to -mm tree

[Minchan Kim]

Hello Sergey,

On Thu, Mar 05, 2015 at 09:18:45AM +0900, Sergey Senozhatsky wrote:
> Hello,
> 
> On (03/04/15 14:02), akpm@linux-foundation.org wrote:
> [..]
> > +++ a/drivers/block/zram/zram_drv.c
> > @@ -70,6 +70,27 @@ static inline struct zram *dev_to_zram(s
> >  	return (struct zram *)dev_to_disk(dev)->private_data;
> >  }
> >  
> > +static ssize_t compact_store(struct device *dev,
> > +		struct device_attribute *attr, const char *buf, size_t len)
> > +{
> > +	unsigned long nr_migrated;
> > +	struct zram *zram = dev_to_zram(dev);
> > +	struct zram_meta *meta;
> > +
> > +	down_read(&zram->init_lock);
> > +	if (!init_done(zram)) {
> > +		up_read(&zram->init_lock);
> > +		return -EINVAL;
> > +	}
> > +
> > +	meta = zram->meta;
> > +	nr_migrated = zs_compact(meta->mem_pool);
> > +	atomic64_add(nr_migrated, &zram->stats.num_migrated);
> > +	up_read(&zram->init_lock);
> > +
> > +	return len;
> > +}
> > +
> >  /* flag operations require table entry bit_spin_lock() being held */
> >  static int zram_test_flag(struct zram_meta *meta, u32 index,
> >  			enum zram_pageflags flag)
> 
> 
> let's stick to "helpers, attrs show/store, mm (meta, page), IO, zram control"
> function layout.
> 
> so can we please put compact_store() after, say,
> 	354 static ssize_t comp_algorithm_store(...)
> 
> function?

I will clean it up after Andrew releases new mmotm. :)

Thanks for the review!

-- 
Kind regards,
Minchan Kim

Re: + zram-support-compaction.patch added to -mm tree

[Sergey Senozhatsky]

On (03/04/15 14:02), akpm@linux-foundation.org wrote:
> +What:		/sys/block/zram<id>/compact
> +Date:		August 2015
> +Contact:	Minchan Kim <minchan@kernel.org>
> +Description:
> +		The compact file is write-only and trigger compaction for
> +		allocator zrm uses. The allocator moves some objects so that
> +		it could free fragment space.
> +
> +What:		/sys/block/zram<id>/num_migrated
> +Date:		August 2015
> +Contact:	Minchan Kim <minchan@kernel.org>
> +Description:
> +		The compact file is read-only and shows how many object
> +		migrated by compaction.
> diff -puN drivers/block/zram/zram_drv.c~zram-support-compaction drivers/block/zram/zram_drv.c
> --- a/drivers/block/zram/zram_drv.c~zram-support-compaction
> +++ a/drivers/block/zram/zram_drv.c
> @@ -70,6 +70,27 @@ static inline struct zram *dev_to_zram(s
>  	return (struct zram *)dev_to_disk(dev)->private_data;
>  }

First of all, my apologies to Andrew Morton. if I reply to this email, my mutt for
some reason replaces akpm at linux-foundation.org with linux-kernel at vger.kernel.org
(I can't see why this is happening, but this is somehow a `stable behaviour'). I didn't
spot this, so this is why Andrew was not Cc-d to my previous reply to this eamil.




rather a discussion question.

Minchan, do you want to provide num_migrated as part of zsmalloc stats rather
than having yet another zram attr? we already provide zsmalloc stats and this
type of information seems to belong there. just idea.

	-ss

>  
> +static ssize_t compact_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t len)
> +{
> +	unsigned long nr_migrated;
> +	struct zram *zram = dev_to_zram(dev);
> +	struct zram_meta *meta;
> +
> +	down_read(&zram->init_lock);
> +	if (!init_done(zram)) {
> +		up_read(&zram->init_lock);
> +		return -EINVAL;
> +	}
> +
> +	meta = zram->meta;
> +	nr_migrated = zs_compact(meta->mem_pool);
> +	atomic64_add(nr_migrated, &zram->stats.num_migrated);
> +	up_read(&zram->init_lock);
> +
> +	return len;
> +}

Re: + zram-support-compaction.patch added to -mm tree

[Sergey Senozhatsky]

On (03/05/15 14:29), Sergey Senozhatsky wrote:
> On (03/04/15 14:02), akpm@linux-foundation.org wrote:
> > +What:		/sys/block/zram<id>/compact
> > +Date:		August 2015
> > +Contact:	Minchan Kim <minchan@kernel.org>
> > +Description:
> > +		The compact file is write-only and trigger compaction for
> > +		allocator zrm uses. The allocator moves some objects so that
> > +		it could free fragment space.
> > +
> > +What:		/sys/block/zram<id>/num_migrated
> > +Date:		August 2015
> > +Contact:	Minchan Kim <minchan@kernel.org>
> > +Description:
> > +		The compact file is read-only and shows how many object
> > +		migrated by compaction.
> Minchan, do you want to provide num_migrated as part of zsmalloc stats rather
> than having yet another zram attr? we already provide zsmalloc stats and this
> type of information seems to belong there. just idea.
> 

can compaction memory gain be calculated as, for example,

  mem_used_total #before_compaction - mem_used_total #after_compaction ?


if yes, then do we need this attr at all?

the other thing is - why does this attr return the sum of all compactions?
wouldn't it be more informative to return the number of bytes gained after
the most recent compaction? but, once again, is this attr worth having?

	-ss

Re: + zram-support-compaction.patch added to -mm tree

[Minchan Kim]

Hello Sergey,

On Thu, Mar 05, 2015 at 02:29:42PM +0900, Sergey Senozhatsky wrote:
> On (03/04/15 14:02), akpm@linux-foundation.org wrote:
> > +What:		/sys/block/zram<id>/compact
> > +Date:		August 2015
> > +Contact:	Minchan Kim <minchan@kernel.org>
> > +Description:
> > +		The compact file is write-only and trigger compaction for
> > +		allocator zrm uses. The allocator moves some objects so that
> > +		it could free fragment space.
> > +
> > +What:		/sys/block/zram<id>/num_migrated
> > +Date:		August 2015
> > +Contact:	Minchan Kim <minchan@kernel.org>
> > +Description:
> > +		The compact file is read-only and shows how many object
> > +		migrated by compaction.
> > diff -puN drivers/block/zram/zram_drv.c~zram-support-compaction drivers/block/zram/zram_drv.c
> > --- a/drivers/block/zram/zram_drv.c~zram-support-compaction
> > +++ a/drivers/block/zram/zram_drv.c
> > @@ -70,6 +70,27 @@ static inline struct zram *dev_to_zram(s
> >  	return (struct zram *)dev_to_disk(dev)->private_data;
> >  }
> 
> First of all, my apologies to Andrew Morton. if I reply to this email, my mutt for
> some reason replaces akpm at linux-foundation.org with linux-kernel at vger.kernel.org
> (I can't see why this is happening, but this is somehow a `stable behaviour'). I didn't
> spot this, so this is why Andrew was not Cc-d to my previous reply to this eamil.
> 
> 
> 
> 
> rather a discussion question.
> 
> Minchan, do you want to provide num_migrated as part of zsmalloc stats rather
> than having yet another zram attr? we already provide zsmalloc stats and this
> type of information seems to belong there. just idea.

Hmm, CONFIG_ZSMALLOC_STAT is actually to show zsmalloc internals. That's why
it is on debugfs. If we add the stat into zsmalloc, we should turn on debugfs
and CONFIG_ZSMALLOC_STAT to see *a* stat. Even, CONFIG_ZSMALLOC_STAT will add
unncessary overheads to account another stats fo zsmalloc internals.

As well, if we add auto-compacion like stuff in zsmalloc(ie, it will trigger
by itself if fragmention is over to predefined theshold), the stat will
accumulate stat while someone want to see snapshot compaction effiecieny
of the moment.

So, I want to keep it in zram now.

-- 
Kind regards,
Minchan Kim

Re: + zram-support-compaction.patch added to -mm tree

[Sergey Senozhatsky]

On (03/09/15 09:49), Minchan Kim wrote:
> > rather a discussion question.
> > 
> > Minchan, do you want to provide num_migrated as part of zsmalloc stats rather
> > than having yet another zram attr? we already provide zsmalloc stats and this
> > type of information seems to belong there. just idea.
> 
> Hmm, CONFIG_ZSMALLOC_STAT is actually to show zsmalloc internals.

well, to be fair, compaction is a zsmalloc internal. zram has nothing to do with
it.

but do we we even need this stat? it seems that

   mem_total_used (before compaction) - mem_total_user (after comapction)

will give user an idea on how much memory was compacted.

	-ss

> That's why it is on debugfs. If we add the stat into zsmalloc, we should turn on debugfs
> and CONFIG_ZSMALLOC_STAT to see *a* stat. Even, CONFIG_ZSMALLOC_STAT will add
> unncessary overheads to account another stats fo zsmalloc internals.
> 
> As well, if we add auto-compacion like stuff in zsmalloc(ie, it will trigger
> by itself if fragmention is over to predefined theshold), the stat will
> accumulate stat while someone want to see snapshot compaction effiecieny
> of the moment.
> 
> So, I want to keep it in zram now.
> 
> -- 
> Kind regards,
> Minchan Kim
> 

Re: + zram-support-compaction.patch added to -mm tree

[Minchan Kim]

On Mon, Mar 09, 2015 at 09:57:18AM +0900, Sergey Senozhatsky wrote:
> On (03/09/15 09:49), Minchan Kim wrote:
> > > rather a discussion question.
> > > 
> > > Minchan, do you want to provide num_migrated as part of zsmalloc stats rather
> > > than having yet another zram attr? we already provide zsmalloc stats and this
> > > type of information seems to belong there. just idea.
> > 
> > Hmm, CONFIG_ZSMALLOC_STAT is actually to show zsmalloc internals.
> 
> well, to be fair, compaction is a zsmalloc internal. zram has nothing to do with
> it.
> 
> but do we we even need this stat? it seems that
> 
>    mem_total_used (before compaction) - mem_total_user (after comapction)
> 
> will give user an idea on how much memory was compacted.

It's not enough. What I want to know is compaction efficiency per client of
zsmalloc(ie, zram). IOW, (how many of freed pages / how many of objects) per
zs_compact.

> 
> 	-ss
> 
> > That's why it is on debugfs. If we add the stat into zsmalloc, we should turn on debugfs
> > and CONFIG_ZSMALLOC_STAT to see *a* stat. Even, CONFIG_ZSMALLOC_STAT will add
> > unncessary overheads to account another stats fo zsmalloc internals.
> > 
> > As well, if we add auto-compacion like stuff in zsmalloc(ie, it will trigger
> > by itself if fragmention is over to predefined theshold), the stat will
> > accumulate stat while someone want to see snapshot compaction effiecieny
> > of the moment.
> > 
> > So, I want to keep it in zram now.
> > 
> > -- 
> > Kind regards,
> > Minchan Kim
> > 

-- 
Kind regards,
Minchan Kim

Re: + zram-support-compaction.patch added to -mm tree

[Sergey Senozhatsky]

On (03/09/15 10:05), Minchan Kim wrote:
> > well, to be fair, compaction is a zsmalloc internal. zram has nothing to do with
> > it.
> > 
> > but do we we even need this stat? it seems that
> > 
> >    mem_total_used (before compaction) - mem_total_user (after comapction)
> > 
> > will give user an idea on how much memory was compacted.
> 
> It's not enough. What I want to know is compaction efficiency per client of
> zsmalloc(ie, zram). 
>

so what a typical user can do with this information? isn't it an entirely
debug info that makes some hidden sense only to developers?

if you insist on exporting this as a zram stat for everyone how obout
starting to move away from per-stat RO sysfs attrs. it seems that we have
uncomfortably a lot of sysfs attrs, and that doesn't make life easier in
user space. for example, block devices have /sys/block/.../stat file:

/sys/block/sda$ cat stat
   45931       59  2075686   289906    55768     9229  1967800   318033        0   193583   607806

and there are no num_reads, num_writes, num_failed_reads, num_failed_writes,
etc., etc. per-stat sysfs attrs force user-space to do lots of syscalls:
open(), read(), close() with error control on every step; for every stat.

so how about introducing zram<id>/malloc_stats (or any similar name) and
provide compaction and all future allocator related stats there
(via s*printf("%d %d %d", ....)) ?

	-ss

> IOW, (how many of freed pages / how many of objects) per
> zs_compact.
> 
> > 
> > 	-ss
> > 
> > > That's why it is on debugfs. If we add the stat into zsmalloc, we should turn on debugfs
> > > and CONFIG_ZSMALLOC_STAT to see *a* stat. Even, CONFIG_ZSMALLOC_STAT will add
> > > unncessary overheads to account another stats fo zsmalloc internals.
> > > 
> > > As well, if we add auto-compacion like stuff in zsmalloc(ie, it will trigger
> > > by itself if fragmention is over to predefined theshold), the stat will
> > > accumulate stat while someone want to see snapshot compaction effiecieny
> > > of the moment.
> > > 
> > > So, I want to keep it in zram now.
> > > 
> > > -- 
> > > Kind regards,
> > > Minchan Kim
> > > 
> 
> -- 
> Kind regards,
> Minchan Kim
> 

Re: + zram-support-compaction.patch added to -mm tree

[Minchan Kim]

On Mon, Mar 09, 2015 at 10:27:28AM +0900, Sergey Senozhatsky wrote:
> On (03/09/15 10:05), Minchan Kim wrote:
> > > well, to be fair, compaction is a zsmalloc internal. zram has nothing to do with
> > > it.
> > > 
> > > but do we we even need this stat? it seems that
> > > 
> > >    mem_total_used (before compaction) - mem_total_user (after comapction)
> > > 
> > > will give user an idea on how much memory was compacted.
> > 
> > It's not enough. What I want to know is compaction efficiency per client of
> > zsmalloc(ie, zram). 
> >
> 
> so what a typical user can do with this information? isn't it an entirely
> debug info that makes some hidden sense only to developers?

Absolutely true.

> 
> if you insist on exporting this as a zram stat for everyone how obout
> starting to move away from per-stat RO sysfs attrs. it seems that we have
> uncomfortably a lot of sysfs attrs, and that doesn't make life easier in
> user space. for example, block devices have /sys/block/.../stat file:
> 
> /sys/block/sda$ cat stat
>    45931       59  2075686   289906    55768     9229  1967800   318033        0   193583   607806
> 
> and there are no num_reads, num_writes, num_failed_reads, num_failed_writes,
> etc., etc. per-stat sysfs attrs force user-space to do lots of syscalls:
> open(), read(), close() with error control on every step; for every stat.

I absoulte agree with you and I really wanted to tidy it up but was no
time. Sergey, Could you contribute? If you have no time, I will do by
myself but it would be low priority now.


> 
> so how about introducing zram<id>/malloc_stats (or any similar name) and
> provide compaction and all future allocator related stats there
> (via s*printf("%d %d %d", ....)) ?
> 
> 	-ss
> 
> > IOW, (how many of freed pages / how many of objects) per
> > zs_compact.
> > 
> > > 
> > > 	-ss
> > > 
> > > > That's why it is on debugfs. If we add the stat into zsmalloc, we should turn on debugfs
> > > > and CONFIG_ZSMALLOC_STAT to see *a* stat. Even, CONFIG_ZSMALLOC_STAT will add
> > > > unncessary overheads to account another stats fo zsmalloc internals.
> > > > 
> > > > As well, if we add auto-compacion like stuff in zsmalloc(ie, it will trigger
> > > > by itself if fragmention is over to predefined theshold), the stat will
> > > > accumulate stat while someone want to see snapshot compaction effiecieny
> > > > of the moment.
> > > > 
> > > > So, I want to keep it in zram now.
> > > > 
> > > > -- 
> > > > Kind regards,
> > > > Minchan Kim
> > > > 
> > 
> > -- 
> > Kind regards,
> > Minchan Kim
> > 

-- 
Kind regards,
Minchan Kim

Re: + zram-support-compaction.patch added to -mm tree

[Sergey Senozhatsky]

On (03/09/15 10:47), Minchan Kim wrote:
> > > 
> > > It's not enough. What I want to know is compaction efficiency per client of
> > > zsmalloc(ie, zram). 
> > >
> > 
> > so what a typical user can do with this information? isn't it an entirely
> > debug info that makes some hidden sense only to developers?
> 
> Absolutely true.
> 
> > 
> > if you insist on exporting this as a zram stat for everyone how obout
> > starting to move away from per-stat RO sysfs attrs. it seems that we have
> > uncomfortably a lot of sysfs attrs, and that doesn't make life easier in
> > user space. for example, block devices have /sys/block/.../stat file:
> > 
> > /sys/block/sda$ cat stat
> >    45931       59  2075686   289906    55768     9229  1967800   318033        0   193583   607806
> > 
> > and there are no num_reads, num_writes, num_failed_reads, num_failed_writes,
> > etc., etc. per-stat sysfs attrs force user-space to do lots of syscalls:
> > open(), read(), close() with error control on every step; for every stat.
> 
> I absoulte agree with you and I really wanted to tidy it up but was no
> time. Sergey, Could you contribute? If you have no time, I will do by
> myself but it would be low priority now.

sure. I can handle it.


I was thinking for some time already about splitting stats that we
export in two categories and, thus, two files: IO_stats and MM_stats.

zram<id>/io_stat

s*printf( num_reads, num_writes, failed_reads, failed_writes, etc.)

zram<id>/mm_stat
s*printf( orig_data_size, compr_data_size, mem_used_total, num_migrated, etc.)



so hoprefully in several years we can entirely remove ZRAM_ATTR_RO functions.
probably, first moving them under #ifdef CONFIG_OLD_ZRAM_STATS at some point
in the future.

	-ss

Re: + zram-support-compaction.patch added to -mm tree

[Minchan Kim]

On Mon, Mar 09, 2015 at 11:07:17AM +0900, Sergey Senozhatsky wrote:
> On (03/09/15 10:47), Minchan Kim wrote:
> > > > 
> > > > It's not enough. What I want to know is compaction efficiency per client of
> > > > zsmalloc(ie, zram). 
> > > >
> > > 
> > > so what a typical user can do with this information? isn't it an entirely
> > > debug info that makes some hidden sense only to developers?
> > 
> > Absolutely true.
> > 
> > > 
> > > if you insist on exporting this as a zram stat for everyone how obout
> > > starting to move away from per-stat RO sysfs attrs. it seems that we have
> > > uncomfortably a lot of sysfs attrs, and that doesn't make life easier in
> > > user space. for example, block devices have /sys/block/.../stat file:
> > > 
> > > /sys/block/sda$ cat stat
> > >    45931       59  2075686   289906    55768     9229  1967800   318033        0   193583   607806
> > > 
> > > and there are no num_reads, num_writes, num_failed_reads, num_failed_writes,
> > > etc., etc. per-stat sysfs attrs force user-space to do lots of syscalls:
> > > open(), read(), close() with error control on every step; for every stat.
> > 
> > I absoulte agree with you and I really wanted to tidy it up but was no
> > time. Sergey, Could you contribute? If you have no time, I will do by
> > myself but it would be low priority now.
> 
> sure. I can handle it.

Thanks!

> 
> 
> I was thinking for some time already about splitting stats that we
> export in two categories and, thus, two files: IO_stats and MM_stats.
> 
> zram<id>/io_stat
> 
> s*printf( num_reads, num_writes, failed_reads, failed_writes, etc.)

Some of it(ie, num_reads, num_writes) was duplicated with /dev/block/zramx/stat?
I know /dev/block/zramx/stat doesn't work now and I didn't check why it doesn't
work but I hope we make it work so remove duplicate stat, finally. :)

> 
> zram<id>/mm_stat
> s*printf( orig_data_size, compr_data_size, mem_used_total, num_migrated, etc.)
> 
> 
> 
> so hoprefully in several years we can entirely remove ZRAM_ATTR_RO functions.
> probably, first moving them under #ifdef CONFIG_OLD_ZRAM_STATS at some point
> in the future.

Sounds good so we could warn for 1 or 2 years if users are about to use old stat
and finally removes deprecated stat.
Please Cc util-linux zram-control peoples when you send patchset.

> 
> 	-ss

-- 
Kind regards,
Minchan Kim

Re: + zram-support-compaction.patch added to -mm tree

[Sergey Senozhatsky]

On (03/09/15 11:21), Minchan Kim wrote:
> > I was thinking for some time already about splitting stats that we
> > export in two categories and, thus, two files: IO_stats and MM_stats.
> > 
> > zram<id>/io_stat
> > 
> > s*printf( num_reads, num_writes, failed_reads, failed_writes, etc.)
> 
> Some of it(ie, num_reads, num_writes) was duplicated with /dev/block/zramx/stat?
> I know /dev/block/zramx/stat doesn't work now and I didn't check why it doesn't
> work but I hope we make it work so remove duplicate stat, finally. :)
> 

yes, I do recall looking into the issue some months ago. zramX/stat file hanled by block
layer in various places. for example, in:

	blk_finish_request(struct request *req, int error)
		blk_account_io_done():

doing

  part_stat_inc(cpu, part, ios[rw]);
  part_stat_add(cpu, part, ticks[rw], duration);
  part_round_stats(cpu, part);
  part_dec_in_flight(part, rw);


the problem here is that zram has several paths that may issue IO:
-- usual zram_make_request()
-- zram_slot_free_notify()
-- zram_rw_page()

in zram_slot_free_notify() and zram_rw_page() we don't have request queue, request,
etc. so it's a bit troubling.


besides, /sys/block/zramX/stat file exports totally different data:

 struct disk_stats {
         unsigned long sectors[2];       /* READs and WRITEs */
         unsigned long ios[2];
         unsigned long merges[2];
         unsigned long ticks[2];
         unsigned long io_ticks;
         unsigned long time_in_queue;
 };

Documentation/block/stat.txt

Name            units         description
----            -----         -----------
read I/Os       requests      number of read I/Os processed
read merges     requests      number of read I/Os merged with in-queue I/O
read sectors    sectors       number of sectors read
read ticks      milliseconds  total wait time for read requests
write I/Os      requests      number of write I/Os processed
write merges    requests      number of write I/Os merged with in-queue I/O
write sectors   sectors       number of sectors written
write ticks     milliseconds  total wait time for write requests
in_flight       requests      number of I/Os currently in flight
io_ticks        milliseconds  total time this block device has been active
time_in_queue   milliseconds  total wait time for all requests


the only overlaps are num_read and num_write. so we will not be able to move all
(or any significant amount) of our IO stats to that file. that will force users
to gather IO stats accross several files.

I'll take a look later today/tomorrow if I can do anything about it, but it seems
that our own zramX/io_stat file would be simpler solution here. it does sound ugly,
but it doesn't look so bad after all.


> > zram<id>/mm_stat
> > s*printf( orig_data_size, compr_data_size, mem_used_total, num_migrated, etc.)
> > 
> > 
> > 
> > so hoprefully in several years we can entirely remove ZRAM_ATTR_RO functions.
> > probably, first moving them under #ifdef CONFIG_OLD_ZRAM_STATS at some point
> > in the future.
> 
> Sounds good so we could warn for 1 or 2 years if users are about to use old stat
> and finally removes deprecated stat.

good.

> Please Cc util-linux zram-control peoples when you send patchset.
> 

ok.

	-ss

Re: + zram-support-compaction.patch added to -mm tree

[Minchan Kim]

Hello,

On Mon, Mar 09, 2015 at 03:48:56PM +0900, Sergey Senozhatsky wrote:
> On (03/09/15 11:21), Minchan Kim wrote:
> > > I was thinking for some time already about splitting stats that we
> > > export in two categories and, thus, two files: IO_stats and MM_stats.
> > > 
> > > zram<id>/io_stat
> > > 
> > > s*printf( num_reads, num_writes, failed_reads, failed_writes, etc.)
> > 
> > Some of it(ie, num_reads, num_writes) was duplicated with /dev/block/zramx/stat?
> > I know /dev/block/zramx/stat doesn't work now and I didn't check why it doesn't
> > work but I hope we make it work so remove duplicate stat, finally. :)
> > 
> 
> yes, I do recall looking into the issue some months ago. zramX/stat file hanled by block
> layer in various places. for example, in:
> 
> 	blk_finish_request(struct request *req, int error)
> 		blk_account_io_done():
> 
> doing
> 
>   part_stat_inc(cpu, part, ios[rw]);
>   part_stat_add(cpu, part, ticks[rw], duration);
>   part_round_stats(cpu, part);
>   part_dec_in_flight(part, rw);
> 
> 
> the problem here is that zram has several paths that may issue IO:
> -- usual zram_make_request()
> -- zram_slot_free_notify()
> -- zram_rw_page()
> 
> in zram_slot_free_notify() and zram_rw_page() we don't have request queue, request,
> etc. so it's a bit troubling.

I skim the code so I might miss something.

zram_slot_free_notify is just to free allocated space on zsmalloc so
it's not related to I/O operation so it would be okay if we handle
make_request and rw_page. Fortunately, they share core function
called by zram_bvec_rw.  So could we use generic_[start|end]_io_acct
in there? It seems we don't need request queue.

> 
> 
> besides, /sys/block/zramX/stat file exports totally different data:
> 
>  struct disk_stats {
>          unsigned long sectors[2];       /* READs and WRITEs */
>          unsigned long ios[2];
>          unsigned long merges[2];
>          unsigned long ticks[2];
>          unsigned long io_ticks;
>          unsigned long time_in_queue;
>  };
> 
> Documentation/block/stat.txt
> 
> Name            units         description
> ----            -----         -----------
> read I/Os       requests      number of read I/Os processed
> read merges     requests      number of read I/Os merged with in-queue I/O
> read sectors    sectors       number of sectors read
> read ticks      milliseconds  total wait time for read requests
> write I/Os      requests      number of write I/Os processed
> write merges    requests      number of write I/Os merged with in-queue I/O
> write sectors   sectors       number of sectors written
> write ticks     milliseconds  total wait time for write requests
> in_flight       requests      number of I/Os currently in flight
> io_ticks        milliseconds  total time this block device has been active
> time_in_queue   milliseconds  total wait time for all requests
> 
> 
> the only overlaps are num_read and num_write. so we will not be able to move all

When I read above, read/write ticks would be useful to us.

> (or any significant amount) of our IO stats to that file. that will force users
> to gather IO stats accross several files.

I'm not saying let's move all of I/O related stuff.
What I want is to remove duplicated stat if it is and enable zram/stats
so I hope we could use iostat/nmon to monitor zram I/O.

> 
> I'll take a look later today/tomorrow if I can do anything about it, but it seems
> that our own zramX/io_stat file would be simpler solution here. it does sound ugly,
> but it doesn't look so bad after all.

If it is really impossible or makes kernel complicated, I will agree with you.
Otherwise, I really want to see zram in iostat. :)

Thanks for looking this, Sergey!

-- 
Kind regards,
Minchan Kim

Re: + zram-support-compaction.patch added to -mm tree

[Sergey Senozhatsky]

On (03/09/15 23:56), Minchan Kim wrote:
> > in zram_slot_free_notify() and zram_rw_page() we don't have request queue, request,
> > etc. so it's a bit troubling.
> 
> I skim the code so I might miss something.
> 
> zram_slot_free_notify is just to free allocated space on zsmalloc so
> it's not related to I/O operation so it would be okay if we handle
> make_request and rw_page. Fortunately, they share core function
> called by zram_bvec_rw.  So could we use generic_[start|end]_io_acct
> in there? It seems we don't need request queue.
> 

that will do the trick, I think. thanks. I found these two late last
night.

> > 
> > Name            units         description
> > ----            -----         -----------
> > read I/Os       requests      number of read I/Os processed
> > read merges     requests      number of read I/Os merged with in-queue I/O
> > read sectors    sectors       number of sectors read
> > read ticks      milliseconds  total wait time for read requests
> > write I/Os      requests      number of write I/Os processed
> > write merges    requests      number of write I/Os merged with in-queue I/O
> > write sectors   sectors       number of sectors written
> > write ticks     milliseconds  total wait time for write requests
> > in_flight       requests      number of I/Os currently in flight
> > io_ticks        milliseconds  total time this block device has been active
> > time_in_queue   milliseconds  total wait time for all requests
> > 
> > 
> > the only overlaps are num_read and num_write. so we will not be able to move all
> 
> When I read above, read/write ticks would be useful to us.

yes. somehow I didn't manage to shape my thoughts, I was going to say that this
stat file is surely interesting on his own; and was about to let num_reads and
num_writes to sit in both zram<id>/stat and zram<id>/io_stat files.

> > (or any significant amount) of our IO stats to that file. that will force users
> > to gather IO stats accross several files.
> 
> I'm not saying let's move all of I/O related stuff.
> What I want is to remove duplicated stat if it is and enable zram/stats
> so I hope we could use iostat/nmon to monitor zram I/O.

ok. I did some overlapping (as I mentioned above) -- num_reads and num_writes
present in both ./stat and ./io_stat files. will remove them.
so we end up having:
-- block layer stats in zram<id>/stat
-- zram internal IO stats in zram<id>/io_stat   (no num_reads, no num_writes)
-- zram mm stats in zram<id>/mm_stat   (orig size, compressed size, num_migrated, etc.)

> > 
> > I'll take a look later today/tomorrow if I can do anything about it, but it seems
> > that our own zramX/io_stat file would be simpler solution here. it does sound ugly,
> > but it doesn't look so bad after all.
> 
> If it is really impossible or makes kernel complicated, I will agree with you.
> Otherwise, I really want to see zram in iostat. :)

yes, that's the goal. I found our previous discussion on the topic:
https://lkml.org/lkml/2014/9/4/368

6 months later we are finally on it :)  will send the patches later today.

thanks,

	-ss