[Qemu-devel] [PATCH v3 00/10] migration: compression optimization

[Qemu-devel] [PATCH v3 00/10] migration: compression optimization

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Changelog in v3:

Thanks to Peter's comments, the changes in this version are:
1) make compress-wait-thread be true on default to keep current behavior
2) save the compressed-size instead of reduced size and fix calculating
   compression ratio
3) fix calculating xbzrle_counters.cache_miss_rate and
   compression_counters.busy_rate


Xiao Guangrong (10):
  migration: do not wait for free thread
  migration: fix counting normal page for compression
  migration: introduce save_zero_page_to_file
  migration: drop the return value of do_compress_ram_page
  migration: move handle of zero page to the thread
  migration: hold the lock only if it is really needed
  migration: do not flush_compressed_data at the end of each iteration
  migration: handle the error condition properly
  migration: fix calculating xbzrle_counters.cache_miss_rate
  migration: show the statistics of compression

 hmp.c                 |  21 ++++
 include/qemu/queue.h  |   1 +
 migration/migration.c |  33 ++++++
 migration/migration.h |   1 +
 migration/ram.c       | 283 +++++++++++++++++++++++++++++++++++++-------------
 migration/ram.h       |   1 +
 qapi/migration.json   |  49 +++++++--
 7 files changed, 308 insertions(+), 81 deletions(-)

-- 
2.14.4

[Qemu-devel] [PATCH v3 01/10] migration: do not wait for free thread

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Instead of putting the main thread to sleep state to wait for
free compression thread, we can directly post it out as normal
page that reduces the latency and uses CPUs more efficiently

A parameter, compress-wait-thread, is introduced, it can be
enabled if the user really wants the old behavior

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hmp.c                 |  8 ++++++++
 migration/migration.c | 21 +++++++++++++++++++++
 migration/migration.h |  1 +
 migration/ram.c       | 45 ++++++++++++++++++++++++++-------------------
 qapi/migration.json   | 23 ++++++++++++++++++-----
 5 files changed, 74 insertions(+), 24 deletions(-)

diff --git a/hmp.c b/hmp.c
index 2aafb50e8e..47d36e3ccf 100644
--- a/hmp.c
+++ b/hmp.c
@@ -327,6 +327,10 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_THREADS),
             params->compress_threads);
+        assert(params->has_compress_wait_thread);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD),
+            params->compress_wait_thread ? "on" : "off");
         assert(params->has_decompress_threads);
         monitor_printf(mon, "%s: %u\n",
             MigrationParameter_str(MIGRATION_PARAMETER_DECOMPRESS_THREADS),
@@ -1623,6 +1627,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         p->has_compress_threads = true;
         visit_type_int(v, param, &p->compress_threads, &err);
         break;
+    case MIGRATION_PARAMETER_COMPRESS_WAIT_THREAD:
+        p->has_compress_wait_thread = true;
+        visit_type_bool(v, param, &p->compress_wait_thread, &err);
+        break;
     case MIGRATION_PARAMETER_DECOMPRESS_THREADS:
         p->has_decompress_threads = true;
         visit_type_int(v, param, &p->decompress_threads, &err);
diff --git a/migration/migration.c b/migration/migration.c
index b7d9854bda..2ccaadc03d 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -671,6 +671,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->compress_level = s->parameters.compress_level;
     params->has_compress_threads = true;
     params->compress_threads = s->parameters.compress_threads;
+    params->has_compress_wait_thread = true;
+    params->compress_wait_thread = s->parameters.compress_wait_thread;
     params->has_decompress_threads = true;
     params->decompress_threads = s->parameters.decompress_threads;
     params->has_cpu_throttle_initial = true;
@@ -1061,6 +1063,10 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
         dest->compress_threads = params->compress_threads;
     }
 
+    if (params->has_compress_wait_thread) {
+        dest->compress_wait_thread = params->compress_wait_thread;
+    }
+
     if (params->has_decompress_threads) {
         dest->decompress_threads = params->decompress_threads;
     }
@@ -1126,6 +1132,10 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         s->parameters.compress_threads = params->compress_threads;
     }
 
+    if (params->has_compress_wait_thread) {
+        s->parameters.compress_wait_thread = params->compress_wait_thread;
+    }
+
     if (params->has_decompress_threads) {
         s->parameters.decompress_threads = params->decompress_threads;
     }
@@ -1871,6 +1881,15 @@ int migrate_compress_threads(void)
     return s->parameters.compress_threads;
 }
 
