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

[John Snow]

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.

Are we -hosed- ?