Re: [PATCH RFC 10/12] migration: Introduce a helper to return switchover bw estimate

[Juraj Marcin <jmarcin@redhat.com>]

On 2026-03-19 19:13, Peter Xu wrote:
> Add a helper to be able to return an estimate of switchover bw, called
> migration_get_switchover_bw(). Use it to sligitly simplify the current
> code.
> 
> This will be used in new codes later to remove expected_downtime.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.h |  1 +
>  migration/migration.c | 43 ++++++++++++++++++++++---------------------
>  2 files changed, 23 insertions(+), 21 deletions(-)
> 
> diff --git a/migration/migration.h b/migration/migration.h
> index b6888daced..bf3ee6cc07 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -586,6 +586,7 @@ void migration_cancel(void);
>  void migration_populate_vfio_info(MigrationInfo *info);
>  void migration_reset_vfio_bytes_transferred(void);
>  void postcopy_temp_page_reset(PostcopyTmpPage *tmp_page);
> +int64_t migration_downtime_calc_expected(MigrationState *s);

Couldn't find the definition until I looked at the next patch :)

>  
>  /*
>   * Migration thread waiting for return path thread.  Return non-zero if an
> diff --git a/migration/migration.c b/migration/migration.c
> index ad8a824585..56d605ede9 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -984,6 +984,21 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value)
>      migrate_send_rp_message(mis, MIG_RP_MSG_RESUME_ACK, sizeof(buf), &buf);
>  }
>  
> +/*
> + * Returns the estimated switchover bandwidth (unit: bytes / seconds)
> + */
> +static double migration_get_switchover_bw(MigrationState *s)
> +{
> +    uint64_t switchover_bw = migrate_avail_switchover_bandwidth();
> +
> +    if (switchover_bw) {
> +        /* If user specified, prioritize this value and don't estimate */
> +        return (double)switchover_bw;
> +    }
> +
> +    return s->mbps / 8 * 1000 * 1000;
> +}
> +
>  bool migration_is_running(void)
>  {
>      MigrationState *s = current_migration;
> @@ -3126,37 +3141,22 @@ static void migration_update_counters(MigrationState *s,
>  {
>      uint64_t transferred, transferred_pages, time_spent;
>      uint64_t current_bytes; /* bytes transferred since the beginning */
> -    uint64_t switchover_bw;
> -    /* Expected bandwidth when switching over to destination QEMU */
> -    double expected_bw_per_ms;
> -    double bandwidth;
> +    double switchover_bw;
>  
>      if (current_time < s->iteration_start_time + BUFFER_DELAY) {
>          return;
>      }
>  
> -    switchover_bw = migrate_avail_switchover_bandwidth();
>      current_bytes = migration_transferred_bytes();
>      transferred = current_bytes - s->iteration_initial_bytes;
>      time_spent = current_time - s->iteration_start_time;
> -    bandwidth = (double)transferred / time_spent;
> -
> -    if (switchover_bw) {
> -        /*
> -         * If the user specified a switchover bandwidth, let's trust the
> -         * user so that can be more accurate than what we estimated.
> -         */
> -        expected_bw_per_ms = (double)switchover_bw / 1000;
> -    } else {
> -        /* If the user doesn't specify bandwidth, we use the estimated */
> -        expected_bw_per_ms = bandwidth;
> -    }
> -
> -    s->threshold_size = expected_bw_per_ms * migrate_downtime_limit();
> -
>      s->mbps = (((double) transferred * 8.0) /
>                 ((double) time_spent / 1000.0)) / 1000.0 / 1000.0;
>  
> +    /* NOTE: only update this after bandwidth (s->mbps) updated */
> +    switchover_bw = migration_get_switchover_bw(s) / 1000;

It isn't clear from the name, that switchover_bw is in milliseconds, I
think the _per_ms suffix would solve this.

> +    s->threshold_size = switchover_bw * migrate_downtime_limit();
> +
>      transferred_pages = ram_get_total_transferred_pages() -
>                              s->iteration_initial_pages;
>      s->pages_per_second = (double) transferred_pages /

There's also expected_bw_per_ms which should be probably replaced by
switchover_bw[_per_ms], otherwise it doesn't compile.

> @@ -3178,7 +3178,8 @@ static void migration_update_counters(MigrationState *s,
>  
>      trace_migrate_transferred(transferred, time_spent,
>                                /* Both in unit bytes/ms */
> -                              bandwidth, switchover_bw / 1000,
> +                              (uint64_t)s->mbps,
> +                              (uint64_t)switchover_bw,
>                                s->threshold_size);
>  }
>  
> -- 
> 2.50.1
> 

Except the comments above, the change looks good. With those resolved:

Reviewed-by: Juraj Marcin <jmarcin@redhat.com>