[Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Juan Quintela]

Hi

(Li special edition)

Current migration code returns number of bytes transferred and from
there we decide if we.have sent something or not.  Problem, we need
two results: number of pages written, and number of bytes written
(depending on compression, zero pages, etc, it is not possible to
derive one value from the other).

So, I changed all relevant function to return the number of written
pages, and then pass as uint64_t *bytes_transferred to update the
written bytes.

On current code, makes things a bit easier to understand, but is not
strictely necesary.  But for the compression patches from Li, it makes
a big difference, we can return that we have written a page, even if
we have just started the write, but having writtten in reality zero
bytes.

Once there, I add doxygen documentation to all function that I touched
(yes, I was long due).

save_block_hdr really saved headers for pages, not blocks.  Rename it,
and simplify the interface.

Li, does this make your life easier?  I hope so.  Should make really
easy to remove the one_bytes_sent "hack", and allow my other suggestions.

Comments?

Later, Juan.

PD.  Code is obviously correct, but I run it through virt-test
     anyways.  And it failed :-(  In despair, I tried virt-test on HEAD,
     and it also failed, so ....  I add another thing to my ToDo list.

PD2.  Modifying ram_save_page() has become complicated, very
      complicated. I think that know a couple of simplications, but
      require touching RDMA code, and anyways, fixing migration in
      "master" looks like a higher priority.


Juan Quintela (6):
  ram: make all save_page functions take a uint64_t parameter
  ram_find_and_save_block:  change calling convention
  ram_save_page: change calling covention
  save_xbzrle_page: change calling convention
  save_block_hdr: we can recalculate the cont parameter here
  rename save_block_hdr to save_page_header

 arch_init.c                   | 176 ++++++++++++++++++++++++++----------------
 include/migration/migration.h |   2 +-
 include/migration/qemu-file.h |   2 +-
 migration/qemu-file.c         |   3 +-
 migration/rdma.c              |   2 +-
 5 files changed, 114 insertions(+), 71 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/6] ram: make all save_page functions take a uint64_t parameter

[Juan Quintela]

It used to be an int, but then we can't pass directly the
bytes_transferred parameter, that would happen later in the series.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c                   | 11 ++++++++---
 include/migration/migration.h |  2 +-
 include/migration/qemu-file.h |  2 +-
 migration/qemu-file.c         |  3 ++-
 migration/rdma.c              |  2 +-
 5 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 89c8fa4..0066cf5 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -580,6 +580,7 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
                          bool last_stage)
 {
     int bytes_sent;
+    uint64_t bytes_xmit;
     int cont;
     ram_addr_t current_addr;
     MemoryRegion *mr = block->mr;
@@ -593,17 +594,21 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,

     /* In doubt sent page as normal */
     bytes_sent = -1;
+    bytes_xmit = 0;
     ret = ram_control_save_page(f, block->offset,
-                           offset, TARGET_PAGE_SIZE, &bytes_sent);
+                           offset, TARGET_PAGE_SIZE, &bytes_xmit);
+    if (bytes_xmit) {
+        bytes_sent = bytes_xmit;
+    }

     XBZRLE_cache_lock();

     current_addr = block->offset + offset;
     if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
         if (ret != RAM_SAVE_CONTROL_DELAYED) {
-            if (bytes_sent > 0) {
+            if (bytes_xmit > 0) {
                 acct_info.norm_pages++;
-            } else if (bytes_sent == 0) {
+            } else if (bytes_xmit == 0) {
                 acct_info.dup_pages++;
             }
         }
diff --git a/include/migration/migration.h b/include/migration/migration.h
index f37348b..bb3b1b3 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -173,6 +173,6 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags);

 size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
                              ram_addr_t offset, size_t size,
-                             int *bytes_sent);
+                             uint64_t *bytes_sent);

 #endif
diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index a923cec..94a8c97 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -82,7 +82,7 @@ typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
                                ram_addr_t block_offset,
                                ram_addr_t offset,
                                size_t size,
-                               int *bytes_sent);
+                               uint64_t *bytes_sent);

 /*
  * Stop any read or write (depending on flags) on the underlying
diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index e66e557..1a4f986 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -161,7 +161,8 @@ void ram_control_load_hook(QEMUFile *f, uint64_t flags)
 }

 size_t ram_control_save_page(QEMUFile *f, ram_addr_t block_offset,
-                         ram_addr_t offset, size_t size, int *bytes_sent)
+                             ram_addr_t offset, size_t size,
+                             uint64_t *bytes_sent)
 {
     if (f->ops->save_page) {
         int ret = f->ops->save_page(f, f->opaque, block_offset,
diff --git a/migration/rdma.c b/migration/rdma.c
index 6bee30c..93f3e5c 100644
--- a/migration/rdma.c
+++ b/migration/rdma.c
@@ -2656,7 +2656,7 @@ static int qemu_rdma_close(void *opaque)
  */
 static size_t qemu_rdma_save_page(QEMUFile *f, void *opaque,
                                   ram_addr_t block_offset, ram_addr_t offset,
-                                  size_t size, int *bytes_sent)
+                                  size_t size, uint64_t *bytes_sent)
 {
     QEMUFileRDMA *rfile = opaque;
     RDMAContext *rdma = rfile->rdma;
-- 
2.1.0

[Qemu-devel] [PATCH 2/6] ram_find_and_save_block: change calling convention

[Juan Quintela]

Add a parameter to pass the number of bytes written, and make it return
the number of pages written instead.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 0066cf5..1133b26 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -650,14 +650,19 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
     return bytes_sent;
 }

-/*
- * ram_find_and_save_block: Finds a page to send and sends it to f
+/**
+ * ram_find_and_save_block: Finds a dirty page and sends it to f
  *
- * Returns:  The number of bytes written.
+ * Returns:  The number of pages written
  *           0 means no dirty pages
+ *
+ * @f: QEMUFile where to send the data
+ * @last_stage: if we are at the completion stage
+ * @bytes_transferred: increase it with the number of transferred bytes
  */

-static int ram_find_and_save_block(QEMUFile *f, bool last_stage)
+static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
+                                   uint64_t *bytes_transferred)
 {
     RAMBlock *block = last_seen_block;
     ram_addr_t offset = last_offset;
@@ -696,7 +701,9 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage)
     last_seen_block = block;
     last_offset = offset;

