Re: [Qemu-devel] [PATCH v10 05/12] migration: introduce postcopy-only pending

[Dr. David Alan Gilbert]

* John Snow (address@hidden) wrote:
> 
> 
> On 03/12/2018 11:30 AM, Dr. David Alan Gilbert wrote:
> > * Vladimir Sementsov-Ogievskiy (address@hidden) wrote:
> >> There would be savevm states (dirty-bitmap) which can migrate only in
> >> postcopy stage. The corresponding pending is introduced here.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> >> ---
> >>  include/migration/register.h | 17 +++++++++++++++--
> >>  migration/savevm.h           |  5 +++--
> >>  hw/s390x/s390-stattrib.c     |  7 ++++---
> >>  migration/block.c            |  7 ++++---
> >>  migration/migration.c        | 16 +++++++++-------
> >>  migration/ram.c              |  9 +++++----
> >>  migration/savevm.c           | 13 ++++++++-----
> >>  migration/trace-events       |  2 +-
> >>  8 files changed, 49 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/include/migration/register.h b/include/migration/register.h
> >> index f4f7bdc177..9436a87678 100644
> >> --- a/include/migration/register.h
> >> +++ b/include/migration/register.h
> >> @@ -37,8 +37,21 @@ typedef struct SaveVMHandlers {
> >>      int (*save_setup)(QEMUFile *f, void *opaque);
> >>      void (*save_live_pending)(QEMUFile *f, void *opaque,
> >>                                uint64_t threshold_size,
> >> -                              uint64_t *non_postcopiable_pending,
> >> -                              uint64_t *postcopiable_pending);
> >> +                              uint64_t *res_precopy_only,
> >> +                              uint64_t *res_compatible,
> >> +                              uint64_t *res_postcopy_only);
> >> +    /* Note for save_live_pending:
> >> +     * - res_precopy_only is for data which must be migrated in precopy 
> >> phase
> >> +     *     or in stopped state, in other words - before target vm start
> >> +     * - res_compatible is for data which may be migrated in any phase
> >> +     * - res_postcopy_only is for data which must be migrated in postcopy 
> >> phase
> >> +     *     or in stopped state, in other words - after source vm stop
> >> +     *
> >> +     * Sum of res_postcopy_only, res_compatible and res_postcopy_only is 
> >> the
> >> +     * whole amount of pending data.
> >> +     */
> >> +
> >> +
> >>      LoadStateHandler *load_state;
> >>      int (*load_setup)(QEMUFile *f, void *opaque);
> >>      int (*load_cleanup)(void *opaque);
> >> diff --git a/migration/savevm.h b/migration/savevm.h
> >> index 295c4a1f2c..cf4f0d37ca 100644
> >> --- a/migration/savevm.h
> >> +++ b/migration/savevm.h
> >> @@ -38,8 +38,9 @@ void qemu_savevm_state_complete_postcopy(QEMUFile *f);
> >>  int qemu_savevm_state_complete_precopy(QEMUFile *f, bool iterable_only,
> >>                                         bool inactivate_disks);
> >>  void qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size,
> >> -                               uint64_t *res_non_postcopiable,
> >> -                               uint64_t *res_postcopiable);
> >> +                               uint64_t *res_precopy_only,
> >> +                               uint64_t *res_compatible,
> >> +                               uint64_t *res_postcopy_only);
> >>  void qemu_savevm_send_ping(QEMUFile *f, uint32_t value);
> >>  void qemu_savevm_send_open_return_path(QEMUFile *f);
> >>  int qemu_savevm_send_packaged(QEMUFile *f, const uint8_t *buf, size_t 
> >> len);
> >> diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
> >> index 2902f54f11..dd3fbfd1eb 100644
> >> --- a/hw/s390x/s390-stattrib.c
> >> +++ b/hw/s390x/s390-stattrib.c
> >> @@ -183,15 +183,16 @@ static int cmma_save_setup(QEMUFile *f, void *opaque)
> >>  }
> >>  
> >>  static void cmma_save_pending(QEMUFile *f, void *opaque, uint64_t 
> >> max_size,
> >> -                             uint64_t *non_postcopiable_pending,
> >> -                             uint64_t *postcopiable_pending)
> >> +                              uint64_t *res_precopy_only,
> >> +                              uint64_t *res_compatible,
> >> +                              uint64_t *res_postcopy_only)
> >>  {
> >>      S390StAttribState *sas = S390_STATTRIB(opaque);
> >>      S390StAttribClass *sac = S390_STATTRIB_GET_CLASS(sas);
> >>      long long res = sac->get_dirtycount(sas);
> >>  
> >>      if (res >= 0) {
> >> -        *non_postcopiable_pending += res;
> >> +        *res_precopy_only += res;
> >>      }
> >>  }
> >>  
> >> diff --git a/migration/block.c b/migration/block.c
> >> index 1f03946797..5652ca3337 100644
> >> --- a/migration/block.c
> >> +++ b/migration/block.c
> >> @@ -866,8 +866,9 @@ static int block_save_complete(QEMUFile *f, void 
> >> *opaque)
> >>  }
> >>  
> >>  static void block_save_pending(QEMUFile *f, void *opaque, uint64_t 
> >> max_size,
> >> -                               uint64_t *non_postcopiable_pending,
> >> -                               uint64_t *postcopiable_pending)
> >> +                               uint64_t *res_precopy_only,
> >> +                               uint64_t *res_compatible,
> >> +                               uint64_t *res_postcopy_only)
> >>  {
> >>      /* Estimate pending number of bytes to send */
> >>      uint64_t pending;
> >> @@ -888,7 +889,7 @@ static void block_save_pending(QEMUFile *f, void 
> >> *opaque, uint64_t max_size,
> >>  
> >>      DPRINTF("Enter save live pending  %" PRIu64 "\n", pending);
> >>      /* We don't do postcopy */
> >> -    *non_postcopiable_pending += pending;
> >> +    *res_precopy_only += pending;
> >>  }
> >>  
> >>  static int block_load(QEMUFile *f, void *opaque, int version_id)
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index c99a4e62d7..3beedd676e 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -2215,21 +2215,23 @@ typedef enum {
> >>   */
> >>  static MigIterateState migration_iteration_run(MigrationState *s)
> >>  {
> >> -    uint64_t pending_size, pend_post, pend_nonpost;
> >> +    uint64_t pending_size, pend_pre, pend_compat, pend_post;
> >>      bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
> >>  
> >> -    qemu_savevm_state_pending(s->to_dst_file, s->threshold_size,
> >> -                              &pend_nonpost, &pend_post);
> >> -    pending_size = pend_nonpost + pend_post;
> >> +    qemu_savevm_state_pending(s->to_dst_file, s->threshold_size, 
> >> &pend_pre,
> >> +                              &pend_compat, &pend_post);
> >> +    pending_size = pend_pre + pend_compat + pend_post;
> >>  
> >>      trace_migrate_pending(pending_size, s->threshold_size,
> >> -                          pend_post, pend_nonpost);
> >> +                          pend_pre, pend_compat, pend_post);
> >>  
> >>      if (pending_size && pending_size >= s->threshold_size) {
> >>          /* Still a significant amount to transfer */
> >>          if (migrate_postcopy() && !in_postcopy &&
> >> -            pend_nonpost <= s->threshold_size &&
> >> -            atomic_read(&s->start_postcopy)) {
> >> +            pend_pre <= s->threshold_size &&
> >> +            (atomic_read(&s->start_postcopy) ||
> >> +             (pend_pre + pend_compat <= s->threshold_size)))
> > 
> > This change does something different from the description;
> > it causes a postcopy_start even if the user never ran the postcopy-start
> > command; so sorry, we can't do that; because postcopy for RAM is
> > something that users can enable but only switch into when they've given
> > up on it completing normally.
> > 
> > However, I guess that leaves you with a problem; which is what happens
> > to the system when you've run out of pend_pre+pend_compat but can't
> > complete because pend_post is non-0; so I don't know the answer to that.
> > 
> > Dave
> > 
> 
> I'm willing to pull the bitmap related changes into my tree for 2.12,
> but it would be _really_ nice to have this patchset in 2.12 -- is there
> anything we can do to get this into the migration queue?
> 
> I'm afraid I don't know what the right answer here is either, so I can't
> fix this up myself and I imagine Vladimir will need some feedback from
> you to change this design.

Well I'm good with everything except the change in semantics for
existing migration;  we can't do that because it can break existing users.


> Are we -hosed- ?

Dave
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK