Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/15/14 13:24), Minchan Kim wrote:
> Hello Sergey,
> 
> On Tue, Jan 14, 2014 at 12:37:40PM +0300, Sergey Senozhatsky wrote:
> > 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> > `show' functions and reduce code duplication.
> > 
> > 2) Account and report back to user numbers of failed READ and WRITE
> > operations.
> > 
> > 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> > cause a number of RW sub-requests. zram used to account `good' compressed
> > sub-queries (with compressed size less than 50% of original size), `bad'
> > compressed sub-queries (with compressed size greater that 75% of original
> > size), leaving sub-requests with compression size between 50% and 75% of
> > original size not accounted and not reported. Account each sub-request
> > compression size so we can calculate real device compression ratio.
> > 
> > 4) reported zram stats:
> >   - num_writes  -- number of writes
> >   - num_reads  -- number of reads
> >   - pages_stored -- number of pages currently stored
> >   - compressed_size -- compressed size of pages stored
> >   - pages_zero  -- number of zero filled pages
> >   - failed_read -- number of failed reads
> >   - failed_writes -- can happen when memory is too low
> >   - invalid_io   -- non-page-aligned I/O requests
> >   - notify_free  -- number of swap slot free notifications
> >   - memory_used -- zs pool zs_get_total_size_bytes()
> > 
> > Signed-off-by: Sergey Senozhatsky 
> 
>

Hello Minchan,
I wouldn't say that it changes behaviour. The only things that is new
is that zram now accounts failed RW requests. the rest is a part of
clean up.

-ss

> So this patch includes some clean up and behavior changes?
> Please seaprate them and each patch in behavior change includes
> why it's bad or good(ie, motivation).
> 
> It could make reviewer/maintainer happy, even you because
> some of them could be picked up and other things could be dropped.
> Sorry for the bothering you.
> 
> Thanks.
> 
> > ---
> >  drivers/block/zram/zram_drv.c | 167 
> > --
> >  drivers/block/zram/zram_drv.h |  17 ++---
> >  2 files changed, 54 insertions(+), 130 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 2a7682c..8bddaff 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -42,6 +42,17 @@ static struct zram *zram_devices;
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +#define ZRAM_ATTR_RO(name) \
> > +static ssize_t zram_attr_##name##_show(struct device *d,   \
> > +   struct device_attribute *attr, char *b) \
> > +{  \
> > +   struct zram *zram = dev_to_zram(d); \
> > +   return sprintf(b, "%llu\n", \
> > +   (u64)atomic64_read(>stats.name)); \
> > +}  \
> > +static struct device_attribute dev_attr_##name =   \
> > +   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> > +
> >  static inline int init_done(struct zram *zram)
> >  {
> > return zram->meta != NULL;
> > @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device 
> > *dev)
> > return (struct zram *)dev_to_disk(dev)->private_data;
> >  }
> >  
> > -static ssize_t disksize_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n", zram->disksize);
> > -}
> > -
> > -static ssize_t initstate_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%u\n", init_done(zram));
> > -}
> > -
> > -static ssize_t num_reads_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n",
> > -   (u64)atomic64_read(>stats.num_reads));
> > -}
> > -
> > -static ssize_t num_writes_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n",
> > -   (u64)atomic64_read(>stats.num_writes));
> > -}
> > -
> > -static ssize_t invalid_io_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n",
> > -   (u64)atomic64_read(>stats.invalid_io));
> > -}
> > -
> > -static ssize_t notify_free_show(struct device *dev,
> > -   struct device_attribute *attr, char 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/15/14 13:24), Minchan Kim wrote:
 Hello Sergey,
 
 On Tue, Jan 14, 2014 at 12:37:40PM +0300, Sergey Senozhatsky wrote:
  1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
  `show' functions and reduce code duplication.
  
  2) Account and report back to user numbers of failed READ and WRITE
  operations.
  
  3) Remove `good' and `bad' compressed sub-requests stats. RW request may
  cause a number of RW sub-requests. zram used to account `good' compressed
  sub-queries (with compressed size less than 50% of original size), `bad'
  compressed sub-queries (with compressed size greater that 75% of original
  size), leaving sub-requests with compression size between 50% and 75% of
  original size not accounted and not reported. Account each sub-request
  compression size so we can calculate real device compression ratio.
  
  4) reported zram stats:
- num_writes  -- number of writes
- num_reads  -- number of reads
- pages_stored -- number of pages currently stored
- compressed_size -- compressed size of pages stored
- pages_zero  -- number of zero filled pages
- failed_read -- number of failed reads
- failed_writes -- can happen when memory is too low
- invalid_io   -- non-page-aligned I/O requests
- notify_free  -- number of swap slot free notifications
- memory_used -- zs pool zs_get_total_size_bytes()
  
  Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
 


Hello Minchan,
I wouldn't say that it changes behaviour. The only things that is new
is that zram now accounts failed RW requests. the rest is a part of
clean up.

-ss

 So this patch includes some clean up and behavior changes?
 Please seaprate them and each patch in behavior change includes
 why it's bad or good(ie, motivation).
 
 It could make reviewer/maintainer happy, even you because
 some of them could be picked up and other things could be dropped.
 Sorry for the bothering you.
 
 Thanks.
 
  ---
   drivers/block/zram/zram_drv.c | 167 
  --
   drivers/block/zram/zram_drv.h |  17 ++---
   2 files changed, 54 insertions(+), 130 deletions(-)
  
  diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
  index 2a7682c..8bddaff 100644
  --- a/drivers/block/zram/zram_drv.c
  +++ b/drivers/block/zram/zram_drv.c
  @@ -42,6 +42,17 @@ static struct zram *zram_devices;
   /* Module params (documentation at end) */
   static unsigned int num_devices = 1;
   
  +#define ZRAM_ATTR_RO(name) \
  +static ssize_t zram_attr_##name##_show(struct device *d,   \
  +   struct device_attribute *attr, char *b) \
  +{  \
  +   struct zram *zram = dev_to_zram(d); \
  +   return sprintf(b, %llu\n, \
  +   (u64)atomic64_read(zram-stats.name)); \
  +}  \
  +static struct device_attribute dev_attr_##name =   \
  +   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
  +
   static inline int init_done(struct zram *zram)
   {
  return zram-meta != NULL;
  @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device 
  *dev)
  return (struct zram *)dev_to_disk(dev)-private_data;
   }
   
  -static ssize_t disksize_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n, zram-disksize);
  -}
  -
  -static ssize_t initstate_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %u\n, init_done(zram));
  -}
  -
  -static ssize_t num_reads_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.num_reads));
  -}
  -
  -static ssize_t num_writes_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.num_writes));
  -}
  -
  -static ssize_t invalid_io_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.invalid_io));
  -}
  -
  -static ssize_t notify_free_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.notify_free));
  -}
  -
  -static ssize_t 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Minchan Kim]

Hello Sergey,

On Tue, Jan 14, 2014 at 12:37:40PM +0300, Sergey Senozhatsky wrote:
> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> `show' functions and reduce code duplication.
> 
> 2) Account and report back to user numbers of failed READ and WRITE
> operations.
> 
> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> cause a number of RW sub-requests. zram used to account `good' compressed
> sub-queries (with compressed size less than 50% of original size), `bad'
> compressed sub-queries (with compressed size greater that 75% of original
> size), leaving sub-requests with compression size between 50% and 75% of
> original size not accounted and not reported. Account each sub-request
> compression size so we can calculate real device compression ratio.
> 
> 4) reported zram stats:
>   - num_writes  -- number of writes
>   - num_reads  -- number of reads
>   - pages_stored -- number of pages currently stored
>   - compressed_size -- compressed size of pages stored
>   - pages_zero  -- number of zero filled pages
>   - failed_read -- number of failed reads
>   - failed_writes -- can happen when memory is too low
>   - invalid_io   -- non-page-aligned I/O requests
>   - notify_free  -- number of swap slot free notifications
>   - memory_used -- zs pool zs_get_total_size_bytes()
> 
> Signed-off-by: Sergey Senozhatsky 

So this patch includes some clean up and behavior changes?
Please seaprate them and each patch in behavior change includes
why it's bad or good(ie, motivation).

It could make reviewer/maintainer happy, even you because
some of them could be picked up and other things could be dropped.
Sorry for the bothering you.

Thanks.

> ---
>  drivers/block/zram/zram_drv.c | 167 
> --
>  drivers/block/zram/zram_drv.h |  17 ++---
>  2 files changed, 54 insertions(+), 130 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 2a7682c..8bddaff 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -42,6 +42,17 @@ static struct zram *zram_devices;
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +#define ZRAM_ATTR_RO(name)   \
> +static ssize_t zram_attr_##name##_show(struct device *d, \
> + struct device_attribute *attr, char *b) \
> +{\
> + struct zram *zram = dev_to_zram(d); \
> + return sprintf(b, "%llu\n", \
> + (u64)atomic64_read(>stats.name)); \
> +}\
> +static struct device_attribute dev_attr_##name = \
> + __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> +
>  static inline int init_done(struct zram *zram)
>  {
>   return zram->meta != NULL;
> @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
>   return (struct zram *)dev_to_disk(dev)->private_data;
>  }
>  
> -static ssize_t disksize_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n", zram->disksize);
> -}
> -
> -static ssize_t initstate_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%u\n", init_done(zram));
> -}
> -
> -static ssize_t num_reads_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n",
> - (u64)atomic64_read(>stats.num_reads));
> -}
> -
> -static ssize_t num_writes_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n",
> - (u64)atomic64_read(>stats.num_writes));
> -}
> -
> -static ssize_t invalid_io_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n",
> - (u64)atomic64_read(>stats.invalid_io));
> -}
> -
> -static ssize_t notify_free_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n",
> - (u64)atomic64_read(>stats.notify_free));
> -}
> -
> -static ssize_t zero_pages_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%u\n", 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 03:09 PM, Sergey Senozhatsky wrote:
> On (01/14/14 15:02), Jerome Marchand wrote:
>> On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
>>> On (01/14/14 14:43), Jerome Marchand wrote:
>>> [..]
>>
>> That's weird: good/bad_compress are accounted, but it seems to me that
>> they are to never used in any way. If so, there is indeed no reason to
>> keep them.
>>
>>
>>> Account each sub-request
>>> compression size so we can calculate real device compression ratio.
>>
>> Your patch doesn't change the way pages_stored and compr[essed]_size
>> are accounted. What does your patch change that allow us to calculate
>> the "real" compression ratio?
>>
>>>
>>> 4) reported zram stats:
>>>   - num_writes  -- number of writes
>>>   - num_reads  -- number of reads
>>>   - pages_stored -- number of pages currently stored
>>>   - compressed_size -- compressed size of pages stored
>>
>> Wouldn't it be more practical to report the original and compressed
>> data sizes using the same units as it is currently done?
>>
>
> hm, do we really need pages_stored stats? what kind of unseful 
> information it
> shows to end user?.. perhaps, it's better to replace it with accounted 
> passed
> bvec->bv_len (as uncompressed_size).
>

 That's really going to complicates things. We would need to keep track
 of which sectors of a particular page has been written to. It's much
 easier to keep current page granularity and consider any partial I/O
 as an whole page I/O.

>>>
>>> fair enough. thank you.
>>>
>>> 2/3 and 3/3 were changed according to your comments:
>>> - 2/3 drop READA check
>>> - 3/3 update commit message.
>>>
>>> ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?
>>
>> The READA thing was my only concern for 2/3, so yes for this one.
>> Concerning the third patch, I'd like to see what other people think
>> about which stats we want to report.
>>
> 
> good. so I hold on for a bit to minimize the traffic and see what other
> people think. thank you.
> 
>   -ss
> 
>
>> Jerome
>>
>>>   - pages_zero  -- number of zero filled pages
>>>   - failed_read -- number of failed reads
>>>   - failed_writes -- can happen when memory is too low
>>>   - invalid_io   -- non-page-aligned I/O requests
>>>   - notify_free  -- number of swap slot free notifications
>>>   - memory_used -- zs pool zs_get_total_size_bytes()

I also wonder if we should add size of meta-data to memory_used,
especially the table size which is probably not always negligible.

Jerome

>>>
>>> Signed-off-by: Sergey Senozhatsky 


>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 15:02), Jerome Marchand wrote:
> On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
> > On (01/14/14 14:43), Jerome Marchand wrote:
> > [..]
> 
>  That's weird: good/bad_compress are accounted, but it seems to me that
>  they are to never used in any way. If so, there is indeed no reason to
>  keep them.
> 
> 
> > Account each sub-request
> > compression size so we can calculate real device compression ratio.
> 
>  Your patch doesn't change the way pages_stored and compr[essed]_size
>  are accounted. What does your patch change that allow us to calculate
>  the "real" compression ratio?
> 
> >
> > 4) reported zram stats:
> >   - num_writes  -- number of writes
> >   - num_reads  -- number of reads
> >   - pages_stored -- number of pages currently stored
> >   - compressed_size -- compressed size of pages stored
> 
>  Wouldn't it be more practical to report the original and compressed
>  data sizes using the same units as it is currently done?
> 
> >>>
> >>> hm, do we really need pages_stored stats? what kind of unseful 
> >>> information it
> >>> shows to end user?.. perhaps, it's better to replace it with accounted 
> >>> passed
> >>> bvec->bv_len (as uncompressed_size).
> >>>
> >>
> >> That's really going to complicates things. We would need to keep track
> >> of which sectors of a particular page has been written to. It's much
> >> easier to keep current page granularity and consider any partial I/O
> >> as an whole page I/O.
> >>
> > 
> > fair enough. thank you.
> > 
> > 2/3 and 3/3 were changed according to your comments:
> > - 2/3 drop READA check
> > - 3/3 update commit message.
> > 
> > ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?
> 
> The READA thing was my only concern for 2/3, so yes for this one.
> Concerning the third patch, I'd like to see what other people think
> about which stats we want to report.
> 

good. so I hold on for a bit to minimize the traffic and see what other
people think. thank you.

-ss

> >>>
>  Jerome
> 
> >   - pages_zero  -- number of zero filled pages
> >   - failed_read -- number of failed reads
> >   - failed_writes -- can happen when memory is too low
> >   - invalid_io   -- non-page-aligned I/O requests
> >   - notify_free  -- number of swap slot free notifications
> >   - memory_used -- zs pool zs_get_total_size_bytes()
> >
> > Signed-off-by: Sergey Senozhatsky 
> >>
> >>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
> On (01/14/14 14:43), Jerome Marchand wrote:
> [..]

 That's weird: good/bad_compress are accounted, but it seems to me that
 they are to never used in any way. If so, there is indeed no reason to
 keep them.


> Account each sub-request
> compression size so we can calculate real device compression ratio.

 Your patch doesn't change the way pages_stored and compr[essed]_size
 are accounted. What does your patch change that allow us to calculate
 the "real" compression ratio?

>
> 4) reported zram stats:
>   - num_writes  -- number of writes
>   - num_reads  -- number of reads
>   - pages_stored -- number of pages currently stored
>   - compressed_size -- compressed size of pages stored

 Wouldn't it be more practical to report the original and compressed
 data sizes using the same units as it is currently done?

>>>
>>> hm, do we really need pages_stored stats? what kind of unseful information 
>>> it
>>> shows to end user?.. perhaps, it's better to replace it with accounted 
>>> passed
>>> bvec->bv_len (as uncompressed_size).
>>>
>>
>> That's really going to complicates things. We would need to keep track
>> of which sectors of a particular page has been written to. It's much
>> easier to keep current page granularity and consider any partial I/O
>> as an whole page I/O.
>>
> 
> fair enough. thank you.
> 
> 2/3 and 3/3 were changed according to your comments:
> - 2/3 drop READA check
> - 3/3 update commit message.
> 
> ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?

The READA thing was my only concern for 2/3, so yes for this one.
Concerning the third patch, I'd like to see what other people think
about which stats we want to report.

> 
>   -ss
> 
>>>
 Jerome

>   - pages_zero  -- number of zero filled pages
>   - failed_read -- number of failed reads
>   - failed_writes -- can happen when memory is too low
>   - invalid_io   -- non-page-aligned I/O requests
>   - notify_free  -- number of swap slot free notifications
>   - memory_used -- zs pool zs_get_total_size_bytes()
>
> Signed-off-by: Sergey Senozhatsky 
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 14:43), Jerome Marchand wrote:
[..]
> >>
> >> That's weird: good/bad_compress are accounted, but it seems to me that
> >> they are to never used in any way. If so, there is indeed no reason to
> >> keep them.
> >>
> >>
> >>> Account each sub-request
> >>> compression size so we can calculate real device compression ratio.
> >>
> >> Your patch doesn't change the way pages_stored and compr[essed]_size
> >> are accounted. What does your patch change that allow us to calculate
> >> the "real" compression ratio?
> >>
> >>>
> >>> 4) reported zram stats:
> >>>   - num_writes  -- number of writes
> >>>   - num_reads  -- number of reads
> >>>   - pages_stored -- number of pages currently stored
> >>>   - compressed_size -- compressed size of pages stored
> >>
> >> Wouldn't it be more practical to report the original and compressed
> >> data sizes using the same units as it is currently done?
> >>
> > 
> > hm, do we really need pages_stored stats? what kind of unseful information 
> > it
> > shows to end user?.. perhaps, it's better to replace it with accounted 
> > passed
> > bvec->bv_len (as uncompressed_size).
> > 
> 
> That's really going to complicates things. We would need to keep track
> of which sectors of a particular page has been written to. It's much
> easier to keep current page granularity and consider any partial I/O
> as an whole page I/O.
> 

fair enough. thank you.

2/3 and 3/3 were changed according to your comments:
- 2/3 drop READA check
- 3/3 update commit message.

ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?

-ss

> > 
> >> Jerome
> >>
> >>>   - pages_zero  -- number of zero filled pages
> >>>   - failed_read -- number of failed reads
> >>>   - failed_writes -- can happen when memory is too low
> >>>   - invalid_io   -- non-page-aligned I/O requests
> >>>   - notify_free  -- number of swap slot free notifications
> >>>   - memory_used -- zs pool zs_get_total_size_bytes()
> >>>
> >>> Signed-off-by: Sergey Senozhatsky 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 12:10 PM, Sergey Senozhatsky wrote:
> On (01/14/14 11:38), Jerome Marchand wrote:
>> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
>>> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
>>> `show' functions and reduce code duplication.
>>>
>>> 2) Account and report back to user numbers of failed READ and WRITE
>>> operations.
>>>
>>> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
>>> cause a number of RW sub-requests. zram used to account `good' compressed
>>> sub-queries (with compressed size less than 50% of original size), `bad'
>>> compressed sub-queries (with compressed size greater that 75% of original
>>> size), leaving sub-requests with compression size between 50% and 75% of
>>> original size not accounted and not reported.
>>
>> That's weird: good/bad_compress are accounted, but it seems to me that
>> they are to never used in any way. If so, there is indeed no reason to
>> keep them.
>>
>>
>>> Account each sub-request
>>> compression size so we can calculate real device compression ratio.
>>
>> Your patch doesn't change the way pages_stored and compr[essed]_size
>> are accounted. What does your patch change that allow us to calculate
>> the "real" compression ratio?
>>
>>>
>>> 4) reported zram stats:
>>>   - num_writes  -- number of writes
>>>   - num_reads  -- number of reads
>>>   - pages_stored -- number of pages currently stored
>>>   - compressed_size -- compressed size of pages stored
>>
>> Wouldn't it be more practical to report the original and compressed
>> data sizes using the same units as it is currently done?
>>
> 
> hm, do we really need pages_stored stats? what kind of unseful information it
> shows to end user?.. perhaps, it's better to replace it with accounted passed
> bvec->bv_len (as uncompressed_size).
> 