-    return bytes_sent;
+    *bytes_transferred += bytes_sent;
+
+    return (bytes_sent != 0);
 }

 static uint64_t bytes_transferred;
@@ -869,7 +876,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     int ret;
     int i;
     int64_t t0;
-    int total_sent = 0;
+    int pages_sent = 0;

     qemu_mutex_lock_ramlist();

@@ -882,14 +889,14 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
     t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     i = 0;
     while ((ret = qemu_file_rate_limit(f)) == 0) {
-        int bytes_sent;
+        int pages;

-        bytes_sent = ram_find_and_save_block(f, false);
-        /* no more blocks to sent */
-        if (bytes_sent == 0) {
+        pages = ram_find_and_save_block(f, false, &bytes_transferred);
+        /* no more pages to sent */
+        if (pages == 0) {
             break;
         }
-        total_sent += bytes_sent;
+        pages_sent += pages;
         acct_info.iterations++;
         check_guest_throttling();
         /* we want to check in the 1st loop, just in case it was the 1st time
@@ -916,12 +923,6 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
      */
     ram_control_after_iterate(f, RAM_CONTROL_ROUND);

-    bytes_transferred += total_sent;
-
-    /*
-     * Do not count these 8 bytes into total_sent, so that we can
-     * return 0 if no page had been dirtied.
-     */
     qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
     bytes_transferred += 8;

@@ -930,7 +931,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
         return ret;
     }

-    return total_sent;
+    return pages_sent;
 }

 static int ram_save_complete(QEMUFile *f, void *opaque)
@@ -944,14 +945,13 @@ static int ram_save_complete(QEMUFile *f, void *opaque)

     /* flush all remaining blocks regardless of rate limiting */
     while (true) {
-        int bytes_sent;
+        int pages;

-        bytes_sent = ram_find_and_save_block(f, true);
+        pages = ram_find_and_save_block(f, true, &bytes_transferred);
         /* no more blocks to sent */
-        if (bytes_sent == 0) {
+        if (pages == 0) {
             break;
         }
-        bytes_transferred += bytes_sent;
     }

     ram_control_after_iterate(f, RAM_CONTROL_FINISH);
-- 
2.1.0

[Qemu-devel] [PATCH 3/6] ram_save_page: change calling covention

[Juan Quintela]

Add a parameter to pass the number of bytes written, and make it return
the number of pages written instead.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 57 ++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 19 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 1133b26..f243c6e 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -571,15 +571,21 @@ static void migration_bitmap_sync(void)
     }
 }

-/*
+/**
  * ram_save_page: Send the given page to the stream
  *
- * Returns: Number of bytes written.
+ * Returns: Number of pages written.
+ *
+ * @f: QEMUFile where to send the data
+ * @block: block that contains the page we want to send
+ * @offset: offset inside the block for the page
+ * @last_stage: if we are at the completion stage
+ * @bytes_transferred: increase it with the number of transferred bytes
  */
 static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
-                         bool last_stage)
+                         bool last_stage, uint64_t *bytes_transferred)
 {
-    int bytes_sent;
+    int pages = -1;
     uint64_t bytes_xmit;
     int cont;
     ram_addr_t current_addr;
@@ -593,12 +599,12 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
     p = memory_region_get_ram_ptr(mr) + offset;

     /* In doubt sent page as normal */
-    bytes_sent = -1;
     bytes_xmit = 0;
     ret = ram_control_save_page(f, block->offset,
                            offset, TARGET_PAGE_SIZE, &bytes_xmit);
     if (bytes_xmit) {
-        bytes_sent = bytes_xmit;
+        *bytes_transferred += bytes_xmit;
+        pages = 1;
     }

     XBZRLE_cache_lock();
@@ -614,17 +620,29 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
         }
     } else if (is_zero_range(p, TARGET_PAGE_SIZE)) {
         acct_info.dup_pages++;
-        bytes_sent = save_block_hdr(f, block, offset, cont,
-                                    RAM_SAVE_FLAG_COMPRESS);
+        *bytes_transferred += save_block_hdr(f, block, offset, cont,
+                                             RAM_SAVE_FLAG_COMPRESS);
         qemu_put_byte(f, 0);
-        bytes_sent++;
+        *bytes_transferred += 1;
+        pages = 1;
         /* Must let xbzrle know, otherwise a previous (now 0'd) cached
          * page would be stale
          */
         xbzrle_cache_zero_page(current_addr);
     } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
+        int bytes_sent;
+
         bytes_sent = save_xbzrle_page(f, &p, current_addr, block,
                                       offset, cont, last_stage);
+
+        if (bytes_sent > 0) {
+            *bytes_transferred += bytes_sent;
+            pages = 1;
+        } else if (bytes_sent == 0) {
+            pages = 0;
+        } else {
+            pages = -1;
+        }
         if (!last_stage) {
             /* Can't send this cached data async, since the cache page
              * might get updated before it gets to the wire
@@ -634,20 +652,22 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
     }

     /* XBZRLE overflow or normal page */
-    if (bytes_sent == -1) {
-        bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_PAGE);
+    if (pages == -1) {
+        *bytes_transferred += save_block_hdr(f, block, offset, cont,
+                                             RAM_SAVE_FLAG_PAGE);
         if (send_async) {
             qemu_put_buffer_async(f, p, TARGET_PAGE_SIZE);
         } else {
             qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
         }
-        bytes_sent += TARGET_PAGE_SIZE;
+        *bytes_transferred += TARGET_PAGE_SIZE;
+        pages = 1;
         acct_info.norm_pages++;
     }

     XBZRLE_cache_unlock();

-    return bytes_sent;
+    return pages;
 }

 /**
@@ -667,7 +687,7 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
     RAMBlock *block = last_seen_block;
     ram_addr_t offset = last_offset;
     bool complete_round = false;
-    int bytes_sent = 0;
+    int pages = 0;
     MemoryRegion *mr;

     if (!block)
@@ -689,10 +709,11 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
                 ram_bulk_stage = false;
             }
         } else {
-            bytes_sent = ram_save_page(f, block, offset, last_stage);
+            pages = ram_save_page(f, block, offset, last_stage,
+                                  bytes_transferred);

             /* if page is unmodified, continue to the next */
