Re: [PATCH RFC 05/12] migration/treewide: Merge @state_pending_{exact|estimate} APIs

[Peter Xu <peterx@redhat.com>]

On Tue, Mar 24, 2026 at 04:05:00PM +0530, Prasad Pandit wrote:
> > +typedef struct MigPendingData {
> > +    /* 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;
> > +    /* Is this a fastpath query (which can be inaccurate)? */
> > +    bool fastpath;
> > +} MigPendingData ;
> > +
> 
> * Pending precopy bytes are: bytes still pending to be sent + dirty
> (updated) bytes; How do we measure/estimate pending postcopy bytes?
> 
> * Could we do away with separating pending bytes into Precopy and
> Postcopy bytes? Instead go with just pending_bytes on the source side?
> 
> * Let's not add another term/variable called stopcopy. It'll only add
> to the confusion.

What would you suggest to solve this problem?

> 
> 
> >  /**
> >   * struct SaveVMHandlers: handler structure to finely control
> >   * migration of complex subsystems and devices, such as RAM, block and
> > @@ -197,46 +206,17 @@ typedef struct SaveVMHandlers {
> >      bool (*save_postcopy_prepare)(QEMUFile *f, void *opaque, Error **errp);
> >
> >      /**
> > -     * @state_pending_estimate
> > -     *
> > -     * This estimates the remaining data to transfer
> > -     *
> > -     * Sum of @can_postcopy and @must_postcopy is the whole amount of
> > -     * pending data.
> > -     *
> > -     * @opaque: data pointer passed to register_savevm_live()
> > -     * @must_precopy: amount of data that must be migrated in precopy
> > -     *                or in stopped state, i.e. that must be migrated
> > -     *                before target start.
> > -     * @can_postcopy: amount of data that can be migrated in postcopy
> > -     *                or in stopped state, i.e. after target start.
> > -     *                Some can also be migrated during precopy (RAM).
> > -     *                Some must be migrated after source stops
> > -     *                (block-dirty-bitmap)
> > -     */
> > -    void (*state_pending_estimate)(void *opaque, uint64_t *must_precopy,
> > -                                   uint64_t *can_postcopy);
> > -
> > -    /**
> > -     * @state_pending_exact
> > -     *
> > -     * This calculates the exact remaining data to transfer
> > +     * @save_query_pending
> >       *
> > -     * Sum of @can_postcopy and @must_postcopy is the whole amount of
> > -     * pending data.
> > +     * This estimates the remaining data to transfer on the source side.
> > +     * It's highly suggested that the module should implement both fastpath
> > +     * and slowpath version of it when it can be slow (for more information
> > +     * please check pending->fastpath field).
> >       *
> >       * @opaque: data pointer passed to register_savevm_live()
> > -     * @must_precopy: amount of data that must be migrated in precopy
> > -     *                or in stopped state, i.e. that must be migrated
> > -     *                before target start.
> > -     * @can_postcopy: amount of data that can be migrated in postcopy
> > -     *                or in stopped state, i.e. after target start.
> > -     *                Some can also be migrated during precopy (RAM).
> > -     *                Some must be migrated after source stops
> > -     *                (block-dirty-bitmap)
> > +     * @pending: pointer to a MigPendingData struct
> >       */
> > -    void (*state_pending_exact)(void *opaque, uint64_t *must_precopy,
> > -                                uint64_t *can_postcopy);
> > +    void (*save_query_pending)(void *opaque, MigPendingData *pending);
> >
> >      /**
> >       * @load_state
> > diff --git a/migration/savevm.h b/migration/savevm.h
> > index b3d1e8a13c..b116933bce 100644
> > --- a/migration/savevm.h
> > +++ b/migration/savevm.h
> > @@ -14,6 +14,8 @@
> >  #ifndef MIGRATION_SAVEVM_H
> >  #define MIGRATION_SAVEVM_H
> >
> > +#include "migration/register.h"
> > +
> >  #define QEMU_VM_FILE_MAGIC           0x5145564d
> >  #define QEMU_VM_FILE_VERSION_COMPAT  0x00000002
> >  #define QEMU_VM_FILE_VERSION         0x00000003
> > @@ -43,6 +45,7 @@ int qemu_savevm_state_iterate(QEMUFile *f, bool postcopy);
> >  void qemu_savevm_state_cleanup(void);
> >  void qemu_savevm_state_complete_postcopy(QEMUFile *f);
> >  int qemu_savevm_state_complete_precopy(MigrationState *s);
> > +void qemu_savevm_query_pending(MigPendingData *pending, bool fastpath);
> >  void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
> >                                       uint64_t *can_postcopy);
> >  void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
> > diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> > index d808ece3b9..b1ec51c77a 100644
> > --- a/hw/s390x/s390-stattrib.c
> > +++ b/hw/s390x/s390-stattrib.c
> > @@ -187,15 +187,14 @@ static int cmma_save_setup(QEMUFile *f, void *opaque, Error **errp)
> >      return 0;
> >  }
> >
> > -static void cmma_state_pending(void *opaque, uint64_t *must_precopy,
> > -                               uint64_t *can_postcopy)
> > +static void cmma_state_pending(void *opaque, MigPendingData *pending)
> >  {
> >      S390StAttribState *sas = S390_STATTRIB(opaque);
> >      S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
> >      long long res = sac->get_dirtycount(sas);
> >
> >      if (res >= 0) {
> > -        *must_precopy += res;
> > +        pending->precopy_bytes += res;
> >      }
> >  }
> >
> > @@ -340,8 +339,7 @@ static SaveVMHandlers savevm_s390_stattrib_handlers = {
> >      .save_setup = cmma_save_setup,
> >      .save_live_iterate = cmma_save_iterate,
> >      .save_complete = cmma_save_complete,
> > -    .state_pending_exact = cmma_state_pending,
> > -    .state_pending_estimate = cmma_state_pending,
> > +    .save_query_pending = cmma_state_pending,
> >      .save_cleanup = cmma_save_cleanup,
> >      .load_state = cmma_load,
> >      .is_active = cmma_active,
> > diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> > index 827d3ded63..c054c749b0 100644
> > --- a/hw/vfio/migration.c
> > +++ b/hw/vfio/migration.c
> > @@ -570,42 +570,51 @@ static void vfio_save_cleanup(void *opaque)
> >      trace_vfio_save_cleanup(vbasedev->name);
> >  }
> >
> > -static void vfio_state_pending_estimate(void *opaque, uint64_t *must_precopy,
> > -                                        uint64_t *can_postcopy)
> > +static void vfio_state_pending_sync(VFIODevice *vbasedev)
> >  {
> > -    VFIODevice *vbasedev = opaque;
> >      VFIOMigration *migration = vbasedev->migration;
> >
> > -    if (!vfio_device_state_is_precopy(vbasedev)) {
> > -        return;
> > -    }
> > +    vfio_query_stop_copy_size(vbasedev);
> >
> > -    *must_precopy +=
> > -        migration->precopy_init_size + migration->precopy_dirty_size;
> > +    if (vfio_device_state_is_precopy(vbasedev)) {
> > +        vfio_query_precopy_size(migration);
> > +    }
> >
> > -    trace_vfio_state_pending_estimate(vbasedev->name, *must_precopy,
> > -                                      *can_postcopy,
> > -                                      migration->precopy_init_size,
> > -                                      migration->precopy_dirty_size);
> > +    /*
> > +     * In all cases, all PRECOPY data should be no more than STOPCOPY data.
> > +     * Otherwise we have a problem.  So far, only dump some errors.
> > +     */
> > +    if (migration->precopy_init_size + migration->precopy_dirty_size <
> > +        migration->stopcopy_size) {
> > +        error_report_once("%s: wrong pending data (init=%" PRIx64
> > +                          ", dirty=%"PRIx64", stop=%"PRIx64")",
> > +                          __func__, migration->precopy_init_size,
> > +                          migration->precopy_dirty_size,
> > +                          migration->stopcopy_size);
> > +    }
> >  }
> >
> > -static void vfio_state_pending_exact(void *opaque, uint64_t *must_precopy,
> > -                                     uint64_t *can_postcopy)
> > +static void vfio_state_pending(void *opaque, MigPendingData *pending)
> >  {
> >      VFIODevice *vbasedev = opaque;
> >      VFIOMigration *migration = vbasedev->migration;
> > +    uint64_t remain;
> >
> > -    vfio_query_stop_copy_size(vbasedev);
> > -    *must_precopy += migration->stopcopy_size;
> > -
> > -    if (vfio_device_state_is_precopy(vbasedev)) {
> > -        vfio_query_precopy_size(migration);
> > +    if (pending->fastpath) {
> > +        if (!vfio_device_state_is_precopy(vbasedev)) {
> > +            return;
> > +        }
> > +        remain = migration->precopy_init_size + migration->precopy_dirty_size;
> > +    } else {
> > +        vfio_state_pending_sync(vbasedev);
> > +        remain = migration->stopcopy_size;
> >      }
> >
> > -    trace_vfio_state_pending_exact(vbasedev->name, *must_precopy, *can_postcopy,
> > -                                   migration->stopcopy_size,
> > -                                   migration->precopy_init_size,
> > -                                   migration->precopy_dirty_size);
> > +    pending->precopy_bytes += remain;
> > +
> > +    trace_vfio_state_pending(vbasedev->name, migration->stopcopy_size,
> > +                             migration->precopy_init_size,
> > +                             migration->precopy_dirty_size);
> >  }
> >
> >  static bool vfio_is_active_iterate(void *opaque)
> > @@ -850,8 +859,7 @@ static const SaveVMHandlers savevm_vfio_handlers = {
> >      .save_prepare = vfio_save_prepare,
> >      .save_setup = vfio_save_setup,
> >      .save_cleanup = vfio_save_cleanup,
> > -    .state_pending_estimate = vfio_state_pending_estimate,
> > -    .state_pending_exact = vfio_state_pending_exact,
> > +    .save_query_pending = vfio_state_pending,
> >      .is_active_iterate = vfio_is_active_iterate,
> >      .save_live_iterate = vfio_save_iterate,
> >      .save_complete = vfio_save_complete_precopy,
> > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> > index a061aad817..376a9b43ac 100644
> > --- a/migration/block-dirty-bitmap.c
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -766,9 +766,7 @@ static int dirty_bitmap_save_complete(QEMUFile *f, void *opaque)
> >      return 0;
> >  }
> >
> > -static void dirty_bitmap_state_pending(void *opaque,
> > -                                       uint64_t *must_precopy,
> > -                                       uint64_t *can_postcopy)
> > +static void dirty_bitmap_state_pending(void *opaque, MigPendingData *data)
> >  {
> >      DBMSaveState *s = &((DBMState *)opaque)->save;
> >      SaveBitmapState *dbms;
> > @@ -788,7 +786,7 @@ static void dirty_bitmap_state_pending(void *opaque,
> >
> >      trace_dirty_bitmap_state_pending(pending);
> >
> > -    *can_postcopy += pending;
> > +    data->postcopy_bytes += pending;
> >  }
> >
> >  /* First occurrence of this bitmap. It should be created if doesn't exist */
> > @@ -1250,8 +1248,7 @@ static SaveVMHandlers savevm_dirty_bitmap_handlers = {
> >      .save_setup = dirty_bitmap_save_setup,
> >      .save_complete = dirty_bitmap_save_complete,
> >      .has_postcopy = dirty_bitmap_has_postcopy,
> > -    .state_pending_exact = dirty_bitmap_state_pending,
> > -    .state_pending_estimate = dirty_bitmap_state_pending,
> > +    .save_query_pending = dirty_bitmap_state_pending,
> >      .save_live_iterate = dirty_bitmap_save_iterate,
> >      .is_active_iterate = dirty_bitmap_is_active_iterate,
> >      .load_state = dirty_bitmap_load,
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 979751f61b..89f761a471 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -3443,30 +3443,17 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
> >      return qemu_fflush(f);
> >  }
> >
> > -static void ram_state_pending_estimate(void *opaque, uint64_t *must_precopy,
> > -                                       uint64_t *can_postcopy)
> > -{
> > -    RAMState **temp = opaque;
> > -    RAMState *rs = *temp;
> > -
> > -    uint64_t remaining_size = rs->migration_dirty_pages * TARGET_PAGE_SIZE;
> > -
> > -    if (migrate_postcopy_ram()) {
> > -        /* We can do postcopy, and all the data is postcopiable */
> > -        *can_postcopy += remaining_size;
> > -    } else {
> > -        *must_precopy += remaining_size;
> > -    }
> > -}
> > -
> > -static void ram_state_pending_exact(void *opaque, uint64_t *must_precopy,
> > -                                    uint64_t *can_postcopy)
> > +static void ram_state_pending(void *opaque, MigPendingData *pending)
> >  {
> >      RAMState **temp = opaque;
> >      RAMState *rs = *temp;
> >      uint64_t remaining_size;
> >
> > -    if (!migration_in_postcopy()) {
> > +    /*
> > +     * Sync is only needed either with: (1) a fast query, or (2) postcopy
> > +     * as started (in which case no new dirty will generate anymore).
> 
> * as -> has
> * (... no new dirty will generate anymore)  - ...?
> 
> > +     */
> > +    if (!pending->fastpath && !migration_in_postcopy()) {
> 
> * The comment above and 'if' conditionals don't seem to match. We are
> saying a call to 'migration_bitmap_sync_precopy()' is needed either
> with fast query ie. 'pending->fastpath = true'  OR Postcopy has
> started ie. migration_in_postcopy() = true, right? What is the
> right/intended behaviour here?

It should have been s/only/not/.. I'll update comment to:

    /*
     * Sync is not needed either with: (1) a fast query, or (2) after
     * postcopy has started (no new dirty will generate anymore).
     */

> 
> >          bql_lock();
> >          WITH_RCU_READ_LOCK_GUARD() {
> >              migration_bitmap_sync_precopy(false);
> > @@ -3478,9 +3465,9 @@ static void ram_state_pending_exact(void *opaque, uint64_t *must_precopy,
> >
> >      if (migrate_postcopy_ram()) {
> >          /* We can do postcopy, and all the data is postcopiable */
> > -        *can_postcopy += remaining_size;
> > +        pending->postcopy_bytes += remaining_size;
> >      } else {
> > -        *must_precopy += remaining_size;
> > +        pending->precopy_bytes += remaining_size;
> >      }
> >  }
> >
> > @@ -4703,8 +4690,7 @@ static SaveVMHandlers savevm_ram_handlers = {
> >      .save_live_iterate = ram_save_iterate,
> >      .save_complete = ram_save_complete,
> >      .has_postcopy = ram_has_postcopy,
> > -    .state_pending_exact = ram_state_pending_exact,
> > -    .state_pending_estimate = ram_state_pending_estimate,
> > +    .save_query_pending = ram_state_pending,
> >      .load_state = ram_load,
> >      .save_cleanup = ram_save_cleanup,
> >      .load_setup = ram_load_setup,
> > diff --git a/migration/savevm.c b/migration/savevm.c
> > index dd58f2a705..6268e68382 100644
> > --- a/migration/savevm.c
> > +++ b/migration/savevm.c
> > @@ -1762,46 +1762,45 @@ int qemu_savevm_state_complete_precopy(MigrationState *s)
> >      return qemu_fflush(f);
> >  }
> >
> > -/* Give an estimate of the amount left to be transferred,
> > - * the result is split into the amount for units that can and
> > - * for units that can't do postcopy.
> > - */
> > -void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
> > -                                        uint64_t *can_postcopy)
> > +void qemu_savevm_query_pending(MigPendingData *pending, bool fastpath)
> >  {
> >      SaveStateEntry *se;
> >
> > -    *must_precopy = 0;
> > -    *can_postcopy = 0;
> > +    pending->precopy_bytes = 0;
> > +    pending->postcopy_bytes = 0;
> > +    pending->fastpath = fastpath;
> >
> >      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> > -        if (!se->ops || !se->ops->state_pending_estimate) {
> > +        if (!se->ops || !se->ops->save_query_pending) {
> >              continue;
> >          }
> >          if (!qemu_savevm_state_active(se)) {
> >              continue;
> >          }
> > -        se->ops->state_pending_estimate(se->opaque, must_precopy, can_postcopy);
> > +        se->ops->save_query_pending(se->opaque, pending);
> >      }
> >  }
> >
> > +void qemu_savevm_state_pending_estimate(uint64_t *must_precopy,
> > +                                        uint64_t *can_postcopy)
> > +{
> > +    MigPendingData pending;
> > +
> > +    qemu_savevm_query_pending(&pending, true);
> > +
> > +    *must_precopy = pending.precopy_bytes;
> > +    *can_postcopy = pending.postcopy_bytes;
> > +}
> > +
> >  void qemu_savevm_state_pending_exact(uint64_t *must_precopy,
> >                                       uint64_t *can_postcopy)
> >  {
> > -    SaveStateEntry *se;
> > +    MigPendingData pending;
> >
> > -    *must_precopy = 0;
> > -    *can_postcopy = 0;
> > +    qemu_savevm_query_pending(&pending, false);
> >
> > -    QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
> > -        if (!se->ops || !se->ops->state_pending_exact) {
> > -            continue;
> > -        }
> > -        if (!qemu_savevm_state_active(se)) {
> > -            continue;
> > -        }
> > -        se->ops->state_pending_exact(se->opaque, must_precopy, can_postcopy);
> > -    }
> > +    *must_precopy = pending.precopy_bytes;
> > +    *can_postcopy = pending.postcopy_bytes;
> >  }
> 
> * IIUC, we send fastpath=true from _pending_estimate() because we want
> to avoid call to migration_bitmap_sync_precopy(), as calling it won't
> be fast. And we send fastpath=false from _pending_exact() function
> because we want to trigger call to migration_bitmap_sync_precopy()
> routine.
> 
> * Instead of calling the boolean parameter 'fastpath',  let's use
> 'bool use_precision'  OR 'bool sync_data'. And then in
> qemu_savevm_query_pending()
> 
>     qemu_savevm_query_pending():
>             if (use_precision || sync_data) {
>                      migration_bitmap_sync_precopy()
>             }
> 
> * When reading code, equating fastpath with 'estimation' and slowpath
> with 'precision/accuracy' is unnatural.

I'll use "exact" as suggested by Juraj.

Thanks,

-- 
Peter Xu