That's really going to complicates things. We would need to keep track
of which sectors of a particular page has been written to. It's much
easier to keep current page granularity and consider any partial I/O
as an whole page I/O.

>   -ss
> 
>> Jerome
>>
>>>   - pages_zero  -- number of zero filled pages
>>>   - failed_read -- number of failed reads
>>>   - failed_writes -- can happen when memory is too low
>>>   - invalid_io   -- non-page-aligned I/O requests
>>>   - notify_free  -- number of swap slot free notifications
>>>   - memory_used -- zs pool zs_get_total_size_bytes()
>>>
>>> Signed-off-by: Sergey Senozhatsky 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 13:15), Jerome Marchand wrote:
> On 01/14/2014 11:57 AM, Sergey Senozhatsky wrote:
> > 
> > Hello Jerome,
> > 
> > On (01/14/14 11:38), Jerome Marchand wrote:
> >> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> >>> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> >>> `show' functions and reduce code duplication.
> >>>
> >>> 2) Account and report back to user numbers of failed READ and WRITE
> >>> operations.
> >>>
> >>> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> >>> cause a number of RW sub-requests. zram used to account `good' compressed
> >>> sub-queries (with compressed size less than 50% of original size), `bad'
> >>> compressed sub-queries (with compressed size greater that 75% of original
> >>> size), leaving sub-requests with compression size between 50% and 75% of
> >>> original size not accounted and not reported.
> >>
> >> That's weird: good/bad_compress are accounted, but it seems to me that
> >> they are to never used in any way. If so, there is indeed no reason to
> >> keep them.
> >>
> >>
> >>> Account each sub-request
> >>> compression size so we can calculate real device compression ratio.
> >>
> >> Your patch doesn't change the way pages_stored and compr[essed]_size
> >> are accounted. What does your patch change that allow us to calculate
> >> the "real" compression ratio?
> > 
> > we have compressed size, number of stored pages and reported by zs pool
> > (as a zram memory_used attr) number of bytes used
> > 
> > u64 zs_get_total_size_bytes(struct zs_pool *pool)
> > {
> > int i;
> > u64 npages = 0;
> > 
> > for (i = 0; i < ZS_SIZE_CLASSES; i++)
> > npages += pool->size_class[i].pages_allocated;
> > 
> > return npages << PAGE_SHIFT;
> > }
> > 
> > looks enough to calculate device overall data compression ratio.
> 
> Yes. But don't we have all that already without your patch applied?
> What does this patch change?
> 
>

oh. yes, bad wording. the commit message must be "*zram accounts* each
sub-request compression size so we can calculate real device compression
ratio." instead of  "Account each sub-request compression size so we can
calculate real device compression ratio.". otherwise, there is a false
feeling that patch change/introduce this functionality. will re-write
that commit message. sorry.

the patch does not change a lot of things and may be considered mainly as
a clean up patch. it:
-- removes unused and misleading bad/good stats
-- makes some attrs names more readable e.g. mem_used_total to
memory_used, compr_data_size to compressed_size
-- accounts and reports numbers of failed RW requests
-- removes ATTR_show() code duplication using ZRAM_ATTR_RO macro

-ss

> > 
> > -ss
> > 
> >>>
> >>> 4) reported zram stats:
> >>>   - num_writes  -- number of writes
> >>>   - num_reads  -- number of reads
> >>>   - pages_stored -- number of pages currently stored
> >>>   - compressed_size -- compressed size of pages stored
> >>
> >> Wouldn't it be more practical to report the original and compressed
> >> data sizes using the same units as it is currently done?
> >>
> > 
> > sorry, not sure I understand.
> 
> Currently users have access to orig_data_size and compr_data_size,
> both in bytes. With your patch, they have access to pages_stored in
> pages and compressed_size in bytes. I find the current set more
> practical.
> 
> Jerome
> 
> > 
> >> Jerome
> >>
> >>>   - pages_zero  -- number of zero filled pages
> >>>   - failed_read -- number of failed reads
> >>>   - failed_writes -- can happen when memory is too low
> >>>   - invalid_io   -- non-page-aligned I/O requests
> >>>   - notify_free  -- number of swap slot free notifications
> >>>   - memory_used -- zs pool zs_get_total_size_bytes()
> >>>
> >>> Signed-off-by: Sergey Senozhatsky 
> >>> ---
> >>>  drivers/block/zram/zram_drv.c | 167 
> >>> --
> >>>  drivers/block/zram/zram_drv.h |  17 ++---
> >>>  2 files changed, 54 insertions(+), 130 deletions(-)
> >>>
> >>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> >>> index 2a7682c..8bddaff 100644
> >>> --- a/drivers/block/zram/zram_drv.c
> >>> +++ b/drivers/block/zram/zram_drv.c
> >>> @@ -42,6 +42,17 @@ static struct zram *zram_devices;
> >>>  /* Module params (documentation at end) */
> >>>  static unsigned int num_devices = 1;
> >>>  
> >>> +#define ZRAM_ATTR_RO(name)   
> >>> \
> >>> +static ssize_t zram_attr_##name##_show(struct device *d, \
> >>> + struct device_attribute *attr, char *b) \
> >>> +{
> >>> \
> >>> + struct zram *zram = dev_to_zram(d); \
> >>> + return sprintf(b, "%llu\n", \
> >>> + (u64)atomic64_read(>stats.name)); \
> >>> +}  

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 11:57 AM, Sergey Senozhatsky wrote:
> 
> Hello Jerome,
> 
> On (01/14/14 11:38), Jerome Marchand wrote:
>> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
>>> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
>>> `show' functions and reduce code duplication.
>>>
>>> 2) Account and report back to user numbers of failed READ and WRITE
>>> operations.
>>>
>>> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
>>> cause a number of RW sub-requests. zram used to account `good' compressed
>>> sub-queries (with compressed size less than 50% of original size), `bad'
>>> compressed sub-queries (with compressed size greater that 75% of original
>>> size), leaving sub-requests with compression size between 50% and 75% of
>>> original size not accounted and not reported.
>>
>> That's weird: good/bad_compress are accounted, but it seems to me that
>> they are to never used in any way. If so, there is indeed no reason to
>> keep them.
>>
>>
>>> Account each sub-request
>>> compression size so we can calculate real device compression ratio.
>>
>> Your patch doesn't change the way pages_stored and compr[essed]_size
>> are accounted. What does your patch change that allow us to calculate
>> the "real" compression ratio?
> 
> we have compressed size, number of stored pages and reported by zs pool
> (as a zram memory_used attr) number of bytes used
> 
> u64 zs_get_total_size_bytes(struct zs_pool *pool)
> {
> int i;
> u64 npages = 0;
> 
> for (i = 0; i < ZS_SIZE_CLASSES; i++)
> npages += pool->size_class[i].pages_allocated;
> 
> return npages << PAGE_SHIFT;
> }
> 
> looks enough to calculate device overall data compression ratio.

Yes. But don't we have all that already without your patch applied?
What does this patch change?

> 
>   -ss
> 
>>>
>>> 4) reported zram stats:
>>>   - num_writes  -- number of writes
>>>   - num_reads  -- number of reads
>>>   - pages_stored -- number of pages currently stored
>>>   - compressed_size -- compressed size of pages stored
>>
>> Wouldn't it be more practical to report the original and compressed
>> data sizes using the same units as it is currently done?
>>
> 
> sorry, not sure I understand.

Currently users have access to orig_data_size and compr_data_size,
both in bytes. With your patch, they have access to pages_stored in
pages and compressed_size in bytes. I find the current set more
practical.

Jerome