-            if (bytes_sent > 0) {
+            if (pages > 0) {
                 last_sent_block = block;
                 break;
             }
@@ -701,9 +722,7 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
     last_seen_block = block;
     last_offset = offset;

-    *bytes_transferred += bytes_sent;
-
-    return (bytes_sent != 0);
+    return pages;
 }

 static uint64_t bytes_transferred;
-- 
2.1.0

[Qemu-devel] [PATCH 4/6] save_xbzrle_page: change calling convention

[Juan Quintela]

Add a parameter to pass the number of bytes written, and make it return
the number of pages written instead.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index f243c6e..834f40c 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -352,11 +352,27 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)

 #define ENCODING_FLAG_XBZRLE 0x1

+/**
+ * save_xbzrle_page: compress and send current page
+ *
+ * Returns: 1 means that we wrote the page
+ *          0 means that page is identical to the one already sent
+ *          -1 means that xbzrle would be longer than normal
+ *
+ * @f: QEMUFile where to send the data
+ * @current_data:
+ * @current_addr:
+ * @block: block that contains the page we want to send
+ * @offset: offset inside the block for the page
+ * @last_stage: if we are at the completion stage
+ * @bytes_transferred: increase it with the number of transferred bytes
+ */
 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)
+                            ram_addr_t offset, int cont, bool last_stage,
+                            uint64_t *bytes_transferred)
 {
-    int encoded_len = 0, bytes_sent = -1;
+    int encoded_len = 0, bytes_xbzrle;
     uint8_t *prev_cached_page;

     if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) {
@@ -403,15 +419,16 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
     }

     /* Send XBZRLE based compressed page */
-    bytes_sent = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_XBZRLE);
+    bytes_xbzrle = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_XBZRLE);
     qemu_put_byte(f, ENCODING_FLAG_XBZRLE);
     qemu_put_be16(f, encoded_len);
     qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
-    bytes_sent += encoded_len + 1 + 2;
+    bytes_xbzrle += encoded_len + 1 + 2;
     acct_info.xbzrle_pages++;
-    acct_info.xbzrle_bytes += bytes_sent;
+    acct_info.xbzrle_bytes += bytes_xbzrle;
+    *bytes_transferred += bytes_xbzrle;

-    return bytes_sent;
+    return 1;
 }

 static inline
@@ -630,19 +647,8 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
          */
         xbzrle_cache_zero_page(current_addr);
     } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
-        int bytes_sent;
-
-        bytes_sent = save_xbzrle_page(f, &p, current_addr, block,
-                                      offset, cont, last_stage);
-
-        if (bytes_sent > 0) {
-            *bytes_transferred += bytes_sent;
-            pages = 1;
-        } else if (bytes_sent == 0) {
-            pages = 0;
-        } else {
-            pages = -1;
-        }
+        pages = save_xbzrle_page(f, &p, current_addr, block,
+                                 offset, cont, last_stage, bytes_transferred);
         if (!last_stage) {
             /* Can't send this cached data async, since the cache page
              * might get updated before it gets to the wire
-- 
2.1.0

[Qemu-devel] [PATCH 5/6] save_block_hdr: we can recalculate the cont parameter here

[Juan Quintela]

No need to pass it through all the callers.  Once there, update
last_sent_block here.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 47 ++++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 834f40c..95a61e2 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -304,34 +304,39 @@ uint64_t xbzrle_mig_pages_overflow(void)
     return acct_info.xbzrle_overflows;
 }

+/* This is the last block that we have visited serching for dirty pages
+ */
+static RAMBlock *last_seen_block;
+/* This is the last block from where we have sent data */
+static RAMBlock *last_sent_block;
+static ram_addr_t last_offset;
+static unsigned long *migration_bitmap;
+static uint64_t migration_dirty_pages;
+static uint32_t last_version;
+static bool ram_bulk_stage;
+
 static size_t save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
-                             int cont, int flag)
+                             int flag)
 {
     size_t size;

-    qemu_put_be64(f, offset | cont | flag);
+    if (block == last_sent_block) {
+        offset |= RAM_SAVE_FLAG_CONTINUE;
+    }
+
+    qemu_put_be64(f, offset | flag);
     size = 8;

-    if (!cont) {
+    if (block != last_sent_block) {
         qemu_put_byte(f, strlen(block->idstr));
         qemu_put_buffer(f, (uint8_t *)block->idstr,
                         strlen(block->idstr));
         size += 1 + strlen(block->idstr);
+        last_sent_block = block;
     }
     return size;
 }

-/* This is the last block that we have visited serching for dirty pages
- */
-static RAMBlock *last_seen_block;
-/* This is the last block from where we have sent data */
-static RAMBlock *last_sent_block;
-static ram_addr_t last_offset;
-static unsigned long *migration_bitmap;
-static uint64_t migration_dirty_pages;
-static uint32_t last_version;
-static bool ram_bulk_stage;
-
 /* Update the xbzrle cache to reflect a page that's been sent as all 0.
  * The important thing is that a stale (not-yet-0'd) page be replaced
  * by the new data.
@@ -369,7 +374,7 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
  */
 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,
+                            ram_addr_t offset, bool last_stage,
                             uint64_t *bytes_transferred)
 {
     int encoded_len = 0, bytes_xbzrle;
@@ -419,7 +424,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
     }

     /* Send XBZRLE based compressed page */
-    bytes_xbzrle = save_block_hdr(f, block, offset, cont, RAM_SAVE_FLAG_XBZRLE);
+    bytes_xbzrle = save_block_hdr(f, block, offset, RAM_SAVE_FLAG_XBZRLE);
     qemu_put_byte(f, ENCODING_FLAG_XBZRLE);
     qemu_put_be16(f, encoded_len);
     qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
