Re: [PATCH for-8.2 v2 2/2] migration: Allow user to specify migration switchover bandwidth

[Joao Martins <joao.m.martins@oracle.com>]

On 03/08/2023 16:53, Peter Xu wrote:
> @@ -2694,7 +2694,17 @@ static void migration_update_counters(MigrationState *s,
>      transferred = current_bytes - s->iteration_initial_bytes;
>      time_spent = current_time - s->iteration_start_time;
>      bandwidth = (double)transferred / time_spent;
> -    s->threshold_size = bandwidth * migrate_downtime_limit();
> +    if (migrate_max_switchover_bandwidth()) {
> +        /*
> +         * If the user specified an available bandwidth, let's trust the
> +         * user so that can be more accurate than what we estimated.
> +         */
> +        avail_bw = migrate_max_switchover_bandwidth();
> +    } else {
> +        /* If the user doesn't specify bandwidth, we use the estimated */
> +        avail_bw = bandwidth;
> +    }
> +    s->threshold_size = avail_bw * migrate_downtime_limit();
>  

[ sorry for giving review comments in piecemeal :/ ]

There might be something odd with the calculation. It would be right if
downtime_limit was in seconds. But we are multipling a value that is in
bytes/sec with a time unit that is in miliseconds. When avail_bw is set to
switchover_bandwidth, it sounds to me this should be a:

	/* bytes/msec; @max-switchover-bandwidth is per-seconds */
	avail_bw = migrate_max_switchover_bandwidth() / 1000.0;

Otherwise it looks like that we end up overestimating how much we can still send
during switchover? If this is correct and I am not missing some assumption, then
same is applicable to the threshold_size calculation in general without
switchover-bandwidth but likely in a different way:

	/* bytes/msec; but @bandwidth is calculated in 100msec quantas */
	avail_bw = bandwidth / 100.0;

There's a very good chance I'm missing details, so apologies beforehand on
wasting your time if I didn't pick up on it through the code.

	Joao