+int migrate_compress_wait_thread(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->parameters.compress_wait_thread;
+}
+
 int migrate_decompress_threads(void)
 {
     MigrationState *s;
@@ -3131,6 +3150,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_UINT8("x-compress-threads", MigrationState,
                       parameters.compress_threads,
                       DEFAULT_MIGRATE_COMPRESS_THREAD_COUNT),
+    DEFINE_PROP_BOOL("x-compress-wait-thread", MigrationState,
+                      parameters.compress_wait_thread, true),
     DEFINE_PROP_UINT8("x-decompress-threads", MigrationState,
                       parameters.decompress_threads,
                       DEFAULT_MIGRATE_DECOMPRESS_THREAD_COUNT),
diff --git a/migration/migration.h b/migration/migration.h
index 64a7b33735..a46b9e6c8d 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -271,6 +271,7 @@ bool migrate_use_return_path(void);
 bool migrate_use_compression(void);
 int migrate_compress_level(void);
 int migrate_compress_threads(void);
+int migrate_compress_wait_thread(void);
 int migrate_decompress_threads(void);
 bool migrate_use_events(void);
 bool migrate_postcopy_blocktime(void);
diff --git a/migration/ram.c b/migration/ram.c
index 24dea2730c..ae9e83c2b6 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1889,30 +1889,34 @@ static int compress_page_with_multi_thread(RAMState *rs, RAMBlock *block,
                                            ram_addr_t offset)
 {
     int idx, thread_count, bytes_xmit = -1, pages = -1;
+    bool wait = migrate_compress_wait_thread();
 
     thread_count = migrate_compress_threads();
     qemu_mutex_lock(&comp_done_lock);
-    while (true) {
-        for (idx = 0; idx < thread_count; idx++) {
-            if (comp_param[idx].done) {
-                comp_param[idx].done = false;
-                bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
-                qemu_mutex_lock(&comp_param[idx].mutex);
-                set_compress_params(&comp_param[idx], block, offset);
-                qemu_cond_signal(&comp_param[idx].cond);
-                qemu_mutex_unlock(&comp_param[idx].mutex);
-                pages = 1;
-                ram_counters.normal++;
-                ram_counters.transferred += bytes_xmit;
-                break;
-            }
-        }
-        if (pages > 0) {
+retry:
+    for (idx = 0; idx < thread_count; idx++) {
+        if (comp_param[idx].done) {
+            comp_param[idx].done = false;
+            bytes_xmit = qemu_put_qemu_file(rs->f, comp_param[idx].file);
+            qemu_mutex_lock(&comp_param[idx].mutex);
+            set_compress_params(&comp_param[idx], block, offset);
+            qemu_cond_signal(&comp_param[idx].cond);
+            qemu_mutex_unlock(&comp_param[idx].mutex);
+            pages = 1;
+            ram_counters.normal++;
+            ram_counters.transferred += bytes_xmit;
             break;
-        } else {
-            qemu_cond_wait(&comp_done_cond, &comp_done_lock);
         }
     }
+
+    /*
+     * wait for the free thread if the user specifies 'compress-wait-thread',
+     * otherwise we will post the page out in the main thread as normal page.
+     */
+    if (pages < 0 && wait) {
+        qemu_cond_wait(&comp_done_cond, &comp_done_lock);
+        goto retry;
+    }
     qemu_mutex_unlock(&comp_done_lock);
 
     return pages;
@@ -2226,7 +2230,10 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
      * CPU resource.
      */
     if (block == rs->last_sent_block && save_page_use_compression(rs)) {
-        return compress_page_with_multi_thread(rs, block, offset);
+        res = compress_page_with_multi_thread(rs, block, offset);
+        if (res > 0) {
+            return res;
+        }
     } else if (migrate_use_multifd()) {
         return ram_save_multifd_page(rs, block, offset);
     }
diff --git a/qapi/migration.json b/qapi/migration.json
index 186e8a7303..e6991fcbd2 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -462,6 +462,11 @@
 # @compress-threads: Set compression thread count to be used in live migration,
 #          the compression thread count is an integer between 1 and 255.
 #
+# @compress-wait-thread: Wait if no thread is free to compress the memory page
+#          if it's enabled, otherwise, the page will be posted out immediately
+#          in the main thread without compression. It's true on default.
+#          (Since: 3.1)
+#
 # @decompress-threads: Set decompression thread count to be used in live
 #          migration, the decompression thread count is an integer between 1
 #          and 255. Usually, decompression is at least 4 times as fast as
@@ -526,11 +531,11 @@
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
-  'data': ['compress-level', 'compress-threads', 'decompress-threads',
-           'cpu-throttle-initial', 'cpu-throttle-increment',
-           'tls-creds', 'tls-hostname', 'max-bandwidth',
-           'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
-           'x-multifd-channels', 'x-multifd-page-count',
+  'data': ['compress-level', 'compress-threads', 'compress-wait-thread',
+           'decompress-threads', 'cpu-throttle-initial',
+           'cpu-throttle-increment', 'tls-creds', 'tls-hostname',
+           'max-bandwidth', 'downtime-limit', 'x-checkpoint-delay',
+           'block-incremental', 'x-multifd-channels', 'x-multifd-page-count',
            'xbzrle-cache-size', 'max-postcopy-bandwidth' ] }
 
 ##
@@ -540,6 +545,9 @@
 #
 # @compress-threads: compression thread count
 #
+# @compress-wait-thread: Wait if no thread is free to compress the memory page
+#                        (Since: 3.1)
+#
 # @decompress-threads: decompression thread count
 #
 # @cpu-throttle-initial: Initial percentage of time guest cpus are
@@ -610,6 +618,7 @@
 { 'struct': 'MigrateSetParameters',
   'data': { '*compress-level': 'int',
             '*compress-threads': 'int',
+            '*compress-wait-thread': 'bool',
             '*decompress-threads': 'int',
             '*cpu-throttle-initial': 'int',
             '*cpu-throttle-increment': 'int',
@@ -649,6 +658,9 @@
 #
 # @compress-threads: compression thread count
 #
+# @compress-wait-thread: Wait if no thread is free to compress the memory page
+#                        (Since: 3.1)
+#
 # @decompress-threads: decompression thread count
 #
 # @cpu-throttle-initial: Initial percentage of time guest cpus are
@@ -714,6 +726,7 @@
 { 'struct': 'MigrationParameters',
   'data': { '*compress-level': 'uint8',
             '*compress-threads': 'uint8',
+            '*compress-wait-thread': 'bool',
             '*decompress-threads': 'uint8',
             '*cpu-throttle-initial': 'uint8',
             '*cpu-throttle-increment': 'uint8',
-- 
2.14.4

[Qemu-devel] [PATCH v3 02/10] migration: fix counting normal page for compression

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

The compressed page is not normal page

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index ae9e83c2b6..d631b9a6fe 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1903,7 +1903,6 @@ retry:
             qemu_cond_signal(&comp_param[idx].cond);
             qemu_mutex_unlock(&comp_param[idx].mutex);
             pages = 1;
-            ram_counters.normal++;
             ram_counters.transferred += bytes_xmit;
             break;
         }
-- 
2.14.4

[Qemu-devel] [PATCH v3 03/10] migration: introduce save_zero_page_to_file

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

It will be used by the compression threads

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 40 ++++++++++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 10 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index d631b9a6fe..49ace30614 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1667,27 +1667,47 @@ static void migration_bitmap_sync(RAMState *rs)
 /**
  * save_zero_page: send the zero page to the stream
  *
- * Returns the number of pages written.
+ * Returns the size of data written to the file, 0 means the page is not
+ * a zero page
  *
  * @rs: current RAM state
+ * @file: the file where the data is saved
  * @block: block that contains the page we want to send
  * @offset: offset inside the block for the page
  */
-static int save_zero_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
+static int save_zero_page_to_file(RAMState *rs, QEMUFile *file,
+                                  RAMBlock *block, ram_addr_t offset)
 {
     uint8_t *p = block->host + offset;
-    int pages = -1;
+    int len = 0;
 
     if (is_zero_range(p, TARGET_PAGE_SIZE)) {
-        ram_counters.duplicate++;
-        ram_counters.transferred +=
-            save_page_header(rs, rs->f, block, offset | RAM_SAVE_FLAG_ZERO);
-        qemu_put_byte(rs->f, 0);
-        ram_counters.transferred += 1;
-        pages = 1;
+        len += save_page_header(rs, file, block, offset | RAM_SAVE_FLAG_ZERO);
+        qemu_put_byte(file, 0);
+        len += 1;
     }
+    return len;
+}
 
-    return pages;
+/**
+ * save_zero_page: send the zero page to the stream
+ *
+ * Returns the number of pages written.
+ *
+ * @rs: current RAM state
+ * @block: block that contains the page we want to send
+ * @offset: offset inside the block for the page
+ */
+static int save_zero_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
+{
+    int len = save_zero_page_to_file(rs, rs->f, block, offset);
+
+    if (len) {
+        ram_counters.duplicate++;
+        ram_counters.transferred += len;
+        return 1;
+    }
+    return -1;
 }
 
 static void ram_release_pages(const char *rbname, uint64_t offset, int pages)
-- 
2.14.4

[Qemu-devel] [PATCH v3 04/10] migration: drop the return value of do_compress_ram_page

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

It is not used and cleans the code up a little

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 49ace30614..e463de4f69 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -381,8 +381,8 @@ static QemuThread *decompress_threads;
 static QemuMutex decomp_done_lock;
 static QemuCond decomp_done_cond;
 
-static int do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
-                                ram_addr_t offset, uint8_t *source_buf);
+static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
+                                 ram_addr_t offset, uint8_t *source_buf);
 
 static void *do_data_compress(void *opaque)
 {
@@ -1842,15 +1842,14 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
     return 1;
 }
 
-static int do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
-                                ram_addr_t offset, uint8_t *source_buf)
+static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
+                                 ram_addr_t offset, uint8_t *source_buf)
 {
     RAMState *rs = ram_state;
-    int bytes_sent, blen;
     uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
+    int ret;
 
-    bytes_sent = save_page_header(rs, f, block, offset |
-                                  RAM_SAVE_FLAG_COMPRESS_PAGE);
+    save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
 
     /*
      * copy it to a internal buffer to avoid it being modified by VM
@@ -1858,17 +1857,14 @@ static int do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
      * decompression
      */
     memcpy(source_buf, p, TARGET_PAGE_SIZE);
-    blen = qemu_put_compression_data(f, stream, source_buf, TARGET_PAGE_SIZE);
-    if (blen < 0) {
-        bytes_sent = 0;
-        qemu_file_set_error(migrate_get_current()->to_dst_file, blen);
+    ret = qemu_put_compression_data(f, stream, source_buf, TARGET_PAGE_SIZE);
+    if (ret < 0) {
+        qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
         error_report("compressed data failed!");
-    } else {
-        bytes_sent += blen;
-        ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
+        return;
     }
 
-    return bytes_sent;
+    ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
 }
 
 static void flush_compressed_data(RAMState *rs)
-- 
2.14.4

[Qemu-devel] [PATCH v3 05/10] migration: move handle of zero page to the thread

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Detecting zero page is not a light work, moving it to the thread to
speed the main thread up, btw, handling ram_release_pages() for the
zero page is moved to the thread as well

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 96 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 70 insertions(+), 26 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index e463de4f69..d804d01aae 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -340,6 +340,7 @@ typedef struct PageSearchStatus PageSearchStatus;
 struct CompressParam {
     bool done;
     bool quit;
+    bool zero_page;
     QEMUFile *file;
     QemuMutex mutex;
     QemuCond cond;
@@ -381,7 +382,7 @@ static QemuThread *decompress_threads;
 static QemuMutex decomp_done_lock;
 static QemuCond decomp_done_cond;
 
-static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
+static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
                                  ram_addr_t offset, uint8_t *source_buf);
 
 static void *do_data_compress(void *opaque)
@@ -389,6 +390,7 @@ static void *do_data_compress(void *opaque)
     CompressParam *param = opaque;
     RAMBlock *block;
     ram_addr_t offset;
+    bool zero_page;
 
     qemu_mutex_lock(&param->mutex);
     while (!param->quit) {
@@ -398,11 +400,12 @@ static void *do_data_compress(void *opaque)
             param->block = NULL;
             qemu_mutex_unlock(&param->mutex);
 
-            do_compress_ram_page(param->file, &param->stream, block, offset,
-                                 param->originbuf);
+            zero_page = do_compress_ram_page(param->file, &param->stream,
+                                             block, offset, param->originbuf);
 
             qemu_mutex_lock(&comp_done_lock);
             param->done = true;
+            param->zero_page = zero_page;
             qemu_cond_signal(&comp_done_cond);
             qemu_mutex_unlock(&comp_done_lock);
 
@@ -1842,13 +1845,19 @@ static int ram_save_multifd_page(RAMState *rs, RAMBlock *block,
     return 1;
 }
 
-static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
+static bool do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
                                  ram_addr_t offset, uint8_t *source_buf)
 {
     RAMState *rs = ram_state;
     uint8_t *p = block->host + (offset & TARGET_PAGE_MASK);
+    bool zero_page = false;
     int ret;
 
+    if (save_zero_page_to_file(rs, f, block, offset)) {
+        zero_page = true;
+        goto exit;
+    }
+
     save_page_header(rs, f, block, offset | RAM_SAVE_FLAG_COMPRESS_PAGE);
 
     /*
@@ -1861,10 +1870,21 @@ static void do_compress_ram_page(QEMUFile *f, z_stream *stream, RAMBlock *block,
     if (ret < 0) {
         qemu_file_set_error(migrate_get_current()->to_dst_file, ret);
         error_report("compressed data failed!");
-        return;
+        return false;
     }
 
+exit:
     ram_release_pages(block->idstr, offset & TARGET_PAGE_MASK, 1);
+    return zero_page;
+}
+
+static void
+update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
+{
+    if (param->zero_page) {
+        ram_counters.duplicate++;
+    }
+    ram_counters.transferred += bytes_xmit;
 }
 
 static void flush_compressed_data(RAMState *rs)
@@ -1888,7 +1908,12 @@ static void flush_compressed_data(RAMState *rs)
         qemu_mutex_lock(&comp_param[idx].mutex);
         if (!comp_param[idx].quit) {
             len = qemu_put_qemu_file(rs->f, comp_param[idx].file);
-            ram_counters.transferred += len;
+            /*
+             * it's safe to fetch zero_page without holding comp_done_lock
+             * as there is no further request submitted to the thread,
+             * i.e, the thread should be waiting for a request at this point.
+             */
+            update_compress_thread_counts(&comp_param[idx], len);
         }
         qemu_mutex_unlock(&comp_param[idx].mutex);
     }
@@ -1919,7 +1944,7 @@ retry:
             qemu_cond_signal(&comp_param[idx].cond);
             qemu_mutex_unlock(&comp_param[idx].mutex);
             pages = 1;
-            ram_counters.transferred += bytes_xmit;
+            update_compress_thread_counts(&comp_param[idx], bytes_xmit);
             break;
         }
     }
@@ -2193,6 +2218,39 @@ static bool save_page_use_compression(RAMState *rs)
     return false;
 }
 
