[Qemu-devel] [PATCH 00/11] migration: cleanup migration_thread()

[Qemu-devel] [PATCH 00/11] migration: cleanup migration_thread()

[Peter Xu]

Firstly this series is something as a first attempt of me to cleanup
some migration code.  It may not be a good idea, but I still think it
worth a try, so I posted it.  Let me know if any of you don't like it,
so I can stop.  At least after the series the migration_thread()
function can be far shorter and much easier for first-time readers
AFAICT.

For this single function, the most complexity part is quite a lot of
local variables crossly referenced everywhere, and during the cleanup
I do think the COLO part is hacky too.

There can be some functional changes too in the future:

- cancel migration as cleanup of QEMU quit
- add migration locks to protect migration internal states
- ...

But for this single series, there is still no functional change yet.

Please have a look.  Any feedback is welcomed.  Thanks,

Peter Xu (11):
  migration: assert colo instead of check
  migration: qemu_savevm_state_cleanup() in cleanup
  migration: remove "enable_colo" var
  migration: split use of MigrationState.total_time
  migration: move vm_old_running into global state
  migration: introduce vm_down_start_time
  migration: introduce migrate_calculate_complete
  migration: use switch at the end of migration
  migration: cleanup stats update into function
  migration: major cleanup for migrate iterations
  migration: put the finish part into a new function

 migration/migration.c | 289 ++++++++++++++++++++++++++++----------------------
 migration/migration.h |  26 ++++-
 2 files changed, 187 insertions(+), 128 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 01/11] migration: assert colo instead of check

[Peter Xu]

When reaching here if we are still "active" it means we must be in colo
state.  Assert it instead of check it in if condition.

