All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Dr. David Alan Gilbert" <dave@treblig.org>
To: Peter Xu <peterx@redhat.com>
Cc: qemu-devel@nongnu.org, Fabiano Rosas <farosas@suse.de>,
	Prasad Pandit <ppandit@redhat.com>,
	Juraj Marcin <jmarcin@redhat.com>
Subject: Re: [PATCH v2 2/2] migration/hmp: Add "info migrate -a", reorg the dump
Date: Wed, 14 May 2025 20:29:53 +0000	[thread overview]
Message-ID: <aCT9QRPd4cDSshGI@gallifrey> (raw)
In-Reply-To: <20250514200137.581935-3-peterx@redhat.com>

* Peter Xu (peterx@redhat.com) wrote:
> A new parameter "-a" is added to "info migrate" to dump all info, while
> when not specified it only dumps the important ones.  When at it, reorg
> everything to make it easier to read for human.
> 
> The general rule is:
> 
>   - Put important things at the top
>   - Reuse a single line when things are very relevant, hence reducing lines
>     needed to show the results
>   - Remove almost useless ones (e.g. "normal_bytes", while we also have
>     both "page size" and "normal" pages)
>   - Regroup things, so that related fields will show together
>   - etc.

Thanks for the update,

Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>

Note that you did miss the change (which would be fine as a follow up)
where I point out that I think your unit abbreviations are slightly wrong
(although I think I was wrong as well...)
I think your throughput is in Mbps (capital M or Mb/s or Mbit/s) - ie.
10^6 bits/second.

While I think all your KB are KiB not KB (i.e. 2^10 bytes).

Dave