+/*
+ * try to compress the page before posting it out, return true if the page
+ * has been properly handled by compression, otherwise needs other
+ * paths to handle it
+ */
+static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
+{
+    if (!save_page_use_compression(rs)) {
+        return false;
+    }
+
+    /*
+     * When starting the process of a new block, the first page of
+     * the block should be sent out before other pages in the same
+     * block, and all the pages in last block should have been sent
+     * out, keeping this order is important, because the 'cont' flag
+     * is used to avoid resending the block name.
+     *
+     * We post the fist page as normal page as compression will take
+     * much CPU resource.
+     */
+    if (block != rs->last_sent_block) {
+        flush_compressed_data(rs);
+        return false;
+    }
+
+    if (compress_page_with_multi_thread(rs, block, offset) > 0) {
+        return true;
+    }
+
+    return false;
+}
+
 /**
  * ram_save_target_page: save one target page
  *
@@ -2213,15 +2271,8 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
         return res;
     }
 
-    /*
-     * When starting the process of a new block, the first page of
-     * the block should be sent out before other pages in the same
-     * block, and all the pages in last block should have been sent
-     * out, keeping this order is important, because the 'cont' flag
-     * is used to avoid resending the block name.
-     */
-    if (block != rs->last_sent_block && save_page_use_compression(rs)) {
-            flush_compressed_data(rs);
+    if (save_compress_page(rs, block, offset)) {
+        return 1;
     }
 
     res = save_zero_page(rs, block, offset);
@@ -2239,17 +2290,10 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
     }
 
     /*
-     * Make sure the first page is sent out before other pages.
-     *
-     * we post it as normal page as compression will take much
-     * CPU resource.
+     * do not use multifd for compression as the first page in the new
+     * block should be posted out before sending the compressed page
      */
