[Qemu-devel] [PATCH v3 00/10] migration: Optimizate the xbzrle and fix two corruption issues

[Qemu-devel] [PATCH v3 00/10] migration: Optimizate the xbzrle and fix two corruption issues

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

V2-->V3
* rename the bitmap_sync_cnt to bitmap_sync_counter
* expose xbzrle cache miss rate

V1-->V2
* expose the counter that logs the times of updating the dirty bitmap to end user.

a. Optimization the xbzrle remarkable decrease the cache misses.
    The efficiency of compress increases more than fifty times.
    Before the patch set, the cache almost totally miss when the 
    number of cache item less than the dirty page number. Now the
    hot pages in the cache will not be replaced by other pages. 

b. Reducing the data copy

c. Fix two corruption issues.

ChenLiang (10):
  XBZRLE: Fix one XBZRLE corruption issues
  migration: Add counters of updating the dirty bitmap
  migration: expose the bitmap_sync_counter to the end user
  migration: expose xbzrle cache miss rate
  XBZRLE: optimize XBZRLE to decrease the cache missing
  XBZRLE: rebuild the cache_is_cached function
  migration: Fix the migrate auto converge process
  migration: optimize xbzrle by reducing data copy
  migration: clear the dead code
  XBZRLE: update the doc of XBZRLE

 arch_init.c                    |  98 ++++++++++++++++++---------------------
 docs/xbzrle.txt                |   7 +++
 hmp.c                          |   4 ++
 include/migration/migration.h  |   2 +
 include/migration/page_cache.h |  10 ++--
 migration.c                    |   3 ++
 page_cache.c                   | 101 +++++++++++------------------------------
 qapi-schema.json               |   9 +++-
 qmp-commands.hx                |  15 ++++--
 9 files changed, 112 insertions(+), 137 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH v3 01/10] XBZRLE: Fix one XBZRLE corruption issues

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

The page may not be inserted into cache after executing save_xbzrle_page.
In case of failure to insert, the original page should be sent rather
than the page in the cache.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 60c975d..2ac68c2 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -340,7 +340,7 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
 
 #define ENCODING_FLAG_XBZRLE 0x1
 
-static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
+static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
                             ram_addr_t current_addr, RAMBlock *block,
                             ram_addr_t offset, int cont, bool last_stage)
 {
@@ -348,19 +348,23 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
     uint8_t *prev_cached_page;
 
     if (!cache_is_cached(XBZRLE.cache, current_addr)) {
+        acct_info.xbzrle_cache_miss++;
         if (!last_stage) {
-            if (cache_insert(XBZRLE.cache, current_addr, current_data) == -1) {
+            if (cache_insert(XBZRLE.cache, current_addr, *current_data) == -1) {
                 return -1;
+            } else {
+                /* update *current_data when the page has been
+                   inserted into cache */
+                *current_data = get_cached_data(XBZRLE.cache, current_addr);
             }
         }
-        acct_info.xbzrle_cache_miss++;
         return -1;
     }
 
     prev_cached_page = get_cached_data(XBZRLE.cache, current_addr);
 
     /* save current buffer into memory */
-    memcpy(XBZRLE.current_buf, current_data, TARGET_PAGE_SIZE);
+    memcpy(XBZRLE.current_buf, *current_data, TARGET_PAGE_SIZE);
 
     /* XBZRLE encoding (if there is no overflow) */
     encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
@@ -373,7 +377,10 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t *current_data,
         DPRINTF("Overflow\n");
         acct_info.xbzrle_overflows++;
         /* update data in the cache */
-        memcpy(prev_cached_page, current_data, TARGET_PAGE_SIZE);
+        if (!last_stage) {
+            memcpy(prev_cached_page, *current_data, TARGET_PAGE_SIZE);
+            *current_data = prev_cached_page;
+        }
         return -1;
     }
 
@@ -598,15 +605,9 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
                  */
                 xbzrle_cache_zero_page(current_addr);
             } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
-                bytes_sent = save_xbzrle_page(f, p, current_addr, block,
+                bytes_sent = save_xbzrle_page(f, &p, current_addr, block,
                                               offset, cont, last_stage);
                 if (!last_stage) {
-                    /* We must send exactly what's in the xbzrle cache
-                     * even if the page wasn't xbzrle compressed, so that
-                     * it's right next time.
-                     */
-                    p = get_cached_data(XBZRLE.cache, current_addr);
-
                     /* Can't send this cached data async, since the cache page
                      * might get updated before it gets to the wire
                      */
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 02/10] migration: Add counters of updating the dirty bitmap

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

Add counters to log the times of updating the dirty bitmap.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 arch_init.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch_init.c b/arch_init.c
index 2ac68c2..d1e9199 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -110,6 +110,8 @@ static bool mig_throttle_on;
 static int dirty_rate_high_cnt;
 static void check_guest_throttling(void);
 
+static uint64_t bitmap_sync_counter;
+
 /***********************************************************/
 /* ram save/restore */
 
@@ -487,6 +489,8 @@ static void migration_bitmap_sync(void)
     int64_t end_time;
     int64_t bytes_xfer_now;
 
+    bitmap_sync_counter++;
+
     if (!bytes_xfer_prev) {
         bytes_xfer_prev = ram_bytes_transferred();
     }
@@ -734,6 +738,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     migration_dirty_pages = ram_pages;
     mig_throttle_on = false;
     dirty_rate_high_cnt = 0;
+    bitmap_sync_counter = 0;
 
     if (migrate_use_xbzrle()) {
         qemu_mutex_lock_iothread();
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 03/10] migration: expose the bitmap_sync_counter to the end user

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

expose the counter that log the times of updating the dirty bitmap to
end user.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 arch_init.c                   |  1 +
 hmp.c                         |  2 ++
 include/migration/migration.h |  1 +
 migration.c                   |  2 ++
 qapi-schema.json              |  4 +++-
 qmp-commands.hx               | 13 +++++++++----
 6 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index d1e9199..a4f9a87 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -536,6 +536,7 @@ static void migration_bitmap_sync(void)
         s->dirty_bytes_rate = s->dirty_pages_rate * TARGET_PAGE_SIZE;
         start_time = end_time;
         num_dirty_pages_period = 0;
+        s->dirty_sync_counter = bitmap_sync_counter;
     }
 }
 
