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)?
>
> 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)?
>
Alright, I'll make some revisions to the docs in the upcoming version
and see if it clarifies the meaning of these two pieces of information.
> >
> > 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)
>
--
Best regards
