Re: [PATCH RFC 01/12] migration: Fix low possibility downtime violation

[Juraj Marcin <jmarcin@redhat.com>]

Hi Prasad,

On 2026-03-30 17:22, Prasad Pandit wrote:
> Hello Juraj,
> 
> On Fri, 27 Mar 2026 at 20:05, Juraj Marcin <jmarcin@redhat.com> wrote:
> > > * What is the 'size' difference between < s->threshold_size  Vs  <=
> > > s->threshold_size?  Going through the source IIUC
> > > 1) 'pending_size' is measured in Bytes.
> > >      static void ram_state_pending_exact/_estimate()
> > >          remaining_size = rs->migration_dirty_pages *
> > > TARGET_PAGE_SIZE(=4096 bytes);
> > >          100 dirty pages * 4096bytes  => 409600 dirty bytes => 409600
> > > * 8 => 3,276,800 dirty bits
> > >
> > > 2) 's->threshold_size' is derived from bandwidth (100M bits/s) and
> > > downtime(=300 ms)
> > >         100,000,000 bits/s => 100,000 bits/ms
> > >         100,000 bits/ms * 300ms => 30,000,000 bits in 300 ms
> > >         30,000,000 bits / 8  =>  3,750,000 Bytes / 300 ms
> > >         s->threshold_size = 30,000,000 bits (= 3.75MBytes) can be
> > > transferred in 300ms downtime.
> > >
> > > * Are we comparing pending_size(=409600 bytes)  <=
> > > s->threshold_size(=30,000,000 bits)?
> >
> > While threshold_size is indeed derived from bandwidth, bandwidth is in
> > bytes:
> >
> >     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;
> >
> > Conversion to bits only happens for the mbps statistic:
> >
> >     s->mbps = (((double) transferred * 8.0) /
> >                ((double) time_spent / 1000.0)) / 1000.0 / 1000.0;
> >
> > >
> > > *  static void migration_update_counters()
> > >         transferred = current_bytes - s->iteration_initial_bytes;
> > >         bandwidth = (double)transferred / time_spent
> > >         if (switchover_bw) {
> > >             expected_bw_per_ms = (double)switchover_bw / 1000;
> > >         } else {
> > >             expected_bw_per_ms = bandwidth;
> > >         }
> > > => ^^^^^^^  Should we divide 'bandwidth' by 1000 here (for bw_per_ms) ?
> >
> > switchover_bw is expected to be in bytes/sec, however, time_spent is
> > already in msec, thus bandwidth is also bytes/msec, the existing code is
> > correct.
> 
> * I see, this is not readily clear though. This needs good
> improvement. Maybe we should add an explanation in comments OR include
> an example calculation OR define helper function(s) to convert
> bandwidth from Mb/s <-> Mb/ms and MBps <-> Mbps etc.
> 
> >     s->mbps = (((double) transferred * 8.0) /
> >                ((double) time_spent / 1000.0)) / 1000.0 / 1000.0;
> 
> * If time_spent is in msec, continuing the above 100 mbps example, we
> got to 3,750,000 Bytes / 300 ms above. Now to get mbps
> 
>      s->mbps = (3,750,000 * 8.0)  / 300 (time_spent) bits/ms  =>
> 30,000,000 bits / 300ms
>      s->mbps = 30,000,000 bits * 1000ms (=1sec) / 300ms  =>  100,000,000 mbps.
> 
> *  (time_spent / 1000.0) / 1000.0 / 1000.0  is not very apparent.
> 
> > @Peter, not sure if it is necessary, but it could be usefull to mention
> > in MigrationParameters docs, that avail-switchover-bandwidth is in
> > bytes, not bits?
> 
> * I request that we use 'Mbps' notation for bandwidth at every user
> interface, be it --bandwidth <option> OR a definition in a
> configuration file OR documentation. Users should always specify and
> read bandwidth in 'Mbps'. Because that is the notation used
> everywhere. Asking users to specify 100,000,000 bps / 8 => 12,500,000
> Bytes/s  instead of 100Mbps does not seem right, is not user friendly.