@@ -604,15 +609,12 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
 {
     int pages = -1;
     uint64_t bytes_xmit;
-    int cont;
     ram_addr_t current_addr;
     MemoryRegion *mr = block->mr;
     uint8_t *p;
     int ret;
     bool send_async = true;

-    cont = (block == last_sent_block) ? RAM_SAVE_FLAG_CONTINUE : 0;
-
     p = memory_region_get_ram_ptr(mr) + offset;

     /* In doubt sent page as normal */
@@ -637,7 +639,7 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
         }
     } else if (is_zero_range(p, TARGET_PAGE_SIZE)) {
         acct_info.dup_pages++;
-        *bytes_transferred += save_block_hdr(f, block, offset, cont,
+        *bytes_transferred += save_block_hdr(f, block, offset,
                                              RAM_SAVE_FLAG_COMPRESS);
         qemu_put_byte(f, 0);
         *bytes_transferred += 1;
@@ -648,7 +650,7 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
         xbzrle_cache_zero_page(current_addr);
     } else if (!ram_bulk_stage && migrate_use_xbzrle()) {
         pages = save_xbzrle_page(f, &p, current_addr, block,
-                                 offset, cont, last_stage, bytes_transferred);
+                                 offset, last_stage, bytes_transferred);
         if (!last_stage) {
             /* Can't send this cached data async, since the cache page
              * might get updated before it gets to the wire
@@ -659,7 +661,7 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,

     /* XBZRLE overflow or normal page */
     if (pages == -1) {
-        *bytes_transferred += save_block_hdr(f, block, offset, cont,
+        *bytes_transferred += save_block_hdr(f, block, offset,
                                              RAM_SAVE_FLAG_PAGE);
         if (send_async) {
             qemu_put_buffer_async(f, p, TARGET_PAGE_SIZE);
@@ -720,7 +722,6 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,

             /* if page is unmodified, continue to the next */
             if (pages > 0) {
-                last_sent_block = block;
                 break;
             }
         }
-- 
2.1.0

[Qemu-devel] [PATCH 6/6] rename save_block_hdr to save_page_header

[Juan Quintela]

It has always been a page header, not a block header.  Once there, the
flag argument was only passed to make a bit or with it, just do the or
on the caller.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 arch_init.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 95a61e2..804b17d 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -315,8 +315,19 @@ static uint64_t migration_dirty_pages;
 static uint32_t last_version;
 static bool ram_bulk_stage;

-static size_t save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
-                             int flag)
+/**
+ * save_page_header: Write page header to wire
+ *
+ * If this is the 1st block, it also writes the block identification
+ *
+ * Returns: Number of bytes written
+ *
+ * @f: QEMUFile where to send the data
+ * @block: block that contains the page we want to send
+ * @offset: offset inside the block for the page
+ *          in the lower bits, it contains flags
+ */
+static size_t save_page_header(QEMUFile *f, RAMBlock *block, ram_addr_t offset)
 {
     size_t size;

@@ -324,7 +335,7 @@ static size_t save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t offset,
         offset |= RAM_SAVE_FLAG_CONTINUE;
     }

-    qemu_put_be64(f, offset | flag);
+    qemu_put_be64(f, offset);
     size = 8;

     if (block != last_sent_block) {
@@ -424,7 +435,7 @@ static int save_xbzrle_page(QEMUFile *f, uint8_t **current_data,
     }

     /* Send XBZRLE based compressed page */
-    bytes_xbzrle = save_block_hdr(f, block, offset, RAM_SAVE_FLAG_XBZRLE);
+    bytes_xbzrle = save_page_header(f, block, offset | RAM_SAVE_FLAG_XBZRLE);
     qemu_put_byte(f, ENCODING_FLAG_XBZRLE);
     qemu_put_be16(f, encoded_len);
     qemu_put_buffer(f, XBZRLE.encoded_buf, encoded_len);
@@ -639,8 +650,8 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
         }
     } else if (is_zero_range(p, TARGET_PAGE_SIZE)) {
         acct_info.dup_pages++;
-        *bytes_transferred += save_block_hdr(f, block, offset,
-                                             RAM_SAVE_FLAG_COMPRESS);
+        *bytes_transferred += save_page_header(f, block,
+                                               offset | RAM_SAVE_FLAG_COMPRESS);
         qemu_put_byte(f, 0);
         *bytes_transferred += 1;
         pages = 1;
@@ -661,8 +672,8 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,

     /* XBZRLE overflow or normal page */
     if (pages == -1) {
-        *bytes_transferred += save_block_hdr(f, block, offset,
-                                             RAM_SAVE_FLAG_PAGE);
+        *bytes_transferred += save_page_header(f, block,
+                                               offset | RAM_SAVE_FLAG_PAGE);
         if (send_async) {
             qemu_put_buffer_async(f, p, TARGET_PAGE_SIZE);
         } else {
-- 
2.1.0

Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Li, Liang Z]

> (Li special edition)
> 
> Current migration code returns number of bytes transferred and from there
> we decide if we.have sent something or not.  Problem, we need two results:
> number of pages written, and number of bytes written (depending on
> compression, zero pages, etc, it is not possible to derive one value from the
> other).
> 
> So, I changed all relevant function to return the number of written pages,
> and then pass as uint64_t *bytes_transferred to update the written bytes.
> 
> On current code, makes things a bit easier to understand, but is not strictely
> necesary.  But for the compression patches from Li, it makes a big difference,
> we can return that we have written a page, even if we have just started the
> write, but having writtten in reality zero bytes.
> 
> Once there, I add doxygen documentation to all function that I touched (yes,
> I was long due).
> 
> save_block_hdr really saved headers for pages, not blocks.  Rename it, and
> simplify the interface.
> 
> Li, does this make your life easier?  I hope so.  Should make really easy to
> remove the one_bytes_sent "hack", and allow my other suggestions.

It's great!  the 'one_byte_sent' always make me feel uncomfortable. Thanks, Juan.

> Comments?

The Chinese New Year coming, I am going to take leaves for about 2 weeks.
I am not sure if I can access Internet during the vacation, so maybe I can't give reply
for a long time.  But I will review your patches and change mine according to your changes
and suggestions.

Happy Chinese New Year! Now, I'm going to catch the train ...

Liang

Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Hi
> 
> (Li special edition)
> 
> Current migration code returns number of bytes transferred and from
> there we decide if we.have sent something or not.  Problem, we need
> two results: number of pages written, and number of bytes written
> (depending on compression, zero pages, etc, it is not possible to
> derive one value from the other).
> 
> So, I changed all relevant function to return the number of written
> pages, and then pass as uint64_t *bytes_transferred to update the
> written bytes.
> 
> On current code, makes things a bit easier to understand, but is not
> strictely necesary.  But for the compression patches from Li, it makes
> a big difference, we can return that we have written a page, even if
> we have just started the write, but having writtten in reality zero
> bytes.
> 
> Once there, I add doxygen documentation to all function that I touched
> (yes, I was long due).
> 
> save_block_hdr really saved headers for pages, not blocks.  Rename it,
> and simplify the interface.
> 
> Li, does this make your life easier?  I hope so.  Should make really
> easy to remove the one_bytes_sent "hack", and allow my other suggestions.
> 
> Comments?

I like it; it generally seems to make sense to separate the concept
of whether we've actually sent any pages from the actual byte counting.

While you're there though; do we actually care about bean counting
the individual header bytes?  For example the &bytes_transferred += 1 in the
RAM_SAVE_FLAG_COMPRESS case where it puts the 0, or the EOS mark where
we add 8 bytes?   Do we need to keep track of anything other than
stuff that's potentially big enough to make an impact on the
bandwidth calculations?

Dave

> 
> Later, Juan.
> 
> PD.  Code is obviously correct, but I run it through virt-test
>      anyways.  And it failed :-(  In despair, I tried virt-test on HEAD,
>      and it also failed, so ....  I add another thing to my ToDo list.
> 
> PD2.  Modifying ram_save_page() has become complicated, very
>       complicated. I think that know a couple of simplications, but
>       require touching RDMA code, and anyways, fixing migration in
>       "master" looks like a higher priority.
> 
> 
> Juan Quintela (6):
>   ram: make all save_page functions take a uint64_t parameter
>   ram_find_and_save_block:  change calling convention
>   ram_save_page: change calling covention
>   save_xbzrle_page: change calling convention
>   save_block_hdr: we can recalculate the cont parameter here
>   rename save_block_hdr to save_page_header
> 
>  arch_init.c                   | 176 ++++++++++++++++++++++++++----------------
>  include/migration/migration.h |   2 +-
>  include/migration/qemu-file.h |   2 +-
>  migration/qemu-file.c         |   3 +-
>  migration/rdma.c              |   2 +-
>  5 files changed, 114 insertions(+), 71 deletions(-)
> 
> -- 
> 2.1.0
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 1/6] ram: make all save_page functions take a uint64_t parameter

[Amit Shah]

On (Thu) 12 Feb 2015 [23:03:06], Juan Quintela wrote:
> It used to be an int, but then we can't pass directly the
> bytes_transferred parameter, that would happen later in the series.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c                   | 11 ++++++++---
>  include/migration/migration.h |  2 +-
>  include/migration/qemu-file.h |  2 +-
>  migration/qemu-file.c         |  3 ++-
>  migration/rdma.c              |  2 +-
>  5 files changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Amit Shah <amit.shah@redhat.com>

		Amit

Re: [Qemu-devel] [PATCH 2/6] ram_find_and_save_block: change calling convention

[Amit Shah]

On (Thu) 12 Feb 2015 [23:03:07], Juan Quintela wrote:
> Add a parameter to pass the number of bytes written, and make it return
> the number of pages written instead.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 44 ++++++++++++++++++++++----------------------
>  1 file changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 0066cf5..1133b26 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -650,14 +650,19 @@ static int ram_save_page(QEMUFile *f, RAMBlock* block, ram_addr_t offset,
>      return bytes_sent;
>  }
> 
> -/*
> - * ram_find_and_save_block: Finds a page to send and sends it to f
> +/**
> + * ram_find_and_save_block: Finds a dirty page and sends it to f
>   *
> - * Returns:  The number of bytes written.
> + * Returns:  The number of pages written
>   *           0 means no dirty pages
> + *
> + * @f: QEMUFile where to send the data
> + * @last_stage: if we are at the completion stage
> + * @bytes_transferred: increase it with the number of transferred bytes
>   */
> 
> -static int ram_find_and_save_block(QEMUFile *f, bool last_stage)
> +static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
> +                                   uint64_t *bytes_transferred)
>  {
>      RAMBlock *block = last_seen_block;
>      ram_addr_t offset = last_offset;
> @@ -696,7 +701,9 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage)
>      last_seen_block = block;
>      last_offset = offset;
> 
> -    return bytes_sent;
> +    *bytes_transferred += bytes_sent;
> +
> +    return (bytes_sent != 0);

Shouldn't page be counted here?

>  }
> 
>  static uint64_t bytes_transferred;
> @@ -869,7 +876,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      int ret;
>      int i;
>      int64_t t0;
> -    int total_sent = 0;
> +    int pages_sent = 0;
> 
>      qemu_mutex_lock_ramlist();
> 
> @@ -882,14 +889,14 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>      t0 = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>      i = 0;
>      while ((ret = qemu_file_rate_limit(f)) == 0) {
> -        int bytes_sent;
> +        int pages;

Let's make these unsigned right from the start; we're counting
positive numbers.

		Amit

Re: [Qemu-devel] [PATCH 4/6] save_xbzrle_page: change calling convention

[Amit Shah]

On (Thu) 12 Feb 2015 [23:03:09], Juan Quintela wrote:
> Add a parameter to pass the number of bytes written, and make it return
> the number of pages written instead.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index f243c6e..834f40c 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -352,11 +352,27 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
> 
>  #define ENCODING_FLAG_XBZRLE 0x1
> 
> +/**
> + * save_xbzrle_page: compress and send current page
> + *
> + * Returns: 1 means that we wrote the page
> + *          0 means that page is identical to the one already sent
> + *          -1 means that xbzrle would be longer than normal
> + *
> + * @f: QEMUFile where to send the data
> + * @current_data:
> + * @current_addr:
> + * @block: block that contains the page we want to send
> + * @offset: offset inside the block for the page
> + * @last_stage: if we are at the completion stage
> + * @bytes_transferred: increase it with the number of transferred bytes
> + */
>  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)
> +                            ram_addr_t offset, int cont, bool last_stage,
> +                            uint64_t *bytes_transferred)
>  {
> -    int encoded_len = 0, bytes_sent = -1;
> +    int encoded_len = 0, bytes_xbzrle;
>      uint8_t *prev_cached_page;
> 
>      if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) {

This patch exposes a bug in here:

    if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) {
        acct_info.xbzrle_cache_miss++;
        if (!last_stage) {
             if (cache_insert(XBZRLE.cache, current_addr, *current_data,
                              bitmap_sync_count) == -1) {
        	                     return -1;
             } else {
                 /* update *current_data when the page has been
                    inserted into cache */
                 *current_data = get_cached_data(XBZRLE.cache, current_addr);
             }
        }
        return -1;
    }

The return -1 should not be done for a successful insert of page in
cache.

Also, return -1 for both cache overflow and cache miss are perhaps not
relevant for the caller but the doctext needs to be updated.

		Amit

Re: [Qemu-devel] [PATCH 6/6] rename save_block_hdr to save_page_header

[Amit Shah]

On (Thu) 12 Feb 2015 [23:03:11], Juan Quintela wrote:
> It has always been a page header, not a block header.  Once there, the
> flag argument was only passed to make a bit or with it, just do the or
> on the caller.

I prefer not to do that -- if one looks at the function signature,
he'll only notice 'offset' there and not 'offset|flag', so it's
confusing to callers of function why the | is being done for an offset
field.

Let's keep that bit as-is?

		Amit

Re: [Qemu-devel] [PATCH 5/6] save_block_hdr: we can recalculate the cont parameter here

[Li, Liang Z]

> -----Original Message-----
> From: Juan Quintela [mailto:quintela@redhat.com]
> Sent: Friday, February 13, 2015 6:03 AM
> To: qemu-devel@nongnu.org
> Cc: Li, Liang Z
> Subject: [PATCH 5/6] save_block_hdr: we can recalculate the cont parameter
> here
> 
> No need to pass it through all the callers.  Once there, update
> last_sent_block here.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  arch_init.c | 47 ++++++++++++++++++++++++-----------------------
>  1 file changed, 24 insertions(+), 23 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 834f40c..95a61e2 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -304,34 +304,39 @@ uint64_t xbzrle_mig_pages_overflow(void)
>      return acct_info.xbzrle_overflows;
>  }
> 
> +/* This is the last block that we have visited serching for dirty pages
> +*/ static RAMBlock *last_seen_block;
> +/* This is the last block from where we have sent data */ static
> +RAMBlock *last_sent_block; static ram_addr_t last_offset; static
> +unsigned long *migration_bitmap; static uint64_t migration_dirty_pages;
> +static uint32_t last_version; static bool ram_bulk_stage;
> +
>  static size_t save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t
> offset,
> -                             int cont, int flag)
> +                             int flag)
>  {
>      size_t size;
> 
> -    qemu_put_be64(f, offset | cont | flag);
> +    if (block == last_sent_block) {
> +        offset |= RAM_SAVE_FLAG_CONTINUE;
> +    }
> +
> +    qemu_put_be64(f, offset | flag);
>      size = 8;
> 
> -    if (!cont) {
> +    if (block != last_sent_block) {
>          qemu_put_byte(f, strlen(block->idstr));
>          qemu_put_buffer(f, (uint8_t *)block->idstr,
>                          strlen(block->idstr));
>          size += 1 + strlen(block->idstr);
> +        last_sent_block = block;
>      }
>      return size;
>  }

If save_block_hdr function is changed to this, then in my patch, it can't not be directly reused in 
compression thread. A lock is needed to access last_sent_block. I prefer the original implementation.

Liang

Re: [Qemu-devel] [PATCH 6/6] rename save_block_hdr to save_page_header

[Li, Liang Z]

> On (Thu) 12 Feb 2015 [23:03:11], Juan Quintela wrote:
> > It has always been a page header, not a block header.  Once there, the
> > flag argument was only passed to make a bit or with it, just do the or
> > on the caller.
> 
> I prefer not to do that -- if one looks at the function signature, he'll only
> notice 'offset' there and not 'offset|flag', so it's confusing to callers of
> function why the | is being done for an offset field.
> 
> Let's keep that bit as-is?
> 

Same opinion.

Liang

Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Li, Liang Z]

> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> Sent: Tuesday, February 17, 2015 10:24 PM
> To: Juan Quintela
> Cc: qemu-devel@nongnu.org; Li, Liang Z
> Subject: Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between
> pages and bytes
> 
> * Juan Quintela (quintela@redhat.com) wrote:
> > Hi
> >
> > (Li special edition)
> >
> > Current migration code returns number of bytes transferred and from
> > there we decide if we.have sent something or not.  Problem, we need
> > two results: number of pages written, and number of bytes written
> > (depending on compression, zero pages, etc, it is not possible to
> > derive one value from the other).
> >
> > So, I changed all relevant function to return the number of written
> > pages, and then pass as uint64_t *bytes_transferred to update the
> > written bytes.
> >
> > On current code, makes things a bit easier to understand, but is not
> > strictely necesary.  But for the compression patches from Li, it makes
> > a big difference, we can return that we have written a page, even if
> > we have just started the write, but having writtten in reality zero
> > bytes.
> >
> > Once there, I add doxygen documentation to all function that I touched
> > (yes, I was long due).
> >
> > save_block_hdr really saved headers for pages, not blocks.  Rename it,
> > and simplify the interface.
> >
> > Li, does this make your life easier?  I hope so.  Should make really
> > easy to remove the one_bytes_sent "hack", and allow my other
> suggestions.
> >
> > Comments?
> 
> I like it; it generally seems to make sense to separate the concept of whether
> we've actually sent any pages from the actual byte counting.
> 
> While you're there though; do we actually care about bean counting the
> individual header bytes?  For example the &bytes_transferred += 1 in the
> RAM_SAVE_FLAG_COMPRESS case where it puts the 0, or the EOS mark
> where
> we add 8 bytes?   Do we need to keep track of anything other than
> stuff that's potentially big enough to make an impact on the bandwidth
> calculations?
> 

