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

[Fabiano Rosas <farosas@suse.de>]

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

> On Tue, Sep 17, 2024 at 4:35 AM Fabiano Rosas <farosas@suse.de> wrote:
>
>> 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)?
>>
>
> Unfortunately, I found that Libvirt already regard the "dirty-sync-count"
> as the "iteration count", so if we substitute the "dirty-sync-count"
> with "iteration count" to represent its original meaning, this could
> break the backward compatibility.
>
> To avoid this side effect, we may keep the "dirty-sync-count" as its
> original meaning and introduce a new field like "dirty-sync-count-internal"
> to represent the *real* "dirty-sync-count"?
>
> diff --git a/migration/migration.c b/migration/migration.c
> index f97f6352d2..663315d7e6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1196,8 +1196,9 @@ static void populate_ram_info(MigrationInfo *info,
> MigrationState *s)
>      info->ram->normal_bytes = info->ram->normal * page_size;
>      info->ram->mbps = s->mbps;
>      info->ram->dirty_sync_count =
> +        stat64_get(&mig_stats.iteration_count);

ok

> +    info->ram->dirty_sync_count_internal =
>          stat64_get(&mig_stats.dirty_sync_count);

Does this need to be exposed at all? If it does then it'll need a name
that doesn't have "internal" in it.

> -    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 =
>
>
>>
>> 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)
>>