Finally I want to use "switch" here rather than lots of complicated if
clauses.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/migration.c b/migration/migration.c
index 4de3b551fe..0ee4b4c27c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2309,7 +2309,8 @@ static void *migration_thread(void *opaque)
         }
         runstate_set(RUN_STATE_POSTMIGRATE);
     } else {
-        if (s->state == MIGRATION_STATUS_ACTIVE && enable_colo) {
+        if (s->state == MIGRATION_STATUS_ACTIVE) {
+            assert(enable_colo);
             migrate_start_colo_process(s);
             qemu_savevm_state_cleanup();
             /*
-- 
2.14.3

[Qemu-devel] [PATCH 02/11] migration: qemu_savevm_state_cleanup() in cleanup

[Peter Xu]

Moving existing callers all into migrate_fd_cleanup().  It simplifies
migration_thread() a bit.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 0ee4b4c27c..edbda43246 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1077,6 +1077,8 @@ static void migrate_fd_cleanup(void *opaque)
     qemu_bh_delete(s->cleanup_bh);
     s->cleanup_bh = NULL;
 
+    qemu_savevm_state_cleanup();
+
     if (s->to_dst_file) {
         Error *local_err = NULL;
 
@@ -2290,13 +2292,6 @@ static void *migration_thread(void *opaque)
     end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
 
     qemu_mutex_lock_iothread();
-    /*
-     * The resource has been allocated by migration will be reused in COLO
-     * process, so don't release them.
-     */
-    if (!enable_colo) {
-        qemu_savevm_state_cleanup();
-    }
     if (s->state == MIGRATION_STATUS_COMPLETED) {
         uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
         s->total_time = end_time - s->total_time;
@@ -2312,7 +2307,6 @@ static void *migration_thread(void *opaque)
         if (s->state == MIGRATION_STATUS_ACTIVE) {
             assert(enable_colo);
             migrate_start_colo_process(s);
-            qemu_savevm_state_cleanup();
             /*
             * Fixme: we will run VM in COLO no matter its old running state.
             * After exited COLO, we will keep running.
-- 
2.14.3

[Qemu-devel] [PATCH 03/11] migration: remove "enable_colo" var

[Peter Xu]

It's only used once, clean it up a bit.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index edbda43246..20f7565527 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2177,7 +2177,6 @@ static void *migration_thread(void *opaque)
     bool entered_postcopy = false;
     /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
     enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
-    bool enable_colo = migrate_colo_enabled();
 
     rcu_register_thread();
 
@@ -2305,7 +2304,7 @@ static void *migration_thread(void *opaque)
         runstate_set(RUN_STATE_POSTMIGRATE);
     } else {
         if (s->state == MIGRATION_STATUS_ACTIVE) {
-            assert(enable_colo);
+            assert(migrate_colo_enabled());
             migrate_start_colo_process(s);
             /*
             * Fixme: we will run VM in COLO no matter its old running state.
-- 
2.14.3

[Qemu-devel] [PATCH 04/11] migration: split use of MigrationState.total_time

[Peter Xu]

It was used either to:

1. store initial timestamp of migration start, and
2. store total time used by last migration

Let's provide two parameters for each of them.  Mix use of the two is
slightly misleading.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 13 +++++++------
 migration/migration.h |  5 ++++-
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 20f7565527..b684c2005d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -613,7 +613,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->has_status = true;
         info->has_total_time = true;
         info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
-            - s->total_time;
+            - s->mig_start_time;
         info->has_expected_downtime = true;
         info->expected_downtime = s->expected_downtime;
         info->has_setup_time = true;
@@ -629,7 +629,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
     case MIGRATION_STATUS_COMPLETED:
         info->has_status = true;
         info->has_total_time = true;
-        info->total_time = s->total_time;
+        info->total_time = s->mig_total_time;
         info->has_downtime = true;
         info->downtime = s->downtime;
         info->has_setup_time = true;
@@ -1270,7 +1270,8 @@ MigrationState *migrate_init(void)
 
     migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
 
-    s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    s->mig_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+    s->mig_total_time = 0;
     return s;
 }
 
@@ -2293,13 +2294,13 @@ static void *migration_thread(void *opaque)
     qemu_mutex_lock_iothread();
     if (s->state == MIGRATION_STATUS_COMPLETED) {
         uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
-        s->total_time = end_time - s->total_time;
+        s->mig_total_time = end_time - s->mig_start_time;
         if (!entered_postcopy) {
             s->downtime = end_time - start_time;
         }
-        if (s->total_time) {
+        if (s->mig_total_time) {
             s->mbps = (((double) transferred_bytes * 8.0) /
-                       ((double) s->total_time)) / 1000;
+                       ((double) s->mig_total_time)) / 1000;
         }
         runstate_set(RUN_STATE_POSTMIGRATE);
     } else {
diff --git a/migration/migration.h b/migration/migration.h
index 663415fe48..ac74a12713 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -103,7 +103,10 @@ struct MigrationState
     } rp_state;
 
     double mbps;
-    int64_t total_time;
+    /* Timestamp when recent migration starts (ms) */
+    int64_t mig_start_time;
+    /* Total time used by latest migration (ms) */
+    int64_t mig_total_time;
     int64_t downtime;
     int64_t expected_downtime;
     bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
-- 
2.14.3

[Qemu-devel] [PATCH 05/11] migration: move vm_old_running into global state

[Peter Xu]

Firstly, it was passed around.  Let's just move it into MigrationState
just like many other variables as state of migration.

One thing to mention is that for postcopy, we actually don't need this
knowledge at all since postcopy can't resume a VM even if it fails (we
can see that from the old code too: when we try to resume we also check
against "entered_postcopy" variable).  So further we do this:

- in postcopy_start(), we don't update vm_old_running since useless
- in migration_thread(), we don't need to check entered_postcopy when
  resume, since it's only used for precopy.

Comment this out too for that variable definition.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 17 +++++++----------
 migration/migration.h |  6 ++++++
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index b684c2005d..62b3766852 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1272,6 +1272,7 @@ MigrationState *migrate_init(void)
 
     s->mig_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     s->mig_total_time = 0;
+    s->old_vm_running = false;
     return s;
 }
 
@@ -1846,7 +1847,7 @@ static int await_return_path_close_on_source(MigrationState *ms)
  * Switch from normal iteration to postcopy
  * Returns non-0 on error
  */
-static int postcopy_start(MigrationState *ms, bool *old_vm_running)
+static int postcopy_start(MigrationState *ms)
 {
     int ret;
     QIOChannelBuffer *bioc;
@@ -1864,7 +1865,6 @@ static int postcopy_start(MigrationState *ms, bool *old_vm_running)
     trace_postcopy_start_set_run();
 
     qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
-    *old_vm_running = runstate_is_running();
     global_state_store();
     ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
     if (ret < 0) {
@@ -2055,11 +2055,9 @@ static int migration_maybe_pause(MigrationState *s,
  *
  * @s: Current migration state
  * @current_active_state: The migration state we expect to be in
- * @*old_vm_running: Pointer to old_vm_running flag
  * @*start_time: Pointer to time to update
  */
 static void migration_completion(MigrationState *s, int current_active_state,
-                                 bool *old_vm_running,
                                  int64_t *start_time)
 {
     int ret;
@@ -2068,7 +2066,7 @@ static void migration_completion(MigrationState *s, int current_active_state,
         qemu_mutex_lock_iothread();
         *start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
         qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
-        *old_vm_running = runstate_is_running();
+        s->old_vm_running = runstate_is_running();
         ret = global_state_store();
 
         if (!ret) {
@@ -2174,7 +2172,6 @@ static void *migration_thread(void *opaque)
     int64_t threshold_size = 0;
     int64_t start_time = initial_time;
     int64_t end_time;
-    bool old_vm_running = false;
     bool entered_postcopy = false;
     /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
     enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
@@ -2233,7 +2230,7 @@ static void *migration_thread(void *opaque)
                     pend_nonpost <= threshold_size &&
                     atomic_read(&s->start_postcopy)) {
 
-                    if (!postcopy_start(s, &old_vm_running)) {
+                    if (!postcopy_start(s)) {
                         current_active_state = MIGRATION_STATUS_POSTCOPY_ACTIVE;
                         entered_postcopy = true;
                     }
@@ -2245,7 +2242,7 @@ static void *migration_thread(void *opaque)
             } else {
                 trace_migration_thread_low_pending(pending_size);
                 migration_completion(s, current_active_state,
-                                     &old_vm_running, &start_time);
+                                     &start_time);
                 break;
             }
         }
@@ -2311,9 +2308,9 @@ static void *migration_thread(void *opaque)
             * Fixme: we will run VM in COLO no matter its old running state.
             * After exited COLO, we will keep running.
             */
-            old_vm_running = true;
+            s->old_vm_running = true;
         }
-        if (old_vm_running && !entered_postcopy) {
+        if (s->old_vm_running) {
             vm_start();
         } else {
             if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
diff --git a/migration/migration.h b/migration/migration.h
index ac74a12713..0f5df2367c 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -111,6 +111,12 @@ struct MigrationState
     int64_t expected_downtime;
     bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
     int64_t setup_time;
+    /*
+     * Whether the old VM is running for the last migration.  This is
+     * used to resume the VM when precopy failed or cancelled somehow.
+     * It's never used for postcopy.
+     */
+    bool old_vm_running;
 
     /* Flag set once the migration has been asked to enter postcopy */
     bool start_postcopy;
-- 
2.14.3

[Qemu-devel] [PATCH 06/11] migration: introduce vm_down_start_time

[Peter Xu]

Introduce MigrationState.vm_down_start_time to replace the local
variable "start_time" in migration_thread to avoid passing things around.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 12 ++++--------
 migration/migration.h |  2 ++
 2 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 62b3766852..2d8b47197e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2055,16 +2055,14 @@ static int migration_maybe_pause(MigrationState *s,
  *
  * @s: Current migration state
  * @current_active_state: The migration state we expect to be in
- * @*start_time: Pointer to time to update
  */
-static void migration_completion(MigrationState *s, int current_active_state,
-                                 int64_t *start_time)
+static void migration_completion(MigrationState *s, int current_active_state)
 {
     int ret;
 
     if (s->state == MIGRATION_STATUS_ACTIVE) {
         qemu_mutex_lock_iothread();
-        *start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+        s->vm_down_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
         qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
         s->old_vm_running = runstate_is_running();
         ret = global_state_store();
@@ -2170,7 +2168,6 @@ static void *migration_thread(void *opaque)
      * measured bandwidth
      */
     int64_t threshold_size = 0;
-    int64_t start_time = initial_time;
     int64_t end_time;
     bool entered_postcopy = false;
     /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
@@ -2241,8 +2238,7 @@ static void *migration_thread(void *opaque)
                 qemu_savevm_state_iterate(s->to_dst_file, entered_postcopy);
             } else {
                 trace_migration_thread_low_pending(pending_size);
-                migration_completion(s, current_active_state,
-                                     &start_time);
+                migration_completion(s, current_active_state);
                 break;
             }
         }
@@ -2293,7 +2289,7 @@ static void *migration_thread(void *opaque)
         uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
         s->mig_total_time = end_time - s->mig_start_time;
         if (!entered_postcopy) {
-            s->downtime = end_time - start_time;
+            s->downtime = end_time - s->vm_down_start_time;
         }
         if (s->mig_total_time) {
             s->mbps = (((double) transferred_bytes * 8.0) /
diff --git a/migration/migration.h b/migration/migration.h
index 0f5df2367c..3ab5506233 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -107,6 +107,8 @@ struct MigrationState
     int64_t mig_start_time;
     /* Total time used by latest migration (ms) */
     int64_t mig_total_time;
+    /* Timestamp when VM is down (ms) to migrate the last stuff */
+    int64_t vm_down_start_time;
     int64_t downtime;
     int64_t expected_downtime;
     bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
-- 
2.14.3

[Qemu-devel] [PATCH 07/11] migration: introduce migrate_calculate_complete

[Peter Xu]

Generalize the calculation part when migration complete into a
function to simplify migration_thread().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2d8b47197e..acef54748b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2151,6 +2151,19 @@ bool migrate_colo_enabled(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
 }
 
+static void migration_calculate_complete(MigrationState *s)
+{
+    uint64_t bytes = qemu_ftell(s->to_dst_file);
+    int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+
+    s->mig_total_time = end_time - s->mig_start_time;
+    s->downtime = end_time - s->vm_down_start_time;
+
+    if (s->mig_total_time) {
+        s->mbps = ((double) bytes * 8.0) / s->mig_total_time / 1000;
+    }
+}
+
 /*
  * Master migration thread on the source VM.
  * It drives the migration and pumps the data down the outgoing channel.
@@ -2168,7 +2181,6 @@ static void *migration_thread(void *opaque)
      * measured bandwidth
      */
     int64_t threshold_size = 0;
-    int64_t end_time;
     bool entered_postcopy = false;
     /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
     enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
@@ -2282,19 +2294,10 @@ static void *migration_thread(void *opaque)
     trace_migration_thread_after_loop();
     /* If we enabled cpu throttling for auto-converge, turn it off. */
     cpu_throttle_stop();
-    end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
 
     qemu_mutex_lock_iothread();
     if (s->state == MIGRATION_STATUS_COMPLETED) {
-        uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
-        s->mig_total_time = end_time - s->mig_start_time;
-        if (!entered_postcopy) {
-            s->downtime = end_time - s->vm_down_start_time;
-        }
-        if (s->mig_total_time) {
-            s->mbps = (((double) transferred_bytes * 8.0) /
-                       ((double) s->mig_total_time)) / 1000;
-        }
+        migration_calculate_complete(s);
         runstate_set(RUN_STATE_POSTMIGRATE);
     } else {
         if (s->state == MIGRATION_STATUS_ACTIVE) {
-- 
2.14.3

[Qemu-devel] [PATCH 08/11] migration: use switch at the end of migration

[Peter Xu]

It converts the old if clauses into switch, explicitly mentions the
possible migration states.  The old nested "if"s are not clear on what
we do on different states.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index acef54748b..bfcba24caa 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2296,19 +2296,23 @@ static void *migration_thread(void *opaque)
     cpu_throttle_stop();
 
     qemu_mutex_lock_iothread();
-    if (s->state == MIGRATION_STATUS_COMPLETED) {
+    switch (s->state) {
+    case MIGRATION_STATUS_COMPLETED:
         migration_calculate_complete(s);
         runstate_set(RUN_STATE_POSTMIGRATE);
-    } else {
-        if (s->state == MIGRATION_STATUS_ACTIVE) {
-            assert(migrate_colo_enabled());
-            migrate_start_colo_process(s);
-            /*
-            * Fixme: we will run VM in COLO no matter its old running state.
-            * After exited COLO, we will keep running.
-            */
-            s->old_vm_running = true;
-        }
+        break;
+
+    case MIGRATION_STATUS_ACTIVE:
+        assert(migrate_colo_enabled());
+        migrate_start_colo_process(s);
+        /*
+         * Fixme: we will run VM in COLO no matter its old running state.
+         * After exited COLO, we will keep running.
+         */
+        s->old_vm_running = true;
+        /* Fallthrough */
+    case MIGRATION_STATUS_FAILED:
+    case MIGRATION_STATUS_CANCELLED:
         if (s->old_vm_running) {
             vm_start();
         } else {
@@ -2316,6 +2320,12 @@ static void *migration_thread(void *opaque)
                 runstate_set(RUN_STATE_POSTMIGRATE);
             }
         }
+        break;
+
+    default:
+        /* Should not reach here, but if so, forgive the VM. */
+        error_report("%s: Unknown ending state %d", __func__, s->state);
+        break;
     }
     qemu_bh_schedule(s->cleanup_bh);
     qemu_mutex_unlock_iothread();
-- 
2.14.3

[Qemu-devel] [PATCH 09/11] migration: cleanup stats update into function

[Peter Xu]

We have quite a few lines in migration_thread() that calculates some
statistics for the migration interations.  Isolate it into a single
function to improve readability.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 82 +++++++++++++++++++++++++++++----------------------
 migration/migration.h | 13 ++++++++
 2 files changed, 59 insertions(+), 36 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index bfcba24caa..2629f907e9 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1273,6 +1273,8 @@ MigrationState *migrate_init(void)
     s->mig_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     s->mig_total_time = 0;
     s->old_vm_running = false;
+    s->initial_bytes = 0;
+    s->threshold_size = 0;
     return s;
 }
 
@@ -2164,6 +2166,39 @@ static void migration_calculate_complete(MigrationState *s)
     }
 }
 
+static void migration_update_statistics(MigrationState *s,
+                                        int64_t current_time)
+{
+    uint64_t transferred = qemu_ftell(s->to_dst_file) - s->initial_bytes;
+    uint64_t time_spent = current_time - s->initial_time;
+    double bandwidth = (double)transferred / time_spent;
+    int64_t threshold_size = bandwidth * s->parameters.downtime_limit;
+
+    if (current_time < s->initial_time + BUFFER_DELAY) {
+        return;
+    }
+
+    s->mbps = (((double) transferred * 8.0) /
+               ((double) time_spent / 1000.0)) / 1000.0 / 1000.0;
+
+    /*
+     * if we haven't sent anything, we don't want to
+     * recalculate. 10000 is a small enough number for our purposes
+     */
+    if (ram_counters.dirty_pages_rate && transferred > 10000) {
+        s->expected_downtime = ram_counters.dirty_pages_rate *
+            qemu_target_page_size() / bandwidth;
+    }
+
+    qemu_file_reset_rate_limit(s->to_dst_file);
+
+    s->initial_time = current_time;
+    s->initial_bytes = qemu_ftell(s->to_dst_file);
+
+    trace_migrate_transferred(transferred, time_spent,
+                              bandwidth, threshold_size);
+}
+
 /*
  * Master migration thread on the source VM.
  * It drives the migration and pumps the data down the outgoing channel.
@@ -2171,22 +2206,15 @@ static void migration_calculate_complete(MigrationState *s)
 static void *migration_thread(void *opaque)
 {
     MigrationState *s = opaque;
-    /* Used by the bandwidth calcs, updated later */
-    int64_t initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
-    int64_t initial_bytes = 0;
-    /*
-     * The final stage happens when the remaining data is smaller than
-     * this threshold; it's calculated from the requested downtime and
-     * measured bandwidth
-     */
-    int64_t threshold_size = 0;
     bool entered_postcopy = false;
     /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
     enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
 
     rcu_register_thread();
 
+    s->initial_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
+
     qemu_savevm_state_header(s->to_dst_file);
 
     /*
@@ -2226,17 +2254,17 @@ static void *migration_thread(void *opaque)
         if (!qemu_file_rate_limit(s->to_dst_file)) {
             uint64_t pend_post, pend_nonpost;
 
-            qemu_savevm_state_pending(s->to_dst_file, threshold_size,
+            qemu_savevm_state_pending(s->to_dst_file, s->threshold_size,
                                       &pend_nonpost, &pend_post);
             pending_size = pend_nonpost + pend_post;
-            trace_migrate_pending(pending_size, threshold_size,
+            trace_migrate_pending(pending_size, s->threshold_size,
                                   pend_post, pend_nonpost);
-            if (pending_size && pending_size >= threshold_size) {
+            if (pending_size && pending_size >= s->threshold_size) {
                 /* Still a significant amount to transfer */
 
                 if (migrate_postcopy() &&
                     s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE &&
-                    pend_nonpost <= threshold_size &&
+                    pend_nonpost <= s->threshold_size &&
                     atomic_read(&s->start_postcopy)) {
 
                     if (!postcopy_start(s)) {
@@ -2261,33 +2289,15 @@ static void *migration_thread(void *opaque)
             trace_migration_thread_file_err();
             break;
         }
+
         current_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
-        if (current_time >= initial_time + BUFFER_DELAY) {
-            uint64_t transferred_bytes = qemu_ftell(s->to_dst_file) -
-                                         initial_bytes;
-            uint64_t time_spent = current_time - initial_time;
-            double bandwidth = (double)transferred_bytes / time_spent;
-            threshold_size = bandwidth * s->parameters.downtime_limit;
-
-            s->mbps = (((double) transferred_bytes * 8.0) /
-                    ((double) time_spent / 1000.0)) / 1000.0 / 1000.0;
-
-            trace_migrate_transferred(transferred_bytes, time_spent,
-                                      bandwidth, threshold_size);
-            /* if we haven't sent anything, we don't want to recalculate
-               10000 is a small enough number for our purposes */
-            if (ram_counters.dirty_pages_rate && transferred_bytes > 10000) {
-                s->expected_downtime = ram_counters.dirty_pages_rate *
-                    qemu_target_page_size() / bandwidth;
-            }
 
-            qemu_file_reset_rate_limit(s->to_dst_file);
-            initial_time = current_time;
-            initial_bytes = qemu_ftell(s->to_dst_file);
-        }
+        /* Conditionally update statistics */
+        migration_update_statistics(s, current_time);
+
         if (qemu_file_rate_limit(s->to_dst_file)) {
             /* usleep expects microseconds */
-            g_usleep((initial_time + BUFFER_DELAY - current_time)*1000);
+            g_usleep((s->initial_time + BUFFER_DELAY - current_time) * 1000);
         }
     }
 
diff --git a/migration/migration.h b/migration/migration.h
index 3ab5506233..248f7d9a5c 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -90,6 +90,19 @@ struct MigrationState
     QEMUBH *cleanup_bh;
     QEMUFile *to_dst_file;
 
+    /*
+     * Migration thread statistic variables, mostly used in
+     * migration_thread() iterations only.
+     */
+    uint64_t initial_bytes;
+    int64_t initial_time;
+    /*
+     * The final stage happens when the remaining data is smaller than
+     * this threshold; it's calculated from the requested downtime and
+     * measured bandwidth
+     */
+    int64_t threshold_size;
+
     /* params from 'migrate-set-parameters' */
     MigrationParameters parameters;
 
-- 
2.14.3

[Qemu-devel] [PATCH 10/11] migration: major cleanup for migrate iterations

[Peter Xu]

The major work for migration iterations are to move RAM/block/... data
via qemu_savevm_state_iterate().  Generalize those part into a single
function.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 90 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 55 insertions(+), 35 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 2629f907e9..c6c39738d2 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2056,11 +2056,11 @@ static int migration_maybe_pause(MigrationState *s,
  *   The caller 'breaks' the loop when this returns.
  *
  * @s: Current migration state
- * @current_active_state: The migration state we expect to be in
  */
-static void migration_completion(MigrationState *s, int current_active_state)
+static void migration_completion(MigrationState *s)
 {
     int ret;
+    int current_active_state = s->state;
 
     if (s->state == MIGRATION_STATUS_ACTIVE) {
         qemu_mutex_lock_iothread();
@@ -2199,6 +2199,51 @@ static void migration_update_statistics(MigrationState *s,
                               bandwidth, threshold_size);
 }
 
+/* Migration thread iteration status */
+typedef enum {
+    MIG_ITERATE_RESUME,         /* Resume current iteration */
+    MIG_ITERATE_SKIP,           /* Skip current iteration */
+    MIG_ITERATE_BREAK,          /* Break the loop */
+} MigIterateState;
+
+/*
+ * Return true if continue to the next iteration directly, false
+ * otherwise.
+ */
+static MigIterateState migration_iteration_run(MigrationState *s)
+{
+    uint64_t pending_size, pend_post, pend_nonpost;
+    bool in_postcopy = s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE;
+
+    qemu_savevm_state_pending(s->to_dst_file, s->threshold_size,
+                              &pend_nonpost, &pend_post);
+    pending_size = pend_nonpost + pend_post;
+
+    trace_migrate_pending(pending_size, s->threshold_size,
+                          pend_post, pend_nonpost);
+
+    if (pending_size && pending_size >= s->threshold_size) {
+        /* Still a significant amount to transfer */
+        if (migrate_postcopy() && !in_postcopy &&
+            pend_nonpost <= s->threshold_size &&
+            atomic_read(&s->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,
+            s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
+    } else {
+        trace_migration_thread_low_pending(pending_size);
+        migration_completion(s);
+        return MIG_ITERATE_BREAK;
+    }
+
+    return MIG_ITERATE_RESUME;
+}
+
 /*
  * Master migration thread on the source VM.
  * It drives the migration and pumps the data down the outgoing channel.
@@ -2207,9 +2252,6 @@ static void *migration_thread(void *opaque)
 {
     MigrationState *s = opaque;
     int64_t setup_start = qemu_clock_get_ms(QEMU_CLOCK_HOST);
-    bool entered_postcopy = false;
-    /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
-    enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
 
     rcu_register_thread();
 
@@ -2249,43 +2291,21 @@ static void *migration_thread(void *opaque)
     while (s->state == MIGRATION_STATUS_ACTIVE ||
            s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE) {
         int64_t current_time;
-        uint64_t pending_size;
 
         if (!qemu_file_rate_limit(s->to_dst_file)) {
-            uint64_t pend_post, pend_nonpost;
-
-            qemu_savevm_state_pending(s->to_dst_file, s->threshold_size,
-                                      &pend_nonpost, &pend_post);
-            pending_size = pend_nonpost + pend_post;
-            trace_migrate_pending(pending_size, s->threshold_size,
-                                  pend_post, pend_nonpost);
-            if (pending_size && pending_size >= s->threshold_size) {
-                /* Still a significant amount to transfer */
-
-                if (migrate_postcopy() &&
-                    s->state != MIGRATION_STATUS_POSTCOPY_ACTIVE &&
-                    pend_nonpost <= s->threshold_size &&
-                    atomic_read(&s->start_postcopy)) {
-
-                    if (!postcopy_start(s)) {
-                        current_active_state = MIGRATION_STATUS_POSTCOPY_ACTIVE;
-                        entered_postcopy = true;
-                    }
-
-                    continue;
-                }
-                /* Just another iteration step */
-                qemu_savevm_state_iterate(s->to_dst_file, entered_postcopy);
-            } else {
-                trace_migration_thread_low_pending(pending_size);
-                migration_completion(s, current_active_state);
+            MigIterateState iter_state = migration_iteration_run(s);
+            if (iter_state == MIG_ITERATE_SKIP) {
+                continue;
+            } else if (iter_state == MIG_ITERATE_BREAK) {
                 break;
             }
         }
 
         if (qemu_file_get_error(s->to_dst_file)) {
-            migrate_set_state(&s->state, current_active_state,
-                              MIGRATION_STATUS_FAILED);
+            if (migration_is_setup_or_active(s->state)) {
+                migrate_set_state(&s->state, s->state,
+                                  MIGRATION_STATUS_FAILED);
+            }
             trace_migration_thread_file_err();
             break;
         }
-- 
2.14.3

[Qemu-devel] [PATCH 11/11] migration: put the finish part into a new function

[Peter Xu]

This patch only moved the last part of migration_thread() into a new
function migration_iteration_finish() to make it much shorter.  With
previous works to remove some local variables, now it's fairly easy to
do that.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 80 +++++++++++++++++++++++++++------------------------
 1 file changed, 42 insertions(+), 38 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index c6c39738d2..51379bba2c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2244,6 +2244,47 @@ static MigIterateState migration_iteration_run(MigrationState *s)
     return MIG_ITERATE_RESUME;
 }
 
+static void migration_iteration_finish(MigrationState *s)
+{
+    /* If we enabled cpu throttling for auto-converge, turn it off. */
+    cpu_throttle_stop();
+
+    qemu_mutex_lock_iothread();
+    switch (s->state) {
+    case MIGRATION_STATUS_COMPLETED:
+        migration_calculate_complete(s);
+        runstate_set(RUN_STATE_POSTMIGRATE);
+        break;
+
+    case MIGRATION_STATUS_ACTIVE:
+        assert(migrate_colo_enabled());
+        migrate_start_colo_process(s);
+        /*
+         * Fixme: we will run VM in COLO no matter its old running state.
+         * After exited COLO, we will keep running.
+         */
+        s->old_vm_running = true;
+        /* Fallthrough */
+    case MIGRATION_STATUS_FAILED:
+    case MIGRATION_STATUS_CANCELLED:
+        if (s->old_vm_running) {
+            vm_start();
+        } else {
+            if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
+                runstate_set(RUN_STATE_POSTMIGRATE);
+            }
+        }
+        break;
+
+    default:
+        /* Should not reach here, but if so, forgive the VM. */
+        error_report("%s: Unknown ending state %d", __func__, s->state);
+        break;
+    }
+    qemu_bh_schedule(s->cleanup_bh);
+    qemu_mutex_unlock_iothread();
+}
+
 /*
  * Master migration thread on the source VM.
  * It drives the migration and pumps the data down the outgoing channel.
@@ -2322,44 +2363,7 @@ static void *migration_thread(void *opaque)
     }
 
     trace_migration_thread_after_loop();
-    /* If we enabled cpu throttling for auto-converge, turn it off. */
-    cpu_throttle_stop();
-
-    qemu_mutex_lock_iothread();
-    switch (s->state) {
-    case MIGRATION_STATUS_COMPLETED:
-        migration_calculate_complete(s);
-        runstate_set(RUN_STATE_POSTMIGRATE);
-        break;
-
-    case MIGRATION_STATUS_ACTIVE:
-        assert(migrate_colo_enabled());
-        migrate_start_colo_process(s);
-        /*
-         * Fixme: we will run VM in COLO no matter its old running state.
-         * After exited COLO, we will keep running.
-         */
-        s->old_vm_running = true;
-        /* Fallthrough */
-    case MIGRATION_STATUS_FAILED:
-    case MIGRATION_STATUS_CANCELLED:
-        if (s->old_vm_running) {
-            vm_start();
-        } else {
-            if (runstate_check(RUN_STATE_FINISH_MIGRATE)) {
-                runstate_set(RUN_STATE_POSTMIGRATE);
-            }
-        }
-        break;
-
-    default:
-        /* Should not reach here, but if so, forgive the VM. */
-        error_report("%s: Unknown ending state %d", __func__, s->state);
-        break;
-    }
-    qemu_bh_schedule(s->cleanup_bh);
-    qemu_mutex_unlock_iothread();
-
+    migration_iteration_finish(s);
     rcu_unregister_thread();
     return NULL;
 }
-- 
2.14.3

Re: [Qemu-devel] [PATCH 01/11] migration: assert colo instead of check

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> When reaching here if we are still "active" it means we must be in colo
> state.  Assert it instead of check it in if condition.

I don't think so.

> Finally I want to use "switch" here rather than lots of complicated if
> clauses.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 4de3b551fe..0ee4b4c27c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2309,7 +2309,8 @@ static void *migration_thread(void *opaque)
>          }
>          runstate_set(RUN_STATE_POSTMIGRATE);
>      } else {
> -        if (s->state == MIGRATION_STATUS_ACTIVE && enable_colo) {
> +        if (s->state == MIGRATION_STATUS_ACTIVE) {

We want to run this code iff:
- we are in ACTIVE state
- we are using colo

We can be doing a normal migration, with colo compliled in, but not
enabled, no?

Later, Juan.


> +            assert(enable_colo);
>              migrate_start_colo_process(s);
>              qemu_savevm_state_cleanup();
>              /*

Re: [Qemu-devel] [PATCH 03/11] migration: remove "enable_colo" var

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It's only used once, clean it up a bit.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

See my previous comment on patch 1.  We can remove the variable, but the
move to ose assert is wrong IMHO.

Later, Juan.


> ---
>  migration/migration.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index edbda43246..20f7565527 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2177,7 +2177,6 @@ static void *migration_thread(void *opaque)
>      bool entered_postcopy = false;
>      /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
>      enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
> -    bool enable_colo = migrate_colo_enabled();
>  
>      rcu_register_thread();
>  
> @@ -2305,7 +2304,7 @@ static void *migration_thread(void *opaque)
>          runstate_set(RUN_STATE_POSTMIGRATE);
>      } else {
>          if (s->state == MIGRATION_STATUS_ACTIVE) {
> -            assert(enable_colo);
> +            assert(migrate_colo_enabled());
>              migrate_start_colo_process(s);
>              /*
>              * Fixme: we will run VM in COLO no matter its old running state.

Re: [Qemu-devel] [PATCH 04/11] migration: split use of MigrationState.total_time

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It was used either to:
>
> 1. store initial timestamp of migration start, and
> 2. store total time used by last migration
>
> Let's provide two parameters for each of them.  Mix use of the two is
> slightly misleading.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

If you have to respin, I would like to use the names:

start_time and total_time, i.e. without the mig_ preffix, because they
are in an struct that is clearly named migration O:-)

For the rest, good cleanup.

Later, Juan.


> ---
>  migration/migration.c | 13 +++++++------
>  migration/migration.h |  5 ++++-
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 20f7565527..b684c2005d 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -613,7 +613,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>          info->has_status = true;
>          info->has_total_time = true;
>          info->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME)
> -            - s->total_time;
> +            - s->mig_start_time;
>          info->has_expected_downtime = true;
>          info->expected_downtime = s->expected_downtime;
>          info->has_setup_time = true;
> @@ -629,7 +629,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
>      case MIGRATION_STATUS_COMPLETED:
>          info->has_status = true;
>          info->has_total_time = true;
> -        info->total_time = s->total_time;
> +        info->total_time = s->mig_total_time;
>          info->has_downtime = true;
>          info->downtime = s->downtime;
>          info->has_setup_time = true;
> @@ -1270,7 +1270,8 @@ MigrationState *migrate_init(void)
>  
>      migrate_set_state(&s->state, MIGRATION_STATUS_NONE, MIGRATION_STATUS_SETUP);
>  
> -    s->total_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    s->mig_start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    s->mig_total_time = 0;
>      return s;
>  }
>  
> @@ -2293,13 +2294,13 @@ static void *migration_thread(void *opaque)
>      qemu_mutex_lock_iothread();
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
> -        s->total_time = end_time - s->total_time;
> +        s->mig_total_time = end_time - s->mig_start_time;
>          if (!entered_postcopy) {
>              s->downtime = end_time - start_time;
>          }
> -        if (s->total_time) {
> +        if (s->mig_total_time) {
>              s->mbps = (((double) transferred_bytes * 8.0) /
> -                       ((double) s->total_time)) / 1000;
> +                       ((double) s->mig_total_time)) / 1000;
>          }
>          runstate_set(RUN_STATE_POSTMIGRATE);
>      } else {
> diff --git a/migration/migration.h b/migration/migration.h
> index 663415fe48..ac74a12713 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -103,7 +103,10 @@ struct MigrationState
>      } rp_state;
>  
>      double mbps;
> -    int64_t total_time;
> +    /* Timestamp when recent migration starts (ms) */
> +    int64_t mig_start_time;
> +    /* Total time used by latest migration (ms) */
> +    int64_t mig_total_time;
>      int64_t downtime;
>      int64_t expected_downtime;
>      bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];

Re: [Qemu-devel] [PATCH 01/11] migration: assert colo instead of check

[Peter Xu]

On Wed, Jan 03, 2018 at 09:38:52AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > When reaching here if we are still "active" it means we must be in colo
> > state.  Assert it instead of check it in if condition.
> 
> I don't think so.
> 
> > Finally I want to use "switch" here rather than lots of complicated if
> > clauses.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 4de3b551fe..0ee4b4c27c 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2309,7 +2309,8 @@ static void *migration_thread(void *opaque)
> >          }
> >          runstate_set(RUN_STATE_POSTMIGRATE);
> >      } else {
> > -        if (s->state == MIGRATION_STATUS_ACTIVE && enable_colo) {
> > +        if (s->state == MIGRATION_STATUS_ACTIVE) {
> 
> We want to run this code iff:
> - we are in ACTIVE state
> - we are using colo
> 
> We can be doing a normal migration, with colo compliled in, but not
> enabled, no?

If COLO is not enabled (even if it is compiled in), IMHO we won't
reach this state.  Note that we have this in migration_completion():

    if (!migrate_colo_enabled()) {
        migrate_set_state(&s->state, current_active_state,
                          MIGRATION_STATUS_COMPLETED);
    }

That's where we kept the ACTIVE state, and that should be the only
place. And, this is exactly what this patch is going to do - I want to
remove COLO hacks if possible.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 04/11] migration: split use of MigrationState.total_time

[Peter Xu]

On Wed, Jan 03, 2018 at 09:58:10AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > It was used either to:
> >
> > 1. store initial timestamp of migration start, and
> > 2. store total time used by last migration
> >
> > Let's provide two parameters for each of them.  Mix use of the two is
> > slightly misleading.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks!

> 
> If you have to respin, I would like to use the names:

(I think it very possible :-)

> 
> start_time and total_time, i.e. without the mig_ preffix, because they
> are in an struct that is clearly named migration O:-)

Oh, it's my bad (or good?) habit of keeping some prefix so that cscope
won't mix these variables with others.  I think the problem is that
cscope is always using a global namespace for variables.  Considering
this do you still like me to change? :) Any suggestions on better
usage of cscope would be greatly welcomed too!

(Sure I can rename that!  It's not a big deal)

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 05/11] migration: move vm_old_running into global state

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Firstly, it was passed around.  Let's just move it into MigrationState
> just like many other variables as state of migration.
>
> One thing to mention is that for postcopy, we actually don't need this
> knowledge at all since postcopy can't resume a VM even if it fails (we
> can see that from the old code too: when we try to resume we also check
> against "entered_postcopy" variable).  So further we do this:
>
> - in postcopy_start(), we don't update vm_old_running since useless
> - in migration_thread(), we don't need to check entered_postcopy when
>   resume, since it's only used for precopy.
>
> Comment this out too for that variable definition.

Reviewed-by: Juan Quintela <quintela@redhat.com>

But I wonder if we can came with a better name.  Best one that I can
think is
   vm_was_running

Any other name that I came is bad for precopy or colo.

i.e. restart_vm_on_cancel_error

is meaningful for precopy, but not for colo.

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 17 +++++++----------
>  migration/migration.h |  6 ++++++
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/migration/migration.h b/migration/migration.h
> index ac74a12713..0f5df2367c 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -111,6 +111,12 @@ struct MigrationState
>      int64_t expected_downtime;
>      bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
>      int64_t setup_time;
> +    /*
> +     * Whether the old VM is running for the last migration.  This is
> +     * used to resume the VM when precopy failed or cancelled somehow.
> +     * It's never used for postcopy.
> +     */
> +    bool old_vm_running;

I think this comment is right for precopy, but not for colo.  BTW, I
think that I would put the postcopy comment on its use, not here.

/me tries to improve the comment

  Guest was running when we enter the completion stage.  If migration don't
  sucess, we need to continue running guest on source.

What do you think?

Later, Juan.


>  
>      /* Flag set once the migration has been asked to enter postcopy */
>      bool start_postcopy;

Re: [Qemu-devel] [PATCH 06/11] migration: introduce vm_down_start_time

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Introduce MigrationState.vm_down_start_time to replace the local
> variable "start_time" in migration_thread to avoid passing things around.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

But I would suggest renaming the variable?

Later, Juan.


> diff --git a/migration/migration.h b/migration/migration.h
> index 0f5df2367c..3ab5506233 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -107,6 +107,8 @@ struct MigrationState
>      int64_t mig_start_time;
>      /* Total time used by latest migration (ms) */
>      int64_t mig_total_time;
> +    /* Timestamp when VM is down (ms) to migrate the last stuff */
> +    int64_t vm_down_start_time;

downtime_start?


>      int64_t downtime;
>      int64_t expected_downtime;
>      bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];

Re: [Qemu-devel] [PATCH 07/11] migration: introduce migrate_calculate_complete

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Generalize the calculation part when migration complete into a
> function to simplify migration_thread().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>



> ---
>  migration/migration.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 2d8b47197e..acef54748b 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2151,6 +2151,19 @@ bool migrate_colo_enabled(void)
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
>  }
>  
> +static void migration_calculate_complete(MigrationState *s)
> +{
> +    uint64_t bytes = qemu_ftell(s->to_dst_file);
> +    int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +
> +    s->mig_total_time = end_time - s->mig_start_time;

Why have you remove the if (!entered_postcopy) for the following line?

I like the rest/idea of the patch.

Later, Juan.

> +    s->downtime = end_time - s->vm_down_start_time;
> +
> +    if (s->mig_total_time) {
> +        s->mbps = ((double) bytes * 8.0) / s->mig_total_time / 1000;
> +    }
> +}
> +
>  /*
>   * Master migration thread on the source VM.
>   * It drives the migration and pumps the data down the outgoing channel.
> @@ -2168,7 +2181,6 @@ static void *migration_thread(void *opaque)
>       * measured bandwidth
>       */
>      int64_t threshold_size = 0;
> -    int64_t end_time;
>      bool entered_postcopy = false;
>      /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
>      enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
> @@ -2282,19 +2294,10 @@ static void *migration_thread(void *opaque)
>      trace_migration_thread_after_loop();
>      /* If we enabled cpu throttling for auto-converge, turn it off. */
>      cpu_throttle_stop();
> -    end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>  
>      qemu_mutex_lock_iothread();
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
> -        uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
> -        s->mig_total_time = end_time - s->mig_start_time;
> -        if (!entered_postcopy) {
> -            s->downtime = end_time - s->vm_down_start_time;
> -        }
> -        if (s->mig_total_time) {
> -            s->mbps = (((double) transferred_bytes * 8.0) /
> -                       ((double) s->mig_total_time)) / 1000;
> -        }
> +        migration_calculate_complete(s);
>          runstate_set(RUN_STATE_POSTMIGRATE);
>      } else {
>          if (s->state == MIGRATION_STATUS_ACTIVE) {

Re: [Qemu-devel] [PATCH 08/11] migration: use switch at the end of migration

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It converts the old if clauses into switch, explicitly mentions the
> possible migration states.  The old nested "if"s are not clear on what
> we do on different states.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index acef54748b..bfcba24caa 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2296,19 +2296,23 @@ static void *migration_thread(void *opaque)
>      cpu_throttle_stop();
>  
>      qemu_mutex_lock_iothread();
> -    if (s->state == MIGRATION_STATUS_COMPLETED) {
> +    switch (s->state) {
> +    case MIGRATION_STATUS_COMPLETED:
>          migration_calculate_complete(s);
>          runstate_set(RUN_STATE_POSTMIGRATE);
> -    } else {
> -        if (s->state == MIGRATION_STATUS_ACTIVE) {
> -            assert(migrate_colo_enabled());
> -            migrate_start_colo_process(s);
> -            /*
> -            * Fixme: we will run VM in COLO no matter its old running state.
> -            * After exited COLO, we will keep running.
> -            */
> -            s->old_vm_running = true;
> -        }
> +        break;
> +
> +    case MIGRATION_STATUS_ACTIVE:
> +        assert(migrate_colo_enabled());
> +        migrate_start_colo_process(s);

I think this line is wrong, you still have to proctect it with the if
(migrate_cole_enabled).

Rest of cleanup, is something that I like.

Later, Juan.


> +        /*
> +         * Fixme: we will run VM in COLO no matter its old running state.
> +         * After exited COLO, we will keep running.
> +         */
> +        s->old_vm_running = true;
> +        /* Fallthrough */

Actually, I don't like Fallthrought on C, but well, problem is C O:-)

> +    case MIGRATION_STATUS_FAILED:
> +    case MIGRATION_STATUS_CANCELLED:
>          if (s->old_vm_running) {
>              vm_start();
>          } else {
> @@ -2316,6 +2320,12 @@ static void *migration_thread(void *opaque)
>                  runstate_set(RUN_STATE_POSTMIGRATE);
>              }
>          }
> +        break;
> +
> +    default:
> +        /* Should not reach here, but if so, forgive the VM. */
> +        error_report("%s: Unknown ending state %d", __func__, s->state);
> +        break;
>      }
>      qemu_bh_schedule(s->cleanup_bh);
>      qemu_mutex_unlock_iothread();

Re: [Qemu-devel] [PATCH 02/11] migration: qemu_savevm_state_cleanup() in cleanup

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Moving existing callers all into migrate_fd_cleanup().  It simplifies
> migration_thread() a bit.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I am trying to see if we can call migrate_fd_cleanup() twice.  As far as
I can see, we are not doing it.  But, and it is a big but, we are not
checking that we are not calling qemu_savevm_state_cleanup() twice.  If
that happens, we can get double frees and similar.

I put the reviewed-by anyways, because I *think* that we are doing it
right now, and otherwise, we should make sure that we are not calling it
twice, not papering over it.

Once here, I have notice that we call block_cleanup_parameters() in
*three* places.  We call notifier_list_notify() on two of this places (I
can't see any good reason *why* we don't call the notifier for
migrate_fd_cancel).

So, still more review/cleanups to do on this arera.

Later, Juan.

> ---
>  migration/migration.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 0ee4b4c27c..edbda43246 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1077,6 +1077,8 @@ static void migrate_fd_cleanup(void *opaque)
>      qemu_bh_delete(s->cleanup_bh);
>      s->cleanup_bh = NULL;
>  
> +    qemu_savevm_state_cleanup();
> +
>      if (s->to_dst_file) {
>          Error *local_err = NULL;
>  
> @@ -2290,13 +2292,6 @@ static void *migration_thread(void *opaque)
>      end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
>  
>      qemu_mutex_lock_iothread();
> -    /*
> -     * The resource has been allocated by migration will be reused in COLO
> -     * process, so don't release them.
> -     */
> -    if (!enable_colo) {
> -        qemu_savevm_state_cleanup();
> -    }
>      if (s->state == MIGRATION_STATUS_COMPLETED) {
>          uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
>          s->total_time = end_time - s->total_time;
> @@ -2312,7 +2307,6 @@ static void *migration_thread(void *opaque)
>          if (s->state == MIGRATION_STATUS_ACTIVE) {
>              assert(enable_colo);
>              migrate_start_colo_process(s);
> -            qemu_savevm_state_cleanup();
>              /*
>              * Fixme: we will run VM in COLO no matter its old running state.
>              * After exited COLO, we will keep running.

Re: [Qemu-devel] [PATCH 05/11] migration: move vm_old_running into global state

[Peter Xu]

On Wed, Jan 03, 2018 at 10:05:07AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Firstly, it was passed around.  Let's just move it into MigrationState
> > just like many other variables as state of migration.
> >
> > One thing to mention is that for postcopy, we actually don't need this
> > knowledge at all since postcopy can't resume a VM even if it fails (we
> > can see that from the old code too: when we try to resume we also check
> > against "entered_postcopy" variable).  So further we do this:
> >
> > - in postcopy_start(), we don't update vm_old_running since useless
> > - in migration_thread(), we don't need to check entered_postcopy when
> >   resume, since it's only used for precopy.
> >
> > Comment this out too for that variable definition.
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Taken.

> 
> But I wonder if we can came with a better name.  Best one that I can
> think is
>    vm_was_running
> 
> Any other name that I came is bad for precopy or colo.
> 
> i.e. restart_vm_on_cancel_error
> 
> is meaningful for precopy, but not for colo.

Yeah actually spent >10 seconds thinking about a better naming but
failed, so the old one is kept.  I'll use vm_was_running.

> 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 17 +++++++----------
> >  migration/migration.h |  6 ++++++
> >  2 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/migration/migration.h b/migration/migration.h
> > index ac74a12713..0f5df2367c 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -111,6 +111,12 @@ struct MigrationState
> >      int64_t expected_downtime;
> >      bool enabled_capabilities[MIGRATION_CAPABILITY__MAX];
> >      int64_t setup_time;
> > +    /*
> > +     * Whether the old VM is running for the last migration.  This is
> > +     * used to resume the VM when precopy failed or cancelled somehow.
> > +     * It's never used for postcopy.
> > +     */
> > +    bool old_vm_running;
> 
> I think this comment is right for precopy, but not for colo.  BTW, I
> think that I would put the postcopy comment on its use, not here.

Or, how about I just don't mention postcopy at all?

> 
> /me tries to improve the comment
> 
>   Guest was running when we enter the completion stage.  If migration don't
>   sucess, we need to continue running guest on source.
> 
> What do you think?

I think it's generally good.  Maybe a tiny fix like:

  s/Guest was/Whether guest was/
  s/If migration don't sucess/If migration failed/

?  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 04/11] migration: split use of MigrationState.total_time

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Jan 03, 2018 at 09:58:10AM +0100, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > It was used either to:
>> >
>> > 1. store initial timestamp of migration start, and
>> > 2. store total time used by last migration
>> >
>> > Let's provide two parameters for each of them.  Mix use of the two is
>> > slightly misleading.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> 
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> Thanks!
>
>> 
>> If you have to respin, I would like to use the names:
>
> (I think it very possible :-)
>
>> 
>> start_time and total_time, i.e. without the mig_ preffix, because they
>> are in an struct that is clearly named migration O:-)
>
> Oh, it's my bad (or good?) habit of keeping some prefix so that cscope
> won't mix these variables with others.  I think the problem is that
> cscope is always using a global namespace for variables.  Considering
> this do you still like me to change? :) Any suggestions on better
> usage of cscope would be greatly welcomed too!

I only use cscope very ocassionally, so I can't comment about its usage.
As said, I put the reviewed-by anyways.  But if you dont want to use
generic names like start_time/total_time, then please use the full name:

- migration_start_time
- migration_total_time

It is only used a couple of times, and clearer to read.  I normally only
put _prefixes_ if context don't make clear what the variable means.  If
I need *context* I tend to use the full name of things, not
abbreviations.  But yes, not all the code is coherent/consistent.

Later, Juan.


>
> (Sure I can rename that!  It's not a big deal)

Re: [Qemu-devel] [PATCH 04/11] migration: split use of MigrationState.total_time

[Peter Xu]

On Wed, Jan 03, 2018 at 10:20:39AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Wed, Jan 03, 2018 at 09:58:10AM +0100, Juan Quintela wrote:
> >> Peter Xu <peterx@redhat.com> wrote:
> >> > It was used either to:
> >> >
> >> > 1. store initial timestamp of migration start, and
> >> > 2. store total time used by last migration
> >> >
> >> > Let's provide two parameters for each of them.  Mix use of the two is
> >> > slightly misleading.
> >> >
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> 
> >> Reviewed-by: Juan Quintela <quintela@redhat.com>
> >
> > Thanks!
> >
> >> 
> >> If you have to respin, I would like to use the names:
> >
> > (I think it very possible :-)
> >
> >> 
> >> start_time and total_time, i.e. without the mig_ preffix, because they
> >> are in an struct that is clearly named migration O:-)
> >
> > Oh, it's my bad (or good?) habit of keeping some prefix so that cscope
> > won't mix these variables with others.  I think the problem is that
> > cscope is always using a global namespace for variables.  Considering
> > this do you still like me to change? :) Any suggestions on better
> > usage of cscope would be greatly welcomed too!
> 
> I only use cscope very ocassionally, so I can't comment about its usage.
> As said, I put the reviewed-by anyways.  But if you dont want to use
> generic names like start_time/total_time, then please use the full name:
> 
> - migration_start_time
> - migration_total_time
> 
> It is only used a couple of times, and clearer to read.  I normally only
> put _prefixes_ if context don't make clear what the variable means.  If
> I need *context* I tend to use the full name of things, not
> abbreviations.  But yes, not all the code is coherent/consistent.

I'll use the shorter ones.  Thanks!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 02/11] migration: qemu_savevm_state_cleanup() in cleanup

[Peter Xu]

On Wed, Jan 03, 2018 at 10:15:41AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Moving existing callers all into migrate_fd_cleanup().  It simplifies
> > migration_thread() a bit.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks.

> 
> I am trying to see if we can call migrate_fd_cleanup() twice.  As far as
> I can see, we are not doing it.  But, and it is a big but, we are not
> checking that we are not calling qemu_savevm_state_cleanup() twice.  If
> that happens, we can get double frees and similar.
> 
> I put the reviewed-by anyways, because I *think* that we are doing it
> right now, and otherwise, we should make sure that we are not calling it
> twice, not papering over it.
> 
> Once here, I have notice that we call block_cleanup_parameters() in
> *three* places.  We call notifier_list_notify() on two of this places (I
> can't see any good reason *why* we don't call the notifier for
> migrate_fd_cancel).

Indeed.

IMHO we can remove two calls of block_cleanup_parameters(), only keep
the one in migrate_fd_cleanup(), and remove on notifier_list_notify()
in migrate_fd_error() (these can be two more patches).  What do you
think?

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 06/11] migration: introduce vm_down_start_time

[Peter Xu]

On Wed, Jan 03, 2018 at 10:10:21AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Introduce MigrationState.vm_down_start_time to replace the local
> > variable "start_time" in migration_thread to avoid passing things around.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> 
> But I would suggest renaming the variable?
> 
> Later, Juan.
> 
> 
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 0f5df2367c..3ab5506233 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -107,6 +107,8 @@ struct MigrationState
> >      int64_t mig_start_time;
> >      /* Total time used by latest migration (ms) */
> >      int64_t mig_total_time;
> > +    /* Timestamp when VM is down (ms) to migrate the last stuff */
> > +    int64_t vm_down_start_time;
> 
> downtime_start?

Will do.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 09/11] migration: cleanup stats update into function

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> We have quite a few lines in migration_thread() that calculates some
> statistics for the migration interations.  Isolate it into a single
> function to improve readability.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>



> +static void migration_update_statistics(MigrationState *s,


migration_update_counters()?

statistics for me mean that they are only used for informative
purposes.  Here we *act* on that values.


>  
> -            qemu_file_reset_rate_limit(s->to_dst_file);
> -            initial_time = current_time;
> -            initial_bytes = qemu_ftell(s->to_dst_file);
> -        }
> +        /* Conditionally update statistics */

No need for the comment.  If we think it is needed just rename the
function to:
   conditionally_update_statistics()?

I still preffer the:
   migration_update_counters.


> diff --git a/migration/migration.h b/migration/migration.h
> index 3ab5506233..248f7d9a5c 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -90,6 +90,19 @@ struct MigrationState
>      QEMUBH *cleanup_bh;
>      QEMUFile *to_dst_file;
>  
> +    /*
> +     * Migration thread statistic variables, mostly used in
> +     * migration_thread() iterations only.
> +     */
> +    uint64_t initial_bytes;

       /* bytes already send at the beggining of current interation */
       uint64_t iteration_initial_bytes;

> +    int64_t initial_time;
       /* time at the start of current iteration */
       int64_t iteration_start_time;

What do you think?

> +    /*
> +     * The final stage happens when the remaining data is smaller than
> +     * this threshold; it's calculated from the requested downtime and
> +     * measured bandwidth
> +     */
> +    int64_t threshold_size;
> +
>      /* params from 'migrate-set-parameters' */
>      MigrationParameters parameters;

Later, Juan.

Re: [Qemu-devel] [PATCH 10/11] migration: major cleanup for migrate iterations

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> The major work for migration iterations are to move RAM/block/... data
> via qemu_savevm_state_iterate().  Generalize those part into a single
> function.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

I am not fan of having to add yet another state enumeration, but I can
think of a clearer way of doing it, so the reviewed-by.

Thanks, Juan.

Re: [Qemu-devel] [PATCH 11/11] migration: put the finish part into a new function

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> This patch only moved the last part of migration_thread() into a new
> function migration_iteration_finish() to make it much shorter.  With
> previous works to remove some local variables, now it's fairly easy to
> do that.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Thanks, Juan.

Re: [Qemu-devel] [PATCH 02/11] migration: qemu_savevm_state_cleanup() in cleanup

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Jan 03, 2018 at 10:15:41AM +0100, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > Moving existing callers all into migrate_fd_cleanup().  It simplifies
>> > migration_thread() a bit.
>> >
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> 
>> Reviewed-by: Juan Quintela <quintela@redhat.com>
>
> Thanks.
>
>> 
>> I am trying to see if we can call migrate_fd_cleanup() twice.  As far as
>> I can see, we are not doing it.  But, and it is a big but, we are not
>> checking that we are not calling qemu_savevm_state_cleanup() twice.  If
>> that happens, we can get double frees and similar.
>> 
>> I put the reviewed-by anyways, because I *think* that we are doing it
>> right now, and otherwise, we should make sure that we are not calling it
>> twice, not papering over it.
>> 
>> Once here, I have notice that we call block_cleanup_parameters() in
>> *three* places.  We call notifier_list_notify() on two of this places (I
>> can't see any good reason *why* we don't call the notifier for
>> migrate_fd_cancel).
>
> Indeed.
>
> IMHO we can remove two calls of block_cleanup_parameters(), only keep
> the one in migrate_fd_cleanup(), and remove on notifier_list_notify()
> in migrate_fd_error() (these can be two more patches).  What do you
> think?

I think we need to make sure that we have a function that we always
call at the end.  I think that we have that on migration_fd_cleanup(),
so put everything there should be ok, no?

Later, Juan.

Re: [Qemu-devel] [PATCH 05/11] migration: move vm_old_running into global state

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Jan 03, 2018 at 10:05:07AM +0100, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > Firstly, it was passed around.  Let's just move it into MigrationState
>> > just like many other variables as state of migration.
>> >
>> > One thing to mention is that for postcopy, we actually don't need this
>> > knowledge at all since postcopy can't resume a VM even if it fails (we
>> > can see that from the old code too: when we try to resume we also check
>> > against "entered_postcopy" variable).  So further we do this:
>> >
>> > - in postcopy_start(), we don't update vm_old_running since useless
>> > - in migration_thread(), we don't need to check entered_postcopy when
>> >   resume, since it's only used for precopy.
>> >
>> > Comment this out too for that variable definition.

>> I think this comment is right for precopy, but not for colo.  BTW, I
>> think that I would put the postcopy comment on its use, not here.
>
> Or, how about I just don't mention postcopy at all?

Fully agree.
>> 
>> /me tries to improve the comment
>> 
>>   Guest was running when we enter the completion stage.  If migration don't
>>   sucess, we need to continue running guest on source.
>> 
>> What do you think?
>
> I think it's generally good.  Maybe a tiny fix like:
>
>   s/Guest was/Whether guest was/

ok.

>   s/If migration don't sucess/If migration failed/

We also use it in case of migration_cancel.  Cancel is not one error,
that is why I wrote it that way.  What about:

Whether guest was running when we enter the completion stage.  If
migration is interrupted by any reason, we need to continue running the
guest on source.

What do you think?

Later, Juan.


> ?  Thanks,

Re: [Qemu-devel] [PATCH 02/11] migration: qemu_savevm_state_cleanup() in cleanup

[Peter Xu]

On Wed, Jan 03, 2018 at 11:21:31AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Wed, Jan 03, 2018 at 10:15:41AM +0100, Juan Quintela wrote:
> >> Peter Xu <peterx@redhat.com> wrote:
> >> > Moving existing callers all into migrate_fd_cleanup().  It simplifies
> >> > migration_thread() a bit.
> >> >
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> 
> >> Reviewed-by: Juan Quintela <quintela@redhat.com>
> >
> > Thanks.
> >
> >> 
> >> I am trying to see if we can call migrate_fd_cleanup() twice.  As far as
> >> I can see, we are not doing it.  But, and it is a big but, we are not
> >> checking that we are not calling qemu_savevm_state_cleanup() twice.  If
> >> that happens, we can get double frees and similar.
> >> 
> >> I put the reviewed-by anyways, because I *think* that we are doing it
> >> right now, and otherwise, we should make sure that we are not calling it
> >> twice, not papering over it.
> >> 
> >> Once here, I have notice that we call block_cleanup_parameters() in
> >> *three* places.  We call notifier_list_notify() on two of this places (I
> >> can't see any good reason *why* we don't call the notifier for
> >> migrate_fd_cancel).
> >
> > Indeed.
> >
> > IMHO we can remove two calls of block_cleanup_parameters(), only keep
> > the one in migrate_fd_cleanup(), and remove on notifier_list_notify()
> > in migrate_fd_error() (these can be two more patches).  What do you
> > think?
> 
> I think we need to make sure that we have a function that we always
> call at the end.  I think that we have that on migration_fd_cleanup(),
> so put everything there should be ok, no?

IMHO that's exactly what I mean, no? :)

For notifier_list_notify(), it's different - I just remove the extra
one in migrate_fd_error() because it'll be called in
migrate_fd_cleanup() as well, which is a duplicate.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 05/11] migration: move vm_old_running into global state

[Peter Xu]

On Wed, Jan 03, 2018 at 11:26:12AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Wed, Jan 03, 2018 at 10:05:07AM +0100, Juan Quintela wrote:
> >> Peter Xu <peterx@redhat.com> wrote:
> >> > Firstly, it was passed around.  Let's just move it into MigrationState
> >> > just like many other variables as state of migration.
> >> >
> >> > One thing to mention is that for postcopy, we actually don't need this
> >> > knowledge at all since postcopy can't resume a VM even if it fails (we
> >> > can see that from the old code too: when we try to resume we also check
> >> > against "entered_postcopy" variable).  So further we do this:
> >> >
> >> > - in postcopy_start(), we don't update vm_old_running since useless
> >> > - in migration_thread(), we don't need to check entered_postcopy when
> >> >   resume, since it's only used for precopy.
> >> >
> >> > Comment this out too for that variable definition.
> 
> >> I think this comment is right for precopy, but not for colo.  BTW, I
> >> think that I would put the postcopy comment on its use, not here.
> >
> > Or, how about I just don't mention postcopy at all?
> 
> Fully agree.
> >> 
> >> /me tries to improve the comment
> >> 
> >>   Guest was running when we enter the completion stage.  If migration don't
> >>   sucess, we need to continue running guest on source.
> >> 
> >> What do you think?
> >
> > I think it's generally good.  Maybe a tiny fix like:
> >
> >   s/Guest was/Whether guest was/
> 
> ok.
> 
> >   s/If migration don't sucess/If migration failed/
> 
> We also use it in case of migration_cancel.  Cancel is not one error,
> that is why I wrote it that way.  What about:
> 
> Whether guest was running when we enter the completion stage.  If
> migration is interrupted by any reason, we need to continue running the
> guest on source.
> 
> What do you think?

