Re: [PATCH RFC 07/12] migration: Introduce stopcopy_bytes in save_query_pending()

[Peter Xu <peterx@redhat.com>]

On Mon, Apr 06, 2026 at 03:20:21PM +0300, Avihai Horon wrote:
> 
> On 4/2/2026 5:09 PM, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, Mar 25, 2026 at 06:54:58PM +0200, Avihai Horon wrote:
> > > On 3/20/2026 1:12 AM, Peter Xu wrote:
> > > > External email: Use caution opening links or attachments
> > > > 
> > > > 
> > > > Allow modules to report data that can only be migrated after VM is stopped.
> > > > 
> > > > When this concept is introduced, we will need to account stopcopy size to
> > > > be part of pending_size as before.
> > > > 
> > > > One thing to mention is, when there can be stopcopy size, it means the old
> > > > "pending_size" may not always be able to reach low enough to kickoff an
> > > > slow version of query sync.  While it used to be almost guaranteed to
> > > > happen because if we keep iterating, normally pending_size can go to zero
> > > > for precopy-only because we assume everything reported can be migrated in
> > > > precopy phase.
> > > > 
> > > > So we need to make sure QEMU will kickoff a synchronized version of query
> > > > pending when all precopy data is migrated too.  This might be important to
> > > > VFIO to keep making progress even if the downtime cannot yet be satisfied.
> > > > 
> > > > So far, this patch should introduce no functional change, as no module yet
> > > > report stopcopy size.
> > > > 
> > > > This will pave way for VFIO to properly report its pending data sizes,
> > > > which was actually buggy today.  Will be done in follow up patches.
> > > > 
> > > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > > ---
> > > >    include/migration/register.h | 12 +++++++++
> > > >    migration/migration.c        | 52 ++++++++++++++++++++++++++++++------
> > > >    migration/savevm.c           |  7 +++--
> > > >    migration/trace-events       |  2 +-
> > > >    4 files changed, 62 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/include/migration/register.h b/include/migration/register.h
> > > > index 2320c3a981..3824958ba5 100644
> > > > --- a/include/migration/register.h
> > > > +++ b/include/migration/register.h
> > > > @@ -17,12 +17,24 @@
> > > >    #include "hw/core/vmstate-if.h"
> > > > 
> > > >    typedef struct MigPendingData {
> > > > +    /*
> > > > +     * Modules can only update these fields in a query request via its
> > > > +     * save_query_pending() API.
> > > > +     */
> > > Move comment to patch #5?
> > > 
> > > >        /* How many bytes are pending for precopy / stopcopy? */
> > > >        uint64_t precopy_bytes;
> > > >        /* How many bytes are pending that can be transferred in postcopy? */
> > > >        uint64_t postcopy_bytes;
> > > > +    /* How many bytes that can only be transferred when VM stopped? */
> > > > +    uint64_t stopcopy_bytes;
> > > Keep consistent phrasing?
> > > 
> > > /* Amount of pending bytes that can be transferred either in precopy or
> > > stopcopy */
> > > uint64_t precopy_bytes;
> > > /* Amount of pending bytes that can be transferred in postcopy */
> > > uint64_t postcopy_bytes;
> > > /* Amount of pending bytes that can be transferred only in stopcopy */
> > > uint64_t stopcopy_bytes;
> > > 
> > > > +
> > > > +    /*
> > > > +     * Modules should never update these fields.
> > > > +     */
> > > Move comment to patch #5?
> > Sure, I'll address all above.
> > 
> > > >        /* Is this a fastpath query (which can be inaccurate)? */
> > > >        bool fastpath;
> > > > +    /* Total pending data */
> > > > +    uint64_t total_bytes;
> > > >    } MigPendingData ;
> > > > 
> > > >    /**
> > > > diff --git a/migration/migration.c b/migration/migration.c
> > > > index 99c4d09000..42facb16d1 100644
> > > > --- a/migration/migration.c
> > > > +++ b/migration/migration.c
> > > > @@ -3198,6 +3198,44 @@ typedef enum {
> > > >        MIG_ITERATE_BREAK,          /* Break the loop */
> > > >    } MigIterateState;
> > > > 
> > > > +/* Are we ready to move to the next iteration phase? */
> > > > +static bool migration_iteration_next_ready(MigrationState *s,
> > > > +                                           MigPendingData *pending)
> > > > +{
> > > > +    /*
> > > > +     * If the estimated values already suggest us to switchover, mark this
> > > > +     * iteration finished, time to do a slow sync.
> > > > +     */
> > > > +    if (pending->total_bytes <= s->threshold_size) {
> > > > +        return true;
> > > > +    }
> > > > +
> > > > +    /*
> > > > +     * Since we may have modules reporting stop-only data, we also want to
> > > > +     * re-query with slow mode if all precopy data is moved over.  This
> > > > +     * will also mark the current iteration done.
> > > > +     *
> > > > +     * This could happen when e.g. a module (like, VFIO) reports stopcopy
> > > > +     * size too large so it will never yet satisfy the downtime with the
> > > > +     * current setup (above check).  Here, slow version of re-query helps
> > > > +     * because we keep trying the best to move whatever we have.
> > > > +     */
> > > > +    if (pending->precopy_bytes == 0) {
> > > > +        return true;
> > > > +    }
> > > > +
> > > > +    return false;
> > > > +}
> > > > +
> > > > +static void migration_iteration_go_next(MigPendingData *pending)
> > > > +{
> > > > +    /*
> > > > +     * Do a slow sync will achieve this.  TODO: move RAM iteration code
> > > > +     * into the core layer.
> > > > +     */
> > > > +    qemu_savevm_query_pending(pending, false);
> > > > +}
> > > I think the iteration terminology here could be confusing, because these two
> > > functions are called from migration_iteration_run() and they don't refer to
> > > the same iteration concept.
> > > How about migration_pass_next_ready/migration_pass_go_next?
> > > Or migration_dirty_sync_ready/migration_dirty_sync?
> > I get the confusion, but IIUC the word "iteration" normally was used to
> > describe one full walk of system resources, introducing "pass" may causing
> > another way of confusion to me.
> > 
> > E.g. see Dan's post in 2016 and since then iteration was defined as so:
> > 
> > https://www.berrange.com/posts/2016/05/12/analysis-of-techniques-for-ensuring-migration-completion-with-kvm/
> > 
> > Here, migration_iteration_run() was named to say "run the current
> > iteration", but it doesn't necessarily mean it can only be run once for the
> > current iteration.
> 
> Ah, thanks, reading the code with that in mind makes it much clearer.
> 
> However, I think iteration meaning is still not consistent, as
> update_iteration_initial_status() and MigrationState->iteration_{start_time,
> initial_bytes, initial_pages} refer to iteration as the 100ms period for
> rate measurement. Or am I missing something here?