-    if (block == rs->last_sent_block && save_page_use_compression(rs)) {
-        res = compress_page_with_multi_thread(rs, block, offset);
-        if (res > 0) {
-            return res;
-        }
-    } else if (migrate_use_multifd()) {
+    if (!save_page_use_compression(rs) && migrate_use_multifd()) {
         return ram_save_multifd_page(rs, block, offset);
     }
 
-- 
2.14.4

[Qemu-devel] [PATCH v3 06/10] migration: hold the lock only if it is really needed

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Try to hold src_page_req_mutex only if the queue is not
empty

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 include/qemu/queue.h | 1 +
 migration/ram.c      | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index 59fd1203a1..ac418efc43 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -341,6 +341,7 @@ struct {                                                                \
 /*
  * Simple queue access methods.
  */
+#define QSIMPLEQ_EMPTY_ATOMIC(head) (atomic_read(&((head)->sqh_first)) == NULL)
 #define QSIMPLEQ_EMPTY(head)        ((head)->sqh_first == NULL)
 #define QSIMPLEQ_FIRST(head)        ((head)->sqh_first)
 #define QSIMPLEQ_NEXT(elm, field)   ((elm)->field.sqe_next)
diff --git a/migration/ram.c b/migration/ram.c
index d804d01aae..99ecf9b315 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2026,6 +2026,10 @@ static RAMBlock *unqueue_page(RAMState *rs, ram_addr_t *offset)
 {
     RAMBlock *block = NULL;
 
+    if (QSIMPLEQ_EMPTY_ATOMIC(&rs->src_page_requests)) {
+        return NULL;
+    }
+
     qemu_mutex_lock(&rs->src_page_req_mutex);
     if (!QSIMPLEQ_EMPTY(&rs->src_page_requests)) {
         struct RAMSrcPageRequest *entry =
-- 
2.14.4

[Qemu-devel] [PATCH v3 07/10] migration: do not flush_compressed_data at the end of each iteration

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

flush_compressed_data() needs to wait all compression threads to
finish their work, after that all threads are free until the
migration feeds new request to them, reducing its call can improve
the throughput and use CPU resource more effectively

We do not need to flush all threads at the end of iteration, the
data can be kept locally until the memory block is changed or
memory migration starts over in that case we will meet a dirtied
page which may still exists in compression threads's ring

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 99ecf9b315..55966bc2c1 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -306,6 +306,8 @@ struct RAMState {
     uint64_t iterations;
     /* number of dirty bits in the bitmap */
     uint64_t migration_dirty_pages;
+    /* last dirty_sync_count we have seen */
+    uint64_t dirty_sync_count_prev;
     /* protects modification of the bitmap */
     QemuMutex bitmap_mutex;
     /* The RAMBlock used in the last src_page_requests */
@@ -3173,6 +3175,17 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
 
     ram_control_before_iterate(f, RAM_CONTROL_ROUND);
 
+    /*
+     * if memory migration starts over, we will meet a dirtied page which
+     * may still exists in compression threads's ring, so we should flush
+     * the compressed data to make sure the new page is not overwritten by
+     * the old one in the destination.
+     */
+    if (ram_counters.dirty_sync_count != rs->dirty_sync_count_prev) {
+        rs->dirty_sync_count_prev = ram_counters.dirty_sync_count;
+        flush_compressed_data(rs);
+    }
+
     t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     i = 0;
     while ((ret = qemu_file_rate_limit(f)) == 0 ||
@@ -3205,7 +3218,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         }
         i++;
     }
-    flush_compressed_data(rs);
     rcu_read_unlock();
 
     /*
-- 
2.14.4

[Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

ram_find_and_save_block() can return negative if any error hanppens,
however, it is completely ignored in current code

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 55966bc2c1..09be01dca2 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2367,7 +2367,8 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
  *
  * Called within an RCU critical section.
  *
- * Returns the number of pages written where zero means no dirty pages
+ * Returns the number of pages written where zero means no dirty pages,
+ * or negative on error
  *
  * @rs: current RAM state
  * @last_stage: if we are at the completion stage
@@ -3202,6 +3203,12 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
             done = 1;
             break;
         }
+
+        if (pages < 0) {
+            qemu_file_set_error(f, pages);
+            break;
+        }
+
         rs->iterations++;
 
         /* we want to check in the 1st loop, just in case it was the 1st time
@@ -3243,7 +3250,7 @@ out:
 /**
  * ram_save_complete: function called to send the remaining amount of ram
  *
- * Returns zero to indicate success
+ * Returns zero to indicate success or negative on error
  *
  * Called with iothread lock
  *
@@ -3254,6 +3261,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
 {
     RAMState **temp = opaque;
     RAMState *rs = *temp;
+    int ret = 0;
 
     rcu_read_lock();
 
@@ -3274,6 +3282,10 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
         if (pages == 0) {
             break;
         }
+        if (pages < 0) {
+            ret = pages;
+            break;
+        }
     }
 
     flush_compressed_data(rs);
@@ -3285,7 +3297,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
     qemu_fflush(f);
 
-    return 0;
+    return ret;
 }
 
 static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
-- 
2.14.4

[Qemu-devel] [PATCH v3 09/10] migration: fix calculating xbzrle_counters.cache_miss_rate

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

As Peter pointed out:
| - xbzrle_counters.cache_miss is done in save_xbzrle_page(), so it's
|   per-guest-page granularity
|
| - RAMState.iterations is done for each ram_find_and_save_block(), so
|   it's per-host-page granularity
|
| An example is that when we migrate a 2M huge page in the guest, we
| will only increase the RAMState.iterations by 1 (since
| ram_find_and_save_block() will be called once), but we might increase
| xbzrle_counters.cache_miss for 2M/4K=512 times (we'll call
| save_xbzrle_page() that many times) if all the pages got cache miss.
| Then IMHO the cache miss rate will be 512/1=51200% (while it should
| actually be just 100% cache miss).

And he also suggested as xbzrle_counters.cache_miss_rate is the only
user of rs->iterations we can adapt it to count guest page numbers

After that, rename 'iterations' to 'handle_pages' to better reflect
its meaning

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 migration/ram.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 09be01dca2..bd7c18d1f9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -300,10 +300,10 @@ struct RAMState {
     uint64_t num_dirty_pages_period;
     /* xbzrle misses since the beginning of the period */
     uint64_t xbzrle_cache_miss_prev;
-    /* number of iterations at the beginning of period */
-    uint64_t iterations_prev;
-    /* Iterations since start */
-    uint64_t iterations;
+    /* total handled pages at the beginning of period */
+    uint64_t handle_pages_prev;
+    /* total handled pages since start */
+    uint64_t handle_pages;
     /* number of dirty bits in the bitmap */
     uint64_t migration_dirty_pages;
     /* last dirty_sync_count we have seen */
@@ -1587,19 +1587,19 @@ uint64_t ram_pagesize_summary(void)
 
 static void migration_update_rates(RAMState *rs, int64_t end_time)
 {
-    uint64_t iter_count = rs->iterations - rs->iterations_prev;
+    uint64_t page_count = rs->handle_pages - rs->handle_pages_prev;
 
     /* calculate period counters */
     ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
                 / (end_time - rs->time_last_bitmap_sync);
 
-    if (!iter_count) {
+    if (!page_count) {
         return;
     }
 
     if (migrate_use_xbzrle()) {
         xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
-            rs->xbzrle_cache_miss_prev) / iter_count;
+            rs->xbzrle_cache_miss_prev) / page_count;
         rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
     }
 }
@@ -1657,7 +1657,7 @@ static void migration_bitmap_sync(RAMState *rs)
 
         migration_update_rates(rs, end_time);
 
-        rs->iterations_prev = rs->iterations;
+        rs->handle_pages_prev = rs->handle_pages;
 
         /* reset period counters */
         rs->time_last_bitmap_sync = end_time;
@@ -3209,7 +3209,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
             break;
         }
 
-        rs->iterations++;
+        rs->handle_pages += pages;
 
         /* we want to check in the 1st loop, just in case it was the 1st time
            and we had to sync the dirty bitmap.
-- 
2.14.4

[Qemu-devel] [PATCH v3 10/10] migration: show the statistics of compression

[guangrong.xiao]

From: Xiao Guangrong <xiaoguangrong@tencent.com>

Currently, it includes:
pages: amount of pages compressed and transferred to the target VM
busy: amount of count that no free thread to compress data
busy-rate: rate of thread busy
compressed-size: amount of bytes after compression
compression-rate: rate of compressed size

Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
---
 hmp.c                 | 13 +++++++++++++
 migration/migration.c | 12 ++++++++++++
 migration/ram.c       | 41 ++++++++++++++++++++++++++++++++++++++++-
 migration/ram.h       |  1 +
 qapi/migration.json   | 26 +++++++++++++++++++++++++-
 5 files changed, 91 insertions(+), 2 deletions(-)

diff --git a/hmp.c b/hmp.c
index 47d36e3ccf..e76e45e672 100644
--- a/hmp.c
+++ b/hmp.c
@@ -271,6 +271,19 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->xbzrle_cache->overflow);
     }
 
+    if (info->has_compression) {
+        monitor_printf(mon, "compression pages: %" PRIu64 " pages\n",
+                       info->compression->pages);
+        monitor_printf(mon, "compression busy: %" PRIu64 "\n",
+                       info->compression->busy);
+        monitor_printf(mon, "compression busy rate: %0.2f\n",
+                       info->compression->busy_rate);
+        monitor_printf(mon, "compressed size: %" PRIu64 "\n",
+                       info->compression->compressed_size);
+        monitor_printf(mon, "compression rate: %0.2f\n",
+                       info->compression->compression_rate);
+    }
+
     if (info->has_cpu_throttle_percentage) {
         monitor_printf(mon, "cpu throttle percentage: %" PRIu64 "\n",
                        info->cpu_throttle_percentage);
diff --git a/migration/migration.c b/migration/migration.c
index 2ccaadc03d..4da0a20275 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -754,6 +754,18 @@ static void populate_ram_info(MigrationInfo *info, MigrationState *s)
         info->xbzrle_cache->overflow = xbzrle_counters.overflow;
     }
 
+    if (migrate_use_compression()) {
+        info->has_compression = true;
+        info->compression = g_malloc0(sizeof(*info->compression));
+        info->compression->pages = compression_counters.pages;
+        info->compression->busy = compression_counters.busy;
+        info->compression->busy_rate = compression_counters.busy_rate;
+        info->compression->compressed_size =
+                                    compression_counters.compressed_size;
+        info->compression->compression_rate =
+                                    compression_counters.compression_rate;
+    }
+
     if (cpu_throttle_active()) {
         info->has_cpu_throttle_percentage = true;
         info->cpu_throttle_percentage = cpu_throttle_get_percentage();
diff --git a/migration/ram.c b/migration/ram.c
index bd7c18d1f9..d1cb453e53 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -301,6 +301,15 @@ struct RAMState {
     /* xbzrle misses since the beginning of the period */
     uint64_t xbzrle_cache_miss_prev;
     /* total handled pages at the beginning of period */
+
+    /* compression statistics since the beginning of the period */
+    /* amount of count that no free thread to compress data */
+    uint64_t compress_thread_busy_prev;
+    /* amount bytes after compression */
+    uint64_t compressed_size_prev;
+    /* amount of compressed pages */
+    uint64_t compress_pages_prev;
+
     uint64_t handle_pages_prev;
     /* total handled pages since start */
     uint64_t handle_pages;
@@ -339,6 +348,8 @@ struct PageSearchStatus {
 };
 typedef struct PageSearchStatus PageSearchStatus;
 
+CompressionStats compression_counters;
+
 struct CompressParam {
     bool done;
     bool quit;
@@ -1588,6 +1599,7 @@ uint64_t ram_pagesize_summary(void)
 static void migration_update_rates(RAMState *rs, int64_t end_time)
 {
     uint64_t page_count = rs->handle_pages - rs->handle_pages_prev;
+    double compressed_size;
 
     /* calculate period counters */
     ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
@@ -1602,6 +1614,26 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
             rs->xbzrle_cache_miss_prev) / page_count;
         rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
     }
+
+    if (migrate_use_compression()) {
+        compression_counters.busy_rate = (double)(compression_counters.busy -
+            rs->compress_thread_busy_prev) / page_count;
+        rs->compress_thread_busy_prev = compression_counters.busy;
+
+        compressed_size = compression_counters.compressed_size -
+                          rs->compressed_size_prev;
+        if (compressed_size) {
+            double uncompressed_size = (compression_counters.pages -
+                                    rs->compress_pages_prev) * TARGET_PAGE_SIZE;
+
+            /* Compression-Ratio = Uncompressed-size / Compressed-size */
+            compression_counters.compression_rate =
+                                        uncompressed_size / compressed_size;
+
+            rs->compress_pages_prev = compression_counters.pages;
+            rs->compressed_size_prev = compression_counters.compressed_size;
+        }
+    }
 }
 
 static void migration_bitmap_sync(RAMState *rs)
@@ -1883,10 +1915,16 @@ exit:
 static void
 update_compress_thread_counts(const CompressParam *param, int bytes_xmit)
 {
+    ram_counters.transferred += bytes_xmit;
+
     if (param->zero_page) {
         ram_counters.duplicate++;
+        return;
     }
-    ram_counters.transferred += bytes_xmit;
+
+    /* 8 means a header with RAM_SAVE_FLAG_CONTINUE. */
+    compression_counters.compressed_size += bytes_xmit - 8;
+    compression_counters.pages++;
 }
 
 static void flush_compressed_data(RAMState *rs)
@@ -2254,6 +2292,7 @@ static bool save_compress_page(RAMState *rs, RAMBlock *block, ram_addr_t offset)
         return true;
     }
 
+    compression_counters.busy++;
     return false;
 }
 
diff --git a/migration/ram.h b/migration/ram.h
index 457bf54b8c..a139066846 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -36,6 +36,7 @@
 
 extern MigrationStats ram_counters;
 extern XBZRLECacheStats xbzrle_counters;
+extern CompressionStats compression_counters;
 
 int xbzrle_cache_resize(int64_t new_size, Error **errp);
 uint64_t ram_bytes_remaining(void);
diff --git a/qapi/migration.json b/qapi/migration.json
index e6991fcbd2..69e1510429 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -75,6 +75,27 @@
            'cache-miss': 'int', 'cache-miss-rate': 'number',
            'overflow': 'int' } }
 
+##
+# @CompressionStats:
+#
+# Detailed migration compression statistics
+#
+# @pages: amount of pages compressed and transferred to the target VM
+#
+# @busy: count of times that no free thread was available to compress data
+#
+# @busy-rate: rate of thread busy
+#
+# @compressed-size: amount of bytes after compression
+#
+# @compression-rate: rate of compressed size
+#
+# Since: 3.1
+##
+{ 'struct': 'CompressionStats',
+  'data': {'pages': 'int', 'busy': 'int', 'busy-rate': 'number',
+	   'compressed-size': 'int', 'compression-rate': 'number' } }
+
 ##
 # @MigrationStatus:
 #
@@ -172,6 +193,8 @@
 #           only present when the postcopy-blocktime migration capability
 #           is enabled. (Since 3.0)
 #
+# @compression: migration compression statistics, only returned if compression
+#           feature is on and status is 'active' or 'completed' (Since 3.1)
 #
 # Since: 0.14.0
 ##