Sure.  I was mostly trying to fix the typo and grammar issue.  Will
take your advise.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 07/11] migration: introduce migrate_calculate_complete

[Peter Xu]

On Wed, Jan 03, 2018 at 10:12:10AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > Generalize the calculation part when migration complete into a
> > function to simplify migration_thread().
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> 
> 
> > ---
> >  migration/migration.c | 25 ++++++++++++++-----------
> >  1 file changed, 14 insertions(+), 11 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 2d8b47197e..acef54748b 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2151,6 +2151,19 @@ bool migrate_colo_enabled(void)
> >      return s->enabled_capabilities[MIGRATION_CAPABILITY_X_COLO];
> >  }
> >  
> > +static void migration_calculate_complete(MigrationState *s)
> > +{
> > +    uint64_t bytes = qemu_ftell(s->to_dst_file);
> > +    int64_t end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> > +
> > +    s->mig_total_time = end_time - s->mig_start_time;
> 
> Why have you remove the if (!entered_postcopy) for the following line?

Ah good catch...  I'll do this:

  if (!s->downtime) {
    /* It is precopy migration */
    ...
  }

Thanks,

> 
> I like the rest/idea of the patch.
> 
> Later, Juan.
> 
> > +    s->downtime = end_time - s->vm_down_start_time;
> > +
> > +    if (s->mig_total_time) {
> > +        s->mbps = ((double) bytes * 8.0) / s->mig_total_time / 1000;
> > +    }
> > +}
> > +
> >  /*
> >   * Master migration thread on the source VM.
> >   * It drives the migration and pumps the data down the outgoing channel.
> > @@ -2168,7 +2181,6 @@ static void *migration_thread(void *opaque)
> >       * measured bandwidth
> >       */
> >      int64_t threshold_size = 0;
> > -    int64_t end_time;
> >      bool entered_postcopy = false;
> >      /* The active state we expect to be in; ACTIVE or POSTCOPY_ACTIVE */
> >      enum MigrationStatus current_active_state = MIGRATION_STATUS_ACTIVE;
> > @@ -2282,19 +2294,10 @@ static void *migration_thread(void *opaque)
> >      trace_migration_thread_after_loop();
> >      /* If we enabled cpu throttling for auto-converge, turn it off. */
> >      cpu_throttle_stop();
> > -    end_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> >  
> >      qemu_mutex_lock_iothread();
> >      if (s->state == MIGRATION_STATUS_COMPLETED) {
> > -        uint64_t transferred_bytes = qemu_ftell(s->to_dst_file);
> > -        s->mig_total_time = end_time - s->mig_start_time;
> > -        if (!entered_postcopy) {
> > -            s->downtime = end_time - s->vm_down_start_time;
> > -        }
> > -        if (s->mig_total_time) {
> > -            s->mbps = (((double) transferred_bytes * 8.0) /
> > -                       ((double) s->mig_total_time)) / 1000;
> > -        }
> > +        migration_calculate_complete(s);
> >          runstate_set(RUN_STATE_POSTMIGRATE);
> >      } else {
> >          if (s->state == MIGRATION_STATUS_ACTIVE) {

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 09/11] migration: cleanup stats update into function

[Peter Xu]

On Wed, Jan 03, 2018 at 11:08:49AM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > We have quite a few lines in migration_thread() that calculates some
> > statistics for the migration interations.  Isolate it into a single
> > function to improve readability.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> 
> 
> > +static void migration_update_statistics(MigrationState *s,
> 
> 
> migration_update_counters()?

Sure, or...

> 
> statistics for me mean that they are only used for informative
> purposes.  Here we *act* on that values.
> 
> 
> >  
> > -            qemu_file_reset_rate_limit(s->to_dst_file);
> > -            initial_time = current_time;
> > -            initial_bytes = qemu_ftell(s->to_dst_file);
> > -        }
> > +        /* Conditionally update statistics */
> 
> No need for the comment.  If we think it is needed just rename the
> function to:
>    conditionally_update_statistics()?
> 
> I still preffer the:
>    migration_update_counters.

... migration_update_counters_conditionally()?

> 
> 
> > diff --git a/migration/migration.h b/migration/migration.h
> > index 3ab5506233..248f7d9a5c 100644
> > --- a/migration/migration.h
> > +++ b/migration/migration.h
> > @@ -90,6 +90,19 @@ struct MigrationState
> >      QEMUBH *cleanup_bh;
> >      QEMUFile *to_dst_file;
> >  
> > +    /*
> > +     * Migration thread statistic variables, mostly used in
> > +     * migration_thread() iterations only.
> > +     */
> > +    uint64_t initial_bytes;
> 
>        /* bytes already send at the beggining of current interation */
>        uint64_t iteration_initial_bytes;
> 
> > +    int64_t initial_time;
>        /* time at the start of current iteration */
>        int64_t iteration_start_time;
> 
> What do you think?

Will change both.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 09/11] migration: cleanup stats update into function