> 
>> Jerome
>>
>>>   - pages_zero  -- number of zero filled pages
>>>   - failed_read -- number of failed reads
>>>   - failed_writes -- can happen when memory is too low
>>>   - invalid_io   -- non-page-aligned I/O requests
>>>   - notify_free  -- number of swap slot free notifications
>>>   - memory_used -- zs pool zs_get_total_size_bytes()
>>>
>>> Signed-off-by: Sergey Senozhatsky 
>>> ---
>>>  drivers/block/zram/zram_drv.c | 167 
>>> --
>>>  drivers/block/zram/zram_drv.h |  17 ++---
>>>  2 files changed, 54 insertions(+), 130 deletions(-)
>>>
>>> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
>>> index 2a7682c..8bddaff 100644
>>> --- a/drivers/block/zram/zram_drv.c
>>> +++ b/drivers/block/zram/zram_drv.c
>>> @@ -42,6 +42,17 @@ static struct zram *zram_devices;
>>>  /* Module params (documentation at end) */
>>>  static unsigned int num_devices = 1;
>>>  
>>> +#define ZRAM_ATTR_RO(name) \
>>> +static ssize_t zram_attr_##name##_show(struct device *d,   \
>>> +   struct device_attribute *attr, char *b) \
>>> +{  \
>>> +   struct zram *zram = dev_to_zram(d); \
>>> +   return sprintf(b, "%llu\n", \
>>> +   (u64)atomic64_read(>stats.name)); \
>>> +}  \
>>> +static struct device_attribute dev_attr_##name =   \
>>> +   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
>>> +
>>>  static inline int init_done(struct zram *zram)
>>>  {
>>> return zram->meta != NULL;
>>> @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device 
>>> *dev)
>>> return (struct zram *)dev_to_disk(dev)->private_data;
>>>  }
>>>  
>>> -static ssize_t disksize_show(struct device *dev,
>>> -   struct device_attribute *attr, char *buf)
>>> -{
>>> -   struct zram *zram = dev_to_zram(dev);
>>> -
>>> -   return sprintf(buf, "%llu\n", zram->disksize);
>>> -}
>>> -
>>> -static ssize_t initstate_show(struct device *dev,
>>> -   struct device_attribute *attr, char *buf)
>>> -{
>>> -   struct zram *zram = dev_to_zram(dev);
>>> -
>>> -   return sprintf(buf, "%u\n", init_done(zram));
>>> -}
>>> -
>>> -static ssize_t num_reads_show(struct device *dev,
>>> -   struct device_attribute *attr, char *buf)
>>> -{

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 11:38), Jerome Marchand wrote:
> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> > 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> > `show' functions and reduce code duplication.
> > 
> > 2) Account and report back to user numbers of failed READ and WRITE
> > operations.
> > 
> > 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> > cause a number of RW sub-requests. zram used to account `good' compressed
> > sub-queries (with compressed size less than 50% of original size), `bad'
> > compressed sub-queries (with compressed size greater that 75% of original
> > size), leaving sub-requests with compression size between 50% and 75% of
> > original size not accounted and not reported.
> 
> That's weird: good/bad_compress are accounted, but it seems to me that
> they are to never used in any way. If so, there is indeed no reason to
> keep them.
> 
>
> > Account each sub-request
> > compression size so we can calculate real device compression ratio.
> 
> Your patch doesn't change the way pages_stored and compr[essed]_size
> are accounted. What does your patch change that allow us to calculate
> the "real" compression ratio?
> 
> > 
> > 4) reported zram stats:
> >   - num_writes  -- number of writes
> >   - num_reads  -- number of reads
> >   - pages_stored -- number of pages currently stored
> >   - compressed_size -- compressed size of pages stored
> 
> Wouldn't it be more practical to report the original and compressed
> data sizes using the same units as it is currently done?
> 

hm, do we really need pages_stored stats? what kind of unseful information it
shows to end user?.. perhaps, it's better to replace it with accounted passed
bvec->bv_len (as uncompressed_size).

-ss

> Jerome
> 
> >   - pages_zero  -- number of zero filled pages
> >   - failed_read -- number of failed reads
> >   - failed_writes -- can happen when memory is too low
> >   - invalid_io   -- non-page-aligned I/O requests
> >   - notify_free  -- number of swap slot free notifications
> >   - memory_used -- zs pool zs_get_total_size_bytes()
> > 
> > Signed-off-by: Sergey Senozhatsky 
> > ---
> >  drivers/block/zram/zram_drv.c | 167 
> > --
> >  drivers/block/zram/zram_drv.h |  17 ++---
> >  2 files changed, 54 insertions(+), 130 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 2a7682c..8bddaff 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -42,6 +42,17 @@ static struct zram *zram_devices;
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +#define ZRAM_ATTR_RO(name) \
> > +static ssize_t zram_attr_##name##_show(struct device *d,   \
> > +   struct device_attribute *attr, char *b) \
> > +{  \
> > +   struct zram *zram = dev_to_zram(d); \
> > +   return sprintf(b, "%llu\n", \
> > +   (u64)atomic64_read(>stats.name)); \
> > +}  \
> > +static struct device_attribute dev_attr_##name =   \
> > +   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> > +
> >  static inline int init_done(struct zram *zram)
> >  {
> > return zram->meta != NULL;
> > @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device 
> > *dev)
> > return (struct zram *)dev_to_disk(dev)->private_data;
> >  }
> >  
> > -static ssize_t disksize_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n", zram->disksize);
> > -}
> > -
> > -static ssize_t initstate_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%u\n", init_done(zram));
> > -}
> > -
> > -static ssize_t num_reads_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n",
> > -   (u64)atomic64_read(>stats.num_reads));
> > -}
> > -
> > -static ssize_t num_writes_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n",
> > -   (u64)atomic64_read(>stats.num_writes));
> > -}
> > -
> > -static ssize_t invalid_io_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n",
> > -   

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

Hello Jerome,

On (01/14/14 11:38), Jerome Marchand wrote:
> On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> > 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> > `show' functions and reduce code duplication.
> > 
> > 2) Account and report back to user numbers of failed READ and WRITE
> > operations.
> > 
> > 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> > cause a number of RW sub-requests. zram used to account `good' compressed
> > sub-queries (with compressed size less than 50% of original size), `bad'
> > compressed sub-queries (with compressed size greater that 75% of original
> > size), leaving sub-requests with compression size between 50% and 75% of
> > original size not accounted and not reported.
> 
> That's weird: good/bad_compress are accounted, but it seems to me that
> they are to never used in any way. If so, there is indeed no reason to
> keep them.
> 
>
> > Account each sub-request
> > compression size so we can calculate real device compression ratio.
> 
> Your patch doesn't change the way pages_stored and compr[essed]_size
> are accounted. What does your patch change that allow us to calculate
> the "real" compression ratio?

we have compressed size, number of stored pages and reported by zs pool
(as a zram memory_used attr) number of bytes used

u64 zs_get_total_size_bytes(struct zs_pool *pool)
{
int i;
u64 npages = 0;

for (i = 0; i < ZS_SIZE_CLASSES; i++)
npages += pool->size_class[i].pages_allocated;

return npages << PAGE_SHIFT;
}

looks enough to calculate device overall data compression ratio.

-ss

> > 
> > 4) reported zram stats:
> >   - num_writes  -- number of writes
> >   - num_reads  -- number of reads
> >   - pages_stored -- number of pages currently stored
> >   - compressed_size -- compressed size of pages stored
> 
> Wouldn't it be more practical to report the original and compressed
> data sizes using the same units as it is currently done?
> 

sorry, not sure I understand.

> Jerome
> 
> >   - pages_zero  -- number of zero filled pages
> >   - failed_read -- number of failed reads
> >   - failed_writes -- can happen when memory is too low
> >   - invalid_io   -- non-page-aligned I/O requests
> >   - notify_free  -- number of swap slot free notifications
> >   - memory_used -- zs pool zs_get_total_size_bytes()
> > 
> > Signed-off-by: Sergey Senozhatsky 
> > ---
> >  drivers/block/zram/zram_drv.c | 167 
> > --
> >  drivers/block/zram/zram_drv.h |  17 ++---
> >  2 files changed, 54 insertions(+), 130 deletions(-)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 2a7682c..8bddaff 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -42,6 +42,17 @@ static struct zram *zram_devices;
> >  /* Module params (documentation at end) */
> >  static unsigned int num_devices = 1;
> >  
> > +#define ZRAM_ATTR_RO(name) \
> > +static ssize_t zram_attr_##name##_show(struct device *d,   \
> > +   struct device_attribute *attr, char *b) \
> > +{  \
> > +   struct zram *zram = dev_to_zram(d); \
> > +   return sprintf(b, "%llu\n", \
> > +   (u64)atomic64_read(>stats.name)); \
> > +}  \
> > +static struct device_attribute dev_attr_##name =   \
> > +   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> > +
> >  static inline int init_done(struct zram *zram)
> >  {
> > return zram->meta != NULL;
> > @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device 
> > *dev)
> > return (struct zram *)dev_to_disk(dev)->private_data;
> >  }
> >  
> > -static ssize_t disksize_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n", zram->disksize);
> > -}
> > -
> > -static ssize_t initstate_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%u\n", init_done(zram));
> > -}
> > -
> > -static ssize_t num_reads_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n",
> > -   (u64)atomic64_read(>stats.num_reads));
> > -}
> > -
> > -static ssize_t num_writes_show(struct device *dev,
> > -   struct device_attribute *attr, char *buf)
> > -{
> > -   struct zram *zram = dev_to_zram(dev);
> > -
> > -   return sprintf(buf, "%llu\n",
> > -   

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
> 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
> `show' functions and reduce code duplication.
> 
> 2) Account and report back to user numbers of failed READ and WRITE
> operations.
> 
> 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
> cause a number of RW sub-requests. zram used to account `good' compressed
> sub-queries (with compressed size less than 50% of original size), `bad'
> compressed sub-queries (with compressed size greater that 75% of original
> size), leaving sub-requests with compression size between 50% and 75% of
> original size not accounted and not reported.

That's weird: good/bad_compress are accounted, but it seems to me that
they are to never used in any way. If so, there is indeed no reason to
keep them.

> Account each sub-request
> compression size so we can calculate real device compression ratio.

Your patch doesn't change the way pages_stored and compr[essed]_size
are accounted. What does your patch change that allow us to calculate
the "real" compression ratio?

> 
> 4) reported zram stats:
>   - num_writes  -- number of writes
>   - num_reads  -- number of reads
>   - pages_stored -- number of pages currently stored
>   - compressed_size -- compressed size of pages stored

Wouldn't it be more practical to report the original and compressed
data sizes using the same units as it is currently done?

Jerome

>   - pages_zero  -- number of zero filled pages
>   - failed_read -- number of failed reads
>   - failed_writes -- can happen when memory is too low
>   - invalid_io   -- non-page-aligned I/O requests
>   - notify_free  -- number of swap slot free notifications
>   - memory_used -- zs pool zs_get_total_size_bytes()
> 
> Signed-off-by: Sergey Senozhatsky 
> ---
>  drivers/block/zram/zram_drv.c | 167 
> --
>  drivers/block/zram/zram_drv.h |  17 ++---
>  2 files changed, 54 insertions(+), 130 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 2a7682c..8bddaff 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -42,6 +42,17 @@ static struct zram *zram_devices;
>  /* Module params (documentation at end) */
>  static unsigned int num_devices = 1;
>  
> +#define ZRAM_ATTR_RO(name)   \
> +static ssize_t zram_attr_##name##_show(struct device *d, \
> + struct device_attribute *attr, char *b) \
> +{\
> + struct zram *zram = dev_to_zram(d); \
> + return sprintf(b, "%llu\n", \
> + (u64)atomic64_read(>stats.name)); \
> +}\
> +static struct device_attribute dev_attr_##name = \
> + __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
> +
>  static inline int init_done(struct zram *zram)
>  {
>   return zram->meta != NULL;
> @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
>   return (struct zram *)dev_to_disk(dev)->private_data;
>  }
>  
> -static ssize_t disksize_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n", zram->disksize);
> -}
> -
> -static ssize_t initstate_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%u\n", init_done(zram));
> -}
> -
> -static ssize_t num_reads_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n",
> - (u64)atomic64_read(>stats.num_reads));
> -}
> -
> -static ssize_t num_writes_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n",
> - (u64)atomic64_read(>stats.num_writes));
> -}
> -
> -static ssize_t invalid_io_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n",
> - (u64)atomic64_read(>stats.invalid_io));
> -}
> -
> -static ssize_t notify_free_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - struct zram *zram = dev_to_zram(dev);
> -
> - return sprintf(buf, "%llu\n",
> - (u64)atomic64_read(>stats.notify_free));
> -}
> -
> -static ssize_t zero_pages_show(struct device *dev,
> - struct device_attribute *attr, char *buf)
> -{
> - 

[PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
`show' functions and reduce code duplication.

2) Account and report back to user numbers of failed READ and WRITE
operations.

3) Remove `good' and `bad' compressed sub-requests stats. RW request may
cause a number of RW sub-requests. zram used to account `good' compressed
sub-queries (with compressed size less than 50% of original size), `bad'
compressed sub-queries (with compressed size greater that 75% of original
size), leaving sub-requests with compression size between 50% and 75% of
original size not accounted and not reported. Account each sub-request
compression size so we can calculate real device compression ratio.