QAPI schema defines the type of 'avail-switchover-bandwidth' and other
'*-bandwidth' properties as size, which allows users to use size suffix,
for example '100M'. QAPI parser then parses this value to
100 * 1000 * 1000 bytes.

Changing from bytes to bits is API breaking change, I don't think it is
justified.

> 
> > >
> > >       s->threshold_size = expected_bw_per_ms * migrate_downtime_limit();
> > >
> > > migration_iteration_run():
> > >    /* Should we switch to postcopy now? */
> > >    if (must_precopy <= s->threshold_size &&
> > >       can_switchover && qatomic_read(&s->start_postcopy)) {
> > >       if (postcopy_start(s, &local_err)) {
> > >           migrate_error_propagate(s, error_copy(local_err));
> > >           error_report_err(local_err);
> > >       }
> > >       return MIG_ITERATE_SKIP;
> > >    }
> > > * Here we should check pending_size <= s->threshold_size,  because
> > > must_precopy is zero(0) when postcopy is enabled. And we switch to
> > > postcopy mode even when pending_size > s->threshold_size.
> > >   I wonder if we really need both 'must_precopy' and 'can_postcopy'
> > > variables, they seem to complicate things.
> >
> > With devices that implement pending method, don't support postcopy, and
> > are not yet migrated, must_precopy would not be zero.
> 
> * If must_precopy is not zero(0), then pending_data would not be <
> threshold_size either, right? What is must_precopy data? IIUC, all
> device state is 'must_precopy' data, because we don't send Postcopy
> requests for device state data. And can_postcopy data is only RAM
> pages.
> 
> > Both, must_precopy and can_postcopy are required, that is what allows
> > postcopy to switchover early. pending_size is the overall total that
> > includes also postcopiable data, hence why it is only used to trigger
> > precopy completion.
> >
> > However, the majority of devices don't implement pending methods (yet)
> > and thus are not counted towards the estimate even if they don't support
> > postcopy and affect the downtime.
> >
> > Wondering if VMSD devices could implement some pending estimates based
> > on their defined fields, this would also improve not violating the
> > downtime requirements.
> 
> * Using 'pending_size <= threshold_size'  in one place and using
> 'must_precopy <= threshold_size' in another place is confusing and
> inconsistent.
> 
> * What is threshold_size really? Number of bits/bytes we _can_
> transfer within the given downtime, right? if downtime is 300ms, at
> 100 mbps, threshold_size is 30Mb (ie. 10Mb/100ms). For 500ms,
> threshold_size is 50Mb.

Yes, that is right.

> 
> * Then it is easy to understand that when pending_size is <=
> threshold_size, then we can easily pause the source VM and switch to
> Postcopy, becasue those pending_size bits/bytes can be transferred in
> the given downtime. And if pending_size is more, then we _can not_
> pause the source VM, because we can not transfer more pending data
> within the given downtime value.

Devices report two pending values, 'must_precopy' and 'can_postcopy'.
'must_precopy' is number of bytes that must be migrated in precopy (or
switchover) and cannot be postcopied. 'can_postcopy' is number of bytes
that can be migrated either in precopy or postcopy, those can be RAM
pages (with postcopy-ram enabled) or dirty bitmaps or any device that
would support postcopy.

If switching to postcopy is allowed, we only need to be able to transfer
'must_precopy' bytes in switchover. Rest of the pending data can be
migrated in postcopy ('can_postcopy' bytes). Thus, we only compare
'must_precopy' when deciding whether to switch to postcopy.

However, if postcopy isn't allowed, and we are deciding whether to stop
the source machine and complete the migration, we need to migrate
everything in switchover, including the data that could be postcopied.
Thus, we need to compare 'pending_size = must_precopy + can_postcopy' to
decide if we can complete the migration in the specified downtime.

> 
> 
> Thank you.
> ---
>   - Prasad
>