[Peter Xu]

On Wed, Jan 03, 2018 at 06:55:29PM +0800, Peter Xu wrote:
> On Wed, Jan 03, 2018 at 11:08:49AM +0100, Juan Quintela wrote:
> > Peter Xu <peterx@redhat.com> wrote:
> > > We have quite a few lines in migration_thread() that calculates some
> > > statistics for the migration interations.  Isolate it into a single
> > > function to improve readability.
> > >
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > 
> > 
> > 
> > > +static void migration_update_statistics(MigrationState *s,
> > 
> > 
> > migration_update_counters()?
> 
> Sure, or...
> 
> > 
> > statistics for me mean that they are only used for informative
> > purposes.  Here we *act* on that values.
> > 
> > 
> > >  
> > > -            qemu_file_reset_rate_limit(s->to_dst_file);
> > > -            initial_time = current_time;
> > > -            initial_bytes = qemu_ftell(s->to_dst_file);
> > > -        }
> > > +        /* Conditionally update statistics */
> > 
> > No need for the comment.  If we think it is needed just rename the
> > function to:
> >    conditionally_update_statistics()?
> > 
> > I still preffer the:
> >    migration_update_counters.
> 
> ... migration_update_counters_conditionally()?

Forget that... It's too long to be liked.

I'll use migration_update_counters.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 02/11] migration: qemu_savevm_state_cleanup() in cleanup

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Wed, Jan 03, 2018 at 11:21:31AM +0100, Juan Quintela wrote:
>> Peter Xu <peterx@redhat.com> wrote:
>> > On Wed, Jan 03, 2018 at 10:15:41AM +0100, Juan Quintela wrote:
>> >> Peter Xu <peterx@redhat.com> wrote:
>> >> > Moving existing callers all into migrate_fd_cleanup().  It simplifies
>> >> > migration_thread() a bit.
>> >> >
>> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> >> 
>> >> Reviewed-by: Juan Quintela <quintela@redhat.com>
>> >
>> > Thanks.
>> >
>> >> 
>> >> I am trying to see if we can call migrate_fd_cleanup() twice.  As far as
>> >> I can see, we are not doing it.  But, and it is a big but, we are not
>> >> checking that we are not calling qemu_savevm_state_cleanup() twice.  If
>> >> that happens, we can get double frees and similar.
>> >> 
>> >> I put the reviewed-by anyways, because I *think* that we are doing it
>> >> right now, and otherwise, we should make sure that we are not calling it
>> >> twice, not papering over it.
>> >> 
>> >> Once here, I have notice that we call block_cleanup_parameters() in
>> >> *three* places.  We call notifier_list_notify() on two of this places (I
>> >> can't see any good reason *why* we don't call the notifier for
>> >> migrate_fd_cancel).
>> >
>> > Indeed.
>> >
>> > IMHO we can remove two calls of block_cleanup_parameters(), only keep
>> > the one in migrate_fd_cleanup(), and remove on notifier_list_notify()
>> > in migrate_fd_error() (these can be two more patches).  What do you
>> > think?
>> 
>> I think we need to make sure that we have a function that we always
>> call at the end.  I think that we have that on migration_fd_cleanup(),
>> so put everything there should be ok, no?
>
> IMHO that's exactly what I mean, no? :)
>
> For notifier_list_notify(), it's different - I just remove the extra
> one in migrate_fd_error() because it'll be called in
> migrate_fd_cleanup() as well, which is a duplicate.