diff --git a/hmp.c b/hmp.c
index 2f279c4..fd493be 100644
--- a/hmp.c
+++ b/hmp.c
@@ -188,6 +188,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->ram->normal);
         monitor_printf(mon, "normal bytes: %" PRIu64 " kbytes\n",
                        info->ram->normal_bytes >> 10);
+        monitor_printf(mon, "dirty sync counter: %" PRIu64 "\n",
+                       info->ram->dirty_sync_counter);
         if (info->ram->dirty_pages_rate) {
             monitor_printf(mon, "dirty pages rate: %" PRIu64 " pages\n",
                            info->ram->dirty_pages_rate);
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 3e1e6c7..ef8b965 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -61,6 +61,7 @@ struct MigrationState
     bool enabled_capabilities[MIGRATION_CAPABILITY_MAX];
     int64_t xbzrle_cache_size;
     int64_t setup_time;
+    int64_t dirty_sync_counter;
 };
 
 void process_incoming_migration(QEMUFile *f);
diff --git a/migration.c b/migration.c
index 00f465e..d2478e4 100644
--- a/migration.c
+++ b/migration.c
@@ -220,6 +220,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->ram->normal_bytes = norm_mig_bytes_transferred();
         info->ram->dirty_pages_rate = s->dirty_pages_rate;
         info->ram->mbps = s->mbps;
+        info->ram->dirty_sync_counter = s->dirty_sync_counter;
 
         if (blk_mig_active()) {
             info->has_disk = true;
@@ -253,6 +254,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
         info->ram->normal = norm_mig_pages_transferred();
         info->ram->normal_bytes = norm_mig_bytes_transferred();
         info->ram->mbps = s->mbps;
+        info->ram->dirty_sync_counter = s->dirty_sync_counter;
         break;
     case MIG_STATE_ERROR:
         info->has_status = true;
diff --git a/qapi-schema.json b/qapi-schema.json
index f4f9439..e26a2d0 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -651,13 +651,15 @@
 #
 # @mbps: throughput in megabits/sec. (since 1.6)
 #
+# @dirty-sync-counter: the times of ram dirty sync (since 2.1)
+#
 # Since: 0.14.0
 ##
 { 'type': 'MigrationStats',
   'data': {'transferred': 'int', 'remaining': 'int', 'total': 'int' ,
            'duplicate': 'int', 'skipped': 'int', 'normal': 'int',
            'normal-bytes': 'int', 'dirty-pages-rate' : 'int',
-           'mbps' : 'number' } }
+           'mbps' : 'number', 'dirty-sync-counter' : 'int' } }
 
 ##
 # @XBZRLECacheStats
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d982cd6..5fd89cf 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2928,6 +2928,7 @@ The main json-object contains the following:
             pages. This is just normal pages times size of one page,
             but this way upper levels don't need to care about page
             size (json-int)
+         - "dirty-sync-counter": the times of ram dirty sync (json-int)
 - "disk": only present if "status" is "active" and it is a block migration,
   it is a json-object with the following disk information:
          - "transferred": amount transferred in bytes (json-int)
@@ -2965,7 +2966,8 @@ Examples:
           "downtime":12345,
           "duplicate":123,
           "normal":123,
-          "normal-bytes":123456
+          "normal-bytes":123456,
+          "dirty-sync-counter":15
         }
      }
    }
@@ -2990,7 +2992,8 @@ Examples:
             "expected-downtime":12345,
             "duplicate":123,
             "normal":123,
-            "normal-bytes":123456
+            "normal-bytes":123456,
+            "dirty-sync-counter":15
          }
       }
    }
@@ -3010,7 +3013,8 @@ Examples:
             "expected-downtime":12345,
             "duplicate":123,
             "normal":123,
-            "normal-bytes":123456
+            "normal-bytes":123456,
+            "dirty-sync-counter":15
          },
          "disk":{
             "total":20971520,
@@ -3036,7 +3040,8 @@ Examples:
             "expected-downtime":12345,
             "duplicate":10,
             "normal":3333,
-            "normal-bytes":3412992
+            "normal-bytes":3412992,
+            "dirty-sync-counter":15
          },
          "xbzrle-cache":{
             "cache-size":67108864,
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 04/10] migration: expose xbzrle cache miss rate

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

expose xbzrle cache miss rate

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 arch_init.c                   | 18 ++++++++++++++++++
 hmp.c                         |  2 ++
 include/migration/migration.h |  1 +
 migration.c                   |  1 +
 qapi-schema.json              |  5 ++++-
 qmp-commands.hx               |  2 ++
 6 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/arch_init.c b/arch_init.c
index a4f9a87..9ceb22e 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -235,6 +235,7 @@ typedef struct AccountingInfo {
     uint64_t xbzrle_bytes;
     uint64_t xbzrle_pages;
     uint64_t xbzrle_cache_miss;
+    double xbzrle_cache_miss_rate;
     uint64_t xbzrle_overflows;
 } AccountingInfo;
 
@@ -290,6 +291,11 @@ uint64_t xbzrle_mig_pages_cache_miss(void)
     return acct_info.xbzrle_cache_miss;
 }
 