4) reported zram stats:
  - num_writes  -- number of writes
  - num_reads  -- number of reads
  - pages_stored -- number of pages currently stored
  - compressed_size -- compressed size of pages stored
  - pages_zero  -- number of zero filled pages
  - failed_read -- number of failed reads
  - failed_writes -- can happen when memory is too low
  - invalid_io   -- non-page-aligned I/O requests
  - notify_free  -- number of swap slot free notifications
  - memory_used -- zs pool zs_get_total_size_bytes()

Signed-off-by: Sergey Senozhatsky 
---
 drivers/block/zram/zram_drv.c | 167 --
 drivers/block/zram/zram_drv.h |  17 ++---
 2 files changed, 54 insertions(+), 130 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 2a7682c..8bddaff 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -42,6 +42,17 @@ static struct zram *zram_devices;
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
 
+#define ZRAM_ATTR_RO(name) \
+static ssize_t zram_attr_##name##_show(struct device *d,   \
+   struct device_attribute *attr, char *b) \
+{  \
+   struct zram *zram = dev_to_zram(d); \
+   return sprintf(b, "%llu\n", \
+   (u64)atomic64_read(>stats.name)); \
+}  \
+static struct device_attribute dev_attr_##name =   \
+   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
+
 static inline int init_done(struct zram *zram)
 {
return zram->meta != NULL;
@@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
return (struct zram *)dev_to_disk(dev)->private_data;
 }
 
-static ssize_t disksize_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, "%llu\n", zram->disksize);
-}
-
-static ssize_t initstate_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, "%u\n", init_done(zram));
-}
-
-static ssize_t num_reads_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, "%llu\n",
-   (u64)atomic64_read(>stats.num_reads));
-}
-
-static ssize_t num_writes_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, "%llu\n",
-   (u64)atomic64_read(>stats.num_writes));
-}
-
-static ssize_t invalid_io_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, "%llu\n",
-   (u64)atomic64_read(>stats.invalid_io));
-}
-
-static ssize_t notify_free_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, "%llu\n",
-   (u64)atomic64_read(>stats.notify_free));
-}
-
-static ssize_t zero_pages_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, "%u\n", atomic_read(>stats.pages_zero));
-}
-
-static ssize_t orig_data_size_show(struct device *dev,
+static ssize_t memory_used_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
+   u64 val = 0;
struct zram *zram = dev_to_zram(dev);
+   struct zram_meta *meta = zram->meta;
 
-   return sprintf(buf, "%llu\n",
-   (u64)(atomic_read(>stats.pages_stored)) << PAGE_SHIFT);
+   down_read(>init_lock);
+   if (init_done(zram))
+   val = zs_get_total_size_bytes(meta->mem_pool);
+   up_read(>init_lock);
+   

[PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
`show' functions and reduce code duplication.

2) Account and report back to user numbers of failed READ and WRITE
operations.

3) Remove `good' and `bad' compressed sub-requests stats. RW request may
cause a number of RW sub-requests. zram used to account `good' compressed
sub-queries (with compressed size less than 50% of original size), `bad'
compressed sub-queries (with compressed size greater that 75% of original
size), leaving sub-requests with compression size between 50% and 75% of
original size not accounted and not reported. Account each sub-request
compression size so we can calculate real device compression ratio.

4) reported zram stats:
  - num_writes  -- number of writes
  - num_reads  -- number of reads
  - pages_stored -- number of pages currently stored
  - compressed_size -- compressed size of pages stored
  - pages_zero  -- number of zero filled pages
  - failed_read -- number of failed reads
  - failed_writes -- can happen when memory is too low
  - invalid_io   -- non-page-aligned I/O requests
  - notify_free  -- number of swap slot free notifications
  - memory_used -- zs pool zs_get_total_size_bytes()

Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
---
 drivers/block/zram/zram_drv.c | 167 --
 drivers/block/zram/zram_drv.h |  17 ++---
 2 files changed, 54 insertions(+), 130 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 2a7682c..8bddaff 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -42,6 +42,17 @@ static struct zram *zram_devices;
 /* Module params (documentation at end) */
 static unsigned int num_devices = 1;
 
+#define ZRAM_ATTR_RO(name) \
+static ssize_t zram_attr_##name##_show(struct device *d,   \
+   struct device_attribute *attr, char *b) \
+{  \
+   struct zram *zram = dev_to_zram(d); \
+   return sprintf(b, %llu\n, \
+   (u64)atomic64_read(zram-stats.name)); \
+}  \
+static struct device_attribute dev_attr_##name =   \
+   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
+
 static inline int init_done(struct zram *zram)
 {
return zram-meta != NULL;
@@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
return (struct zram *)dev_to_disk(dev)-private_data;
 }
 
-static ssize_t disksize_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, %llu\n, zram-disksize);
-}
-
-static ssize_t initstate_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, %u\n, init_done(zram));
-}
-
-static ssize_t num_reads_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, %llu\n,
-   (u64)atomic64_read(zram-stats.num_reads));
-}
-
-static ssize_t num_writes_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, %llu\n,
-   (u64)atomic64_read(zram-stats.num_writes));
-}
-
-static ssize_t invalid_io_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, %llu\n,
-   (u64)atomic64_read(zram-stats.invalid_io));
-}
-
-static ssize_t notify_free_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, %llu\n,
-   (u64)atomic64_read(zram-stats.notify_free));
-}
-
-static ssize_t zero_pages_show(struct device *dev,
-   struct device_attribute *attr, char *buf)
-{
-   struct zram *zram = dev_to_zram(dev);
-
-   return sprintf(buf, %u\n, atomic_read(zram-stats.pages_zero));
-}
-
-static ssize_t orig_data_size_show(struct device *dev,
+static ssize_t memory_used_show(struct device *dev,
struct device_attribute *attr, char *buf)
 {
+   u64 val = 0;
struct zram *zram = dev_to_zram(dev);
+   struct zram_meta *meta = zram-meta;
 
-   return sprintf(buf, %llu\n,
-   (u64)(atomic_read(zram-stats.pages_stored))  PAGE_SHIFT);
+   down_read(zram-init_lock);
+   if (init_done(zram))
+   val = zs_get_total_size_bytes(meta-mem_pool);
+ 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
 `show' functions and reduce code duplication.
 
 2) Account and report back to user numbers of failed READ and WRITE
 operations.
 
 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
 cause a number of RW sub-requests. zram used to account `good' compressed
 sub-queries (with compressed size less than 50% of original size), `bad'
 compressed sub-queries (with compressed size greater that 75% of original
 size), leaving sub-requests with compression size between 50% and 75% of
 original size not accounted and not reported.

That's weird: good/bad_compress are accounted, but it seems to me that
they are to never used in any way. If so, there is indeed no reason to
keep them.

 Account each sub-request
 compression size so we can calculate real device compression ratio.

Your patch doesn't change the way pages_stored and compr[essed]_size
are accounted. What does your patch change that allow us to calculate
the real compression ratio?

 
 4) reported zram stats:
   - num_writes  -- number of writes
   - num_reads  -- number of reads
   - pages_stored -- number of pages currently stored
   - compressed_size -- compressed size of pages stored

Wouldn't it be more practical to report the original and compressed
data sizes using the same units as it is currently done?

Jerome

   - pages_zero  -- number of zero filled pages
   - failed_read -- number of failed reads
   - failed_writes -- can happen when memory is too low
   - invalid_io   -- non-page-aligned I/O requests
   - notify_free  -- number of swap slot free notifications
   - memory_used -- zs pool zs_get_total_size_bytes()
 
 Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
 ---
  drivers/block/zram/zram_drv.c | 167 
 --
  drivers/block/zram/zram_drv.h |  17 ++---
  2 files changed, 54 insertions(+), 130 deletions(-)
 
 diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
 index 2a7682c..8bddaff 100644
 --- a/drivers/block/zram/zram_drv.c
 +++ b/drivers/block/zram/zram_drv.c
 @@ -42,6 +42,17 @@ static struct zram *zram_devices;
  /* Module params (documentation at end) */
  static unsigned int num_devices = 1;
  
 +#define ZRAM_ATTR_RO(name)   \
 +static ssize_t zram_attr_##name##_show(struct device *d, \
 + struct device_attribute *attr, char *b) \
 +{\
 + struct zram *zram = dev_to_zram(d); \
 + return sprintf(b, %llu\n, \
 + (u64)atomic64_read(zram-stats.name)); \
 +}\
 +static struct device_attribute dev_attr_##name = \
 + __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
 +
  static inline int init_done(struct zram *zram)
  {
   return zram-meta != NULL;
 @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
   return (struct zram *)dev_to_disk(dev)-private_data;
  }
  
 -static ssize_t disksize_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n, zram-disksize);
 -}
 -
 -static ssize_t initstate_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %u\n, init_done(zram));
 -}
 -
 -static ssize_t num_reads_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n,
 - (u64)atomic64_read(zram-stats.num_reads));
 -}
 -
 -static ssize_t num_writes_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n,
 - (u64)atomic64_read(zram-stats.num_writes));
 -}
 -
 -static ssize_t invalid_io_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n,
 - (u64)atomic64_read(zram-stats.invalid_io));
 -}
 -
 -static ssize_t notify_free_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n,
 - (u64)atomic64_read(zram-stats.notify_free));
 -}
 -
 -static ssize_t zero_pages_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %u\n, 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

Hello Jerome,

On (01/14/14 11:38), Jerome Marchand wrote:
 On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
  1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
  `show' functions and reduce code duplication.
  
  2) Account and report back to user numbers of failed READ and WRITE
  operations.
  
  3) Remove `good' and `bad' compressed sub-requests stats. RW request may
  cause a number of RW sub-requests. zram used to account `good' compressed
  sub-queries (with compressed size less than 50% of original size), `bad'
  compressed sub-queries (with compressed size greater that 75% of original
  size), leaving sub-requests with compression size between 50% and 75% of
  original size not accounted and not reported.
 
 That's weird: good/bad_compress are accounted, but it seems to me that
 they are to never used in any way. If so, there is indeed no reason to
 keep them.
 

  Account each sub-request
  compression size so we can calculate real device compression ratio.
 
 Your patch doesn't change the way pages_stored and compr[essed]_size
 are accounted. What does your patch change that allow us to calculate
 the real compression ratio?

