[Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Chao Fan]

In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
It's also better for other tools to determine the cpu throttle
value in different architecture.

Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
---
 hmp.c                 | 6 +++---
 migration/migration.c | 2 +-
 qapi-schema.json      | 8 ++++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/hmp.c b/hmp.c
index 261843f..7334bc5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -215,9 +215,9 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->ram->normal_bytes >> 10);
         monitor_printf(mon, "dirty sync count: %" PRIu64 "\n",
                        info->ram->dirty_sync_count);
-        if (info->ram->dirty_pages_rate) {
-            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
-                           info->ram->dirty_pages_rate);
+        if (info->ram->dirty_bytes_rate) {
+            monitor_printf(mon, "dirty bytes rate: %" PRIu64 " bytes/s\n",
+                           info->ram->dirty_bytes_rate);
         }
         if (info->ram->postcopy_requests) {
             monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
diff --git a/migration/migration.c b/migration/migration.c
index 3dab684..7e7b62c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -648,7 +648,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
 
     if (s->state != MIGRATION_STATUS_COMPLETED) {
         info->ram->remaining = ram_bytes_remaining();
-        info->ram->dirty_pages_rate = s->dirty_pages_rate;
+        info->ram->dirty_bytes_rate = s->dirty_bytes_rate;
     }
 }
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 32b4a4b..83a45f8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -565,9 +565,6 @@
 #
 # @normal-bytes: number of normal bytes sent (since 1.2)
 #
-# @dirty-pages-rate: number of pages dirtied by second by the
-#        guest (since 1.3)
-#
 # @mbps: throughput in megabits/sec. (since 1.6)
 #
 # @dirty-sync-count: number of times that dirty ram was synchronized (since 2.1)
@@ -575,12 +572,15 @@
 # @postcopy-requests: The number of page requests received from the destination
 #        (since 2.7)
 #
+# @dirty-bytes-rate: how many bytes  dirtied by second by the
+#        guest (since 2.9)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
            'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
-           'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
+           'normal-bytes': 'int', 'dirty-bytes-rate' : 'int',
            'mbps' : 'number', 'dirty-sync-count' : 'int',
            'postcopy-requests' : 'int' } }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Daniel P. Berrange]

On Tue, Mar 14, 2017 at 06:27:47PM +0800, Chao Fan wrote:
> In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
> It's also better for other tools to determine the cpu throttle
> value in different architecture.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---
>  hmp.c                 | 6 +++---
>  migration/migration.c | 2 +-
>  qapi-schema.json      | 8 ++++----
>  3 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 261843f..7334bc5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -215,9 +215,9 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->ram->normal_bytes >> 10);
>          monitor_printf(mon, "dirty sync count: %" PRIu64 "\n",
>                         info->ram->dirty_sync_count);
> -        if (info->ram->dirty_pages_rate) {
> -            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
> -                           info->ram->dirty_pages_rate);
> +        if (info->ram->dirty_bytes_rate) {
> +            monitor_printf(mon, "dirty bytes rate: %" PRIu64 " bytes/s\n",
> +                           info->ram->dirty_bytes_rate);
>          }
>          if (info->ram->postcopy_requests) {
>              monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
> diff --git a/migration/migration.c b/migration/migration.c
> index 3dab684..7e7b62c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -648,7 +648,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>  
>      if (s->state != MIGRATION_STATUS_COMPLETED) {
>          info->ram->remaining = ram_bytes_remaining();
> -        info->ram->dirty_pages_rate = s->dirty_pages_rate;
> +        info->ram->dirty_bytes_rate = s->dirty_bytes_rate;
>      }
>  }
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 32b4a4b..83a45f8 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -565,9 +565,6 @@
>  #
>  # @normal-bytes: number of normal bytes sent (since 1.2)
>  #
> -# @dirty-pages-rate: number of pages dirtied by second by the
> -#        guest (since 1.3)
> -#
>  # @mbps: throughput in megabits/sec. (since 1.6)
>  #
>  # @dirty-sync-count: number of times that dirty ram was synchronized (since 2.1)
> @@ -575,12 +572,15 @@
>  # @postcopy-requests: The number of page requests received from the destination
>  #        (since 2.7)
>  #
> +# @dirty-bytes-rate: how many bytes  dirtied by second by the
> +#        guest (since 2.9)
> +#
>  # Since: 0.14.0
>  ##
>  { 'struct': 'MigrationStats',
>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>             'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
> -           'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
> +           'normal-bytes': 'int', 'dirty-bytes-rate' : 'int',
>             'mbps' : 'number', 'dirty-sync-count' : 'int',
>             'postcopy-requests' : 'int' } }