+double xbzrle_mig_cache_miss_rate(void)
+{
+    return acct_info.xbzrle_cache_miss_rate;
+}
+
 uint64_t xbzrle_mig_pages_overflow(void)
 {
     return acct_info.xbzrle_overflows;
@@ -488,6 +494,8 @@ static void migration_bitmap_sync(void)
     static int64_t num_dirty_pages_period;
     int64_t end_time;
     int64_t bytes_xfer_now;
+    static uint64_t xbzrle_cache_miss_prev;
+    static uint64_t iterations_prev;
 
     bitmap_sync_counter++;
 
@@ -531,6 +539,16 @@ static void migration_bitmap_sync(void)
         } else {
              mig_throttle_on = false;
         }
+        if (migrate_use_xbzrle()) {
+            if (iterations_prev != 0) {
+                acct_info.xbzrle_cache_miss_rate =
+                   (double)(acct_info.xbzrle_cache_miss -
+                            xbzrle_cache_miss_prev) /
+                   (acct_info.iterations - iterations_prev);
+            }
+            iterations_prev = acct_info.iterations;
+            xbzrle_cache_miss_prev = acct_info.xbzrle_cache_miss;
+        }
         s->dirty_pages_rate = num_dirty_pages_period * 1000
             / (end_time - start_time);
         s->dirty_bytes_rate = s->dirty_pages_rate * TARGET_PAGE_SIZE;
diff --git a/hmp.c b/hmp.c
index fd493be..79dc996 100644
--- a/hmp.c
+++ b/hmp.c
@@ -214,6 +214,8 @@ void hmp_info_migrate(Monitor *mon, const QDict *qdict)
                        info->xbzrle_cache->pages);
         monitor_printf(mon, "xbzrle cache miss: %" PRIu64 "\n",
                        info->xbzrle_cache->cache_miss);
+        monitor_printf(mon, "xbzrle cache miss rate: %0.2f\n",
+                       info->xbzrle_cache->cache_miss_rate);
         monitor_printf(mon, "xbzrle overflow : %" PRIu64 "\n",
                        info->xbzrle_cache->overflow);
     }
diff --git a/include/migration/migration.h b/include/migration/migration.h
index ef8b965..125721c 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -126,6 +126,7 @@ uint64_t xbzrle_mig_bytes_transferred(void);
 uint64_t xbzrle_mig_pages_transferred(void);
 uint64_t xbzrle_mig_pages_overflow(void);
 uint64_t xbzrle_mig_pages_cache_miss(void);
+double xbzrle_mig_cache_miss_rate(void);
 
 void ram_handle_compressed(void *host, uint8_t ch, uint64_t size);
 
diff --git a/migration.c b/migration.c
index d2478e4..8db6553 100644
--- a/migration.c
+++ b/migration.c
@@ -179,6 +179,7 @@ static void get_xbzrle_cache_stats(MigrationInfo *info)
         info->xbzrle_cache->bytes = xbzrle_mig_bytes_transferred();
         info->xbzrle_cache->pages = xbzrle_mig_pages_transferred();
         info->xbzrle_cache->cache_miss = xbzrle_mig_pages_cache_miss();
+        info->xbzrle_cache->cache_miss_rate = xbzrle_mig_cache_miss_rate();
         info->xbzrle_cache->overflow = xbzrle_mig_pages_overflow();
     }
 }
diff --git a/qapi-schema.json b/qapi-schema.json
index e26a2d0..3078a02 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -674,13 +674,16 @@
 #
 # @cache-miss: number of cache miss
 #
+# @cache-miss-rate: rate of cache miss
+#
 # @overflow: number of overflows
 #
 # Since: 1.2
 ##
 { 'type': 'XBZRLECacheStats',
   'data': {'cache-size': 'int', 'bytes': 'int', 'pages': 'int',
-           'cache-miss': 'int', 'overflow': 'int' } }
+           'cache-miss': 'int', 'cache-miss-rate': 'number',
+           'overflow': 'int' } }
 
 ##
 # @MigrationInfo
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5fd89cf..1db17c6 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2940,6 +2940,7 @@ The main json-object contains the following:
          - "bytes": number of bytes transferred for XBZRLE compressed pages
          - "pages": number of XBZRLE compressed pages
          - "cache-miss": number of XBRZRLE page cache misses
+         - "cache-miss-rate": rate of XBRZRLE page cache misses
          - "overflow": number of times XBZRLE overflows.  This means
            that the XBZRLE encoding was bigger than just sent the
            whole page, and then we sent the whole page instead (as as
@@ -3048,6 +3049,7 @@ Examples:
             "bytes":20971520,
             "pages":2444343,
             "cache-miss":2244,
+            "cache-miss-rate":0.123,
             "overflow":34434
          }
       }
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 05/10] XBZRLE: optimize XBZRLE to decrease the cache missing

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

Avoid hot pages being replaced by others to remarkably decrease cache

Sample results with the test program which quote from xbzrle.txt ran in
vm:(migrate bandwidth:1GE and xbzrle cache size 8MB)

the test program:

include <stdlib.h>
include <stdio.h>
int main()
 {
        char *buf = (char *) calloc(4096, 4096);
        while (1) {
            int i;
            for (i = 0; i < 4096 * 4; i++) {
                buf[i * 4096 / 4]++;
            }
            printf(".");
        }
 }

before this patch:
virsh qemu-monitor-command test_vm '{"execute": "query-migrate"}'
{"return":{"expected-downtime":1020,"xbzrle-cache":{"bytes":1108284,
"cache-size":8388608,"cache-miss-rate":0.987013,"pages":18297,"overflow":8,
"cache-miss":1228737},"status":"active","setup-time":10,"total-time":52398,
"ram":{"total":12466991104,"remaining":1695744,"mbps":935.559472,
"transferred":5780760580,"dirty-sync-counter":271,"duplicate":2878530,
"dirty-pages-rate":29130,"skipped":0,"normal-bytes":5748592640,
"normal":1403465}},"id":"libvirt-706"}

