Yong Huang <yong.hu...@smartx.com> writes:
> On Tue, Sep 17, 2024 at 4:35 AM Fabiano Rosas <faro...@suse.de> wrote:
>
>> Hyman Huang <yong.hu...@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.hu...@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)
>>
