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

[Peter Xu <peterx@redhat.com>]

On Fri, Sep 01, 2023 at 07:39:07PM +0100, Joao Martins wrote:
> 
> 
> 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 :/ ]

This is never a problem.

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

My fault, thanks for catching this.  So it seems even if the test will
switchover with this patch, it might be too aggresive if we calculate with
a number 1000x larger than the real bandwidth provided..

I'll rename this to expected_bw_per_ms to be clear when repost, too.

Thanks,

-- 
Peter Xu