cache-miss-rate is 98.7%, totally miss.

after optimizing:
virsh qemu-monitor-command test_vm '{"execute": "query-migrate"}'
{"return":{"expected-downtime":2054,"xbzrle-cache":{"bytes":5066763,
"cache-size":8388608,"cache-miss-rate":0.485924,"pages":194823,"overflow":0,
"cache-miss":210653},"status":"active","setup-time":11,"total-time":18729,
"ram":{"total":12466991104,"remaining":3895296,"mbps":937.663549,
"transferred":1615042219,"dirty-sync-counter":98,"duplicate":2869840,
"dirty-pages-rate":58781,"skipped":0,"normal-bytes":1588404224,
"normal":387794}},"id":"libvirt-266"}

The value of cache-miss-rate decrease 49.13%.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 arch_init.c                    |  8 +++++---
 include/migration/page_cache.h | 10 +++++++---
 page_cache.c                   | 23 +++++++++++++++++++----
 3 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 9ceb22e..21d9cf6 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -343,7 +343,8 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
 
     /* We don't care if this fails to allocate a new cache page
      * as long as it updated an old one */
-    cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE);
+    cache_insert(XBZRLE.cache, current_addr, ZERO_TARGET_PAGE,
+                 bitmap_sync_counter);
 }
 
 #define ENCODING_FLAG_XBZRLE 0x1
@@ -355,10 +356,11 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
     int encoded_len = 0, bytes_sent = -1;
     uint8_t *prev_cached_page;
 
-    if (!cache_is_cached(XBZRLE.cache, current_addr)) {
+    if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_counter)) {
         acct_info.xbzrle_cache_miss++;
         if (!last_stage) {
-            if (cache_insert(XBZRLE.cache, current_addr, *current_data) == -1) {
+            if (cache_insert(XBZRLE.cache, current_addr, *current_data,
+                             bitmap_sync_counter) == -1) {
                 return -1;
             } else {
                 /* update *current_data when the page has been
diff --git a/include/migration/page_cache.h b/include/migration/page_cache.h
index 2d5ce2d..10ed532 100644
--- a/include/migration/page_cache.h
+++ b/include/migration/page_cache.h
@@ -43,8 +43,10 @@ void cache_fini(PageCache *cache);
  *
  * @cache pointer to the PageCache struct
  * @addr: page addr
+ * @current_age: current bitmap generation
  */
-bool cache_is_cached(const PageCache *cache, uint64_t addr);
+bool cache_is_cached(const PageCache *cache, uint64_t addr,
+                     uint64_t current_age);
 
 /**
  * get_cached_data: Get the data cached for an addr
@@ -60,13 +62,15 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr);
  * cache_insert: insert the page into the cache. the page cache
  * will dup the data on insert. the previous value will be overwritten
  *
- * Returns -1 on error
+ * Returns -1 when the page isn't inserted into cache
  *
  * @cache pointer to the PageCache struct
  * @addr: page address
  * @pdata: pointer to the page
+ * @current_age: current bitmap generation
  */
-int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata);
+int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
+                 uint64_t current_age);
 
 /**
  * cache_resize: resize the page cache. In case of size reduction the extra
diff --git a/page_cache.c b/page_cache.c
index b033681..c78157b 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -33,6 +33,9 @@
     do { } while (0)
 #endif
 
+/* the page in cache will not be replaced in two cycles */
+#define CACHED_PAGE_LIFETIME 2
+
 typedef struct CacheItem CacheItem;
 
 struct CacheItem {
@@ -121,7 +124,8 @@ static size_t cache_get_cache_pos(const PageCache *cache,
     return pos;
 }
 
-bool cache_is_cached(const PageCache *cache, uint64_t addr)
+bool cache_is_cached(const PageCache *cache, uint64_t addr,
+                     uint64_t current_age)
 {
     size_t pos;
 
@@ -130,7 +134,12 @@ bool cache_is_cached(const PageCache *cache, uint64_t addr)
 
     pos = cache_get_cache_pos(cache, addr);
 
-    return (cache->page_cache[pos].it_addr == addr);
+    if (cache->page_cache[pos].it_addr == addr) {
+        /* update the it_age when the cache hit */
+        cache->page_cache[pos].it_age = current_age;
+        return true;
+    }
+    return false;
 }
 
 static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
@@ -150,7 +159,8 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
     return cache_get_by_addr(cache, addr)->it_data;
 }
 
-int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
+int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
+                 uint64_t current_age)
 {
 
     CacheItem *it = NULL;
@@ -161,6 +171,11 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
     /* actual update of entry */
     it = cache_get_by_addr(cache, addr);
 
+    if (it->it_data &&
+        it->it_age + CACHED_PAGE_LIFETIME > current_age) {
+        /* the cache page is fresh, don't replace it */
+        return -1;
+    }
     /* allocate page */
     if (!it->it_data) {
         it->it_data = g_try_malloc(cache->page_size);
@@ -173,7 +188,7 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata)
 
     memcpy(it->it_data, pdata, cache->page_size);
 
-    it->it_age = ++cache->max_item_age;
+    it->it_age = current_age;
     it->it_addr = addr;
 
     return 0;
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 06/10] XBZRLE: rebuild the cache_is_cached function

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

Rebuild the cache_is_cached function by cache_get_by_addr.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 page_cache.c | 38 ++++++++++++++++----------------------
 1 file changed, 16 insertions(+), 22 deletions(-)

diff --git a/page_cache.c b/page_cache.c
index c78157b..3190c55 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -124,24 +124,6 @@ static size_t cache_get_cache_pos(const PageCache *cache,
     return pos;
 }
 
-bool cache_is_cached(const PageCache *cache, uint64_t addr,
-                     uint64_t current_age)
-{
-    size_t pos;
-
-    g_assert(cache);
-    g_assert(cache->page_cache);
-
-    pos = cache_get_cache_pos(cache, addr);
-
-    if (cache->page_cache[pos].it_addr == addr) {
-        /* update the it_age when the cache hit */
-        cache->page_cache[pos].it_age = current_age;
-        return true;
-    }
-    return false;
-}
-
 static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
 {
     size_t pos;
@@ -159,14 +141,26 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
     return cache_get_by_addr(cache, addr)->it_data;
 }
 
+bool cache_is_cached(const PageCache *cache, uint64_t addr,
+                     uint64_t current_age)
+{
+    CacheItem *it;
+
+    it = cache_get_by_addr(cache, addr);
+
+    if (it->it_addr == addr) {
+        /* update the it_age when the cache hit */
+        it->it_age = current_age;
+        return true;
+    }
+    return false;
+}
+
 int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
                  uint64_t current_age)
 {
 
-    CacheItem *it = NULL;
-
-    g_assert(cache);
-    g_assert(cache->page_cache);
+    CacheItem *it;
 
     /* actual update of entry */
     it = cache_get_by_addr(cache, addr);
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 07/10] migration: Fix the migrate auto converge process

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

