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

[Prasad Pandit <ppandit@redhat.com>]

On Fri, 20 Mar 2026 at 04:45, Peter Xu <peterx@redhat.com> wrote:
> Add a helper to be able to return an estimate of switchover bw, called

* Add a helper to return ...
* bw -> bandwidth

> migration_get_switchover_bw(). Use it to sligitly simplify the current
> code.

* sligitly -> slightly ;  (Better still: use it to simplify ...)

* This simplification is nice and required.

> 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);

* Let's return unsigned int64 -> uint64_t  here? Downtime should be
positive OR does it return a negative error code?

* This function is not used in this patch, it is unrelated that way.
Let's add it in the patch where it is used?

> --- a/migration/migration.c
> +++ b/migration/migration.c
> +/*
> + * 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;
> +}
> +

* 100 mbps will return => 100 / 8 * 1000 * 1000 => 12,500,000 bytes/sec

* When user specifies bandwidth with
        virsh migrate --available-switchover-bandwidth <number>
bandwidth (in MiB/s) available for the final phase of migration

* Asking users to specify such large number for bytes/sec is not user
friendly. 100mbps OR 156mbps OR 400mbps would have been better, then
above function could do

     if (!switchover_bw) {
         switchover_bw = s->mbps;
     }
     return switchover_bw / 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;

* We got 12.5 MB/sec return value, divide it by 1000 to get bytes/ms
     ie. 12500000 / 1000  ==> 12500 bytes/ms

* It'll help to describe an example calculation in comments around
this math. Instead of dividing by 1000 here, why not make
migration_get_switchover_bw() return a bytes/ms value? That way the
actual calculation and the sample calculation in comments can go in
that function.

> +    s->threshold_size = switchover_bw * migrate_downtime_limit();
> +
           s->threshold_size = 12500 bytes/ms * 300ms  =>  3750000 bytes / 300ms

* IIUC, when pending_size  <=  s->threshold_size(=3.75MB), we can
pause the source VM and switch to the Postcopy phase, right?

>      transferred_pages = ram_get_total_transferred_pages() -
>                              s->iteration_initial_pages;
>      s->pages_per_second = (double) transferred_pages /
> @@ -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);

* Here 's->mbps' is not in bytes/ms unit, is it?

Thank you.
---
  - Prasad