Re: [PULL 5/5] migration: simplify migration_iteration_run()
Vladimir Sementsov-Ogievskiy wrote:
> On 30.01.23 11:03, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela
>> Reviewed-by: Dr. David Alan Gilbert
>> ---
>> migration/migration.c | 24
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 594a42f085..644c61e91d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3764,23 +3764,23 @@ static MigIterateState
>> migration_iteration_run(MigrationState *s)
>> pend_pre, pend_compat, pend_post);
>> }
>> -if (pending_size && pending_size >= s->threshold_size) {
>> -/* Still a significant amount to transfer */
>> -if (!in_postcopy && pend_pre <= s->threshold_size &&
>> -qatomic_read(>start_postcopy)) {
>> -if (postcopy_start(s)) {
>> -error_report("%s: postcopy failed to start", __func__);
>> -}
>> -return MIG_ITERATE_SKIP;
>> -}
>> -/* Just another iteration step */
>> -qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
>> -} else {
>> +if (pending_size < s->threshold_size) {
>
> to keep the logic, formally it should be "if (!pending_size || pending_size <
> s->threshold_size)"...
And here I am, back.
To stand corrected O:-)
I have to do the change that you suggested.
Why? Because it never ever happens with "real" migrations.
But qemu-iotest 181, well, sometimes it fails.
Never when you compile x86_64.
You need to compile all architectures.
And it fails only sometimes.
Ok, changed to
if (!pending_size || pending_size < s->threshold_size)
Thanks, Juan.
> Actually, could s->threshold_size be 0 here? Or, worth an assertion
> assert(s->threshold_size) ?
This test in particular sets the "bandwidth_limit" to 4k, that is one
page, so it could be that gets zero.
Later, Juan.
Re: [PULL 5/5] migration: simplify migration_iteration_run()
Vladimir Sementsov-Ogievskiy wrote:
> On 30.01.23 11:03, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela
>> Reviewed-by: Dr. David Alan Gilbert
>> ---
>> migration/migration.c | 24
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 594a42f085..644c61e91d 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3764,23 +3764,23 @@ static MigIterateState
>> migration_iteration_run(MigrationState *s)
>> pend_pre, pend_compat, pend_post);
>> }
>> -if (pending_size && pending_size >= s->threshold_size) {
>> -/* Still a significant amount to transfer */
>> -if (!in_postcopy && pend_pre <= s->threshold_size &&
>> -qatomic_read(>start_postcopy)) {
>> -if (postcopy_start(s)) {
>> -error_report("%s: postcopy failed to start", __func__);
>> -}
>> -return MIG_ITERATE_SKIP;
>> -}
>> -/* Just another iteration step */
>> -qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
>> -} else {
>> +if (pending_size < s->threshold_size) {
>
> to keep the logic, formally it should be "if (!pending_size || pending_size <
> s->threshold_size)"...
> Actually, could s->threshold_size be 0 here? Or, worth an assertion
> assert(s->threshold_size) ?
It is weird, but it could:
bandwidth = (double)transferred / time_spent;
s->threshold_size = bandwidth * s->parameters.downtime_limit;
That is the (real) only place when we set it, and if the network was
completely down, bandwidth could be zero.
But I think that it is better to explain in the other way. What does
the current code do:
if (too much dirty memory to converge)
do another iteration
else
do final iteration
What the new code do is
if (low enough memory to converge)
do final iteration.
do another iteration.
So, how we decide if we converge?
if pending_size < s->threshold_size
we "could" change it to pending_size <= s->threshold_size for the zero case
But I think that we would be shooting ourselves in the foot, because we
are having network trouble (only reason that s->theshold_size could be
zero) and we still have all the devices state to migrate.
And once that we enter that state, there is no way back, guest is
stopped in source and we are not able to send anything else.
So I think it is better this way.
What do you think?
Later, Juan.
Re: [PULL 5/5] migration: simplify migration_iteration_run()
On 30.01.23 11:03, Juan Quintela wrote:
Signed-off-by: Juan Quintela
Reviewed-by: Dr. David Alan Gilbert
---
migration/migration.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 594a42f085..644c61e91d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3764,23 +3764,23 @@ static MigIterateState
migration_iteration_run(MigrationState *s)
pend_pre, pend_compat, pend_post);
}
-if (pending_size && pending_size >= s->threshold_size) {
-/* Still a significant amount to transfer */
-if (!in_postcopy && pend_pre <= s->threshold_size &&
-qatomic_read(>start_postcopy)) {
-if (postcopy_start(s)) {
-error_report("%s: postcopy failed to start", __func__);
-}
-return MIG_ITERATE_SKIP;
-}
-/* Just another iteration step */
-qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
-} else {
+if (pending_size < s->threshold_size) {
to keep the logic, formally it should be "if (!pending_size || pending_size <
s->threshold_size)"...
Actually, could s->threshold_size be 0 here? Or, worth an assertion
assert(s->threshold_size) ?
trace_migration_thread_low_pending(pending_size);
migration_completion(s);
return MIG_ITERATE_BREAK;
}
+/* Still a significant amount to transfer */
+if (!in_postcopy && pend_pre <= s->threshold_size &&
+qatomic_read(>start_postcopy)) {
+if (postcopy_start(s)) {
+error_report("%s: postcopy failed to start", __func__);
+}
+return MIG_ITERATE_SKIP;
+}
+
+/* Just another iteration step */
+qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
return MIG_ITERATE_RESUME;
}
--
Best regards,
Vladimir
[PULL 5/5] migration: simplify migration_iteration_run()
Signed-off-by: Juan Quintela
Reviewed-by: Dr. David Alan Gilbert
---
migration/migration.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index 594a42f085..644c61e91d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3764,23 +3764,23 @@ static MigIterateState
migration_iteration_run(MigrationState *s)
pend_pre, pend_compat, pend_post);
}
-if (pending_size && pending_size >= s->threshold_size) {
-/* Still a significant amount to transfer */
-if (!in_postcopy && pend_pre <= s->threshold_size &&
-qatomic_read(>start_postcopy)) {
-if (postcopy_start(s)) {
-error_report("%s: postcopy failed to start", __func__);
-}
-return MIG_ITERATE_SKIP;
-}
-/* Just another iteration step */
-qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
-} else {
+if (pending_size < s->threshold_size) {
trace_migration_thread_low_pending(pending_size);
migration_completion(s);
return MIG_ITERATE_BREAK;
}
+/* Still a significant amount to transfer */
+if (!in_postcopy && pend_pre <= s->threshold_size &&
+qatomic_read(>start_postcopy)) {
+if (postcopy_start(s)) {
+error_report("%s: postcopy failed to start", __func__);
+}
+return MIG_ITERATE_SKIP;
+}
+
+/* Just another iteration step */
+qemu_savevm_state_iterate(s->to_dst_file, in_postcopy);
return MIG_ITERATE_RESUME;
}
--
2.39.1