It is inexact and complex to use the migration transfer speed to
determine whether migration is converging.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 arch_init.c | 26 +++-----------------------
 1 file changed, 3 insertions(+), 23 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 21d9cf6..d5479f2 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -107,7 +107,6 @@ int graphic_depth = 32;
 
 const uint32_t arch_type = QEMU_ARCH;
 static bool mig_throttle_on;
-static int dirty_rate_high_cnt;
 static void check_guest_throttling(void);
 
 static uint64_t bitmap_sync_counter;
@@ -492,19 +491,13 @@ static void migration_bitmap_sync(void)
     uint64_t num_dirty_pages_init = migration_dirty_pages;
     MigrationState *s = migrate_get_current();
     static int64_t start_time;
-    static int64_t bytes_xfer_prev;
     static int64_t num_dirty_pages_period;
     int64_t end_time;
-    int64_t bytes_xfer_now;
     static uint64_t xbzrle_cache_miss_prev = 0;
     static uint64_t iterations_prev = 0;
 
     bitmap_sync_counter++;
 
-    if (!bytes_xfer_prev) {
-        bytes_xfer_prev = ram_bytes_transferred();
-    }
-
     if (!start_time) {
         start_time = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
     }
@@ -523,21 +516,9 @@ static void migration_bitmap_sync(void)
     /* more than 1 second = 1000 millisecons */
     if (end_time > start_time + 1000) {
         if (migrate_auto_converge()) {
-            /* The following detection logic can be refined later. For now:
-               Check to see if the dirtied bytes is 50% more than the approx.
-               amount of bytes that just got transferred since the last time we
-               were in this routine. If that happens >N times (for now N==4)
-               we turn on the throttle down logic */
-            bytes_xfer_now = ram_bytes_transferred();
-            if (s->dirty_pages_rate &&
-               (num_dirty_pages_period * TARGET_PAGE_SIZE >
-                   (bytes_xfer_now - bytes_xfer_prev)/2) &&
-               (dirty_rate_high_cnt++ > 4)) {
-                    trace_migration_throttle();
-                    mig_throttle_on = true;
-                    dirty_rate_high_cnt = 0;
-             }
-             bytes_xfer_prev = bytes_xfer_now;
+            if (bitmap_sync_counter > 20) {
+                mig_throttle_on = true;
+            }
         } else {
              mig_throttle_on = false;
         }
@@ -758,7 +739,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     bitmap_set(migration_bitmap, 0, ram_pages);
     migration_dirty_pages = ram_pages;
     mig_throttle_on = false;
-    dirty_rate_high_cnt = 0;
     bitmap_sync_counter = 0;
 
     if (migrate_use_xbzrle()) {
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 08/10] migration: optimize xbzrle by reducing data copy

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

Reducing data copy can reduce cpu overhead.

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index d5479f2..76e13aa 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -372,11 +372,8 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
 
     prev_cached_page = get_cached_data(XBZRLE.cache, current_addr);
 
-    /* save current buffer into memory */
-    memcpy(XBZRLE.current_buf, *current_data, TARGET_PAGE_SIZE);
-
     /* XBZRLE encoding (if there is no overflow) */
-    encoded_len = xbzrle_encode_buffer(prev_cached_page, XBZRLE.current_buf,
+    encoded_len = xbzrle_encode_buffer(prev_cached_page, *current_data,
                                        TARGET_PAGE_SIZE, XBZRLE.encoded_buf,
                                        TARGET_PAGE_SIZE);
     if (encoded_len == 0) {
@@ -395,7 +392,8 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
 
     /* we need to update the data in the cache, in order to get the same data */
     if (!last_stage) {
-        memcpy(prev_cached_page, XBZRLE.current_buf, TARGET_PAGE_SIZE);
+        xbzrle_decode_buffer(XBZRLE.encoded_buf, encoded_len, prev_cached_page,
+                             TARGET_PAGE_SIZE);
     }
 
     /* Send XBZRLE based compressed page */
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 09/10] migration: clear the dead code

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

clear the dead code

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c  | 13 -------------
 page_cache.c | 58 ----------------------------------------------------------
 2 files changed, 71 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 76e13aa..cf3800c 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -163,14 +163,11 @@ static inline bool is_zero_range(uint8_t *p, uint64_t size)
 static struct {
     /* buffer used for XBZRLE encoding */
     uint8_t *encoded_buf;
-    /* buffer for storing page content */
-    uint8_t *current_buf;
     /* Cache for XBZRLE, Protected by lock. */
     PageCache *cache;
     QemuMutex lock;
 } XBZRLE = {
     .encoded_buf = NULL,
-    .current_buf = NULL,
     .cache = NULL,
 };
 /* buffer used for XBZRLE decoding */
@@ -704,10 +701,8 @@ static void migration_end(void)
         cache_fini(XBZRLE.cache);
         g_free(XBZRLE.cache);
         g_free(XBZRLE.encoded_buf);
-        g_free(XBZRLE.current_buf);
         XBZRLE.cache = NULL;
         XBZRLE.encoded_buf = NULL;
-        XBZRLE.current_buf = NULL;
     }
     XBZRLE_cache_unlock();
 }
