[Qemu-devel] [PATCH 0/4] Make info migrate output consistent

[Qemu-devel] [PATCH 0/4] Make info migrate output consistent

[Juan Quintela]

Hi

This series make the output of info migrate be printed in a single
place for the states ACTIVE, CANCELLING and POSTCOPY_ACTIVE.

The code was duplicated, just use the same code.  Once done that, fix
the inconsistences that were not needed.

Please, review.

Thanks, Juan.

Juan Quintela (4):
  ram: Unfold get_xbzrle_cache_stats() into populate_ram_info()
  ram: We only print throttling information sometimes
  ram: Print block stats also in the complete case
  ram: Now POSTCOPY_ACTIVE is the same that STATUS_ACTIVE

 migration/migration.c | 77 ++++++++++++++++-----------------------------------
 1 file changed, 24 insertions(+), 53 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/4] ram: Unfold get_xbzrle_cache_stats() into populate_ram_info()

[Juan Quintela]

They were called consequtiveley always.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 60da9d6..fd8b406 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -476,20 +476,6 @@ static bool migration_is_setup_or_active(int state)
     }
 }
 
-static void get_xbzrle_cache_stats(MigrationInfo *info)
-{
-    if (migrate_use_xbzrle()) {
-        info->has_xbzrle_cache = true;
-        info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
-        info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
-        info->xbzrle_cache->bytes = xbzrle_mig_bytes_transferred();
-        info->xbzrle_cache->pages = xbzrle_mig_pages_transferred();
-        info->xbzrle_cache->cache_miss = xbzrle_mig_pages_cache_miss();
-        info->xbzrle_cache->cache_miss_rate = xbzrle_mig_cache_miss_rate();
-        info->xbzrle_cache->overflow = xbzrle_mig_pages_overflow();
-    }
-}
-
 static void populate_ram_info(MigrationInfo *info, MigrationState *s)
 {
     info->has_ram = true;
@@ -507,6 +493,17 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
     info->ram->postcopy_requests = ram_postcopy_requests();
     info->ram->page_size = qemu_target_page_size();
 
+    if (migrate_use_xbzrle()) {
+        info->has_xbzrle_cache = true;
+        info->xbzrle_cache = g_malloc0(sizeof(*info->xbzrle_cache));
+        info->xbzrle_cache->cache_size = migrate_xbzrle_cache_size();
+        info->xbzrle_cache->bytes = xbzrle_mig_bytes_transferred();
+        info->xbzrle_cache->pages = xbzrle_mig_pages_transferred();
+        info->xbzrle_cache->cache_miss = xbzrle_mig_pages_cache_miss();
+        info->xbzrle_cache->cache_miss_rate = xbzrle_mig_cache_miss_rate();
+        info->xbzrle_cache->overflow = xbzrle_mig_pages_overflow();
+    }
+
     if (s->state != MIGRATION_STATUS_COMPLETED) {
         info->ram->remaining = ram_bytes_remaining();
         info->ram->dirty_pages_rate = ram_dirty_pages_rate();
@@ -552,7 +549,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
             info->cpu_throttle_percentage = cpu_throttle_get_percentage();
         }
 
-        get_xbzrle_cache_stats(info);
         break;
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
         /* Mostly the same as active; TODO add some postcopy stats */
@@ -575,15 +571,12 @@ MigrationInfo *qmp_query_migrate(Error **errp)
             info->disk->total = blk_mig_bytes_total();
         }
 
-        get_xbzrle_cache_stats(info);
         break;
     case MIGRATION_STATUS_COLO:
         info->has_status = true;
         /* TODO: display COLO specific information (checkpoint info etc.) */
         break;
     case MIGRATION_STATUS_COMPLETED:
-        get_xbzrle_cache_stats(info);
-
         info->has_status = true;
         info->has_total_time = true;
         info->total_time = s->total_time;
-- 
2.9.4

[Qemu-devel] [PATCH 2/4] ram: We only print throttling information sometimes

[Juan Quintela]

Change it to be consistent with everything else.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index fd8b406..7f79da0 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -504,6 +504,11 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
         info->xbzrle_cache->overflow = xbzrle_mig_pages_overflow();
     }
 
+    if (cpu_throttle_active()) {
+        info->has_cpu_throttle_percentage = true;
+        info->cpu_throttle_percentage = cpu_throttle_get_percentage();
+    }
+
     if (s->state != MIGRATION_STATUS_COMPLETED) {
         info->ram->remaining = ram_bytes_remaining();
         info->ram->dirty_pages_rate = ram_dirty_pages_rate();
@@ -544,11 +549,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
             info->disk->total = blk_mig_bytes_total();
         }
 