Can pos of QEMUFile be used to counting the transferred bytes ?

Liang

Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Alex Bennée]

Juan Quintela <quintela@redhat.com> writes:

> Hi
<snip>
>
> PD.  Code is obviously correct, but I run it through virt-test
>      anyways.  And it failed :-(  In despair, I tried virt-test on HEAD,
>      and it also failed, so ....  I add another thing to my ToDo list.

Are you intending to get a proper CI loop added for virt-test?

-- 
Alex Bennée

Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Dr. David Alan Gilbert]

* Li, Liang Z (liang.z.li@intel.com) wrote:
> 
> 
> > -----Original Message-----
> > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > Sent: Tuesday, February 17, 2015 10:24 PM
> > To: Juan Quintela
> > Cc: qemu-devel@nongnu.org; Li, Liang Z
> > Subject: Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between
> > pages and bytes
> > 
> > * Juan Quintela (quintela@redhat.com) wrote:
> > > Hi
> > >
> > > (Li special edition)
> > >
> > > Current migration code returns number of bytes transferred and from
> > > there we decide if we.have sent something or not.  Problem, we need
> > > two results: number of pages written, and number of bytes written
> > > (depending on compression, zero pages, etc, it is not possible to
> > > derive one value from the other).
> > >
> > > So, I changed all relevant function to return the number of written
> > > pages, and then pass as uint64_t *bytes_transferred to update the
> > > written bytes.
> > >
> > > On current code, makes things a bit easier to understand, but is not
> > > strictely necesary.  But for the compression patches from Li, it makes
> > > a big difference, we can return that we have written a page, even if
> > > we have just started the write, but having writtten in reality zero
> > > bytes.
> > >
> > > Once there, I add doxygen documentation to all function that I touched
> > > (yes, I was long due).
> > >
> > > save_block_hdr really saved headers for pages, not blocks.  Rename it,
> > > and simplify the interface.
> > >
> > > Li, does this make your life easier?  I hope so.  Should make really
> > > easy to remove the one_bytes_sent "hack", and allow my other
> > suggestions.
> > >
> > > Comments?
> > 
> > I like it; it generally seems to make sense to separate the concept of whether
> > we've actually sent any pages from the actual byte counting.
> > 
> > While you're there though; do we actually care about bean counting the
> > individual header bytes?  For example the &bytes_transferred += 1 in the
> > RAM_SAVE_FLAG_COMPRESS case where it puts the 0, or the EOS mark
> > where
> > we add 8 bytes?   Do we need to keep track of anything other than
> > stuff that's potentially big enough to make an impact on the bandwidth
> > calculations?
> > 
> 
> Can pos of QEMUFile be used to counting the transferred bytes ?