You can't do this - this breaks every single app that uses dirty-pages-rate.

The only option is to add dirty-bytes-rate as a second field, and leave
the existing field unchanged. Alternatively, output 'page-size' and let
the application do the math.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Chao Fan]

On Tue, Mar 14, 2017 at 10:45:16AM +0000, Daniel P. Berrange wrote:
>On Tue, Mar 14, 2017 at 06:27:47PM +0800, Chao Fan wrote:
>> In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
>> It's also better for other tools to determine the cpu throttle
>> value in different architecture.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> ---
>>  hmp.c                 | 6 +++---
>>  migration/migration.c | 2 +-
>>  qapi-schema.json      | 8 ++++----
>>  3 files changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hmp.c b/hmp.c
>> index 261843f..7334bc5 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -215,9 +215,9 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>>                         info->ram->normal_bytes >> 10);
>>          monitor_printf(mon, "dirty sync count: %" PRIu64 "\n",
>>                         info->ram->dirty_sync_count);
>> -        if (info->ram->dirty_pages_rate) {
>> -            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
>> -                           info->ram->dirty_pages_rate);
>> +        if (info->ram->dirty_bytes_rate) {
>> +            monitor_printf(mon, "dirty bytes rate: %" PRIu64 " bytes/s\n",
>> +                           info->ram->dirty_bytes_rate);
>>          }
>>          if (info->ram->postcopy_requests) {
>>              monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 3dab684..7e7b62c 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -648,7 +648,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>>  
>>      if (s->state != MIGRATION_STATUS_COMPLETED) {
>>          info->ram->remaining = ram_bytes_remaining();
>> -        info->ram->dirty_pages_rate = s->dirty_pages_rate;
>> +        info->ram->dirty_bytes_rate = s->dirty_bytes_rate;
>>      }
>>  }
>>  
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 32b4a4b..83a45f8 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -565,9 +565,6 @@
>>  #
>>  # @normal-bytes: number of normal bytes sent (since 1.2)
>>  #
>> -# @dirty-pages-rate: number of pages dirtied by second by the
>> -#        guest (since 1.3)
>> -#
>>  # @mbps: throughput in megabits/sec. (since 1.6)
>>  #
>>  # @dirty-sync-count: number of times that dirty ram was synchronized (since 2.1)
>> @@ -575,12 +572,15 @@
>>  # @postcopy-requests: The number of page requests received from the destination
>>  #        (since 2.7)
>>  #
>> +# @dirty-bytes-rate: how many bytes  dirtied by second by the
>> +#        guest (since 2.9)
>> +#
>>  # Since: 0.14.0
>>  ##
>>  { 'struct': 'MigrationStats',
>>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>>             'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
>> -           'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
>> +           'normal-bytes': 'int', 'dirty-bytes-rate' : 'int',
>>             'mbps' : 'number', 'dirty-sync-count' : 'int',
>>             'postcopy-requests' : 'int' } }
>
>You can't do this - this breaks every single app that uses dirty-pages-rate.
>
>The only option is to add dirty-bytes-rate as a second field, and leave
>the existing field unchanged. Alternatively, output 'page-size' and let
>the application do the math.
Hi Daniel.

Thank you for you reply.
I think adding dirty-bytes-rate is better. I will change and resend the
new patch.

Thanks,
Chao Fan
>
>Regards,
>Daniel
>-- 
>|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org              -o-             http://virt-manager.org :|
>|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
>
>

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Juan Quintela]

Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
> It's also better for other tools to determine the cpu throttle
> value in different architecture.
>
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>

I agree with Daniel here, you can't change the meaning of a field.  Look
at skipped pages.  It is zero know because it is not used anymore, but
we can't drop it.

I think it is better to expose page_size.  We have now