-        if (cpu_throttle_active()) {
-            info->has_cpu_throttle_percentage = true;
-            info->cpu_throttle_percentage = cpu_throttle_get_percentage();
-        }
-
         break;
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
         /* Mostly the same as active; TODO add some postcopy stats */
-- 
2.9.4

[Qemu-devel] [PATCH 3/4] ram: Print block stats also in the complete case

[Juan Quintela]

To make things easier, I just moved it to populate_ram_info().

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 7f79da0..8289544 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -509,6 +509,14 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
         info->cpu_throttle_percentage = cpu_throttle_get_percentage();
     }
 
+    if (blk_mig_active()) {
+        info->has_disk = true;
+        info->disk = g_malloc0(sizeof(*info->disk));
+        info->disk->transferred = blk_mig_bytes_transferred();
+        info->disk->remaining = blk_mig_bytes_remaining();
+        info->disk->total = blk_mig_bytes_total();
+    }
+
     if (s->state != MIGRATION_STATUS_COMPLETED) {
         info->ram->remaining = ram_bytes_remaining();
         info->ram->dirty_pages_rate = ram_dirty_pages_rate();
@@ -540,15 +548,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->setup_time = s->setup_time;
 
         populate_ram_info(info, s);
-
-        if (blk_mig_active()) {
-            info->has_disk = true;
-            info->disk = g_malloc0(sizeof(*info->disk));
-            info->disk->transferred = blk_mig_bytes_transferred();
-            info->disk->remaining = blk_mig_bytes_remaining();
-            info->disk->total = blk_mig_bytes_total();
-        }
-
         break;
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
         /* Mostly the same as active; TODO add some postcopy stats */
@@ -562,15 +561,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->setup_time = s->setup_time;
 
         populate_ram_info(info, s);
-
-        if (blk_mig_active()) {
-            info->has_disk = true;
-            info->disk = g_malloc0(sizeof(*info->disk));
-            info->disk->transferred = blk_mig_bytes_transferred();
-            info->disk->remaining = blk_mig_bytes_remaining();
-            info->disk->total = blk_mig_bytes_total();
-        }
-
         break;
     case MIGRATION_STATUS_COLO:
         info->has_status = true;
-- 
2.9.4

[Qemu-devel] [PATCH 4/4] ram: Now POSTCOPY_ACTIVE is the same that STATUS_ACTIVE

[Juan Quintela]

Merge them.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 8289544..af4c2cc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -538,19 +538,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         break;
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_CANCELLING:
-        info->has_status = true;
-        info->has_total_time = true;
-        info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
-            - s->total_time;
-        info->has_expected_downtime = true;
-        info->expected_downtime = s->expected_downtime;
-        info->has_setup_time = true;
-        info->setup_time = s->setup_time;
-
-        populate_ram_info(info, s);
-        break;
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
-        /* Mostly the same as active; TODO add some postcopy stats */
         info->has_status = true;
         info->has_total_time = true;
         info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
-- 
2.9.4

Re: [Qemu-devel] [PATCH 1/4] ram: Unfold get_xbzrle_cache_stats() into populate_ram_info()

[Eric Blake]

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

On 06/01/2017 04:40 PM, Juan Quintela wrote:
> They were called consequtiveley always.

s/consequtiveley/consecutively/

> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 2/4] ram: We only print throttling information sometimes

[Eric Blake]

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

On 06/01/2017 04:40 PM, Juan Quintela wrote:
> Change it to be consistent with everything else.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 3/4] ram: Print block stats also in the complete case

[Eric Blake]

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

On 06/01/2017 04:40 PM, Juan Quintela wrote:
> To make things easier, I just moved it to populate_ram_info().
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 26 ++++++++------------------
>  1 file changed, 8 insertions(+), 18 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 4/4] ram: Now POSTCOPY_ACTIVE is the same that STATUS_ACTIVE

[Eric Blake]

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

On 06/01/2017 04:40 PM, Juan Quintela wrote:
> Merge them.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 12 ------------
>  1 file changed, 12 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH 3/4] ram: Print block stats also in the complete case

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> To make things easier, I just moved it to populate_ram_info().

Blk is not RAM; better to create a separate function.

Dave

> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 26 ++++++++------------------
>  1 file changed, 8 insertions(+), 18 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 7f79da0..8289544 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -509,6 +509,14 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>          info->cpu_throttle_percentage = cpu_throttle_get_percentage();
>      }
>  
> +    if (blk_mig_active()) {
> +        info->has_disk = true;
> +        info->disk = g_malloc0(sizeof(*info->disk));
> +        info->disk->transferred = blk_mig_bytes_transferred();
> +        info->disk->remaining = blk_mig_bytes_remaining();
> +        info->disk->total = blk_mig_bytes_total();
> +    }
> +
>      if (s->state != MIGRATION_STATUS_COMPLETED) {
>          info->ram->remaining = ram_bytes_remaining();
>          info->ram->dirty_pages_rate = ram_dirty_pages_rate();
> @@ -540,15 +548,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->setup_time = s->setup_time;
>  
>          populate_ram_info(info, s);
> -
> -        if (blk_mig_active()) {
> -            info->has_disk = true;
> -            info->disk = g_malloc0(sizeof(*info->disk));
> -            info->disk->transferred = blk_mig_bytes_transferred();
> -            info->disk->remaining = blk_mig_bytes_remaining();
> -            info->disk->total = blk_mig_bytes_total();
> -        }
> -
>          break;
>      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
>          /* Mostly the same as active; TODO add some postcopy stats */
> @@ -562,15 +561,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->setup_time = s->setup_time;
>  
>          populate_ram_info(info, s);
> -
> -        if (blk_mig_active()) {
> -            info->has_disk = true;
> -            info->disk = g_malloc0(sizeof(*info->disk));
> -            info->disk->transferred = blk_mig_bytes_transferred();
> -            info->disk->remaining = blk_mig_bytes_remaining();
> -            info->disk->total = blk_mig_bytes_total();
> -        }
> -
>          break;
>      case MIGRATION_STATUS_COLO:
>          info->has_status = true;
> -- 
> 2.9.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 3/4] ram: Print block stats also in the complete case

[Peter Xu]

On Fri, Jun 02, 2017 at 06:55:30PM +0100, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
> > To make things easier, I just moved it to populate_ram_info().
> 
> Blk is not RAM; better to create a separate function.

Agree...

Maybe rename populate_ram_info() into more general name?
migrate_info_populate()?

(Also, I think auto convergence is not RAM-related as well, so same
 comment may apply to patch 2)

Thanks,

> 
> Dave
> 
> > Signed-off-by: Juan Quintela <quintela@redhat.com>
> > ---
> >  migration/migration.c | 26 ++++++++------------------
> >  1 file changed, 8 insertions(+), 18 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 7f79da0..8289544 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -509,6 +509,14 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
> >          info->cpu_throttle_percentage = cpu_throttle_get_percentage();
> >      }
> >  
> > +    if (blk_mig_active()) {
> > +        info->has_disk = true;
> > +        info->disk = g_malloc0(sizeof(*info->disk));
> > +        info->disk->transferred = blk_mig_bytes_transferred();
> > +        info->disk->remaining = blk_mig_bytes_remaining();
> > +        info->disk->total = blk_mig_bytes_total();
> > +    }
> > +
> >      if (s->state != MIGRATION_STATUS_COMPLETED) {
> >          info->ram->remaining = ram_bytes_remaining();
> >          info->ram->dirty_pages_rate = ram_dirty_pages_rate();
> > @@ -540,15 +548,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> >          info->setup_time = s->setup_time;
> >  
> >          populate_ram_info(info, s);
> > -
> > -        if (blk_mig_active()) {
> > -            info->has_disk = true;
> > -            info->disk = g_malloc0(sizeof(*info->disk));
> > -            info->disk->transferred = blk_mig_bytes_transferred();
> > -            info->disk->remaining = blk_mig_bytes_remaining();
> > -            info->disk->total = blk_mig_bytes_total();
> > -        }
> > -
> >          break;
> >      case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> >          /* Mostly the same as active; TODO add some postcopy stats */
> > @@ -562,15 +561,6 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> >          info->setup_time = s->setup_time;
> >  
> >          populate_ram_info(info, s);
> > -
> > -        if (blk_mig_active()) {
> > -            info->has_disk = true;
> > -            info->disk = g_malloc0(sizeof(*info->disk));
> > -            info->disk->transferred = blk_mig_bytes_transferred();
> > -            info->disk->remaining = blk_mig_bytes_remaining();
> > -            info->disk->total = blk_mig_bytes_total();
> > -        }
> > -
> >          break;
> >      case MIGRATION_STATUS_COLO:
> >          info->has_status = true;
> > -- 
> > 2.9.4
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

-- 
Peter Xu