@@ -186,7 +209,8 @@
            '*cpu-throttle-percentage': 'int',
            '*error-desc': 'str',
            '*postcopy-blocktime' : 'uint32',
-           '*postcopy-vcpu-blocktime': ['uint32']} }
+           '*postcopy-vcpu-blocktime': ['uint32'],
+           '*compression': 'CompressionStats'} }
 
 ##
 # @query-migrate:
-- 
2.14.4

Re: [Qemu-devel] [PATCH v3 01/10] migration: do not wait for free thread

[Eric Blake]

On 08/07/2018 04:12 AM, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> Instead of putting the main thread to sleep state to wait for
> free compression thread, we can directly post it out as normal
> page that reduces the latency and uses CPUs more efficiently
> 
> A parameter, compress-wait-thread, is introduced, it can be
> enabled if the user really wants the old behavior
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---

> +++ b/qapi/migration.json
> @@ -462,6 +462,11 @@
>   # @compress-threads: Set compression thread count to be used in live migration,
>   #          the compression thread count is an integer between 1 and 255.
>   #
> +# @compress-wait-thread: Wait if no thread is free to compress the memory page
> +#          if it's enabled, otherwise, the page will be posted out immediately
> +#          in the main thread without compression. It's true on default.
> +#          (Since: 3.1)

Grammar suggestion:

@compress-wait-thread: Controls behavior when all compression threads 
are currently busy. If true (default), wait for a free compression 
thread to become available; otherwise, send the page uncompressed. 
(Since 3.1)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 01/10] migration: do not wait for free thread

[Peter Xu]

On Tue, Aug 07, 2018 at 08:29:54AM -0500, Eric Blake wrote:
> On 08/07/2018 04:12 AM, guangrong.xiao@gmail.com wrote:
> > From: Xiao Guangrong <xiaoguangrong@tencent.com>
> > 
> > Instead of putting the main thread to sleep state to wait for
> > free compression thread, we can directly post it out as normal
> > page that reduces the latency and uses CPUs more efficiently
> > 
> > A parameter, compress-wait-thread, is introduced, it can be
> > enabled if the user really wants the old behavior
> > 
> > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> > ---
> 
> > +++ b/qapi/migration.json
> > @@ -462,6 +462,11 @@
> >   # @compress-threads: Set compression thread count to be used in live migration,
> >   #          the compression thread count is an integer between 1 and 255.
> >   #
> > +# @compress-wait-thread: Wait if no thread is free to compress the memory page
> > +#          if it's enabled, otherwise, the page will be posted out immediately
> > +#          in the main thread without compression. It's true on default.
> > +#          (Since: 3.1)
> 
> Grammar suggestion:
> 
> @compress-wait-thread: Controls behavior when all compression threads are
> currently busy. If true (default), wait for a free compression thread to
> become available; otherwise, send the page uncompressed. (Since 3.1)

Eric's version seems better.  With that:

Reviewed-by: Peter Xu <peterx@redhat.com>

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 05/10] migration: move handle of zero page to the thread

[Peter Xu]

On Tue, Aug 07, 2018 at 05:12:04PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> Detecting zero page is not a light work, moving it to the thread to
> speed the main thread up, btw, handling ram_release_pages() for the
> zero page is moved to the thread as well
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---

[...]

> @@ -2239,17 +2290,10 @@ static int ram_save_target_page(RAMState *rs, PageSearchStatus *pss,
>      }
>  
>      /*
> -     * Make sure the first page is sent out before other pages.
> -     *
> -     * we post it as normal page as compression will take much
> -     * CPU resource.
> +     * do not use multifd for compression as the first page in the new
> +     * block should be posted out before sending the compressed page
>       */
> -    if (block == rs->last_sent_block && save_page_use_compression(rs)) {
> -        res = compress_page_with_multi_thread(rs, block, offset);
> -        if (res > 0) {
> -            return res;
> -        }
> -    } else if (migrate_use_multifd()) {
> +    if (!save_page_use_compression(rs) && migrate_use_multifd()) {

Not related to this patch: I think we should just disallow user to
specify both compression and multifd together.  I vaguely remembered
that I left a comment somewhere in the multifd series but it must have
fallen throw the cracks...  Anyway it's not related to this patch:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

>          return ram_save_multifd_page(rs, block, offset);
>      }
>  
> -- 
> 2.14.4
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 07/10] migration: do not flush_compressed_data at the end of each iteration

[Peter Xu]

On Tue, Aug 07, 2018 at 05:12:06PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> flush_compressed_data() needs to wait all compression threads to
> finish their work, after that all threads are free until the
> migration feeds new request to them, reducing its call can improve
> the throughput and use CPU resource more effectively
> 
> We do not need to flush all threads at the end of iteration, the
> data can be kept locally until the memory block is changed or
> memory migration starts over in that case we will meet a dirtied
> page which may still exists in compression threads's ring
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---
>  migration/ram.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 99ecf9b315..55966bc2c1 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -306,6 +306,8 @@ struct RAMState {
>      uint64_t iterations;
>      /* number of dirty bits in the bitmap */
>      uint64_t migration_dirty_pages;
> +    /* last dirty_sync_count we have seen */
> +    uint64_t dirty_sync_count_prev;
>      /* protects modification of the bitmap */
>      QemuMutex bitmap_mutex;
>      /* The RAMBlock used in the last src_page_requests */
> @@ -3173,6 +3175,17 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>  
>      ram_control_before_iterate(f, RAM_CONTROL_ROUND);
>  
> +    /*
> +     * if memory migration starts over, we will meet a dirtied page which
> +     * may still exists in compression threads's ring, so we should flush
> +     * the compressed data to make sure the new page is not overwritten by
> +     * the old one in the destination.
> +     */
> +    if (ram_counters.dirty_sync_count != rs->dirty_sync_count_prev) {
> +        rs->dirty_sync_count_prev = ram_counters.dirty_sync_count;
> +        flush_compressed_data(rs);

AFAIU this only happens when ram_save_pending() calls
migration_bitmap_sync().  Could we just simply flush there?  Then we
can avoid that new variable.

> +    }
> +
>      t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>      i = 0;
>      while ((ret = qemu_file_rate_limit(f)) == 0 ||
> @@ -3205,7 +3218,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>          }
>          i++;
>      }
> -    flush_compressed_data(rs);
>      rcu_read_unlock();
>  
>      /*
> -- 
> 2.14.4
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[Peter Xu]

On Tue, Aug 07, 2018 at 05:12:07PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> ram_find_and_save_block() can return negative if any error hanppens,
> however, it is completely ignored in current code

Could you hint me where we'll return an error?

(Anyway I agree that the error handling is not that good, mostly
 because the QEMUFile APIs does not provide proper return code, e.g.,
 qemu_put_be64 returns void)

> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---
>  migration/ram.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 55966bc2c1..09be01dca2 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2367,7 +2367,8 @@ static int ram_save_host_page(RAMState *rs, PageSearchStatus *pss,
>   *
>   * Called within an RCU critical section.
>   *
> - * Returns the number of pages written where zero means no dirty pages
> + * Returns the number of pages written where zero means no dirty pages,
> + * or negative on error
>   *
>   * @rs: current RAM state
>   * @last_stage: if we are at the completion stage
> @@ -3202,6 +3203,12 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>              done = 1;
>              break;
>          }
> +
> +        if (pages < 0) {
> +            qemu_file_set_error(f, pages);
> +            break;
> +        }
> +
>          rs->iterations++;
>  
>          /* we want to check in the 1st loop, just in case it was the 1st time
> @@ -3243,7 +3250,7 @@ out:
>  /**
>   * ram_save_complete: function called to send the remaining amount of ram
>   *
> - * Returns zero to indicate success
> + * Returns zero to indicate success or negative on error
>   *
>   * Called with iothread lock
>   *
> @@ -3254,6 +3261,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>  {
>      RAMState **temp = opaque;
>      RAMState *rs = *temp;
> +    int ret = 0;
>  
>      rcu_read_lock();
>  
> @@ -3274,6 +3282,10 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>          if (pages == 0) {
>              break;
>          }
> +        if (pages < 0) {
> +            ret = pages;
> +            break;
> +        }
>      }
>  
>      flush_compressed_data(rs);
> @@ -3285,7 +3297,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
>      qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>      qemu_fflush(f);
>  
> -    return 0;
> +    return ret;
>  }
>  
>  static void ram_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
> -- 
> 2.14.4
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 09/10] migration: fix calculating xbzrle_counters.cache_miss_rate

[Peter Xu]

On Tue, Aug 07, 2018 at 05:12:08PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> As Peter pointed out:
> | - xbzrle_counters.cache_miss is done in save_xbzrle_page(), so it's
> |   per-guest-page granularity
> |
> | - RAMState.iterations is done for each ram_find_and_save_block(), so
> |   it's per-host-page granularity
> |
> | An example is that when we migrate a 2M huge page in the guest, we
> | will only increase the RAMState.iterations by 1 (since
> | ram_find_and_save_block() will be called once), but we might increase
> | xbzrle_counters.cache_miss for 2M/4K=512 times (we'll call
> | save_xbzrle_page() that many times) if all the pages got cache miss.
> | Then IMHO the cache miss rate will be 512/1=51200% (while it should
> | actually be just 100% cache miss).
> 
> And he also suggested as xbzrle_counters.cache_miss_rate is the only
> user of rs->iterations we can adapt it to count guest page numbers
> 
> After that, rename 'iterations' to 'handle_pages' to better reflect
> its meaning
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> ---
>  migration/ram.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 09be01dca2..bd7c18d1f9 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -300,10 +300,10 @@ struct RAMState {
>      uint64_t num_dirty_pages_period;
>      /* xbzrle misses since the beginning of the period */
>      uint64_t xbzrle_cache_miss_prev;
> -    /* number of iterations at the beginning of period */
> -    uint64_t iterations_prev;
> -    /* Iterations since start */
> -    uint64_t iterations;
> +    /* total handled pages at the beginning of period */
> +    uint64_t handle_pages_prev;
> +    /* total handled pages since start */
> +    uint64_t handle_pages;