@@ -759,14 +754,6 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
             return -1;
         }
 
-        XBZRLE.current_buf = g_try_malloc(TARGET_PAGE_SIZE);
-        if (!XBZRLE.current_buf) {
-            DPRINTF("Error allocating current_buf\n");
-            g_free(XBZRLE.encoded_buf);
-            XBZRLE.encoded_buf = NULL;
-            return -1;
-        }
-
         acct_clear();
     }
 
diff --git a/page_cache.c b/page_cache.c
index 3190c55..7dfa762 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -48,7 +48,6 @@ struct PageCache {
     CacheItem *page_cache;
     unsigned int page_size;
     int64_t max_num_items;
-    uint64_t max_item_age;
     int64_t num_items;
 };
 
@@ -76,7 +75,6 @@ PageCache *cache_init(int64_t num_pages, unsigned int page_size)
     }
     cache->page_size = page_size;
     cache->num_items = 0;
-    cache->max_item_age = 0;
     cache->max_num_items = num_pages;
 
     DPRINTF("Setting cache buckets to %" PRId64 "\n", cache->max_num_items);
@@ -187,59 +185,3 @@ int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
 
     return 0;
 }
-
-int64_t cache_resize(PageCache *cache, int64_t new_num_pages)
-{
-    PageCache *new_cache;
-    int64_t i;
-
-    CacheItem *old_it, *new_it;
-
-    g_assert(cache);
-
-    /* cache was not inited */
-    if (cache->page_cache == NULL) {
-        return -1;
-    }
-
-    /* same size */
-    if (pow2floor(new_num_pages) == cache->max_num_items) {
-        return cache->max_num_items;
-    }
-
-    new_cache = cache_init(new_num_pages, cache->page_size);
-    if (!(new_cache)) {
-        DPRINTF("Error creating new cache\n");
-        return -1;
-    }
-
-    /* move all data from old cache */
-    for (i = 0; i < cache->max_num_items; i++) {
-        old_it = &cache->page_cache[i];
-        if (old_it->it_addr != -1) {
-            /* check for collision, if there is, keep MRU page */
-            new_it = cache_get_by_addr(new_cache, old_it->it_addr);
-            if (new_it->it_data && new_it->it_age >= old_it->it_age) {
-                /* keep the MRU page */
-                g_free(old_it->it_data);
-            } else {
-                if (!new_it->it_data) {
-                    new_cache->num_items++;
-                }
-                g_free(new_it->it_data);
-                new_it->it_data = old_it->it_data;
-                new_it->it_age = old_it->it_age;
-                new_it->it_addr = old_it->it_addr;
-            }
-        }
-    }
-
-    g_free(cache->page_cache);
-    cache->page_cache = new_cache->page_cache;
-    cache->max_num_items = new_cache->max_num_items;
-    cache->num_items = new_cache->num_items;
-
-    g_free(new_cache);
-
-    return cache->max_num_items;
-}
-- 
1.7.12.4

[Qemu-devel] [PATCH v3 10/10] XBZRLE: update the doc of XBZRLE

[arei.gonglei]

From: ChenLiang <chenliang88@huawei.com>

update the doc of XBZRLE

Signed-off-by: ChenLiang <chenliang88@huawei.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 docs/xbzrle.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/docs/xbzrle.txt b/docs/xbzrle.txt
index cc3a26a..cdf1e3e 100644
--- a/docs/xbzrle.txt
+++ b/docs/xbzrle.txt
@@ -71,6 +71,13 @@ encoded buffer:
 encoded length 24
 e9 07 0f 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 03 01 67 01 01 69
 
+The strategy of updating cache
+=================================
+Keeping the hot page in cache is effective to decrease cache missing.
+XBZRLE use a counter as the age of page. The counter will increase
+after the ram dirty bitmap syncing. When cache conflicts XBZRLE only
+replace the old page in cache.
+
 Usage
 ======================
 1. Verify the destination QEMU version is able to decode the new format.
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v3 06/10] XBZRLE: rebuild the cache_is_cached function

[Dr. David Alan Gilbert]

* arei.gonglei@huawei.com (arei.gonglei@huawei.com) wrote:
> From: ChenLiang <chenliang88@huawei.com>
> 
> Rebuild the cache_is_cached function by cache_get_by_addr.
> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