we have compressed size, number of stored pages and reported by zs pool
(as a zram memory_used attr) number of bytes used

u64 zs_get_total_size_bytes(struct zs_pool *pool)
{
int i;
u64 npages = 0;

for (i = 0; i  ZS_SIZE_CLASSES; i++)
npages += pool-size_class[i].pages_allocated;

return npages  PAGE_SHIFT;
}

looks enough to calculate device overall data compression ratio.

-ss

  
  4) reported zram stats:
- num_writes  -- number of writes
- num_reads  -- number of reads
- pages_stored -- number of pages currently stored
- compressed_size -- compressed size of pages stored
 
 Wouldn't it be more practical to report the original and compressed
 data sizes using the same units as it is currently done?
 

sorry, not sure I understand.

 Jerome
 
- pages_zero  -- number of zero filled pages
- failed_read -- number of failed reads
- failed_writes -- can happen when memory is too low
- invalid_io   -- non-page-aligned I/O requests
- notify_free  -- number of swap slot free notifications
- memory_used -- zs pool zs_get_total_size_bytes()
  
  Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
  ---
   drivers/block/zram/zram_drv.c | 167 
  --
   drivers/block/zram/zram_drv.h |  17 ++---
   2 files changed, 54 insertions(+), 130 deletions(-)
  
  diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
  index 2a7682c..8bddaff 100644
  --- a/drivers/block/zram/zram_drv.c
  +++ b/drivers/block/zram/zram_drv.c
  @@ -42,6 +42,17 @@ static struct zram *zram_devices;
   /* Module params (documentation at end) */
   static unsigned int num_devices = 1;
   
  +#define ZRAM_ATTR_RO(name) \
  +static ssize_t zram_attr_##name##_show(struct device *d,   \
  +   struct device_attribute *attr, char *b) \
  +{  \
  +   struct zram *zram = dev_to_zram(d); \
  +   return sprintf(b, %llu\n, \
  +   (u64)atomic64_read(zram-stats.name)); \
  +}  \
  +static struct device_attribute dev_attr_##name =   \
  +   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
  +
   static inline int init_done(struct zram *zram)
   {
  return zram-meta != NULL;
  @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device 
  *dev)
  return (struct zram *)dev_to_disk(dev)-private_data;
   }
   
  -static ssize_t disksize_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n, zram-disksize);
  -}
  -
  -static ssize_t initstate_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %u\n, init_done(zram));
  -}
  -
  -static ssize_t num_reads_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.num_reads));
  -}
  -
  -static ssize_t num_writes_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.num_writes));
  -}
  -
  -static ssize_t invalid_io_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 11:38), Jerome Marchand wrote:
 On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
  1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
  `show' functions and reduce code duplication.
  
  2) Account and report back to user numbers of failed READ and WRITE
  operations.
  
  3) Remove `good' and `bad' compressed sub-requests stats. RW request may
  cause a number of RW sub-requests. zram used to account `good' compressed
  sub-queries (with compressed size less than 50% of original size), `bad'
  compressed sub-queries (with compressed size greater that 75% of original
  size), leaving sub-requests with compression size between 50% and 75% of
  original size not accounted and not reported.
 
 That's weird: good/bad_compress are accounted, but it seems to me that
 they are to never used in any way. If so, there is indeed no reason to
 keep them.
 

  Account each sub-request
  compression size so we can calculate real device compression ratio.
 
 Your patch doesn't change the way pages_stored and compr[essed]_size
 are accounted. What does your patch change that allow us to calculate
 the real compression ratio?
 
  
  4) reported zram stats:
- num_writes  -- number of writes
- num_reads  -- number of reads
- pages_stored -- number of pages currently stored
- compressed_size -- compressed size of pages stored
 
 Wouldn't it be more practical to report the original and compressed
 data sizes using the same units as it is currently done?
 

hm, do we really need pages_stored stats? what kind of unseful information it
shows to end user?.. perhaps, it's better to replace it with accounted passed
bvec-bv_len (as uncompressed_size).

-ss

 Jerome
 
- pages_zero  -- number of zero filled pages
- failed_read -- number of failed reads
- failed_writes -- can happen when memory is too low
- invalid_io   -- non-page-aligned I/O requests
- notify_free  -- number of swap slot free notifications
- memory_used -- zs pool zs_get_total_size_bytes()
  
  Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
  ---
   drivers/block/zram/zram_drv.c | 167 
  --
   drivers/block/zram/zram_drv.h |  17 ++---
   2 files changed, 54 insertions(+), 130 deletions(-)
  
  diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
  index 2a7682c..8bddaff 100644
  --- a/drivers/block/zram/zram_drv.c
  +++ b/drivers/block/zram/zram_drv.c
  @@ -42,6 +42,17 @@ static struct zram *zram_devices;
   /* Module params (documentation at end) */
   static unsigned int num_devices = 1;
   
  +#define ZRAM_ATTR_RO(name) \
  +static ssize_t zram_attr_##name##_show(struct device *d,   \
  +   struct device_attribute *attr, char *b) \
  +{  \
  +   struct zram *zram = dev_to_zram(d); \
  +   return sprintf(b, %llu\n, \
  +   (u64)atomic64_read(zram-stats.name)); \
  +}  \
  +static struct device_attribute dev_attr_##name =   \
  +   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
  +
   static inline int init_done(struct zram *zram)
   {
  return zram-meta != NULL;
  @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device 
  *dev)
  return (struct zram *)dev_to_disk(dev)-private_data;
   }
   
  -static ssize_t disksize_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n, zram-disksize);
  -}
  -
  -static ssize_t initstate_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %u\n, init_done(zram));
  -}
  -
  -static ssize_t num_reads_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.num_reads));
  -}
  -
  -static ssize_t num_writes_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.num_writes));
  -}
  -
  -static ssize_t invalid_io_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  -   return sprintf(buf, %llu\n,
  -   (u64)atomic64_read(zram-stats.invalid_io));
  -}
  -
  -static ssize_t notify_free_show(struct device *dev,
  -   struct device_attribute *attr, char *buf)
  -{
  -   struct zram *zram = dev_to_zram(dev);
  -
  

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 11:57 AM, Sergey Senozhatsky wrote:
 
 Hello Jerome,
 
 On (01/14/14 11:38), Jerome Marchand wrote:
 On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
 `show' functions and reduce code duplication.

 2) Account and report back to user numbers of failed READ and WRITE
 operations.

 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
 cause a number of RW sub-requests. zram used to account `good' compressed
 sub-queries (with compressed size less than 50% of original size), `bad'
 compressed sub-queries (with compressed size greater that 75% of original
 size), leaving sub-requests with compression size between 50% and 75% of
 original size not accounted and not reported.

 That's weird: good/bad_compress are accounted, but it seems to me that
 they are to never used in any way. If so, there is indeed no reason to
 keep them.


 Account each sub-request
 compression size so we can calculate real device compression ratio.

 Your patch doesn't change the way pages_stored and compr[essed]_size
 are accounted. What does your patch change that allow us to calculate
 the real compression ratio?
 
 we have compressed size, number of stored pages and reported by zs pool
 (as a zram memory_used attr) number of bytes used
 
 u64 zs_get_total_size_bytes(struct zs_pool *pool)
 {
 int i;
 u64 npages = 0;
 
 for (i = 0; i  ZS_SIZE_CLASSES; i++)
 npages += pool-size_class[i].pages_allocated;
 
 return npages  PAGE_SHIFT;
 }
 
 looks enough to calculate device overall data compression ratio.

Yes. But don't we have all that already without your patch applied?
What does this patch change?

 
   -ss
 

 4) reported zram stats:
   - num_writes  -- number of writes
   - num_reads  -- number of reads
   - pages_stored -- number of pages currently stored
   - compressed_size -- compressed size of pages stored

 Wouldn't it be more practical to report the original and compressed
 data sizes using the same units as it is currently done?

 
 sorry, not sure I understand.

Currently users have access to orig_data_size and compr_data_size,
both in bytes. With your patch, they have access to pages_stored in
pages and compressed_size in bytes. I find the current set more
practical.

Jerome

 
 Jerome

   - pages_zero  -- number of zero filled pages
   - failed_read -- number of failed reads
   - failed_writes -- can happen when memory is too low
   - invalid_io   -- non-page-aligned I/O requests
   - notify_free  -- number of swap slot free notifications
   - memory_used -- zs pool zs_get_total_size_bytes()

 Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
 ---
  drivers/block/zram/zram_drv.c | 167 
 --
  drivers/block/zram/zram_drv.h |  17 ++---
  2 files changed, 54 insertions(+), 130 deletions(-)

 diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
 index 2a7682c..8bddaff 100644
 --- a/drivers/block/zram/zram_drv.c
 +++ b/drivers/block/zram/zram_drv.c
 @@ -42,6 +42,17 @@ static struct zram *zram_devices;
  /* Module params (documentation at end) */
  static unsigned int num_devices = 1;
  
 +#define ZRAM_ATTR_RO(name) \
 +static ssize_t zram_attr_##name##_show(struct device *d,   \
 +   struct device_attribute *attr, char *b) \
 +{  \
 +   struct zram *zram = dev_to_zram(d); \
 +   return sprintf(b, %llu\n, \
 +   (u64)atomic64_read(zram-stats.name)); \
 +}  \
 +static struct device_attribute dev_attr_##name =   \
 +   __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
 +
  static inline int init_done(struct zram *zram)
  {
 return zram-meta != NULL;
 @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device 
 *dev)
 return (struct zram *)dev_to_disk(dev)-private_data;
  }
  
 -static ssize_t disksize_show(struct device *dev,
 -   struct device_attribute *attr, char *buf)
 -{
 -   struct zram *zram = dev_to_zram(dev);
 -
 -   return sprintf(buf, %llu\n, zram-disksize);
 -}
 -
 -static ssize_t initstate_show(struct device *dev,
 -   struct device_attribute *attr, char *buf)
 -{
 -   struct zram *zram = dev_to_zram(dev);
 -
 -   return sprintf(buf, %u\n, init_done(zram));
 -}
 -
 -static ssize_t num_reads_show(struct device *dev,
 -   struct device_attribute *attr, char *buf)
 -{
 -   struct zram *zram = dev_to_zram(dev);
 -
 -   return sprintf(buf, %llu\n,
 -   (u64)atomic64_read(zram-stats.num_reads));
 -}
 -
 -static ssize_t num_writes_show(struct device *dev,
 -   struct device_attribute *attr, char *buf)
 -{
 -   struct zram 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 13:15), Jerome Marchand wrote:
 On 01/14/2014 11:57 AM, Sergey Senozhatsky wrote:
  
  Hello Jerome,
  
  On (01/14/14 11:38), Jerome Marchand wrote:
  On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
  1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
  `show' functions and reduce code duplication.
 
  2) Account and report back to user numbers of failed READ and WRITE
  operations.
 
  3) Remove `good' and `bad' compressed sub-requests stats. RW request may
  cause a number of RW sub-requests. zram used to account `good' compressed
  sub-queries (with compressed size less than 50% of original size), `bad'
  compressed sub-queries (with compressed size greater that 75% of original
  size), leaving sub-requests with compression size between 50% and 75% of
  original size not accounted and not reported.
 
  That's weird: good/bad_compress are accounted, but it seems to me that
  they are to never used in any way. If so, there is indeed no reason to
  keep them.
 
 
  Account each sub-request
  compression size so we can calculate real device compression ratio.
 
  Your patch doesn't change the way pages_stored and compr[essed]_size
  are accounted. What does your patch change that allow us to calculate
  the real compression ratio?
  
  we have compressed size, number of stored pages and reported by zs pool
  (as a zram memory_used attr) number of bytes used
  
  u64 zs_get_total_size_bytes(struct zs_pool *pool)
  {
  int i;
  u64 npages = 0;
  
  for (i = 0; i  ZS_SIZE_CLASSES; i++)
  npages += pool-size_class[i].pages_allocated;
  
  return npages  PAGE_SHIFT;
  }
  
  looks enough to calculate device overall data compression ratio.
 
 Yes. But don't we have all that already without your patch applied?
 What does this patch change?
 


