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

[Joao Martins]

On 01/09/2023 18:59, Joao Martins wrote:
> 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;
> 

Nevermind this part. I was wrong in the @bandwidth adjustment as it is already
calculated in bytes/ms. It's max_switchover_bandwidth that needs an adjustment
it seems.

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