I think we probably can.

Dave

> Liang
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Li, Liang Z]

> Current migration code returns number of bytes transferred and from there
> we decide if we.have sent something or not.  Problem, we need two results:
> number of pages written, and number of bytes written (depending on
> compression, zero pages, etc, it is not possible to derive one value from the
> other).
> 
> So, I changed all relevant function to return the number of written pages,
> and then pass as uint64_t *bytes_transferred to update the written bytes.
> 
> On current code, makes things a bit easier to understand, but is not strictely
> necesary.  But for the compression patches from Li, it makes a big difference,
> we can return that we have written a page, even if we have just started the
> write, but having writtten in reality zero bytes.
> 
> Once there, I add doxygen documentation to all function that I touched (yes,
> I was long due).
> 
> save_block_hdr really saved headers for pages, not blocks.  Rename it, and
> simplify the interface.
> 
> Li, does this make your life easier?  I hope so.  Should make really easy to
> remove the one_bytes_sent "hack", and allow my other suggestions.
> 
> Comments?
> 
> Later, Juan.
> 
> PD.  Code is obviously correct, but I run it through virt-test
>      anyways.  And it failed :-(  In despair, I tried virt-test on HEAD,
>      and it also failed, so ....  I add another thing to my ToDo list.
> 
> PD2.  Modifying ram_save_page() has become complicated, very
>       complicated. I think that know a couple of simplications, but
>       require touching RDMA code, and anyways, fixing migration in
>       "master" looks like a higher priority.

Hi Juan, when could you make your patches be merged?  I have prepared my 
Version 6 patches,  and I am waiting for your patches being merged before I can
send out my patches.

Liang

Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Juan Quintela]