trasferred: bytes
total: bytes
duplicate: number of zero pages
skipped: always zero.
normal: number of normal pages
normal_bytes: the same  in bytes
mbps: megabytes per second?  I can't even remember this one
dirty_sync_count: number of times we have go through the whole memory
postcopy_requests = number of pages asked by postcopy faults?
dirty_pages_rate = pages by some kind of unit

And we haven't yet started with compression or xbzrle.  I think that the
best approach at this point is putting everything in pages except the
things that don't make sense.

We can put everything on bytes, but then everything is HUGE.

Anyways, what do libvirt/management apps preffer?

Later, Juan.


> ---
>  hmp.c                 | 6 +++---
>  migration/migration.c | 2 +-
>  qapi-schema.json      | 8 ++++----
>  3 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/hmp.c b/hmp.c
> index 261843f..7334bc5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -215,9 +215,9 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>                         info->ram->normal_bytes >> 10);
>          monitor_printf(mon, "dirty sync count: %" PRIu64 "\n",
>                         info->ram->dirty_sync_count);
> -        if (info->ram->dirty_pages_rate) {
> -            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
> -                           info->ram->dirty_pages_rate);
> +        if (info->ram->dirty_bytes_rate) {
> +            monitor_printf(mon, "dirty bytes rate: %" PRIu64 " bytes/s\n",
> +                           info->ram->dirty_bytes_rate);
>          }
>          if (info->ram->postcopy_requests) {
>              monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
> diff --git a/migration/migration.c b/migration/migration.c
> index 3dab684..7e7b62c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -648,7 +648,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>  
>      if (s->state != MIGRATION_STATUS_COMPLETED) {
>          info->ram->remaining = ram_bytes_remaining();
> -        info->ram->dirty_pages_rate = s->dirty_pages_rate;
> +        info->ram->dirty_bytes_rate = s->dirty_bytes_rate;
>      }
>  }
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 32b4a4b..83a45f8 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -565,9 +565,6 @@
>  #
>  # @normal-bytes: number of normal bytes sent (since 1.2)
>  #
> -# @dirty-pages-rate: number of pages dirtied by second by the
> -#        guest (since 1.3)
> -#
>  # @mbps: throughput in megabits/sec. (since 1.6)
>  #
>  # @dirty-sync-count: number of times that dirty ram was synchronized (since 2.1)
> @@ -575,12 +572,15 @@
>  # @postcopy-requests: The number of page requests received from the destination
>  #        (since 2.7)
>  #
> +# @dirty-bytes-rate: how many bytes  dirtied by second by the
> +#        guest (since 2.9)
> +#
>  # Since: 0.14.0
>  ##
>  { 'struct': 'MigrationStats',
>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>             'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
> -           'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
> +           'normal-bytes': 'int', 'dirty-bytes-rate' : 'int',
>             'mbps' : 'number', 'dirty-sync-count' : 'int',
>             'postcopy-requests' : 'int' } }

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Daniel P. Berrange]

On Tue, Mar 14, 2017 at 01:29:43PM +0100, Juan Quintela wrote:
> Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> > In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
> > It's also better for other tools to determine the cpu throttle
> > value in different architecture.
> >
> > Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> 
> I agree with Daniel here, you can't change the meaning of a field.  Look
> at skipped pages.  It is zero know because it is not used anymore, but
> we can't drop it.
> 
> I think it is better to expose page_size.  We have now
> 
> trasferred: bytes
> total: bytes
> duplicate: number of zero pages
> skipped: always zero.
> normal: number of normal pages
> normal_bytes: the same  in bytes
> mbps: megabytes per second?  I can't even remember this one
> dirty_sync_count: number of times we have go through the whole memory
> postcopy_requests = number of pages asked by postcopy faults?
> dirty_pages_rate = pages by some kind of unit
> 
> And we haven't yet started with compression or xbzrle.  I think that the
> best approach at this point is putting everything in pages except the
> things that don't make sense.
> 
> We can put everything on bytes, but then everything is HUGE.
> 
> Anyways, what do libvirt/management apps preffer?

Since we have many fields already which are reported as page counts, I
think just adding page size would be preferrable to having twice as many
fields reported duplicating bytes + pages.