The name is not that straightforward to me.  I would think about
"[guest|host]_page_count" or something better, or we just keep the old
naming but with a better comment would be fine too.

>      /* number of dirty bits in the bitmap */
>      uint64_t migration_dirty_pages;
>      /* last dirty_sync_count we have seen */
> @@ -1587,19 +1587,19 @@ uint64_t ram_pagesize_summary(void)
>  
>  static void migration_update_rates(RAMState *rs, int64_t end_time)
>  {
> -    uint64_t iter_count = rs->iterations - rs->iterations_prev;
> +    uint64_t page_count = rs->handle_pages - rs->handle_pages_prev;
>  
>      /* calculate period counters */
>      ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
>                  / (end_time - rs->time_last_bitmap_sync);
>  
> -    if (!iter_count) {
> +    if (!page_count) {
>          return;
>      }
>  
>      if (migrate_use_xbzrle()) {
>          xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
> -            rs->xbzrle_cache_miss_prev) / iter_count;
> +            rs->xbzrle_cache_miss_prev) / page_count;
>          rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>      }
>  }
> @@ -1657,7 +1657,7 @@ static void migration_bitmap_sync(RAMState *rs)
>  
>          migration_update_rates(rs, end_time);
>  
> -        rs->iterations_prev = rs->iterations;
> +        rs->handle_pages_prev = rs->handle_pages;
>  
>          /* reset period counters */
>          rs->time_last_bitmap_sync = end_time;
> @@ -3209,7 +3209,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>              break;
>          }
>  
> -        rs->iterations++;
> +        rs->handle_pages += pages;

So it's still counting host pages, is this your intention to only
change the name in the patch?

>  
>          /* we want to check in the 1st loop, just in case it was the 1st time
>             and we had to sync the dirty bitmap.
> -- 
> 2.14.4
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 10/10] migration: show the statistics of compression

[Peter Xu]

On Tue, Aug 07, 2018 at 05:12:09PM +0800, guangrong.xiao@gmail.com wrote:

[...]

> @@ -1602,6 +1614,26 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>              rs->xbzrle_cache_miss_prev) / page_count;
>          rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>      }
> +
> +    if (migrate_use_compression()) {
> +        compression_counters.busy_rate = (double)(compression_counters.busy -
> +            rs->compress_thread_busy_prev) / page_count;

So this is related to the previous patch - I still doubt its
correctness if page_count is the host pages count rather than the
guest pages'.  Other than that the patch looks good to me.

Thanks,

> +        rs->compress_thread_busy_prev = compression_counters.busy;
> +
> +        compressed_size = compression_counters.compressed_size -
> +                          rs->compressed_size_prev;
> +        if (compressed_size) {
> +            double uncompressed_size = (compression_counters.pages -
> +                                    rs->compress_pages_prev) * TARGET_PAGE_SIZE;
> +
> +            /* Compression-Ratio = Uncompressed-size / Compressed-size */
> +            compression_counters.compression_rate =
> +                                        uncompressed_size / compressed_size;
> +
> +            rs->compress_pages_prev = compression_counters.pages;
> +            rs->compressed_size_prev = compression_counters.compressed_size;
> +        }
> +    }

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 01/10] migration: do not wait for free thread

[Xiao Guangrong]

On 08/08/2018 11:51 AM, Peter Xu wrote:
> On Tue, Aug 07, 2018 at 08:29:54AM -0500, Eric Blake wrote:
>> On 08/07/2018 04:12 AM, guangrong.xiao@gmail.com wrote:
>>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>>
>>> Instead of putting the main thread to sleep state to wait for
>>> free compression thread, we can directly post it out as normal
>>> page that reduces the latency and uses CPUs more efficiently
>>>
>>> A parameter, compress-wait-thread, is introduced, it can be
>>> enabled if the user really wants the old behavior
>>>
>>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>>> ---
>>
>>> +++ b/qapi/migration.json
>>> @@ -462,6 +462,11 @@
>>>    # @compress-threads: Set compression thread count to be used in live migration,
>>>    #          the compression thread count is an integer between 1 and 255.
>>>    #
>>> +# @compress-wait-thread: Wait if no thread is free to compress the memory page
>>> +#          if it's enabled, otherwise, the page will be posted out immediately
>>> +#          in the main thread without compression. It's true on default.
>>> +#          (Since: 3.1)
>>
>> Grammar suggestion:
>>
>> @compress-wait-thread: Controls behavior when all compression threads are
>> currently busy. If true (default), wait for a free compression thread to
>> become available; otherwise, send the page uncompressed. (Since 3.1)
> 
> Eric's version seems better.  With that:

It's good to me too. Will apply Eric's suggestion in the next version.

> 
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thank you, Peter and Eric.

Re: [Qemu-devel] [PATCH v3 07/10] migration: do not flush_compressed_data at the end of each iteration

[Xiao Guangrong]