then what call the one when we do a cancel?  the one in cleanup also?

Thanks, Juan.

Re: [Qemu-devel] [PATCH 02/11] migration: qemu_savevm_state_cleanup() in cleanup

[Peter Xu]

On Wed, Jan 03, 2018 at 12:18:54PM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Wed, Jan 03, 2018 at 11:21:31AM +0100, Juan Quintela wrote:
> >> Peter Xu <peterx@redhat.com> wrote:
> >> > On Wed, Jan 03, 2018 at 10:15:41AM +0100, Juan Quintela wrote:
> >> >> Peter Xu <peterx@redhat.com> wrote:
> >> >> > Moving existing callers all into migrate_fd_cleanup().  It simplifies
> >> >> > migration_thread() a bit.
> >> >> >
> >> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> >> 
> >> >> Reviewed-by: Juan Quintela <quintela@redhat.com>
> >> >
> >> > Thanks.
> >> >
> >> >> 
> >> >> I am trying to see if we can call migrate_fd_cleanup() twice.  As far as
> >> >> I can see, we are not doing it.  But, and it is a big but, we are not
> >> >> checking that we are not calling qemu_savevm_state_cleanup() twice.  If
> >> >> that happens, we can get double frees and similar.
> >> >> 
> >> >> I put the reviewed-by anyways, because I *think* that we are doing it
> >> >> right now, and otherwise, we should make sure that we are not calling it
> >> >> twice, not papering over it.
> >> >> 
> >> >> Once here, I have notice that we call block_cleanup_parameters() in
> >> >> *three* places.  We call notifier_list_notify() on two of this places (I
> >> >> can't see any good reason *why* we don't call the notifier for
> >> >> migrate_fd_cancel).
> >> >
> >> > Indeed.
> >> >
> >> > IMHO we can remove two calls of block_cleanup_parameters(), only keep
> >> > the one in migrate_fd_cleanup(), and remove on notifier_list_notify()
> >> > in migrate_fd_error() (these can be two more patches).  What do you
> >> > think?
> >> 
> >> I think we need to make sure that we have a function that we always
> >> call at the end.  I think that we have that on migration_fd_cleanup(),
> >> so put everything there should be ok, no?
> >
> > IMHO that's exactly what I mean, no? :)
> >
> > For notifier_list_notify(), it's different - I just remove the extra
> > one in migrate_fd_error() because it'll be called in
> > migrate_fd_cleanup() as well, which is a duplicate.
> 
> then what call the one when we do a cancel?  the one in cleanup also?

Yes, IIUC migrate_fd_cancel() tells the thread to cancel, then the
migrate_fd_cleanup() is called there (finally in the bottom half of
main loop, of course).

-- 
Peter Xu