Re: [PATCH 5/5] migration: Print expected-downtime on completion

[Peter Xu]

On Tue, Sep 26, 2023 at 05:18:41PM +0100, Joao Martins wrote:
> Right now, migration statistics either print downtime or expected
> downtime depending on migration completing of in progress. Also in the
> beginning of migration by printing the downtime limit as expected
> downtime, when estimation is not available.
> 
> The pending_size is private in migration iteration and not necessarily
> accessible outside. Given the non-determinism of the switchover cost, it
> can be useful to understand if the downtime was far off from the one
> detected by the migration algoritm, thus print the resultant downtime
> alongside its estimation.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>  migration/migration.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index dec6c88fbff9..f08f65b4b1c3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -943,6 +943,10 @@ static void populate_time_info(MigrationInfo *info, 
> MigrationState *s)
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          info->has_total_time = true;
>          info->total_time = s->total_time;
> +        if (s->expected_downtime) {
> +            info->has_expected_downtime = true;
> +            info->expected_downtime = s->expected_downtime;
> +        }

There's another chunk right below that will also show
expected_downtime.. How about we merge them to be clear?

IIUC the current patch will not display expected_downtime during postcopy,
which makes sense.  But it'll pop up again after postcopy completes... so
not ideal either. If so sounds easier to just show it as long as we have a
value, and the user can ignore it.

@@ -913,7 +913,9 @@ static void populate_time_info(MigrationInfo *info, 
MigrationState *s)
     if (migrate_show_downtime(s)) {
         info->has_downtime = true;
         info->downtime = s->downtime;
-    } else {
+    }
+
+    if (s->expected_downtime) {
         info->has_expected_downtime = true;
         info->expected_downtime = s->expected_downtime;
     }

IIUC currently expected_downtime for postcopy makes less sense.  Maybe one
day we can make it reflect reality, by taking more things into account
(besides dirty RAM rate).

>      } else {
>          info->has_total_time = true;
>          info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME) -
> @@ -2844,6 +2848,10 @@ static MigIterateState 
> migration_iteration_run(MigrationState *s)
>  
>      if ((!pending_size || pending_size < s->threshold_size) && 
> can_switchover) {
>          trace_migration_thread_low_pending(pending_size);
> +        if (s->threshold_size) {
> +            s->expected_downtime = (pending_size * 
> s->parameters.downtime_limit) /
> +                                   s->threshold_size;
> +        }

I had a feeling that you did the calculation to avoid accessing ->mbps. :)

I'd suggest we move this into migration_completion(), and use ->mbps
(before the other avail-switchover-bandwidth patch lands).  It's just that
using the bandwidth value seems more straightforward.  Or maybe I missed
something tricky?

>          migration_completion(s);
>          return MIG_ITERATE_BREAK;
>      }
> -- 
> 2.39.3
> 

Thanks,

-- 
Peter Xu