> Before this change, it looks like (one example of a completed case):
> 
>   globals:
>   store-global-state: on
>   only-migratable: off
>   send-configuration: on
>   send-section-footer: on
>   send-switchover-start: on
>   clear-bitmap-shift: 18
>   Migration status: completed
>   total time: 122952 ms
>   downtime: 76 ms
>   setup: 15 ms
>   transferred ram: 130825923 kbytes
>   throughput: 8717.68 mbps
>   remaining ram: 0 kbytes
>   total ram: 16777992 kbytes
>   duplicate: 997263 pages
>   normal: 32622225 pages
>   normal bytes: 130488900 kbytes
>   dirty sync count: 10
>   page size: 4 kbytes
>   multifd bytes: 117134260 kbytes
>   pages-per-second: 169431
>   postcopy request count: 5835
>   precopy ram: 15 kbytes
>   postcopy ram: 13691151 kbytes
> 
> After this change, sample output (default, no "-a" specified):
> 
>   Status: postcopy-active
>   Time (ms): total=40504, setup=14, down=145
>   RAM info:
>     Bandwidth (mbps): 6102.65
>     Sizes (KB): psize=4, total=16777992
>       transferred=37673019, remain=2136404,
>       precopy=3, multifd=26108780, postcopy=11563855
>     Pages: normal=9394288, zero=600672, rate_per_sec=185875
>     Others: dirty_syncs=3, dirty_pages_rate=278378, postcopy_req=4078
> 
> Sample output when "-a" specified:
> 
>   Status: active
>   Time (ms): total=3040, setup=4, exp_down=300
>   RAM info:
>     Throughput (mbps): 10.51
>     Sizes (KB): psize=4, total=4211528
>       transferred=3979, remain=4206452,
>       precopy=3978, multifd=0, postcopy=0
>     Pages: normal=992, zero=277, rate_per_sec=320
>     Others: dirty_syncs=1
>   Globals:
>     store-global-state: on
>     only-migratable: off
>     send-configuration: on
>     send-section-footer: on
>     send-switchover-start: on
>     clear-bitmap-shift: 18
>   XBZRLE: size=67108864, transferred=0, pages=0, miss=188451
>     miss_rate=0.00, encode_rate=0.00, overflow=0
>   CPU Throttle (%): 0
>   Dirty-limit Throttle (us): 0
>   Dirty-limit Ring Full (us): 0
>   Postcopy Blocktime (ms): 0
>   Postcopy vCPU Blocktime: ...
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration-hmp-cmds.c | 186 +++++++++++++++++----------------
>  hmp-commands-info.hx           |   6 +-
>  2 files changed, 99 insertions(+), 93 deletions(-)
> 
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index 49c26daed3..13e93d3c54 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -37,29 +37,28 @@ static void migration_global_dump(Monitor *mon)
>  {
>      MigrationState *ms = migrate_get_current();
>  
> -    monitor_printf(mon, "globals:\n");
> -    monitor_printf(mon, "store-global-state: %s\n",
> +    monitor_printf(mon, "Globals:\n");
> +    monitor_printf(mon, "  store-global-state: %s\n",
>                     ms->store_global_state ? "on" : "off");
> -    monitor_printf(mon, "only-migratable: %s\n",
> +    monitor_printf(mon, "  only-migratable: %s\n",
>                     only_migratable ? "on" : "off");
> -    monitor_printf(mon, "send-configuration: %s\n",
> +    monitor_printf(mon, "  send-configuration: %s\n",
>                     ms->send_configuration ? "on" : "off");
> -    monitor_printf(mon, "send-section-footer: %s\n",
> +    monitor_printf(mon, "  send-section-footer: %s\n",
>                     ms->send_section_footer ? "on" : "off");
> -    monitor_printf(mon, "send-switchover-start: %s\n",
> +    monitor_printf(mon, "  send-switchover-start: %s\n",
>                     ms->send_switchover_start ? "on" : "off");
> -    monitor_printf(mon, "clear-bitmap-shift: %u\n",
> +    monitor_printf(mon, "  clear-bitmap-shift: %u\n",
>                     ms->clear_bitmap_shift);
>  }
>  
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  {
> +    bool show_all = qdict_get_try_bool(qdict, "all", false);
>      MigrationInfo *info;
>  
>      info = qmp_query_migrate(NULL);
>  
> -    migration_global_dump(mon);
> -
>      if (info->blocked_reasons) {
>          strList *reasons = info->blocked_reasons;
>          monitor_printf(mon, "Outgoing migration blocked:\n");
> @@ -70,7 +69,7 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>      }
>  
>      if (info->has_status) {
> -        monitor_printf(mon, "Migration status: %s",
> +        monitor_printf(mon, "Status: %s",
>                         MigrationStatus_str(info->status));
>          if (info->status == MIGRATION_STATUS_FAILED && info->error_desc) {
>              monitor_printf(mon, " (%s)\n", info->error_desc);
> @@ -78,107 +77,130 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>              monitor_printf(mon, "\n");
>          }
>  
> -        monitor_printf(mon, "total time: %" PRIu64 " ms\n",
> -                       info->total_time);
> -        if (info->has_expected_downtime) {
> -            monitor_printf(mon, "expected downtime: %" PRIu64 " ms\n",
> -                           info->expected_downtime);
> -        }
> -        if (info->has_downtime) {
> -            monitor_printf(mon, "downtime: %" PRIu64 " ms\n",
> -                           info->downtime);
> +        if (info->total_time) {
> +            monitor_printf(mon, "Time (ms): total=%" PRIu64,
> +                           info->total_time);
> +            if (info->has_setup_time) {
> +                monitor_printf(mon, ", setup=%" PRIu64,
> +                               info->setup_time);
> +            }
> +            if (info->has_expected_downtime) {
> +                monitor_printf(mon, ", exp_down=%" PRIu64,
> +                               info->expected_downtime);
> +            }
> +            if (info->has_downtime) {
> +                monitor_printf(mon, ", down=%" PRIu64,
> +                               info->downtime);
> +            }
> +            monitor_printf(mon, "\n");
>          }
> -        if (info->has_setup_time) {
> -            monitor_printf(mon, "setup: %" PRIu64 " ms\n",
> -                           info->setup_time);
> +    }
> +
> +    if (info->has_socket_address) {
> +        SocketAddressList *addr;
> +
> +        monitor_printf(mon, "Sockets: [\n");
> +
> +        for (addr = info->socket_address; addr; addr = addr->next) {
> +            char *s = socket_uri(addr->value);
> +            monitor_printf(mon, "\t%s\n", s);
> +            g_free(s);
>          }
> +        monitor_printf(mon, "]\n");
>      }
>  
>      if (info->ram) {
> -        monitor_printf(mon, "transferred ram: %" PRIu64 " kbytes\n",
> -                       info->ram->transferred >> 10);
> -        monitor_printf(mon, "throughput: %0.2f mbps\n",
> +        monitor_printf(mon, "RAM info:\n");
> +        monitor_printf(mon, "  Throughput (mbps): %0.2f\n",
>                         info->ram->mbps);
> -        monitor_printf(mon, "remaining ram: %" PRIu64 " kbytes\n",
> -                       info->ram->remaining >> 10);
> -        monitor_printf(mon, "total ram: %" PRIu64 " kbytes\n",
> +        monitor_printf(mon, "  Sizes (KB): psize=%" PRIu64
> +                       ", total=%" PRIu64 "\n",
> +                       info->ram->page_size >> 10,
>                         info->ram->total >> 10);
> -        monitor_printf(mon, "duplicate: %" PRIu64 " pages\n",
> -                       info->ram->duplicate);
> -        monitor_printf(mon, "normal: %" PRIu64 " pages\n",
> -                       info->ram->normal);
> -        monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
> -                       info->ram->normal_bytes >> 10);
> -        monitor_printf(mon, "dirty sync count: %" PRIu64 "\n",
> -                       info->ram->dirty_sync_count);
> -        monitor_printf(mon, "page size: %" PRIu64 " kbytes\n",
> -                       info->ram->page_size >> 10);
> -        monitor_printf(mon, "multifd bytes: %" PRIu64 " kbytes\n",
> -                       info->ram->multifd_bytes >> 10);
> -        monitor_printf(mon, "pages-per-second: %" PRIu64 "\n",
> +        monitor_printf(mon, "    transferred=%" PRIu64
> +                       ", remain=%" PRIu64 ",\n",
> +                       info->ram->transferred >> 10,
> +                       info->ram->remaining >> 10);
> +        monitor_printf(mon, "    precopy=%" PRIu64
> +                       ", multifd=%" PRIu64
> +                       ", postcopy=%" PRIu64,
> +                       info->ram->precopy_bytes >> 10,
> +                       info->ram->multifd_bytes >> 10,
> +                       info->ram->postcopy_bytes >> 10);
> +
> +        if (info->vfio) {
> +            monitor_printf(mon, ", vfio=%" PRIu64,
> +                           info->vfio->transferred >> 10);
> +        }
> +        monitor_printf(mon, "\n");
> +
> +        monitor_printf(mon, "  Pages: normal=%" PRIu64 ", zero=%" PRIu64
> +                       ", rate_per_sec=%" PRIu64 "\n",
> +                       info->ram->normal,
> +                       info->ram->duplicate,
>                         info->ram->pages_per_second);
> +        monitor_printf(mon, "  Others: dirty_syncs=%" PRIu64,
> +                       info->ram->dirty_sync_count);
>  
>          if (info->ram->dirty_pages_rate) {
> -            monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
> +            monitor_printf(mon, ", dirty_pages_rate=%" PRIu64,
>                             info->ram->dirty_pages_rate);
>          }
>          if (info->ram->postcopy_requests) {
> -            monitor_printf(mon, "postcopy request count: %" PRIu64 "\n",
> +            monitor_printf(mon, ", postcopy_req=%" PRIu64,
>                             info->ram->postcopy_requests);
>          }
> -        if (info->ram->precopy_bytes) {
> -            monitor_printf(mon, "precopy ram: %" PRIu64 " kbytes\n",
> -                           info->ram->precopy_bytes >> 10);
> -        }
>          if (info->ram->downtime_bytes) {
> -            monitor_printf(mon, "downtime ram: %" PRIu64 " kbytes\n",
> -                           info->ram->downtime_bytes >> 10);
> -        }
> -        if (info->ram->postcopy_bytes) {
> -            monitor_printf(mon, "postcopy ram: %" PRIu64 " kbytes\n",
> -                           info->ram->postcopy_bytes >> 10);
> +            monitor_printf(mon, ", downtime_ram=%" PRIu64,
> +                           info->ram->downtime_bytes);
>          }
>          if (info->ram->dirty_sync_missed_zero_copy) {
> -            monitor_printf(mon,
> -                           "Zero-copy-send fallbacks happened: %" PRIu64 " times\n",
> +            monitor_printf(mon, ", zerocopy_fallbacks=%" PRIu64,
>                             info->ram->dirty_sync_missed_zero_copy);
>          }
> +        monitor_printf(mon, "\n");
> +    }
> +
> +    if (!show_all) {
> +        goto out;
>      }
>  
> +    migration_global_dump(mon);
> +
>      if (info->xbzrle_cache) {
> -        monitor_printf(mon, "cache size: %" PRIu64 " bytes\n",
> -                       info->xbzrle_cache->cache_size);
> -        monitor_printf(mon, "xbzrle transferred: %" PRIu64 " kbytes\n",
> -                       info->xbzrle_cache->bytes >> 10);
> -        monitor_printf(mon, "xbzrle pages: %" PRIu64 " pages\n",
> -                       info->xbzrle_cache->pages);
> -        monitor_printf(mon, "xbzrle cache miss: %" PRIu64 " pages\n",
> -                       info->xbzrle_cache->cache_miss);
> -        monitor_printf(mon, "xbzrle cache miss rate: %0.2f\n",
> -                       info->xbzrle_cache->cache_miss_rate);
> -        monitor_printf(mon, "xbzrle encoding rate: %0.2f\n",
> -                       info->xbzrle_cache->encoding_rate);
> -        monitor_printf(mon, "xbzrle overflow: %" PRIu64 "\n",
> +        monitor_printf(mon, "XBZRLE: size=%" PRIu64
> +                       ", transferred=%" PRIu64
> +                       ", pages=%" PRIu64
> +                       ", miss=%" PRIu64 "\n"
> +                       "  miss_rate=%0.2f"
> +                       ", encode_rate=%0.2f"
> +                       ", overflow=%" PRIu64 "\n",
> +                       info->xbzrle_cache->cache_size,
> +                       info->xbzrle_cache->bytes,
> +                       info->xbzrle_cache->pages,
> +                       info->xbzrle_cache->cache_miss,
> +                       info->xbzrle_cache->cache_miss_rate,
> +                       info->xbzrle_cache->encoding_rate,
>                         info->xbzrle_cache->overflow);
>      }
>  
>      if (info->has_cpu_throttle_percentage) {
> -        monitor_printf(mon, "cpu throttle percentage: %" PRIu64 "\n",
> +        monitor_printf(mon, "CPU Throttle (%%): %" PRIu64 "\n",
>                         info->cpu_throttle_percentage);
>      }
>  
>      if (info->has_dirty_limit_throttle_time_per_round) {
> -        monitor_printf(mon, "dirty-limit throttle time: %" PRIu64 " us\n",
> +        monitor_printf(mon, "Dirty-limit Throttle (us): %" PRIu64 "\n",
>                         info->dirty_limit_throttle_time_per_round);
>      }
>  
>      if (info->has_dirty_limit_ring_full_time) {
> -        monitor_printf(mon, "dirty-limit ring full time: %" PRIu64 " us\n",
> +        monitor_printf(mon, "Dirty-limit Ring Full (us): %" PRIu64 "\n",
>                         info->dirty_limit_ring_full_time);
>      }
>  
>      if (info->has_postcopy_blocktime) {
> -        monitor_printf(mon, "postcopy blocktime: %u\n",
> +        monitor_printf(mon, "Postcopy Blocktime (ms): %" PRIu32 "\n",
>                         info->postcopy_blocktime);
>      }
>  
> @@ -189,28 +211,12 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>          visit_type_uint32List(v, NULL, &info->postcopy_vcpu_blocktime,
>                                &error_abort);
>          visit_complete(v, &str);
> -        monitor_printf(mon, "postcopy vcpu blocktime: %s\n", str);
> +        monitor_printf(mon, "Postcopy vCPU Blocktime: %s\n", str);
>          g_free(str);
>          visit_free(v);
>      }
> -    if (info->has_socket_address) {
> -        SocketAddressList *addr;
> -
> -        monitor_printf(mon, "socket address: [\n");
> -
> -        for (addr = info->socket_address; addr; addr = addr->next) {
> -            char *s = socket_uri(addr->value);
> -            monitor_printf(mon, "\t%s\n", s);
> -            g_free(s);
> -        }
> -        monitor_printf(mon, "]\n");
> -    }
> -
> -    if (info->vfio) {
> -        monitor_printf(mon, "vfio device transferred: %" PRIu64 " kbytes\n",
> -                       info->vfio->transferred >> 10);
> -    }
>  
> +out:
>      qapi_free_MigrationInfo(info);
>  }
>  
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index c59cd6637b..639a450ee5 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -475,9 +475,9 @@ ERST
>  
>      {
>          .name       = "migrate",
> -        .args_type  = "",
> -        .params     = "",
> -        .help       = "show migration status",
> +        .args_type  = "all:-a",
> +        .params     = "[-a]",
> +        .help       = "show migration status (-a: all, dump all status)",
>          .cmd        = hmp_info_migrate,
>      },
>  
> -- 
> 2.49.0
> 
-- 
 -----Open up your eyes, open up your mind, open up your code -------   
/ Dr. David Alan Gilbert    |       Running GNU/Linux       | Happy  \ 
\        dave @ treblig.org |                               | In Hex /
 \ _________________________|_____ http://www.treblig.org   |_______/


  reply	other threads:[~2025-05-14 20:30 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-14 20:01 [PATCH v2 0/2] migration: Some fix and enhancements to HMP "info migrate" Peter Xu
2025-05-14 20:01 ` [PATCH v2 1/2] migration: Allow caps to be set when preempt or multifd cap enabled Peter Xu
2025-05-22 20:15   ` Michael Tokarev
2025-05-14 20:01 ` [PATCH v2 2/2] migration/hmp: Add "info migrate -a", reorg the dump Peter Xu
2025-05-14 20:29   ` Dr. David Alan Gilbert [this message]
2025-05-14 21:17     ` Peter Xu
2025-05-14 23:00       ` Dr. David Alan Gilbert
2025-05-21  8:43   ` Zhijian Li (Fujitsu) via
2025-05-21 14:03     ` Peter Xu
2025-05-21 21:04       ` Dr. David Alan Gilbert
2025-05-22  0:55         ` Zhijian Li (Fujitsu) via
2025-05-22 13:16           ` Peter Xu
2025-05-23  2:06             ` Zhijian Li (Fujitsu) via
2025-05-26 15:40               ` Peter Xu
2025-05-22  1:07       ` Zhijian Li (Fujitsu) via
2025-05-19 11:27 ` [PATCH v2 0/2] migration: Some fix and enhancements to HMP "info migrate" Mario Casquero
2025-05-20 17:03   ` Peter Xu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aCT9QRPd4cDSshGI@gallifrey \
    --to=dave@treblig.org \
    --cc=farosas@suse.de \
    --cc=jmarcin@redhat.com \
    --cc=peterx@redhat.com \
    --cc=ppandit@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.