On 08/08/2018 12:52 PM, Peter Xu wrote:
> On Tue, Aug 07, 2018 at 05:12:06PM +0800, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> flush_compressed_data() needs to wait all compression threads to
>> finish their work, after that all threads are free until the
>> migration feeds new request to them, reducing its call can improve
>> the throughput and use CPU resource more effectively
>>
>> We do not need to flush all threads at the end of iteration, the
>> data can be kept locally until the memory block is changed or
>> memory migration starts over in that case we will meet a dirtied
>> page which may still exists in compression threads's ring
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>> ---
>>   migration/ram.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 99ecf9b315..55966bc2c1 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -306,6 +306,8 @@ struct RAMState {
>>       uint64_t iterations;
>>       /* number of dirty bits in the bitmap */
>>       uint64_t migration_dirty_pages;
>> +    /* last dirty_sync_count we have seen */
>> +    uint64_t dirty_sync_count_prev;
>>       /* protects modification of the bitmap */
>>       QemuMutex bitmap_mutex;
>>       /* The RAMBlock used in the last src_page_requests */
>> @@ -3173,6 +3175,17 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>   
>>       ram_control_before_iterate(f, RAM_CONTROL_ROUND);
>>   
>> +    /*
>> +     * if memory migration starts over, we will meet a dirtied page which
>> +     * may still exists in compression threads's ring, so we should flush
>> +     * the compressed data to make sure the new page is not overwritten by
>> +     * the old one in the destination.
>> +     */
>> +    if (ram_counters.dirty_sync_count != rs->dirty_sync_count_prev) {
>> +        rs->dirty_sync_count_prev = ram_counters.dirty_sync_count;
>> +        flush_compressed_data(rs);
> 
> AFAIU this only happens when ram_save_pending() calls
> migration_bitmap_sync().  Could we just simply flush there?  Then we
> can avoid that new variable.
> 

Yup, that's better indeed, will do it.

Re: [Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[Xiao Guangrong]

On 08/08/2018 01:08 PM, Peter Xu wrote:
> On Tue, Aug 07, 2018 at 05:12:07PM +0800, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> ram_find_and_save_block() can return negative if any error hanppens,
>> however, it is completely ignored in current code
> 
> Could you hint me where we'll return an error?
> 

I think control_save_page() may return a error condition but i am not
good at it ... Other places look safe _currently_. These functions were
designed to have error returned anyway.

> (Anyway I agree that the error handling is not that good, mostly
>   because the QEMUFile APIs does not provide proper return code, e.g.,
>   qemu_put_be64 returns void)
> 

Yes, it is, the returned error condition is mixed in file's API and
function's return value... :(

Re: [Qemu-devel] [PATCH v3 09/10] migration: fix calculating xbzrle_counters.cache_miss_rate

[Xiao Guangrong]

On 08/08/2018 02:05 PM, Peter Xu wrote:
> On Tue, Aug 07, 2018 at 05:12:08PM +0800, guangrong.xiao@gmail.com wrote:
>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>
>> As Peter pointed out:
>> | - xbzrle_counters.cache_miss is done in save_xbzrle_page(), so it's
>> |   per-guest-page granularity
>> |
>> | - RAMState.iterations is done for each ram_find_and_save_block(), so
>> |   it's per-host-page granularity
>> |
>> | An example is that when we migrate a 2M huge page in the guest, we
>> | will only increase the RAMState.iterations by 1 (since
>> | ram_find_and_save_block() will be called once), but we might increase
>> | xbzrle_counters.cache_miss for 2M/4K=512 times (we'll call
>> | save_xbzrle_page() that many times) if all the pages got cache miss.
>> | Then IMHO the cache miss rate will be 512/1=51200% (while it should
>> | actually be just 100% cache miss).
>>
>> And he also suggested as xbzrle_counters.cache_miss_rate is the only
>> user of rs->iterations we can adapt it to count guest page numbers
>>
>> After that, rename 'iterations' to 'handle_pages' to better reflect
>> its meaning
>>
>> Suggested-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
>> ---
>>   migration/ram.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/migration/ram.c b/migration/ram.c
>> index 09be01dca2..bd7c18d1f9 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -300,10 +300,10 @@ struct RAMState {
>>       uint64_t num_dirty_pages_period;
>>       /* xbzrle misses since the beginning of the period */
>>       uint64_t xbzrle_cache_miss_prev;
>> -    /* number of iterations at the beginning of period */
>> -    uint64_t iterations_prev;
>> -    /* Iterations since start */
>> -    uint64_t iterations;
>> +    /* total handled pages at the beginning of period */
>> +    uint64_t handle_pages_prev;
>> +    /* total handled pages since start */
>> +    uint64_t handle_pages;
> 
> The name is not that straightforward to me.  I would think about
> "[guest|host]_page_count" or something better, or we just keep the old
> naming but with a better comment would be fine too.

The filed actually indicates total pages (target pages more precisely)
handled during live migration. 'iterations' confuses us completely.

It's target_page_count good to you?

> 
>>       /* number of dirty bits in the bitmap */
>>       uint64_t migration_dirty_pages;
>>       /* last dirty_sync_count we have seen */
>> @@ -1587,19 +1587,19 @@ uint64_t ram_pagesize_summary(void)
>>   
>>   static void migration_update_rates(RAMState *rs, int64_t end_time)
>>   {
>> -    uint64_t iter_count = rs->iterations - rs->iterations_prev;
>> +    uint64_t page_count = rs->handle_pages - rs->handle_pages_prev;
>>   
>>       /* calculate period counters */
>>       ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
>>                   / (end_time - rs->time_last_bitmap_sync);
>>   
>> -    if (!iter_count) {
>> +    if (!page_count) {
>>           return;
>>       }
>>   
>>       if (migrate_use_xbzrle()) {
>>           xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
>> -            rs->xbzrle_cache_miss_prev) / iter_count;
>> +            rs->xbzrle_cache_miss_prev) / page_count;
>>           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>>       }
>>   }
>> @@ -1657,7 +1657,7 @@ static void migration_bitmap_sync(RAMState *rs)
>>   
>>           migration_update_rates(rs, end_time);
>>   
>> -        rs->iterations_prev = rs->iterations;
>> +        rs->handle_pages_prev = rs->handle_pages;
>>   
>>           /* reset period counters */
>>           rs->time_last_bitmap_sync = end_time;
>> @@ -3209,7 +3209,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>               break;
>>           }
>>   
>> -        rs->iterations++;
>> +        rs->handle_pages += pages;
> 
> So it's still counting host pages, is this your intention to only
> change the name in the patch?

Hmm... the value returned by ram_find_and_save_block() isn't the total
target pages posted out?

/**
  * ram_find_and_save_block: finds a dirty page and sends it to f
  *
  * Called within an RCU critical section.
  *
  * Returns the number of pages written where zero means no dirty pages,
  * or negative on error
...

  *
  * On systems where host-page-size > target-page-size it will send all the
  * pages in a host page that are dirty.
  */

Re: [Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[Peter Xu]

On Wed, Aug 08, 2018 at 02:29:52PM +0800, Xiao Guangrong wrote:
> 
> 
> On 08/08/2018 01:08 PM, Peter Xu wrote:
> > On Tue, Aug 07, 2018 at 05:12:07PM +0800, guangrong.xiao@gmail.com wrote:
> > > From: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > 
> > > ram_find_and_save_block() can return negative if any error hanppens,
> > > however, it is completely ignored in current code
> > 
> > Could you hint me where we'll return an error?
> > 
> 
> I think control_save_page() may return a error condition but i am not
> good at it ... Other places look safe _currently_. These functions were
> designed to have error returned anyway.

Ah, the RDMA codes...

Then I feel like this patch would be more suitable to be put into some
of the RDMA series - at least we'd better be clear about what errors
we're going to capture.  For non-RDMA, it seems a bit helpless after
all - AFAIU we're depending on the few qemu_file_get_error() calls to
detect output errors.

> 
> > (Anyway I agree that the error handling is not that good, mostly
> >   because the QEMUFile APIs does not provide proper return code, e.g.,
> >   qemu_put_be64 returns void)
> > 
> 
> Yes, it is, the returned error condition is mixed in file's API and
> function's return value... :(
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 09/10] migration: fix calculating xbzrle_counters.cache_miss_rate

[Peter Xu]

On Wed, Aug 08, 2018 at 02:36:51PM +0800, Xiao Guangrong wrote:
> 
> 
> On 08/08/2018 02:05 PM, Peter Xu wrote:
> > On Tue, Aug 07, 2018 at 05:12:08PM +0800, guangrong.xiao@gmail.com wrote:
> > > From: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > 
> > > As Peter pointed out:
> > > | - xbzrle_counters.cache_miss is done in save_xbzrle_page(), so it's
> > > |   per-guest-page granularity
> > > |
> > > | - RAMState.iterations is done for each ram_find_and_save_block(), so
> > > |   it's per-host-page granularity
> > > |
> > > | An example is that when we migrate a 2M huge page in the guest, we
> > > | will only increase the RAMState.iterations by 1 (since
> > > | ram_find_and_save_block() will be called once), but we might increase
> > > | xbzrle_counters.cache_miss for 2M/4K=512 times (we'll call
> > > | save_xbzrle_page() that many times) if all the pages got cache miss.
> > > | Then IMHO the cache miss rate will be 512/1=51200% (while it should
> > > | actually be just 100% cache miss).
> > > 
> > > And he also suggested as xbzrle_counters.cache_miss_rate is the only
> > > user of rs->iterations we can adapt it to count guest page numbers
> > > 
> > > After that, rename 'iterations' to 'handle_pages' to better reflect
> > > its meaning
> > > 
> > > Suggested-by: Peter Xu <peterx@redhat.com>
> > > Signed-off-by: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > ---
> > >   migration/ram.c | 18 +++++++++---------
> > >   1 file changed, 9 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/migration/ram.c b/migration/ram.c
> > > index 09be01dca2..bd7c18d1f9 100644
> > > --- a/migration/ram.c
> > > +++ b/migration/ram.c
> > > @@ -300,10 +300,10 @@ struct RAMState {
> > >       uint64_t num_dirty_pages_period;
> > >       /* xbzrle misses since the beginning of the period */
> > >       uint64_t xbzrle_cache_miss_prev;
> > > -    /* number of iterations at the beginning of period */
> > > -    uint64_t iterations_prev;
> > > -    /* Iterations since start */
> > > -    uint64_t iterations;
> > > +    /* total handled pages at the beginning of period */
> > > +    uint64_t handle_pages_prev;
> > > +    /* total handled pages since start */
> > > +    uint64_t handle_pages;
> > 
> > The name is not that straightforward to me.  I would think about
> > "[guest|host]_page_count" or something better, or we just keep the old
> > naming but with a better comment would be fine too.
> 
> The filed actually indicates total pages (target pages more precisely)
> handled during live migration. 'iterations' confuses us completely.
> 
> It's target_page_count good to you?

Yes.

> 
> > 
> > >       /* number of dirty bits in the bitmap */
> > >       uint64_t migration_dirty_pages;
> > >       /* last dirty_sync_count we have seen */
> > > @@ -1587,19 +1587,19 @@ uint64_t ram_pagesize_summary(void)
> > >   static void migration_update_rates(RAMState *rs, int64_t end_time)
> > >   {
> > > -    uint64_t iter_count = rs->iterations - rs->iterations_prev;
> > > +    uint64_t page_count = rs->handle_pages - rs->handle_pages_prev;
> > >       /* calculate period counters */
> > >       ram_counters.dirty_pages_rate = rs->num_dirty_pages_period * 1000
> > >                   / (end_time - rs->time_last_bitmap_sync);
> > > -    if (!iter_count) {
> > > +    if (!page_count) {
> > >           return;
> > >       }
> > >       if (migrate_use_xbzrle()) {
> > >           xbzrle_counters.cache_miss_rate = (double)(xbzrle_counters.cache_miss -
> > > -            rs->xbzrle_cache_miss_prev) / iter_count;
> > > +            rs->xbzrle_cache_miss_prev) / page_count;
> > >           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> > >       }
> > >   }
> > > @@ -1657,7 +1657,7 @@ static void migration_bitmap_sync(RAMState *rs)
> > >           migration_update_rates(rs, end_time);
> > > -        rs->iterations_prev = rs->iterations;
> > > +        rs->handle_pages_prev = rs->handle_pages;
> > >           /* reset period counters */
> > >           rs->time_last_bitmap_sync = end_time;
> > > @@ -3209,7 +3209,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
> > >               break;
> > >           }
> > > -        rs->iterations++;
> > > +        rs->handle_pages += pages;
> > 
> > So it's still counting host pages, is this your intention to only
> > change the name in the patch?
> 
> Hmm... the value returned by ram_find_and_save_block() isn't the total
> target pages posted out?

Hmm, I overlooked that. Sorry. :)

Then it looks fine to me:

Reviewed-by: Peter Xu <peterx@redhat.com>

> 
> /**
>  * ram_find_and_save_block: finds a dirty page and sends it to f
>  *
>  * Called within an RCU critical section.
>  *
>  * Returns the number of pages written where zero means no dirty pages,
>  * or negative on error
> ...
> 
>  *
>  * On systems where host-page-size > target-page-size it will send all the
>  * pages in a host page that are dirty.
>  */

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[Xiao Guangrong]

On 08/08/2018 02:56 PM, Peter Xu wrote:
> On Wed, Aug 08, 2018 at 02:29:52PM +0800, Xiao Guangrong wrote:
>>
>>
>> On 08/08/2018 01:08 PM, Peter Xu wrote:
>>> On Tue, Aug 07, 2018 at 05:12:07PM +0800, guangrong.xiao@gmail.com wrote:
>>>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>>>
>>>> ram_find_and_save_block() can return negative if any error hanppens,
>>>> however, it is completely ignored in current code
>>>
>>> Could you hint me where we'll return an error?
>>>
>>
>> I think control_save_page() may return a error condition but i am not
>> good at it ... Other places look safe _currently_. These functions were
>> designed to have error returned anyway.
> 
> Ah, the RDMA codes...
> 
> Then I feel like this patch would be more suitable to be put into some
> of the RDMA series - at least we'd better be clear about what errors
> we're going to capture.  For non-RDMA, it seems a bit helpless after
> all - AFAIU we're depending on the few qemu_file_get_error() calls to
> detect output errors.

So, are you talking about to modify the semantic of these functions,
ram_save_host_page(), ram_save_target_page(), etc, and make them
be:
"Returns the number of pages written where zero means no dirty pages,
error conditions are indicated in the QEMUFile @rs->file if it
happened."

If it's what you want, i will update the comments and make the
implementation more clear to reflect this fact for these
functions

Re: [Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[Peter Xu]

On Wed, Aug 08, 2018 at 03:23:22PM +0800, Xiao Guangrong wrote:
> 
> 
> On 08/08/2018 02:56 PM, Peter Xu wrote:
> > On Wed, Aug 08, 2018 at 02:29:52PM +0800, Xiao Guangrong wrote:
> > > 
> > > 
> > > On 08/08/2018 01:08 PM, Peter Xu wrote:
> > > > On Tue, Aug 07, 2018 at 05:12:07PM +0800, guangrong.xiao@gmail.com wrote:
> > > > > From: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > > > 
> > > > > ram_find_and_save_block() can return negative if any error hanppens,
> > > > > however, it is completely ignored in current code
> > > > 
> > > > Could you hint me where we'll return an error?
> > > > 
> > > 
> > > I think control_save_page() may return a error condition but i am not
> > > good at it ... Other places look safe _currently_. These functions were
> > > designed to have error returned anyway.
> > 
> > Ah, the RDMA codes...
> > 
> > Then I feel like this patch would be more suitable to be put into some
> > of the RDMA series - at least we'd better be clear about what errors
> > we're going to capture.  For non-RDMA, it seems a bit helpless after
> > all - AFAIU we're depending on the few qemu_file_get_error() calls to
> > detect output errors.
> 
> So, are you talking about to modify the semantic of these functions,
> ram_save_host_page(), ram_save_target_page(), etc, and make them
> be:
> "Returns the number of pages written where zero means no dirty pages,
> error conditions are indicated in the QEMUFile @rs->file if it
> happened."
> 
> If it's what you want, i will update the comments and make the
> implementation more clear to reflect this fact for these
> functions

Not really; I am just unclear about how this patch could help current
code, however I have no objection on the content.  Let's see whether
Dave or Juan would like it.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[Dr. David Alan Gilbert]

* Xiao Guangrong (guangrong.xiao@gmail.com) wrote:
> 
> 
> On 08/08/2018 01:08 PM, Peter Xu wrote:
> > On Tue, Aug 07, 2018 at 05:12:07PM +0800, guangrong.xiao@gmail.com wrote:
> > > From: Xiao Guangrong <xiaoguangrong@tencent.com>
> > > 
> > > ram_find_and_save_block() can return negative if any error hanppens,
> > > however, it is completely ignored in current code
> > 
> > Could you hint me where we'll return an error?
> > 
> 
> I think control_save_page() may return a error condition but i am not
> good at it ... Other places look safe _currently_. These functions were
> designed to have error returned anyway.

ram_control_save_page's return is checked by control_save_page which
returns true/false but sets *pages to a return value.

What I'd need to follow closely is the case where ram_control_save_page
returns RAM_SAVE_CONTROL_DELAYED, in that case control_save_page I think
returns with *pages=-1 and returns true.
And I think in that case ram_save_target_page can leak that -1 - hmm.

Now, ram_save_host_page already checks for <0 and will return that,
but I think that would potentially loop in ram_find_and_save_block; I'm
not sure we want to change that or not!

Dave

> 
> > (Anyway I agree that the error handling is not that good, mostly
> >   because the QEMUFile APIs does not provide proper return code, e.g.,
> >   qemu_put_be64 returns void)
> > 
> 
> Yes, it is, the returned error condition is mixed in file's API and
> function's return value... :(
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 08/10] migration: handle the error condition properly

[Xiao Guangrong]

On 08/08/2018 10:11 PM, Dr. David Alan Gilbert wrote:
> * Xiao Guangrong (guangrong.xiao@gmail.com) wrote:
>>
>>
>> On 08/08/2018 01:08 PM, Peter Xu wrote:
>>> On Tue, Aug 07, 2018 at 05:12:07PM +0800, guangrong.xiao@gmail.com wrote:
>>>> From: Xiao Guangrong <xiaoguangrong@tencent.com>
>>>>
>>>> ram_find_and_save_block() can return negative if any error hanppens,
>>>> however, it is completely ignored in current code
>>>
>>> Could you hint me where we'll return an error?
>>>
>>
>> I think control_save_page() may return a error condition but i am not
>> good at it ... Other places look safe _currently_. These functions were
>> designed to have error returned anyway.
> 
> ram_control_save_page's return is checked by control_save_page which
> returns true/false but sets *pages to a return value.
> 
> What I'd need to follow closely is the case where ram_control_save_page
> returns RAM_SAVE_CONTROL_DELAYED, in that case control_save_page I think
> returns with *pages=-1 and returns true.
> And I think in that case ram_save_target_page can leak that -1 - hmm.
> 
> Now, ram_save_host_page already checks for <0 and will return that,
> but I think that would potentially loop in ram_find_and_save_block; I'm
> not sure we want to change that or not!

ram_find_and_save_block() will continue the look only if ram_save_host_page
returns zero:

......
         if (found) {
             pages = ram_save_host_page(rs, &pss, last_stage);
         }
     } while (!pages && again);

