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.
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)
--
2.39.1Hyman 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)
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);
+ info->ram->dirty_sync_count_internal =
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 =
>
> 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)
>
--
Best regards
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)
>>
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)?
>
> 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.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)
>
--
Best regards
© 2016 - 2024 Red Hat, Inc.