The only reason to favour duplicating all fields to report bytes, is if
we needed to vary page size to deal with huge pages (eg if some reported
pages were 4kb and other reported pages with 2MB). You can easily just
scale huge pages counts to be "normal"  pages for purpose of reporting
though.
Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Chao Fan]

On Tue, Mar 14, 2017 at 12:37:30PM +0000, Daniel P. Berrange wrote:
>On Tue, Mar 14, 2017 at 01:29:43PM +0100, Juan Quintela wrote:
>> Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>> > In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
>> > It's also better for other tools to determine the cpu throttle
>> > value in different architecture.
>> >
>> > Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> 
>> I agree with Daniel here, you can't change the meaning of a field.  Look
>> at skipped pages.  It is zero know because it is not used anymore, but
>> we can't drop it.
>> 
>> I think it is better to expose page_size.  We have now
>> 
>> trasferred: bytes
>> total: bytes
>> duplicate: number of zero pages
>> skipped: always zero.
>> normal: number of normal pages
>> normal_bytes: the same  in bytes
>> mbps: megabytes per second?  I can't even remember this one
>> dirty_sync_count: number of times we have go through the whole memory
>> postcopy_requests = number of pages asked by postcopy faults?
>> dirty_pages_rate = pages by some kind of unit
>> 
>> And we haven't yet started with compression or xbzrle.  I think that the
>> best approach at this point is putting everything in pages except the
>> things that don't make sense.
>> 
>> We can put everything on bytes, but then everything is HUGE.
>> 
>> Anyways, what do libvirt/management apps preffer?
>
>Since we have many fields already which are reported as page counts, I
>think just adding page size would be preferrable to having twice as many
>fields reported duplicating bytes + pages.
OK.

>
>The only reason to favour duplicating all fields to report bytes, is if
>we needed to vary page size to deal with huge pages (eg if some reported
>pages were 4kb and other reported pages with 2MB). You can easily just
>scale huge pages counts to be "normal"  pages for purpose of reporting

Thank you very much for your help!

Thanks,
Chao Fan
>though.
>Regards,
>Daniel
>-- 
>|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org              -o-             http://virt-manager.org :|
>|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
>
>

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Chao Fan]

On Tue, Mar 14, 2017 at 01:29:43PM +0100, Juan Quintela wrote:
>Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>> In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
>> It's also better for other tools to determine the cpu throttle
>> value in different architecture.
>>
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>
>I agree with Daniel here, you can't change the meaning of a field.  Look
>at skipped pages.  It is zero know because it is not used anymore, but
>we can't drop it.

OK, I see.
Thank you very much.

>
>I think it is better to expose page_size.  We have now
>
>trasferred: bytes
>total: bytes
>duplicate: number of zero pages
>skipped: always zero.
>normal: number of normal pages
>normal_bytes: the same  in bytes
>mbps: megabytes per second?  I can't even remember this one
>dirty_sync_count: number of times we have go through the whole memory
>postcopy_requests = number of pages asked by postcopy faults?
>dirty_pages_rate = pages by some kind of unit
>
>And we haven't yet started with compression or xbzrle.  I think that the
>best approach at this point is putting everything in pages except the
>things that don't make sense.
>
>We can put everything on bytes, but then everything is HUGE.

Yes, it is.

>
>Anyways, what do libvirt/management apps preffer?

I think maybe both are OK.
About this issue, I will change my method to get the bytes.

Many thanks for your reply.