oh. yes, bad wording. the commit message must be *zram accounts* each
sub-request compression size so we can calculate real device compression
ratio. instead of  Account each sub-request compression size so we can
calculate real device compression ratio.. otherwise, there is a false
feeling that patch change/introduce this functionality. will re-write
that commit message. sorry.

the patch does not change a lot of things and may be considered mainly as
a clean up patch. it:
-- removes unused and misleading bad/good stats
-- makes some attrs names more readable e.g. mem_used_total to
memory_used, compr_data_size to compressed_size
-- accounts and reports numbers of failed RW requests
-- removes ATTR_show() code duplication using ZRAM_ATTR_RO macro

-ss

  
  -ss
  
 
  4) reported zram stats:
- num_writes  -- number of writes
- num_reads  -- number of reads
- pages_stored -- number of pages currently stored
- compressed_size -- compressed size of pages stored
 
  Wouldn't it be more practical to report the original and compressed
  data sizes using the same units as it is currently done?
 
  
  sorry, not sure I understand.
 
 Currently users have access to orig_data_size and compr_data_size,
 both in bytes. With your patch, they have access to pages_stored in
 pages and compressed_size in bytes. I find the current set more
 practical.
 
 Jerome
 
  
  Jerome
 
- pages_zero  -- number of zero filled pages
- failed_read -- number of failed reads
- failed_writes -- can happen when memory is too low
- invalid_io   -- non-page-aligned I/O requests
- notify_free  -- number of swap slot free notifications
- memory_used -- zs pool zs_get_total_size_bytes()
 
  Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
  ---
   drivers/block/zram/zram_drv.c | 167 
  --
   drivers/block/zram/zram_drv.h |  17 ++---
   2 files changed, 54 insertions(+), 130 deletions(-)
 
  diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
  index 2a7682c..8bddaff 100644
  --- a/drivers/block/zram/zram_drv.c
  +++ b/drivers/block/zram/zram_drv.c
  @@ -42,6 +42,17 @@ static struct zram *zram_devices;
   /* Module params (documentation at end) */
   static unsigned int num_devices = 1;
   
  +#define ZRAM_ATTR_RO(name)   
  \
  +static ssize_t zram_attr_##name##_show(struct device *d, \
  + struct device_attribute *attr, char *b) \
  +{
  \
  + struct zram *zram = dev_to_zram(d); \
  + return sprintf(b, %llu\n, \
  + (u64)atomic64_read(zram-stats.name)); \
  +}
  \
  +static struct device_attribute dev_attr_##name = \
  + __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
  +
   static inline int init_done(struct zram *zram)
   {
return zram-meta != NULL;
  @@ -52,97 +63,36 @@ static 

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 12:10 PM, Sergey Senozhatsky wrote:
 On (01/14/14 11:38), Jerome Marchand wrote:
 On 01/14/2014 10:37 AM, Sergey Senozhatsky wrote:
 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
 `show' functions and reduce code duplication.

 2) Account and report back to user numbers of failed READ and WRITE
 operations.

 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
 cause a number of RW sub-requests. zram used to account `good' compressed
 sub-queries (with compressed size less than 50% of original size), `bad'
 compressed sub-queries (with compressed size greater that 75% of original
 size), leaving sub-requests with compression size between 50% and 75% of
 original size not accounted and not reported.

 That's weird: good/bad_compress are accounted, but it seems to me that
 they are to never used in any way. If so, there is indeed no reason to
 keep them.


 Account each sub-request
 compression size so we can calculate real device compression ratio.

 Your patch doesn't change the way pages_stored and compr[essed]_size
 are accounted. What does your patch change that allow us to calculate
 the real compression ratio?


 4) reported zram stats:
   - num_writes  -- number of writes
   - num_reads  -- number of reads
   - pages_stored -- number of pages currently stored
   - compressed_size -- compressed size of pages stored

 Wouldn't it be more practical to report the original and compressed
 data sizes using the same units as it is currently done?

 
 hm, do we really need pages_stored stats? what kind of unseful information it
 shows to end user?.. perhaps, it's better to replace it with accounted passed
 bvec-bv_len (as uncompressed_size).
 

That's really going to complicates things. We would need to keep track
of which sectors of a particular page has been written to. It's much
easier to keep current page granularity and consider any partial I/O
as an whole page I/O.

   -ss
 
 Jerome

   - pages_zero  -- number of zero filled pages
   - failed_read -- number of failed reads
   - failed_writes -- can happen when memory is too low
   - invalid_io   -- non-page-aligned I/O requests
   - notify_free  -- number of swap slot free notifications
   - memory_used -- zs pool zs_get_total_size_bytes()

 Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 14:43), Jerome Marchand wrote:
[..]
 
  That's weird: good/bad_compress are accounted, but it seems to me that
  they are to never used in any way. If so, there is indeed no reason to
  keep them.
 
 
  Account each sub-request
  compression size so we can calculate real device compression ratio.
 
  Your patch doesn't change the way pages_stored and compr[essed]_size
  are accounted. What does your patch change that allow us to calculate
  the real compression ratio?
 
 
  4) reported zram stats:
- num_writes  -- number of writes
- num_reads  -- number of reads
- pages_stored -- number of pages currently stored
- compressed_size -- compressed size of pages stored
 
  Wouldn't it be more practical to report the original and compressed
  data sizes using the same units as it is currently done?
 
  
  hm, do we really need pages_stored stats? what kind of unseful information 
  it
  shows to end user?.. perhaps, it's better to replace it with accounted 
  passed
  bvec-bv_len (as uncompressed_size).
  
 
 That's really going to complicates things. We would need to keep track
 of which sectors of a particular page has been written to. It's much
 easier to keep current page granularity and consider any partial I/O
 as an whole page I/O.
 

fair enough. thank you.

2/3 and 3/3 were changed according to your comments:
- 2/3 drop READA check
- 3/3 update commit message.

ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?

-ss

  
  Jerome
 
- pages_zero  -- number of zero filled pages
- failed_read -- number of failed reads
- failed_writes -- can happen when memory is too low
- invalid_io   -- non-page-aligned I/O requests
- notify_free  -- number of swap slot free notifications
- memory_used -- zs pool zs_get_total_size_bytes()
 
  Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
 On (01/14/14 14:43), Jerome Marchand wrote:
 [..]

 That's weird: good/bad_compress are accounted, but it seems to me that
 they are to never used in any way. If so, there is indeed no reason to
 keep them.


 Account each sub-request
 compression size so we can calculate real device compression ratio.

 Your patch doesn't change the way pages_stored and compr[essed]_size
 are accounted. What does your patch change that allow us to calculate
 the real compression ratio?


 4) reported zram stats:
   - num_writes  -- number of writes
   - num_reads  -- number of reads
   - pages_stored -- number of pages currently stored
   - compressed_size -- compressed size of pages stored

 Wouldn't it be more practical to report the original and compressed
 data sizes using the same units as it is currently done?


 hm, do we really need pages_stored stats? what kind of unseful information 
 it
 shows to end user?.. perhaps, it's better to replace it with accounted 
 passed
 bvec-bv_len (as uncompressed_size).


 That's really going to complicates things. We would need to keep track
 of which sectors of a particular page has been written to. It's much
 easier to keep current page granularity and consider any partial I/O
 as an whole page I/O.

 
 fair enough. thank you.
 
 2/3 and 3/3 were changed according to your comments:
 - 2/3 drop READA check
 - 3/3 update commit message.
 
 ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?

The READA thing was my only concern for 2/3, so yes for this one.
Concerning the third patch, I'd like to see what other people think
about which stats we want to report.

 
   -ss
 

 Jerome

   - pages_zero  -- number of zero filled pages
   - failed_read -- number of failed reads
   - failed_writes -- can happen when memory is too low
   - invalid_io   -- non-page-aligned I/O requests
   - notify_free  -- number of swap slot free notifications
   - memory_used -- zs pool zs_get_total_size_bytes()

 Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Sergey Senozhatsky]

On (01/14/14 15:02), Jerome Marchand wrote:
 On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
  On (01/14/14 14:43), Jerome Marchand wrote:
  [..]
 
  That's weird: good/bad_compress are accounted, but it seems to me that
  they are to never used in any way. If so, there is indeed no reason to
  keep them.
 
 
  Account each sub-request
  compression size so we can calculate real device compression ratio.
 
  Your patch doesn't change the way pages_stored and compr[essed]_size
  are accounted. What does your patch change that allow us to calculate
  the real compression ratio?
 
 
  4) reported zram stats:
- num_writes  -- number of writes
- num_reads  -- number of reads
- pages_stored -- number of pages currently stored
- compressed_size -- compressed size of pages stored
 
  Wouldn't it be more practical to report the original and compressed
  data sizes using the same units as it is currently done?
 
 
  hm, do we really need pages_stored stats? what kind of unseful 
  information it
  shows to end user?.. perhaps, it's better to replace it with accounted 
  passed
  bvec-bv_len (as uncompressed_size).
 
 
  That's really going to complicates things. We would need to keep track
  of which sectors of a particular page has been written to. It's much
  easier to keep current page granularity and consider any partial I/O
  as an whole page I/O.
 
  
  fair enough. thank you.
  
  2/3 and 3/3 were changed according to your comments:
  - 2/3 drop READA check
  - 3/3 update commit message.
  
  ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?
 
 The READA thing was my only concern for 2/3, so yes for this one.
 Concerning the third patch, I'd like to see what other people think
 about which stats we want to report.
 

good. so I hold on for a bit to minimize the traffic and see what other
people think. thank you.

-ss

 
  Jerome
 
- pages_zero  -- number of zero filled pages
- failed_read -- number of failed reads
- failed_writes -- can happen when memory is too low
- invalid_io   -- non-page-aligned I/O requests
- notify_free  -- number of swap slot free notifications
- memory_used -- zs pool zs_get_total_size_bytes()
 
  Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com
 
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Jerome Marchand]

On 01/14/2014 03:09 PM, Sergey Senozhatsky wrote:
 On (01/14/14 15:02), Jerome Marchand wrote:
 On 01/14/2014 02:53 PM, Sergey Senozhatsky wrote:
 On (01/14/14 14:43), Jerome Marchand wrote:
 [..]

 That's weird: good/bad_compress are accounted, but it seems to me that
 they are to never used in any way. If so, there is indeed no reason to
 keep them.


 Account each sub-request
 compression size so we can calculate real device compression ratio.

 Your patch doesn't change the way pages_stored and compr[essed]_size
 are accounted. What does your patch change that allow us to calculate
 the real compression ratio?


 4) reported zram stats:
   - num_writes  -- number of writes
   - num_reads  -- number of reads
   - pages_stored -- number of pages currently stored
   - compressed_size -- compressed size of pages stored

 Wouldn't it be more practical to report the original and compressed
 data sizes using the same units as it is currently done?


 hm, do we really need pages_stored stats? what kind of unseful 
 information it
 shows to end user?.. perhaps, it's better to replace it with accounted 
 passed
 bvec-bv_len (as uncompressed_size).


 That's really going to complicates things. We would need to keep track
 of which sectors of a particular page has been written to. It's much
 easier to keep current page granularity and consider any partial I/O
 as an whole page I/O.


 fair enough. thank you.

 2/3 and 3/3 were changed according to your comments:
 - 2/3 drop READA check
 - 3/3 update commit message.

 ready to re-publish. may I add your ACK to all 3 patches or just to 1/3?

 The READA thing was my only concern for 2/3, so yes for this one.
 Concerning the third patch, I'd like to see what other people think
 about which stats we want to report.

 
 good. so I hold on for a bit to minimize the traffic and see what other
 people think. thank you.
 
   -ss
 

 Jerome

   - pages_zero  -- number of zero filled pages
   - failed_read -- number of failed reads
   - failed_writes -- can happen when memory is too low
   - invalid_io   -- non-page-aligned I/O requests
   - notify_free  -- number of swap slot free notifications
   - memory_used -- zs pool zs_get_total_size_bytes()

I also wonder if we should add size of meta-data to memory_used,
especially the table size which is probably not always negligible.

Jerome


 Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com




--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/3] zram: rework reported to end-user zram statistics

[Minchan Kim]

Hello Sergey,

On Tue, Jan 14, 2014 at 12:37:40PM +0300, Sergey Senozhatsky wrote:
 1) Introduce ZRAM_ATTR_RO macro to generate zram atomic64_t stats
 `show' functions and reduce code duplication.
 
 2) Account and report back to user numbers of failed READ and WRITE
 operations.
 
 3) Remove `good' and `bad' compressed sub-requests stats. RW request may
 cause a number of RW sub-requests. zram used to account `good' compressed
 sub-queries (with compressed size less than 50% of original size), `bad'
 compressed sub-queries (with compressed size greater that 75% of original
 size), leaving sub-requests with compression size between 50% and 75% of
 original size not accounted and not reported. Account each sub-request
 compression size so we can calculate real device compression ratio.
 
 4) reported zram stats:
   - num_writes  -- number of writes
   - num_reads  -- number of reads
   - pages_stored -- number of pages currently stored
   - compressed_size -- compressed size of pages stored
   - pages_zero  -- number of zero filled pages
   - failed_read -- number of failed reads
   - failed_writes -- can happen when memory is too low
   - invalid_io   -- non-page-aligned I/O requests
   - notify_free  -- number of swap slot free notifications
   - memory_used -- zs pool zs_get_total_size_bytes()
 
 Signed-off-by: Sergey Senozhatsky sergey.senozhat...@gmail.com