IMHO, how to change the code really depends on the semantic of these functions,
based on the comments of them, returning error conditions is the current
semantic.

Another choice would be the one squashes error conditions to QEMUFile and
adapt comments and code of these functions to reflect the new semantic
clearly.

Which one do you guys prefer to? :)

Re: [Qemu-devel] [PATCH v3 10/10] migration: show the statistics of compression

[Xiao Guangrong]

On 08/08/2018 02:12 PM, Peter Xu wrote:
> On Tue, Aug 07, 2018 at 05:12:09PM +0800, guangrong.xiao@gmail.com wrote:
> 
> [...]
> 
>> @@ -1602,6 +1614,26 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
>>               rs->xbzrle_cache_miss_prev) / page_count;
>>           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
>>       }
>> +
>> +    if (migrate_use_compression()) {
>> +        compression_counters.busy_rate = (double)(compression_counters.busy -
>> +            rs->compress_thread_busy_prev) / page_count;
> 
> So this is related to the previous patch - I still doubt its
> correctness if page_count is the host pages count rather than the
> guest pages'.  Other than that the patch looks good to me.

I think i can treat it as your Reviewed-by boldly. :)

Re: [Qemu-devel] [PATCH v3 10/10] migration: show the statistics of compression

[Peter Xu]

On Thu, Aug 09, 2018 at 11:13:17AM +0800, Xiao Guangrong wrote:
> 
> 
> On 08/08/2018 02:12 PM, Peter Xu wrote:
> > On Tue, Aug 07, 2018 at 05:12:09PM +0800, guangrong.xiao@gmail.com wrote:
> > 
> > [...]
> > 
> > > @@ -1602,6 +1614,26 @@ static void migration_update_rates(RAMState *rs, int64_t end_time)
> > >               rs->xbzrle_cache_miss_prev) / page_count;
> > >           rs->xbzrle_cache_miss_prev = xbzrle_counters.cache_miss;
> > >       }
> > > +
> > > +    if (migrate_use_compression()) {
> > > +        compression_counters.busy_rate = (double)(compression_counters.busy -
> > > +            rs->compress_thread_busy_prev) / page_count;
> > 
> > So this is related to the previous patch - I still doubt its
> > correctness if page_count is the host pages count rather than the
> > guest pages'.  Other than that the patch looks good to me.
> 
> I think i can treat it as your Reviewed-by boldly. :)

Yes, please do. :)

Regards,

-- 
Peter Xu