Thanks,
Chao Fan
>
>Later, Juan.
>
>
>> ---
>>  hmp.c                 | 6 +++---
>>  migration/migration.c | 2 +-
>>  qapi-schema.json      | 8 ++++----
>>  3 files changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/hmp.c b/hmp.c
>> index 261843f..7334bc5 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -215,9 +215,9 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>>                         info->ram->normal_bytes >> 10);
>>          monitor_printf(mon, "dirty sync count: %" PRIu64 "\n",
>>                         info->ram->dirty_sync_count);
>> -        if (info->ram->dirty_pages_rate) {
>> -            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
>> -                           info->ram->dirty_pages_rate);
>> +        if (info->ram->dirty_bytes_rate) {
>> +            monitor_printf(mon, "dirty bytes rate: %" PRIu64 " bytes/s\n",
>> +                           info->ram->dirty_bytes_rate);
>>          }
>>          if (info->ram->postcopy_requests) {
>>              monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 3dab684..7e7b62c 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -648,7 +648,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>>  
>>      if (s->state != MIGRATION_STATUS_COMPLETED) {
>>          info->ram->remaining = ram_bytes_remaining();
>> -        info->ram->dirty_pages_rate = s->dirty_pages_rate;
>> +        info->ram->dirty_bytes_rate = s->dirty_bytes_rate;
>>      }
>>  }
>>  
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 32b4a4b..83a45f8 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -565,9 +565,6 @@
>>  #
>>  # @normal-bytes: number of normal bytes sent (since 1.2)
>>  #
>> -# @dirty-pages-rate: number of pages dirtied by second by the
>> -#        guest (since 1.3)
>> -#
>>  # @mbps: throughput in megabits/sec. (since 1.6)
>>  #
>>  # @dirty-sync-count: number of times that dirty ram was synchronized (since 2.1)
>> @@ -575,12 +572,15 @@
>>  # @postcopy-requests: The number of page requests received from the destination
>>  #        (since 2.7)
>>  #
>> +# @dirty-bytes-rate: how many bytes  dirtied by second by the
>> +#        guest (since 2.9)
>> +#
>>  # Since: 0.14.0
>>  ##
>>  { 'struct': 'MigrationStats',
>>    'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
>>             'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
>> -           'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
>> +           'normal-bytes': 'int', 'dirty-bytes-rate' : 'int',
>>             'mbps' : 'number', 'dirty-sync-count' : 'int',
>>             'postcopy-requests' : 'int' } }
>
>

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

On 03/14/2017 05:27 AM, Chao Fan wrote:
> In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
> It's also better for other tools to determine the cpu throttle
> value in different architecture.
> 
> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> ---

In addition to the (good) comments you've gotten on not breaking
existing fields, and the choice between a single new field giving the
page size (with all fields favoring pages) or lots of new fields giving
bytes, I have another comment:

> @@ -575,12 +572,15 @@
>  # @postcopy-requests: The number of page requests received from the destination
>  #        (since 2.7)
>  #
> +# @dirty-bytes-rate: how many bytes  dirtied by second by the
> +#        guest (since 2.9)

You've missed soft freeze.  Is this really bug-fix quality to be adding
it into the release this late in the game for 2.9, or should it be
deferred to 2.10?

And while this is just a new field to an existing command, rather than a
new command entirely, it's also worth thinking about Markus' edict for
testsuite coverage:
https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00296.html

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Chao Fan]

On Tue, Mar 14, 2017 at 09:54:34AM -0500, Eric Blake wrote:
>On 03/14/2017 05:27 AM, Chao Fan wrote:
>> In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
>> It's also better for other tools to determine the cpu throttle
>> value in different architecture.
>> 
>> Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> ---
>
>In addition to the (good) comments you've gotten on not breaking
>existing fields, and the choice between a single new field giving the
>page size (with all fields favoring pages) or lots of new fields giving
>bytes, I have another comment:
>
>> @@ -575,12 +572,15 @@
>>  # @postcopy-requests: The number of page requests received from the destination
>>  #        (since 2.7)
>>  #
>> +# @dirty-bytes-rate: how many bytes  dirtied by second by the
>> +#        guest (since 2.9)
>
>You've missed soft freeze.  Is this really bug-fix quality to be adding
>it into the release this late in the game for 2.9, or should it be
>deferred to 2.10?

Ok, I will change it.
Many thanks for your help.

Thanks,
Chao Fan

>
>And while this is just a new field to an existing command, rather than a
>new command entirely, it's also worth thinking about Markus' edict for
>testsuite coverage:
>https://lists.gnu.org/archive/html/qemu-devel/2017-03/msg00296.html
>
>-- 
>Eric Blake   eblake redhat com    +1-919-301-3266
>Libvirt virtualization library http://libvirt.org
>

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Chao Fan]