So this patch includes some clean up and behavior changes?
Please seaprate them and each patch in behavior change includes
why it's bad or good(ie, motivation).

It could make reviewer/maintainer happy, even you because
some of them could be picked up and other things could be dropped.
Sorry for the bothering you.

Thanks.

 ---
  drivers/block/zram/zram_drv.c | 167 
 --
  drivers/block/zram/zram_drv.h |  17 ++---
  2 files changed, 54 insertions(+), 130 deletions(-)
 
 diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
 index 2a7682c..8bddaff 100644
 --- a/drivers/block/zram/zram_drv.c
 +++ b/drivers/block/zram/zram_drv.c
 @@ -42,6 +42,17 @@ static struct zram *zram_devices;
  /* Module params (documentation at end) */
  static unsigned int num_devices = 1;
  
 +#define ZRAM_ATTR_RO(name)   \
 +static ssize_t zram_attr_##name##_show(struct device *d, \
 + struct device_attribute *attr, char *b) \
 +{\
 + struct zram *zram = dev_to_zram(d); \
 + return sprintf(b, %llu\n, \
 + (u64)atomic64_read(zram-stats.name)); \
 +}\
 +static struct device_attribute dev_attr_##name = \
 + __ATTR(name, S_IRUGO, zram_attr_##name##_show, NULL);
 +
  static inline int init_done(struct zram *zram)
  {
   return zram-meta != NULL;
 @@ -52,97 +63,36 @@ static inline struct zram *dev_to_zram(struct device *dev)
   return (struct zram *)dev_to_disk(dev)-private_data;
  }
  
 -static ssize_t disksize_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n, zram-disksize);
 -}
 -
 -static ssize_t initstate_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %u\n, init_done(zram));
 -}
 -
 -static ssize_t num_reads_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n,
 - (u64)atomic64_read(zram-stats.num_reads));
 -}
 -
 -static ssize_t num_writes_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n,
 - (u64)atomic64_read(zram-stats.num_writes));
 -}
 -
 -static ssize_t invalid_io_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n,
 - (u64)atomic64_read(zram-stats.invalid_io));
 -}
 -
 -static ssize_t notify_free_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %llu\n,
 - (u64)atomic64_read(zram-stats.notify_free));
 -}
 -
 -static ssize_t zero_pages_show(struct device *dev,
 - struct device_attribute *attr, char *buf)
 -{
 - struct zram *zram = dev_to_zram(dev);
 -
 - return sprintf(buf, %u\n, atomic_read(zram-stats.pages_zero));
 -}
 -
 -static ssize_t orig_data_size_show(struct device *dev,
 +static