>  page_cache.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 
> diff --git a/page_cache.c b/page_cache.c
> index c78157b..3190c55 100644
> --- a/page_cache.c
> +++ b/page_cache.c
> @@ -124,24 +124,6 @@ static size_t cache_get_cache_pos(const PageCache *cache,
>      return pos;
>  }
>  
> -bool cache_is_cached(const PageCache *cache, uint64_t addr,
> -                     uint64_t current_age)
> -{
> -    size_t pos;
> -
> -    g_assert(cache);
> -    g_assert(cache->page_cache);
> -
> -    pos = cache_get_cache_pos(cache, addr);
> -
> -    if (cache->page_cache[pos].it_addr == addr) {
> -        /* update the it_age when the cache hit */
> -        cache->page_cache[pos].it_age = current_age;
> -        return true;
> -    }
> -    return false;
> -}
> -
>  static CacheItem *cache_get_by_addr(const PageCache *cache, uint64_t addr)
>  {
>      size_t pos;
> @@ -159,14 +141,26 @@ uint8_t *get_cached_data(const PageCache *cache, uint64_t addr)
>      return cache_get_by_addr(cache, addr)->it_data;
>  }
>  
> +bool cache_is_cached(const PageCache *cache, uint64_t addr,
> +                     uint64_t current_age)
> +{
> +    CacheItem *it;
> +
> +    it = cache_get_by_addr(cache, addr);
> +
> +    if (it->it_addr == addr) {
> +        /* update the it_age when the cache hit */
> +        it->it_age = current_age;
> +        return true;
> +    }
> +    return false;
> +}
> +
>  int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
>                   uint64_t current_age)
>  {
>  
> -    CacheItem *it = NULL;
> -
> -    g_assert(cache);
> -    g_assert(cache->page_cache);
> +    CacheItem *it;
>  
>      /* actual update of entry */
>      it = cache_get_by_addr(cache, addr);
> -- 
> 1.7.12.4
> 
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v3 03/10] migration: expose the bitmap_sync_counter to the end user

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On 03/18/2014 06:24 AM, arei.gonglei@huawei.com wrote:
> From: ChenLiang <chenliang88@huawei.com>
> 
> expose the counter that log the times of updating the dirty bitmap to

s/log/logs/

> end user.
> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  arch_init.c                   |  1 +
>  hmp.c                         |  2 ++
>  include/migration/migration.h |  1 +
>  migration.c                   |  2 ++
>  qapi-schema.json              |  4 +++-
>  qmp-commands.hx               | 13 +++++++++----
>  6 files changed, 18 insertions(+), 5 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -651,13 +651,15 @@
>  #
>  # @mbps: throughput in megabits/sec. (since 1.6)
>  #
> +# @dirty-sync-counter: the times of ram dirty sync (since 2.1)

The name could possibly be shortened.  Also, this sentence reads
awkwardly.  Maybe:

@dirty-sync-count: number of times that dirty ram was synchronized
(since 2.1)

> +++ b/qmp-commands.hx
> @@ -2928,6 +2928,7 @@ The main json-object contains the following:
>              pages. This is just normal pages times size of one page,
>              but this way upper levels don't need to care about page
>              size (json-int)
> +         - "dirty-sync-counter": the times of ram dirty sync (json-int)

and if you do change the wording above (or even shorten the variable
name), make it match here

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 02/10] migration: Add counters of updating the dirty bitmap

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 503 bytes --]

On 03/18/2014 06:24 AM, arei.gonglei@huawei.com wrote:
> From: ChenLiang <chenliang88@huawei.com>
> 
> Add counters to log the times of updating the dirty bitmap.
> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  arch_init.c | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 04/10] migration: expose xbzrle cache miss rate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 863 bytes --]

On 03/18/2014 06:24 AM, arei.gonglei@huawei.com wrote:
> From: ChenLiang <chenliang88@huawei.com>
> 
> expose xbzrle cache miss rate
> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  arch_init.c                   | 18 ++++++++++++++++++
>  hmp.c                         |  2 ++
>  include/migration/migration.h |  1 +
>  migration.c                   |  1 +
>  qapi-schema.json              |  5 ++++-
>  qmp-commands.hx               |  2 ++
>  6 files changed, 28 insertions(+), 1 deletion(-)
> 

> +++ b/qapi-schema.json
> @@ -674,13 +674,16 @@
>  #
>  # @cache-miss: number of cache miss
>  #
> +# @cache-miss-rate: rate of cache miss

Missing '(since 2.1)'

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 05/10] XBZRLE: optimize XBZRLE to decrease the cache missing

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

On 03/18/2014 06:24 AM, arei.gonglei@huawei.com wrote:
> From: ChenLiang <chenliang88@huawei.com>

In the subject: s/missing/misses/

> 
> Avoid hot pages being replaced by others to remarkably decrease cache

s/cache/cache misses/

> 
> before this patch:
> virsh qemu-monitor-command test_vm '{"execute": "query-migrate"}'
> {"return":{"expected-downtime":1020,"xbzrle-cache":{"bytes":1108284,

1.1M bytes saved by compression,

> "cache-size":8388608,"cache-miss-rate":0.987013,"pages":18297,"overflow":8,

18k pages sent compressed

> "cache-miss":1228737},"status":"active","setup-time":10,"total-time":52398,
> "ram":{"total":12466991104,"remaining":1695744,"mbps":935.559472,
> "transferred":5780760580,"dirty-sync-counter":271,"duplicate":2878530,
> "dirty-pages-rate":29130,"skipped":0,"normal-bytes":5748592640,
> "normal":1403465}},"id":"libvirt-706"}
> 
> cache-miss-rate is 98.7%, totally miss.
> 
> after optimizing:
> virsh qemu-monitor-command test_vm '{"execute": "query-migrate"}'
> {"return":{"expected-downtime":2054,"xbzrle-cache":{"bytes":5066763,

5.0M bytes saved by compression

> "cache-size":8388608,"cache-miss-rate":0.485924,"pages":194823,"overflow":0,

194k pages sent compressed

> "cache-miss":210653},"status":"active","setup-time":11,"total-time":18729,

And reduced from 52 milleseconds to just under 19 on total time.
Definite improvements!

> "ram":{"total":12466991104,"remaining":3895296,"mbps":937.663549,
> "transferred":1615042219,"dirty-sync-counter":98,"duplicate":2869840,
> "dirty-pages-rate":58781,"skipped":0,"normal-bytes":1588404224,
> "normal":387794}},"id":"libvirt-266"}
> 
> The value of cache-miss-rate decrease 49.13%.