Alex Bennée <alex.bennee@linaro.org> wrote:
> Juan Quintela <quintela@redhat.com> writes:
>
>> Hi
> <snip>
>>
>> PD.  Code is obviously correct, but I run it through virt-test
>>      anyways.  And it failed :-(  In despair, I tried virt-test on HEAD,
>>      and it also failed, so ....  I add another thing to my ToDo list.
>
> Are you intending to get a proper CI loop added for virt-test?

CI loop?

Re: [Qemu-devel] [PATCH 0/6] migration: differentiate between pages and bytes

[Juan Quintela]

"Li, Liang Z" <liang.z.li@intel.com> wrote:
>> Current migration code returns number of bytes transferred and from there
>> we decide if we.have sent something or not.  Problem, we need two results:
>> number of pages written, and number of bytes written (depending on
>> compression, zero pages, etc, it is not possible to derive one value from the
>> other).
>> 
>> So, I changed all relevant function to return the number of written pages,
>> and then pass as uint64_t *bytes_transferred to update the written bytes.
>> 
>> On current code, makes things a bit easier to understand, but is not strictely
>> necesary.  But for the compression patches from Li, it makes a big difference,
>> we can return that we have written a page, even if we have just started the
>> write, but having writtten in reality zero bytes.
>> 
>> Once there, I add doxygen documentation to all function that I touched (yes,
>> I was long due).
>> 
>> save_block_hdr really saved headers for pages, not blocks.  Rename it, and
>> simplify the interface.
>> 
>> Li, does this make your life easier?  I hope so.  Should make really easy to
>> remove the one_bytes_sent "hack", and allow my other suggestions.
>> 
>> Comments?
>> 
>> Later, Juan.
>> 
>> PD.  Code is obviously correct, but I run it through virt-test
>>      anyways.  And it failed :-(  In despair, I tried virt-test on HEAD,
>>      and it also failed, so ....  I add another thing to my ToDo list.
>> 
>> PD2.  Modifying ram_save_page() has become complicated, very
>>       complicated. I think that know a couple of simplications, but
>>       require touching RDMA code, and anyways, fixing migration in
>>       "master" looks like a higher priority.
>
> Hi Juan, when could you make your patches be merged?  I have prepared my 
> Version 6 patches,  and I am waiting for your patches being merged before I can
> send out my patches.

Sorry, I plan to merge them between Today and Tomorrow.

Sorry, Juan.

Re: [Qemu-devel] [PATCH 5/6] save_block_hdr: we can recalculate the cont parameter here

[Juan Quintela]

"Li, Liang Z" <liang.z.li@intel.com> wrote:
>> -----Original Message-----
>> From: Juan Quintela [mailto:quintela@redhat.com]
>> Sent: Friday, February 13, 2015 6:03 AM
>> To: qemu-devel@nongnu.org
>> Cc: Li, Liang Z
>> Subject: [PATCH 5/6] save_block_hdr: we can recalculate the cont parameter
>> here
>> 
>> No need to pass it through all the callers.  Once there, update
>> last_sent_block here.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  arch_init.c | 47 ++++++++++++++++++++++++-----------------------
>>  1 file changed, 24 insertions(+), 23 deletions(-)
>> 
>> diff --git a/arch_init.c b/arch_init.c
>> index 834f40c..95a61e2 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -304,34 +304,39 @@ uint64_t xbzrle_mig_pages_overflow(void)
>>      return acct_info.xbzrle_overflows;
>>  }
>> 
>> +/* This is the last block that we have visited serching for dirty pages
>> +*/ static RAMBlock *last_seen_block;
>> +/* This is the last block from where we have sent data */ static
>> +RAMBlock *last_sent_block; static ram_addr_t last_offset; static
>> +unsigned long *migration_bitmap; static uint64_t migration_dirty_pages;
>> +static uint32_t last_version; static bool ram_bulk_stage;
>> +
>>  static size_t save_block_hdr(QEMUFile *f, RAMBlock *block, ram_addr_t
>> offset,
>> -                             int cont, int flag)
>> +                             int flag)
>>  {
>>      size_t size;
>> 
>> -    qemu_put_be64(f, offset | cont | flag);
>> +    if (block == last_sent_block) {
>> +        offset |= RAM_SAVE_FLAG_CONTINUE;
>> +    }
>> +
>> +    qemu_put_be64(f, offset | flag);
>>      size = 8;
>> 
>> -    if (!cont) {
>> +    if (block != last_sent_block) {
>>          qemu_put_byte(f, strlen(block->idstr));
>>          qemu_put_buffer(f, (uint8_t *)block->idstr,
>>                          strlen(block->idstr));
>>          size += 1 + strlen(block->idstr);
>> +        last_sent_block = block;
>>      }
>>      return size;
>>  }
>
> If save_block_hdr function is changed to this, then in my patch, it
> can't not be directly reused in
> compression thread. A lock is needed to access last_sent_block. I
> prefer the original implementation.

ah

ok, will redo this then.

Thanks for the feedback.

Later, Juan.

Re: [Qemu-devel] [PATCH 6/6] rename save_block_hdr to save_page_header

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> On (Thu) 12 Feb 2015 [23:03:11], Juan Quintela wrote:
>> It has always been a page header, not a block header.  Once there, the
>> flag argument was only passed to make a bit or with it, just do the or
>> on the caller.
>
> I prefer not to do that -- if one looks at the function signature,
> he'll only notice 'offset' there and not 'offset|flag', so it's
> confusing to callers of function why the | is being done for an offset
> field.

Problem with that is offset, at this point is an opaque from the
callee.  The only thing that they can do with it is just to add another
valid flag.  We can rename it if you want, but that don't change the
fact that it is not useable there.

Basically we have RAMBlocks, where we have an offset inside that
RAMBlock.  We get to ram_save_page() and convert that offset into a
pointer in host ram with memory_region_get_ram_ptr().  From there on,
offset is something that can't be used anymore as offset, is just opaque
to be able to reproduce it on the target machine.

id? token?  anyother better name?

Later, Juan.


>
> Let's keep that bit as-is?
>
> 		Amit

Re: [Qemu-devel] [PATCH 4/6] save_xbzrle_page: change calling convention

[Juan Quintela]

Amit Shah <amit.shah@redhat.com> wrote:
> On (Thu) 12 Feb 2015 [23:03:09], Juan Quintela wrote:
>> Add a parameter to pass the number of bytes written, and make it return
>> the number of pages written instead.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  arch_init.c | 44 +++++++++++++++++++++++++-------------------
>>  1 file changed, 25 insertions(+), 19 deletions(-)
>> 
>> diff --git a/arch_init.c b/arch_init.c
>> index f243c6e..834f40c 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -352,11 +352,27 @@ static void xbzrle_cache_zero_page(ram_addr_t current_addr)
>> 
>>  #define ENCODING_FLAG_XBZRLE 0x1
>> 
>> +/**
>> + * save_xbzrle_page: compress and send current page
>> + *
>> + * Returns: 1 means that we wrote the page
>> + *          0 means that page is identical to the one already sent
>> + *          -1 means that xbzrle would be longer than normal
>> + *
>> + * @f: QEMUFile where to send the data
>> + * @current_data:
>> + * @current_addr:
>> + * @block: block that contains the page we want to send
>> + * @offset: offset inside the block for the page
>> + * @last_stage: if we are at the completion stage
>> + * @bytes_transferred: increase it with the number of transferred bytes
>> + */
>>  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)
>> +                            ram_addr_t offset, int cont, bool last_stage,
>> +                            uint64_t *bytes_transferred)
>>  {
>> -    int encoded_len = 0, bytes_sent = -1;
>> +    int encoded_len = 0, bytes_xbzrle;
>>      uint8_t *prev_cached_page;
>> 
>>      if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) {
>
> This patch exposes a bug in here:
>
>     if (!cache_is_cached(XBZRLE.cache, current_addr, bitmap_sync_count)) {
>         acct_info.xbzrle_cache_miss++;
>         if (!last_stage) {
>              if (cache_insert(XBZRLE.cache, current_addr, *current_data,
>                               bitmap_sync_count) == -1) {
>         	                     return -1;
>              } else {
>                  /* update *current_data when the page has been
>                     inserted into cache */
>                  *current_data = get_cached_data(XBZRLE.cache, current_addr);
>              }
>         }
>         return -1;
>     }
>
> The return -1 should not be done for a successful insert of page in
> cache.

Why?
Return -1 means:
>> + *          -1 means that xbzrle would be longer than normal

in the case that page was not on the cache, we just sent the page as
normal page, that is what we want.



>
> Also, return -1 for both cache overflow and cache miss are perhaps not
> relevant for the caller but the doctext needs to be updated.

-1 means: send the full page, please.
I am not sure that caller want to know if it is because the cache was
full or because the page was not in cache, or because the page was not
"comprhensible".

Later, Juan.