Re: [PATCH v1 3/7] qapi/migration: Introduce the iteration-count

[Fabiano Rosas <farosas@suse.de>]

Hyman Huang <yong.huang@smartx.com> writes:

> The original migration information dirty-sync-count could
> no longer reflect iteration count due to the introduction
> of background synchronization in the next commit;
> add the iteration count to compensate.

I agree with the overall idea, but I feel we're lacking some information
on what determines whether some of the lines below want to use the
iteration count vs. the dirty sync count. Since this patch increments
both variables at the same place, they can still be used interchangeably
unless we add some words to explain the distinction.

So to clarify: 

What do we call an iteration? A call to save_live_iterate(),
migration_iteration_run() or something else?

Why dirty-sync-count should ever have reflected "iteration count"? It
might have been this way by coincidence, but did we ever used it in that
sense (aside from info migrate maybe)?

With the new counter, what kind of meaning can a user extract from that
number aside from "some undescribed thing happened N times" (this might
be included in the migration.json docs)?

>
> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> ---
>  migration/migration-stats.h  |  4 ++++
>  migration/migration.c        |  1 +
>  migration/ram.c              | 12 ++++++++----
>  qapi/migration.json          |  6 +++++-
>  tests/qtest/migration-test.c |  2 +-
>  5 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/migration/migration-stats.h b/migration/migration-stats.h
> index 05290ade76..43ee0f4f05 100644
> --- a/migration/migration-stats.h
> +++ b/migration/migration-stats.h
> @@ -50,6 +50,10 @@ typedef struct {
>       * Number of times we have synchronized guest bitmaps.
>       */
>      Stat64 dirty_sync_count;
> +    /*
> +     * Number of migration iteration processed.
> +     */
> +    Stat64 iteration_count;
>      /*
>       * Number of times zero copy failed to send any page using zero
>       * copy.
> diff --git a/migration/migration.c b/migration/migration.c
> index 3dea06d577..055d527ff6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1197,6 +1197,7 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
>      info->ram->mbps = s->mbps;
>      info->ram->dirty_sync_count =
>          stat64_get(&mig_stats.dirty_sync_count);
> +    info->ram->iteration_count = stat64_get(&mig_stats.iteration_count);
>      info->ram->dirty_sync_missed_zero_copy =
>          stat64_get(&mig_stats.dirty_sync_missed_zero_copy);
>      info->ram->postcopy_requests =
> diff --git a/migration/ram.c b/migration/ram.c
> index e205806a5f..ca5a1b5f16 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -594,7 +594,7 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
>      /* We don't care if this fails to allocate a new cache page
>       * as long as it updated an old one */
>      cache_insert(XBZRLE.cache, current_addr, XBZRLE.zero_target_page,
> -                 stat64_get(&mig_stats.dirty_sync_count));
> +                 stat64_get(&mig_stats.iteration_count));
>  }
>  
>  #define ENCODING_FLAG_XBZRLE 0x1
> @@ -620,7 +620,7 @@ static int save_xbzrle_page(RAMState *rs, PageSearchStatus *pss,
>      int encoded_len = 0, bytes_xbzrle;
>      uint8_t *prev_cached_page;
>      QEMUFile *file = pss->pss_channel;
> -    uint64_t generation = stat64_get(&mig_stats.dirty_sync_count);
> +    uint64_t generation = stat64_get(&mig_stats.iteration_count);
>  
>      if (!cache_is_cached(XBZRLE.cache, current_addr, generation)) {
>          xbzrle_counters.cache_miss++;
> @@ -1079,6 +1079,10 @@ static void migration_bitmap_sync(RAMState *rs,
>      RAMBlock *block;
>      int64_t end_time;
>  
> +    if (!background) {
> +        stat64_add(&mig_stats.iteration_count, 1);
> +    }
> +
>      stat64_add(&mig_stats.dirty_sync_count, 1);
>  
>      if (!rs->time_last_bitmap_sync) {
> @@ -1115,8 +1119,8 @@ static void migration_bitmap_sync(RAMState *rs,
>          rs->num_dirty_pages_period = 0;
>          rs->bytes_xfer_prev = migration_transferred_bytes();
>      }
> -    if (migrate_events()) {
> -        uint64_t generation = stat64_get(&mig_stats.dirty_sync_count);
> +    if (!background && migrate_events()) {
> +        uint64_t generation = stat64_get(&mig_stats.iteration_count);
>          qapi_event_send_migration_pass(generation);
>      }
>  }
> diff --git a/qapi/migration.json b/qapi/migration.json
> index b66cccf107..95b490706c 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -60,6 +60,9 @@
>  #     between 0 and @dirty-sync-count * @multifd-channels.  (since
>  #     7.1)
>  #
> +# @iteration-count: The number of iterations since migration started.
> +#     (since 9.2)
> +#
>  # Since: 0.14
>  ##
>  { 'struct': 'MigrationStats',
> @@ -72,7 +75,8 @@
>             'multifd-bytes': 'uint64', 'pages-per-second': 'uint64',
>             'precopy-bytes': 'uint64', 'downtime-bytes': 'uint64',
>             'postcopy-bytes': 'uint64',
> -           'dirty-sync-missed-zero-copy': 'uint64' } }
> +           'dirty-sync-missed-zero-copy': 'uint64',
> +           'iteration-count' : 'int' } }
>  
>  ##
>  # @XBZRLECacheStats:
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index d6768d5d71..b796a90cad 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -278,7 +278,7 @@ static int64_t read_migrate_property_int(QTestState *who, const char *property)
>  
>  static uint64_t get_migration_pass(QTestState *who)
>  {
> -    return read_ram_property_int(who, "dirty-sync-count");
> +    return read_ram_property_int(who, "iteration-count");
>  }
>  
>  static void read_blocktime(QTestState *who)