You're right, that was indeed confusing and maybe some rename on the
variables can help the code provide consistent definition of iterations.

It'll still be tricky though as the word "iterate" should still be fine
when used to walk over the resources, which may not be once per iteration.

> 
> Also the MIG_ITERATE_{SKIP,BREAK,RESUME} return value of
> migration_iteration_run() (and their docs) may be confusing, as it makes
> "iteration" more related to the while loop.
> I just skimmed briefly, but can we make migration_iteration_run() return a
> bool? (I don't think SKIP value is really needed?)
> BTW, while at it I noticed that the migration_iteration_run() doc is stale
> [1].
> 
> [1] https://elixir.bootlin.com/qemu/v11.0.0-rc2/source/migration/migration.c#L3202

True.

> 
> > 
> > OTOH, I don't think it makes much sense we define "iteration" (or any new
> > term at all) to be one run of migration_iteration_run(): currently, we
> > invoke handlers' savevm iterator hook for each migration_iteration_run(),
> > but that's pretty random behavior..
> > 
> > For RAM, that's MAX_WAIT.  For VFIO, it's something else...
> > 
> > It was still a "grey area" and no way to define, even if we may want to
> > define it at some point to make each iterator to follow some rule.
> > 
> > Comparing to that, iteration to be defined to describe one round of walk
> > for all VM resources makes a lot of sense and it's a criticla concept for
> > precopy.
> > 
> Yes, I agree. I guess some refactoring of the migration_iteration_run() area
> could make it clearer (but not necessary as part of this series). Thanks.

I'll add a todo to cleanup these part after this series.

Thanks,

-- 
Peter Xu