On Tue, Mar 14, 2017 at 12:37:30PM +0000, Daniel P. Berrange wrote:
>On Tue, Mar 14, 2017 at 01:29:43PM +0100, Juan Quintela wrote:
>> Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
>> > In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
>> > It's also better for other tools to determine the cpu throttle
>> > value in different architecture.
>> >
>> > Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
>> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> 
>> I agree with Daniel here, you can't change the meaning of a field.  Look
>> at skipped pages.  It is zero know because it is not used anymore, but
>> we can't drop it.
>> 
>> I think it is better to expose page_size.  We have now
>> 
>> trasferred: bytes
>> total: bytes
>> duplicate: number of zero pages
>> skipped: always zero.
>> normal: number of normal pages
>> normal_bytes: the same  in bytes
>> mbps: megabytes per second?  I can't even remember this one
>> dirty_sync_count: number of times we have go through the whole memory
>> postcopy_requests = number of pages asked by postcopy faults?
>> dirty_pages_rate = pages by some kind of unit
>> 
>> And we haven't yet started with compression or xbzrle.  I think that the
>> best approach at this point is putting everything in pages except the
>> things that don't make sense.
>> 
>> We can put everything on bytes, but then everything is HUGE.
>> 
>> Anyways, what do libvirt/management apps preffer?
>
>Since we have many fields already which are reported as page counts, I
>think just adding page size would be preferrable to having twice as many
>fields reported duplicating bytes + pages.
>
>The only reason to favour duplicating all fields to report bytes, is if
>we needed to vary page size to deal with huge pages (eg if some reported
>pages were 4kb and other reported pages with 2MB). You can easily just
>scale huge pages counts to be "normal"  pages for purpose of reporting
>though.
>Regards,
>Daniel

Hi Juan and Daniel,

I am wondering if it's OK to expose page_size in qmp or hmp, just like
a new command 'info page_size'.
If confirmed，I will make the new patch.

Thanks,
Chao Fan

>-- 
>|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
>|: http://libvirt.org              -o-             http://virt-manager.org :|
>|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|
>
>

Re: [Qemu-devel] [PATCH] Output dirty-bytes-rate instead of dirty-pages-rate

[Daniel P. Berrange]

On Wed, Mar 15, 2017 at 12:13:58PM +0800, Chao Fan wrote:
> On Tue, Mar 14, 2017 at 12:37:30PM +0000, Daniel P. Berrange wrote:
> >On Tue, Mar 14, 2017 at 01:29:43PM +0100, Juan Quintela wrote:
> >> Chao Fan <fanc.fnst@cn.fujitsu.com> wrote:
> >> > In hmp, dirty-bytes-rate is more friendly than dirty-pages-rate.
> >> > It's also better for other tools to determine the cpu throttle
> >> > value in different architecture.
> >> >
> >> > Signed-off-by: Chao Fan <fanc.fnst@cn.fujitsu.com>
> >> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
> >> 
> >> I agree with Daniel here, you can't change the meaning of a field.  Look
> >> at skipped pages.  It is zero know because it is not used anymore, but
> >> we can't drop it.
> >> 
> >> I think it is better to expose page_size.  We have now
> >> 
> >> trasferred: bytes
> >> total: bytes
> >> duplicate: number of zero pages
> >> skipped: always zero.
> >> normal: number of normal pages
> >> normal_bytes: the same  in bytes
> >> mbps: megabytes per second?  I can't even remember this one
> >> dirty_sync_count: number of times we have go through the whole memory
> >> postcopy_requests = number of pages asked by postcopy faults?
> >> dirty_pages_rate = pages by some kind of unit
> >> 
> >> And we haven't yet started with compression or xbzrle.  I think that the
> >> best approach at this point is putting everything in pages except the
> >> things that don't make sense.
> >> 
> >> We can put everything on bytes, but then everything is HUGE.
> >> 
> >> Anyways, what do libvirt/management apps preffer?
> >
> >Since we have many fields already which are reported as page counts, I
> >think just adding page size would be preferrable to having twice as many
> >fields reported duplicating bytes + pages.
> >
> >The only reason to favour duplicating all fields to report bytes, is if
> >we needed to vary page size to deal with huge pages (eg if some reported
> >pages were 4kb and other reported pages with 2MB). You can easily just
> >scale huge pages counts to be "normal"  pages for purpose of reporting
> >though.
> 
> I am wondering if it's OK to expose page_size in qmp or hmp, just like
> a new command 'info page_size'.
> If confirmed，I will make the new patch.

I'd suggest having it as a field of "info migrate"

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|