s/decrease/decreased to/

> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  arch_init.c                    |  8 +++++---
>  include/migration/page_cache.h | 10 +++++++---
>  page_cache.c                   | 23 +++++++++++++++++++----
>  3 files changed, 31 insertions(+), 10 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

However, I think it would be worth squashing patch 10 into this one.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 06/10] XBZRLE: rebuild the cache_is_cached function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On 03/18/2014 06:24 AM, arei.gonglei@huawei.com wrote:
> From: ChenLiang <chenliang88@huawei.com>
> 
> Rebuild the cache_is_cached function by cache_get_by_addr.
> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  page_cache.c | 38 ++++++++++++++++----------------------
>  1 file changed, 16 insertions(+), 22 deletions(-)
> 

>  int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
>                   uint64_t current_age)
>  {
>  
> -    CacheItem *it = NULL;
> -
> -    g_assert(cache);
> -    g_assert(cache->page_cache);
> +    CacheItem *it;

Why are you dropping the asserts?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 10/10] XBZRLE: update the doc of XBZRLE

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2370 bytes --]

On 03/18/2014 06:24 AM, arei.gonglei@huawei.com wrote:
> From: ChenLiang <chenliang88@huawei.com>
> 
> update the doc of XBZRLE

Having the subject line and the body of the commit message be identical
is redundant.  And just by looking at the commit message, I can't see
WHY you are updating things.  If you were to keep this as a separate
commit, I'd suggest it look more like:

XBZRLE: document cache miss policy

Add a section to the XBZRLE documentation describing how the page cache
determines which pages are hot.


That said, I think you should squash this documentation update into
patch 5/10 where you actually implement it, so that a single patch
becomes self-documenting why you went with this design.  At which point,
the combined patch commit message should look something like:

XBZRLE: optimize XBZRLE to decrease cache misses

...existing text from 5/10...

Additionally, document the new cache age policy.

> 
> Signed-off-by: ChenLiang <chenliang88@huawei.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  docs/xbzrle.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/docs/xbzrle.txt b/docs/xbzrle.txt
> index cc3a26a..cdf1e3e 100644
> --- a/docs/xbzrle.txt
> +++ b/docs/xbzrle.txt
> @@ -71,6 +71,13 @@ encoded buffer:
>  encoded length 24
>  e9 07 0f 01 02 03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f 03 01 67 01 01 69
>  
> +The strategy of updating cache
> +=================================

Copy-and-paste from bad examples already in this file, but it's nicer
when the length of ==== matches the heading it is paired with.

> +Keeping the hot page in cache is effective to decrease cache missing.
> +XBZRLE use a counter as the age of page. The counter will increase
> +after the ram dirty bitmap syncing. When cache conflicts XBZRLE only
> +replace the old page in cache.

Suggestions for better grammar:

Cache update strategy
=====================
Keeping the hot pages in the cache is effective for decreased cache
misses. XBZRLE uses a counter as the age of each page. The counter will
increase after each ram dirty bitmap sync. When a cache conflict is
detected, XBZRLE will only evict pages in the cache that are older than
a threshold.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 00/10] migration: Optimizate the xbzrle and fix two corruption issues

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1202 bytes --]

On 03/18/2014 06:24 AM, arei.gonglei@huawei.com wrote:
> From: ChenLiang <chenliang88@huawei.com>
> 
> V2-->V3
> * rename the bitmap_sync_cnt to bitmap_sync_counter
> * expose xbzrle cache miss rate
> 
> V1-->V2
> * expose the counter that logs the times of updating the dirty bitmap to end user.
> 
> a. Optimization the xbzrle remarkable decrease the cache misses.
>     The efficiency of compress increases more than fifty times.
>     Before the patch set, the cache almost totally miss when the 
>     number of cache item less than the dirty page number. Now the
>     hot pages in the cache will not be replaced by other pages. 
> 
> b. Reducing the data copy
> 
> c. Fix two corruption issues.

Your QAPI changes were marked 2.1, but this series also includes a
corruption fix.  Please split this series into patches that MUST be
applied for 2.0 as bug fixes, vs. the patches that are a feature
enhancement (or else with strong justification why a speed optimization
is safe enough to be considered a bug fix, if you think the whole series
belongs in 2.0).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 06/10] XBZRLE: rebuild the cache_is_cached function

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1089 bytes --]

On 03/20/2014 01:44 PM, Eric Blake wrote:
> On 03/18/2014 06:24 AM, arei.gonglei@huawei.com wrote:
>> From: ChenLiang <chenliang88@huawei.com>
>>
>> Rebuild the cache_is_cached function by cache_get_by_addr.
>>
>> Signed-off-by: ChenLiang <chenliang88@huawei.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  page_cache.c | 38 ++++++++++++++++----------------------
>>  1 file changed, 16 insertions(+), 22 deletions(-)
>>
> 
>>  int cache_insert(PageCache *cache, uint64_t addr, const uint8_t *pdata,
>>                   uint64_t current_age)
>>  {
>>  
>> -    CacheItem *it = NULL;
>> -
>> -    g_assert(cache);
>> -    g_assert(cache->page_cache);
>> +    CacheItem *it;
> 
> Why are you dropping the asserts?

And if the answer is "because the caller is also asserting the same
thing", then mention it in the commit message.  The best commit messages
are the ones that not only mention WHAT (no silent changes), but also WHY.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]