[Qemu-devel] [PATCH 0/18 RFC v3] Localhost migration

[Qemu-devel] [PATCH 0/18 RFC v3] Localhost migration

[Lei Li]

Hi,

This patch series tries to add a new approach to localhost migration 
support in QEMU. 

When doing localhost migration, the host memory will balloon up 
during the period. This procedure might need to consume twice the 
memory needed to run the guest for some time. So we want to add 
a new live migration mechanism for localhost migration. 

Following I copied from last version that Anthony added for the 
benefit of the other reviewers:

The goal here is to allow "live upgrade" of a running QEMU instance.  
The work flow would look like this:

1) Guests are running QEMU release 1.6.1
2) Admin installs QEMU release 1.6.2 via RPM or deb
3) Admin does localhost migration with page flipping to use new version
   of QEMU.

Page flipping is used in order to avoid requiring that there is enough
free memory to fit an additional copy of the largest guest which is the
requirement today with localhost migration.

You can also read from the link below:
http://lists.gnu.org/archive/html/qemu-devel/2013-06/msg02577.html

The plan is:

1) Add new command to do localhost migration.

   The qmp interface introduced like:

   { 'command': 'localhost-migrate', 'data': {'uri': 'str'} }

2) Use different mechanism than current live migration.

   The very basic work flow like:

       qemu on the source (the source and destination are both on localhost)
              |
              V
           Stop VM
              |
              V   
          Create threads
              |
              V
       Page flipping through vmsplice
              |
              V
       MADV_DONTNEED the ram pages which are already flipped
              |
              V
       Migration completes

   As stopping VM first, we expect/resume the page flipping through vmsplice
   is fast enough to meet *live migration (low downtime).

Notes:
Currently the work flow is not exactly the same as description
above. For the first step, the work flow we implemented is:
stop VM and copy ram pages via unix domain socket, MADV_DONTNEED 
ram pages that already copied. After that, will replace to vmsplice
mechanism instead of copying pages.

Known issues:
Now it has not been completely tested, and there is one problem
on exporting MemoryRegion and RAMBlock into migration-local.c, as 
the code of overriding of rdma hooks like save_page was moved there. 
So still mark it as RFC.

To avoid wasting time and make sure it's the way should be headed, 
send it out earlier to have your suggestions.

Your suggestions and comments would be very appreciated!


Changes since v1&v2:
  - Interface changes including:
      Convert a new qmp command to a capability suggested by Anthony
      and Paolo.
      Create a new URI prefix 'local' instead of a new command-line 
      switch suggested by Michael R. Hines.

  - Integrate to migration_thread from Michael R. Hines.
  - Override rdma hooks suggested by Paolo and Michael R.  

TODO:
- Introduce a mechanism to exchange a PIPE via SCM_RIGHTS.
- benchmark/evaluation.

Lei Li (18):
  migration: export MIG_STATE_xxx flags 
  savevm: export qemu_save_device_state()
  rename is_active to is_block_active
  savevm: set right return value for qemu_file_rate_limit
  savevm: add comments for qemu_file_get_error()
  rdma: bugfix for ram_control_load_hook
  arch_init: export RAM_SAVE_xxx flags
  migration-local: introduce qemu_fopen_local()
  exec: export qemu_get_ram_block()
  migration-local: implementation of outgoing part
  migration: introduce capability localhost
  arch_init: factor out ram_save_blocks()
  arch_init: adjust ram_save_setup() for migrate_is_localhost
  arch_init: skip migration_bitmap_sync for local migration
  migration: adjust migration_thread() for local migration
  migration-local: implementation of incoming part
  migration: add prefix for local migration to incoming migration
  hmp: better fomat for info migrate_capabilities

 Makefile.objs                 |    1 +
 arch_init.c                   |   62 ++++---
 block-migration.c             |    2 +-
 exec.c                        |    7 +-
 hmp.c                         |    5 +-
 include/exec/cpu-all.h        |    1 +
 include/migration/migration.h |   37 ++++
 include/migration/qemu-file.h |   10 +-
 include/migration/vmstate.h   |    2 +-
 include/sysemu/sysemu.h       |    1 +
 migration-local.c             |  397 +++++++++++++++++++++++++++++++++++++++++
 migration.c                   |  107 ++++++----
 qapi-schema.json              |    8 +-
 savevm.c                      |   47 ++++--
 14 files changed, 688 insertions(+), 89 deletions(-)
 create mode 100644 migration-local.c

[Qemu-devel] [PATCH 01/18] migration: export MIG_STATE_xxx flags

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 include/migration/migration.h |   10 ++++++++++
 migration.c                   |    9 ---------
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 140e6b4..6a24e65 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -28,6 +28,16 @@ struct MigrationParams {
     bool shared;
 };
 
+/* Migration status */
+enum {
+    MIG_STATE_ERROR = -1,    /* Migration failed */
+    MIG_STATE_NONE,          /* There has been an error */
+    MIG_STATE_SETUP,         /* Setup stage for a migration */
+    MIG_STATE_CANCELLED,     /* Migration has been cancelled */
+    MIG_STATE_ACTIVE,        /* Whether a migration is active */
+    MIG_STATE_COMPLETED,     /* Migration is successfully completed */
+};
+
 typedef struct MigrationState MigrationState;
 
 struct MigrationState
diff --git a/migration.c b/migration.c
index 1402fa7..2446a6a 100644
--- a/migration.c
+++ b/migration.c
@@ -35,15 +35,6 @@
     do { } while (0)
 #endif
 
-enum {
-    MIG_STATE_ERROR = -1,
-    MIG_STATE_NONE,
-    MIG_STATE_SETUP,
-    MIG_STATE_CANCELLED,
-    MIG_STATE_ACTIVE,
-    MIG_STATE_COMPLETED,
-};
-
 #define MAX_THROTTLE  (32 << 20)      /* Migration speed throttling */
 
 /* Amount of time to allocate to each "chunk" of bandwidth-throttled
-- 
1.7.7.6

[Qemu-devel] [PATCH 02/18] savevm: export qemu_save_device_state()

[Lei Li]

Export qemu_save_device_state() for localhost migration.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 include/sysemu/sysemu.h |    1 +
 savevm.c                |    7 ++++++-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index d7a77b6..f1f15f4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -80,6 +80,7 @@ int qemu_savevm_state_iterate(QEMUFile *f);
 void qemu_savevm_state_complete(QEMUFile *f);
 void qemu_savevm_state_cancel(void);
 uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
+int qemu_save_device_state(QEMUFile *f);
 int qemu_loadvm_state(QEMUFile *f);
 
 /* SLIRP */
diff --git a/savevm.c b/savevm.c
index 03fc4d9..f89e979 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2049,7 +2049,12 @@ static int qemu_savevm_state(QEMUFile *f)
     return ret;
 }
 
-static int qemu_save_device_state(QEMUFile *f)
+/**
+ * Save all of the device states to stream QEMUFile
+ *
+ * Return negative if there has been an error
+ **/
+int qemu_save_device_state(QEMUFile *f)
 {
     SaveStateEntry *se;
 
-- 
1.7.7.6

[Qemu-devel] [PATCH 03/18] rename is_active to is_block_active

[Lei Li]

is_active is used to identify block migration, rename to is_block_active
to make it more clear.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 block-migration.c           |    2 +-
 include/migration/vmstate.h |    2 +-
 savevm.c                    |   16 ++++++++--------
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index f803f20..ce84377 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -834,7 +834,7 @@ SaveVMHandlers savevm_block_handlers = {
     .save_live_pending = block_save_pending,
     .load_state = block_load,
     .cancel = block_migration_cancel,
-    .is_active = block_is_active,
+    .is_block_active = block_is_active,
 };
 
 void blk_mig_init(void)
diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
index 1c31b5d..de47326 100644
--- a/include/migration/vmstate.h
+++ b/include/migration/vmstate.h
@@ -42,7 +42,7 @@ typedef struct SaveVMHandlers {
     int (*save_live_complete)(QEMUFile *f, void *opaque);
 
     /* This runs both outside and inside the iothread lock.  */
-    bool (*is_active)(void *opaque);
+    bool (*is_block_active)(void *opaque);
 
     /* This runs outside the iothread lock in the migration case, and
      * within the lock in the savevm case.  The callback had better only
diff --git a/savevm.c b/savevm.c
index f89e979..68552a7 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1860,8 +1860,8 @@ void qemu_savevm_state_begin(QEMUFile *f,
         if (!se->ops || !se->ops->save_live_setup) {
             continue;
         }
-        if (se->ops && se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
+        if (se->ops && se->ops->is_block_active) {
+            if (!se->ops->is_block_active(se->opaque)) {
                 continue;
             }
         }
@@ -1900,8 +1900,8 @@ int qemu_savevm_state_iterate(QEMUFile *f)
         if (!se->ops || !se->ops->save_live_iterate) {
             continue;
         }
-        if (se->ops && se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
+        if (se->ops && se->ops->is_block_active) {
+            if (!se->ops->is_block_active(se->opaque)) {
                 continue;
             }
         }
@@ -1941,8 +1941,8 @@ void qemu_savevm_state_complete(QEMUFile *f)
         if (!se->ops || !se->ops->save_live_complete) {
             continue;
         }
-        if (se->ops && se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
+        if (se->ops && se->ops->is_block_active) {
+            if (!se->ops->is_block_active(se->opaque)) {
                 continue;
             }
         }
@@ -1995,8 +1995,8 @@ uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size)
         if (!se->ops || !se->ops->save_live_pending) {
             continue;
         }
-        if (se->ops && se->ops->is_active) {
-            if (!se->ops->is_active(se->opaque)) {
+        if (se->ops && se->ops->is_block_active) {
+            if (!se->ops->is_block_active(se->opaque)) {
                 continue;
             }
         }
-- 
1.7.7.6

[Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Lei Li]

Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate
limiting to QEMUFile, but set the return value for qemu_file_rate_limit
to 1 in the case of qemu_file_get_error. It is wrong and should be negative
compared to the original function buffered_rate_limit and the current logic
in ram_save_iterate. As qemu_file_rate_limit is called manually to determine
if it has to exit, add the definition of the meaning of the return values
as well.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 savevm.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/savevm.c b/savevm.c
index 68552a7..6362275 100644
--- a/savevm.c
+++ b/savevm.c
@@ -904,10 +904,20 @@ int64_t qemu_ftell(QEMUFile *f)
     return f->pos;
 }
 
+/*
+ * The meaning of the return values is:
+ *   0: We can continue sending
+ *   1: Time to stop
+ *   negative: There has been an error
+ */
+
 int qemu_file_rate_limit(QEMUFile *f)
 {
-    if (qemu_file_get_error(f)) {
-        return 1;
+    int ret;
+
+    ret = qemu_file_get_error(f);
+    if (ret) {
+        return ret;
     }
     if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) {
         return 1;
-- 
1.7.7.6

[Qemu-devel] [PATCH 05/18] savevm: add comments for qemu_file_get_error()

[Lei Li]

Add comments for qemu_file_get_error(), as its return value
is not very clear.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 savevm.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/savevm.c b/savevm.c
index 6362275..1522d95 100644
--- a/savevm.c
+++ b/savevm.c
@@ -566,6 +566,13 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
     return f;
 }
 
+/*
+ * Get last error for stream f
+ *
+ * Return negtive error value if there has been an error on previous
+ * operations, return 0 no error happened.
+ *
+ */
 int qemu_file_get_error(QEMUFile *f)
 {
     return f->last_error;
-- 
1.7.7.6

[Qemu-devel] [PATCH 06/18] bugfix: wrong error set by ram_control_load_hook()

[Lei Li]

It should set negative error value if there has been an error.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 savevm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/savevm.c b/savevm.c
index 1522d95..f10e031 100644
--- a/savevm.c
+++ b/savevm.c
@@ -649,7 +649,7 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
 
 void ram_control_load_hook(QEMUFile *f, uint64_t flags)
 {
-    int ret = 0;
+    int ret = -1;
 
     if (f->ops->hook_ram_load) {
         ret = f->ops->hook_ram_load(f, f->opaque, flags);
-- 
1.7.7.6

[Qemu-devel] [PATCH 07/18] arch_init: export RAM_SAVE_xxx flags

[Lei Li]

Export RAM_SAVE_xxx flags for localhost migration.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 arch_init.c                   |   12 ------------
 include/migration/migration.h |   14 ++++++++++++++
 2 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 68a7ab7..1ea7c29 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -108,18 +108,6 @@ static bool mig_throttle_on;
 static int dirty_rate_high_cnt;
 static void check_guest_throttling(void);
 
-/***********************************************************/
-/* ram save/restore */
-
-#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
-#define RAM_SAVE_FLAG_COMPRESS 0x02
-#define RAM_SAVE_FLAG_MEM_SIZE 0x04
-#define RAM_SAVE_FLAG_PAGE     0x08
-#define RAM_SAVE_FLAG_EOS      0x10
-#define RAM_SAVE_FLAG_CONTINUE 0x20
-#define RAM_SAVE_FLAG_XBZRLE   0x40
-/* 0x80 is reserved in migration.h start with 0x100 next */
-
 
 static struct defconfig_file {
     const char *filename;
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 6a24e65..5336117 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -158,12 +158,26 @@ void ram_control_before_iterate(QEMUFile *f, uint64_t flags);
 void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
 void ram_control_load_hook(QEMUFile *f, uint64_t flags);
 
+
+/***********************************************************/
+/* ram save/restore */
+
+#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
+#define RAM_SAVE_FLAG_COMPRESS 0x02
+#define RAM_SAVE_FLAG_MEM_SIZE 0x04
+#define RAM_SAVE_FLAG_PAGE     0x08
+#define RAM_SAVE_FLAG_EOS      0x10
+#define RAM_SAVE_FLAG_CONTINUE 0x20
+#define RAM_SAVE_FLAG_XBZRLE   0x40
+
 /* Whenever this is found in the data stream, the flags
  * will be passed to ram_control_load_hook in the incoming-migration
  * side. This lets before_ram_iterate/after_ram_iterate add
  * transport-specific sections to the RAM migration data.
  */
 #define RAM_SAVE_FLAG_HOOK     0x80
+/* Start with 0x100 next */
+
 
 #define RAM_SAVE_CONTROL_NOT_SUPP -1000
 #define RAM_SAVE_CONTROL_DELAYED  -2000
-- 
1.7.7.6

[Qemu-devel] [PATCH 08/18] migration-local: introduce qemu_fopen_local()

[Lei Li]

Introduce read/write backend of QEMUFileLocal used by localhost
migration. The unix domain socket will be replaced by PIPE with
vmsplice mechanism.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 Makefile.objs     |    1 +
 migration-local.c |  211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 212 insertions(+), 0 deletions(-)
 create mode 100644 migration-local.c

diff --git a/Makefile.objs b/Makefile.objs
index f46a4cd..30670cc 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -54,6 +54,7 @@ common-obj-y += migration.o migration-tcp.o
 common-obj-$(CONFIG_RDMA) += migration-rdma.o
 common-obj-y += qemu-char.o #aio.o
 common-obj-y += block-migration.o
+common-obj-y += migration-local.o
 common-obj-y += page_cache.o xbzrle.o
 
 common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
diff --git a/migration-local.c b/migration-local.c
new file mode 100644
index 0000000..93190fd
--- /dev/null
+++ b/migration-local.c
@@ -0,0 +1,211 @@
+/*
+ * QEMU localhost migration
+ *
+ * Copyright IBM, Corp. 2013
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later.
+ * 
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "config-host.h"
+#include "qemu-common.h"
+#include "migration/migration.h"
+#include "exec/cpu-common.h"
+#include "config.h"
+#include "exec/cpu-all.h"
+#include "monitor/monitor.h"
+#include "migration/qemu-file.h"
+#include "qemu/iov.h"
+#include "sysemu/arch_init.h"
+#include "sysemu/sysemu.h"
+#include "block/block.h"
+#include "qemu/sockets.h"
+#include "migration/block.h"
+#include "qemu/thread.h"
+#include "qmp-commands.h"
+#include "trace.h"
+#include "qemu/osdep.h"
+
+//#define DEBUG_MIGRATION_LOCAL
+
+#ifdef DEBUG_MIGRATION_LOCAL
+#define DPRINTF(fmt, ...) \
+    do { printf("migration-local: " fmt, ## __VA_ARGS__); } while (0)
+#else
+#define DPRINTF(fmt, ...) \
+    do { } while (0)
+#endif
+
+/*
+ * Interface for the local migration.
+ */
+typedef struct QEMUFileLocal {
+    QEMUFile *file;
+    int fd;
+    int state;
+
+    /*
+     * This is the last block from where we have sent data
+     * for local migration
+     */
+    RAMBlock *last_block_sent;
+} QEMUFileLocal;
+
+
+static int qemu_local_get_buffer(void *opaque, uint8_t *buf,
+                                 int64_t pos, int size)
+{
+    QEMUFileLocal *s = opaque;
+    ssize_t len;
+
+    for (;;) {
+        len = qemu_recv(s->fd, buf, size, 0);
+        if (len != -1) {
+            break;
+        }
+        if (socket_error() == EAGAIN) {
+            yield_until_fd_readable(s->fd);
+        } else if (socket_error() != EINTR) {
+            break;
+        }
+    }
+
+    if (len == -1) {
+        len = -socket_error();
+    }
+    return len;
+}
+
+static int qemu_local_get_fd(void *opaque)
+{
+    QEMUFileLocal *s = opaque;
+
+    return s->fd;
+}
+
+static int qemu_local_close(void *opaque)
+{
+    QEMUFileLocal *s = opaque;
+
+    closesocket(s->fd);
+    g_free(s);
+
+    return 0;
+}
+
+static size_t qemu_local_put_buffer(void *opaque, struct iovec *iov,
+                                    int iovcnt, int64_t pos)
+{
+    QEMUFileLocal *s = opaque;
+    ssize_t len;
+    ssize_t size = iov_size(iov, iovcnt);
+
+    len = iov_send(s->fd, iov, iovcnt, 0, size);
+    if (len < size) {
+        len = -socket_error();
+    }
+
+    return len;
+}
+
+static size_t local_save_page(QEMUFile *f, RAMBlock *block,
+                              ram_addr_t offset, int flags)
+{
+    MemoryRegion *mr = block->mr;
+    uint8_t *p;
+
+    p = memory_region_get_ram_ptr(mr) + offset;
+
+    if (buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE)) {
+        qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_COMPRESS);
+        if (!flags) {
+            qemu_put_byte(f, strlen(block->idstr));
+            qemu_put_buffer(f, (uint8_t *)block->idstr,
+                            strlen(block->idstr));
+        }
+        qemu_put_byte(f, *p);
+        return 0;
+    }
+ 
+    qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_PAGE);
+    if (!flags) {
+        qemu_put_byte(f, strlen(block->idstr));
+        qemu_put_buffer(f, (uint8_t *)block->idstr,
+                        strlen(block->idstr));
+    }
+    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
+
+    return TARGET_PAGE_SIZE;
+}
+
+static size_t qemu_local_ram_save(QEMUFile *f, void *opaque,
+                                  ram_addr_t block_offset, ram_addr_t offset,
+                                  size_t size, int *bytes_sent)
+{
+    QEMUFileLocal *s = opaque;
+    uint64_t current_addr = block_offset + offset;
+    RAMBlock *block = qemu_get_ram_block(current_addr);
+    MemoryRegion *mr = block->mr;
+    void *ram;
+    int ret;
+    int cont;
+
+    ret = qemu_file_get_error(f);
+    if (ret < 0) {
+        return ret;
+    }
+
+    qemu_fflush(f);
+
+    cont = (block == s->last_block_sent) ? RAM_SAVE_FLAG_CONTINUE : 0;
+
+    ram = memory_region_get_ram_ptr(mr) + offset;
+    s->last_block_sent = block;
+  
+    *bytes_sent = local_save_page(f, block, offset, cont);
+    if (!bytes_sent || *bytes_sent < 0) {
+        return RAM_SAVE_CONTROL_DELAYED;
+    }
+
+    /* DONTNEED the RAM page that has already been copied. */
+    qemu_madvise(ram, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
+
+    return 0;
+}
+
+const QEMUFileOps local_read_ops = {
+    .get_fd     = qemu_local_get_fd,
+    .get_buffer = qemu_local_get_buffer,
+    .close      = qemu_local_close,
+};
+
+const QEMUFileOps local_write_ops = {
+    .get_fd             = qemu_local_get_fd,
+    .writev_buffer      = qemu_local_put_buffer,
+    .close              = qemu_local_close,
+    .save_page          = qemu_local_ram_save,
+};
+
+static void *qemu_fopen_local(int fd, const char *mode)
+{
+    QEMUFileLocal *s;
+
+    if (qemu_file_mode_is_not_valid(mode)) {
+        return NULL;
+    }
+
+    s = g_malloc0(sizeof(QEMUFileLocal));
+    s->fd = fd;
+
+    if (mode[0] == 'w') {
+        qemu_set_block(s->fd);
+        s->file = qemu_fopen_ops(s, &local_write_ops);
+    } else {
+        s->file = qemu_fopen_ops(s, &local_read_ops);
+    }
+
+    return s->file;
+}
-- 
1.7.7.6

[Qemu-devel] [PATCH 09/18] exec: export qemu_get_ram_block()

[Lei Li]

Export qemu_get_ram_block() for localhost migration to
get the RAMBlock of given ram address.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 exec.c                    |    7 ++++++-
 include/exec/cpu-common.h |    1 +
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/exec.c b/exec.c
index 3ca9381..45b054d 100644
--- a/exec.c
+++ b/exec.c
@@ -1303,7 +1303,12 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length)
 }
 #endif /* !_WIN32 */
 
-static RAMBlock *qemu_get_ram_block(ram_addr_t addr)
+/**
+ * Return the RAMBlock of the given ram offset
+ *
+ * abort if RAMBlock not found
+ **/
+RAMBlock *qemu_get_ram_block(ram_addr_t addr)
 {
     RAMBlock *block;
 
diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
index e4996e1..267fcec 100644
--- a/include/exec/cpu-common.h
+++ b/include/exec/cpu-common.h
@@ -54,6 +54,7 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
 /* This should not be used by devices.  */
 MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
 void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev);
+RAMBlock *qemu_get_ram_block(ram_addr_t addr);
 
 void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf,
                             int len, int is_write);
-- 
1.7.7.6

[Qemu-devel] [PATCH 10/18] migration-local: implementation of outgoing part

[Lei Li]

Implementation of outgoing part for localhost migration.
The integration of migration thread and corresponding
adjustment will be in coming patches.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 include/migration/migration.h |    2 +
 migration-local.c             |   85 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index 5336117..d2c7eff 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -92,6 +92,8 @@ void rdma_start_outgoing_migration(void *opaque, const char *host_port, Error **
 
 void rdma_start_incoming_migration(const char *host_port, Error **errp);
 
+void local_start_outgoing_migration(void *opaque, const char *uri, Error **errp);
+
 void migrate_fd_error(MigrationState *s);
 
 void migrate_fd_connect(MigrationState *s);
diff --git a/migration-local.c b/migration-local.c
index 93190fd..cf4a091 100644
--- a/migration-local.c
+++ b/migration-local.c
@@ -209,3 +209,88 @@ static void *qemu_fopen_local(int fd, const char *mode)
 
     return s->file;
 }
+
+/************************************************************************
+ * Outgoing part
+ **/
+
+static QEMUFileLocal *local_migration_init(void)
+{
+    QEMUFileLocal *s = g_malloc0(sizeof(*s));
+
+    s->state = MIG_STATE_SETUP;
+    trace_migrate_set_state(MIG_STATE_SETUP);
+    s->fd = -1;
+    s->last_block_sent = NULL;
+
+    return s;
+}
+
+static void unix_local_wait_for_connect(int fd, void *opaque)
+{
+    MigrationState *s = opaque;
+
+    if (fd < 0) {
+        DPRINTF("migrate connect error\n");
+        s->file = NULL;
+        migrate_fd_error(s);
+    } else {
+        DPRINTF("migrate connect success\n");
+        s->file = qemu_fopen_local(fd, "wb");
+        migrate_fd_connect(s);
+    }
+}
+
+static void unix_local_outgoing_connect(MigrationState *s, const char *path,
+                                        Error **errp)
+{
+    unix_nonblocking_connect(path, unix_local_wait_for_connect, s, errp);
+}
+
+void local_start_outgoing_migration(void *opaque, const char *uri,
+                                    Error **errp)
+{
+    MigrationState *s = opaque;
+    const char *path;
+    QEMUFileLocal *local;
+    Error *local_err = NULL;
+    int is_vm_running;
+    int ret;
+
+    local = local_migration_init();
+    if (local == NULL) {
+        error_setg(errp, "Failed to initialize\n");
+    }
+
+    bdrv_flush_all();
+
+    is_vm_running = runstate_is_running();
+
+    /* Stop the VM first */
+    if (is_vm_running) {
+        ret = vm_stop(RUN_STATE_SAVE_VM);
+        if (ret < 0) {
+            goto fail;
+        }
+    }
+
+    /* Start outgoing migration via unix socket. */
+    if (uri) {
+        /* XXX. Creation of a new unix_start_outgoing_migration_* is
+         * not necessary, just for the first step. This will be replaced
+         * by vmsplice mechanism.
+         **/
+        unix_local_outgoing_connect(s, path, &local_err);
+    } else {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "uri", "a valid migration protocol");
+        goto fail;
+    }
+
+    return;
+
+fail:
+    error_propagate(errp, local_err);
+    g_free(local);
+    migrate_fd_error(s);
+}
-- 
1.7.7.6

[Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Lei Li]

Introduce migration capability localhost.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 include/migration/migration.h |    3 +++
 migration.c                   |   12 ++++++++++++
 qapi-schema.json              |    8 +++++++-
 3 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index d2c7eff..a04f050 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -143,10 +143,13 @@ void migrate_add_blocker(Error *reason);
 void migrate_del_blocker(Error *reason);
 
 bool migrate_rdma_pin_all(void);
+
 bool migrate_zero_blocks(void);
 
 bool migrate_auto_converge(void);
 
+bool migrate_is_localhost(void);
+
 int xbzrle_encode_buffer(uint8_t *old_buf, uint8_t *new_buf, int slen,
                          uint8_t *dst, int dlen);
 int xbzrle_decode_buffer(uint8_t *src, int slen, uint8_t *dst, int dlen);
diff --git a/migration.c b/migration.c
index 2446a6a..2f85358 100644
--- a/migration.c
+++ b/migration.c
@@ -420,6 +420,9 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else if (strstart(uri, "exec:", &p)) {
         exec_start_outgoing_migration(s, p, &local_err);
     } else if (strstart(uri, "unix:", &p)) {
+        if (s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST]) {
+            local_start_outgoing_migration(s, p, &local_err);
+        }
         unix_start_outgoing_migration(s, p, &local_err);
     } else if (strstart(uri, "fd:", &p)) {
         fd_start_outgoing_migration(s, p, &local_err);
@@ -521,6 +524,15 @@ int migrate_use_xbzrle(void)
     return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
 }
 
+bool migrate_is_localhost(void)
+{
+    MigrationState *s;
+
+    s = migrate_get_current();
+
+    return s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST];
+}
+
 int64_t migrate_xbzrle_cache_size(void)
 {
     MigrationState *s;
diff --git a/qapi-schema.json b/qapi-schema.json
index a51f7d2..557337f 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -629,10 +629,16 @@
 # @auto-converge: If enabled, QEMU will automatically throttle down the guest
 #          to speed up convergence of RAM migration. (since 1.6)
 #
+# @localhost: If enabled, QEMU will support localhost migration. This feature
+#             allows live upgrade of a running QEMU instance by doing localhost
+#             migration with page flipping. It requires the source and destination
+#             are both on localhost. Disabled by default. (since 1.7)
+#
 # Since: 1.2
 ##
 { 'enum': 'MigrationCapability',
-  'data': ['xbzrle', 'x-rdma-pin-all', 'auto-converge', 'zero-blocks'] }
+  'data': ['xbzrle', 'x-rdma-pin-all', 'auto-converge', 'zero-blocks',
+           'localhost'] }
 
 ##
 # @MigrationCapabilityStatus
-- 
1.7.7.6

[Qemu-devel] [PATCH 12/18] arch_init: factor out ram_save_blocks()

[Lei Li]

Factor out ram_save_blocks(), which will be used to send ram
blocks hooked by save_page in ram_save_setup stage in coming
patch.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 arch_init.c |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 1ea7c29..434a4ca 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -534,6 +534,20 @@ void acct_update_position(QEMUFile *f, size_t size, bool zero)
     }
 }
 
+static void ram_save_blocks(QEMUFile *f)
+{
+    while (true) {
+        int bytes_sent;
+
+        bytes_sent = ram_save_block(f, true);
+        /* no more blocks to sent */
+        if (bytes_sent == 0) {
+            break;
+        }
+        bytes_transferred += bytes_sent;
+    }
+}
+
 static ram_addr_t ram_save_remaining(void)
 {
     return migration_dirty_pages;
@@ -719,16 +733,7 @@ static int ram_save_complete(QEMUFile *f, void *opaque)
     /* try transferring iterative blocks of memory */
 
     /* flush all remaining blocks regardless of rate limiting */
-    while (true) {
-        int bytes_sent;
-
-        bytes_sent = ram_save_block(f, true);
-        /* no more blocks to sent */
-        if (bytes_sent == 0) {
-            break;
-        }
-        bytes_transferred += bytes_sent;
-    }
+    ram_save_blocks(f);
 
     ram_control_after_iterate(f, RAM_CONTROL_FINISH);
     migration_end();
-- 
1.7.7.6

[Qemu-devel] [PATCH 13/18] arch_init: adjust ram_save_setup() for migrate_is_localhost

[Lei Li]

Send all the ram blocks hooked by save_page, which will copy
ram page and MADV_DONTNEED the page just copied.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 arch_init.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 434a4ca..cbbb4db 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -474,7 +474,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
             /* In doubt sent page as normal */
             bytes_sent = -1;
             ret = ram_control_save_page(f, block->offset,
-                               offset, TARGET_PAGE_SIZE, &bytes_sent);
+                                        offset, TARGET_PAGE_SIZE, &bytes_sent);
 
             if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
                 if (ret != RAM_SAVE_CONTROL_DELAYED) {
@@ -613,11 +613,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     RAMBlock *block;
     int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
 
-    migration_bitmap = bitmap_new(ram_pages);
-    bitmap_set(migration_bitmap, 0, ram_pages);
-    migration_dirty_pages = ram_pages;
-    mig_throttle_on = false;
-    dirty_rate_high_cnt = 0;
+    if (!migrate_is_localhost()) {
+        migration_bitmap = bitmap_new(ram_pages);
+        bitmap_set(migration_bitmap, 0, ram_pages);
+        migration_dirty_pages = ram_pages;
+        mig_throttle_on = false;
+        dirty_rate_high_cnt = 0;
+    }
 
     if (migrate_use_xbzrle()) {
         XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
@@ -641,6 +643,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     migration_bitmap_sync();
     qemu_mutex_unlock_iothread();
 
+    if (migrate_is_localhost()) {
+        ram_save_blocks(f);
+        return 0;
+    }
+
     qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
 
     QTAILQ_FOREACH(block, &ram_list.blocks, next) {
-- 
1.7.7.6

[Qemu-devel] [PATCH 14/18] arch_init: skip migration_bitmap_sync for local migration

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 arch_init.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index cbbb4db..e6069eb 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -375,6 +375,11 @@ static void migration_bitmap_sync(void)
     int64_t end_time;
     int64_t bytes_xfer_now;
 
+    /* No need to sync with bitmap for local migration */
+    if (migrate_is_localhost()) {
+        return;
+    }
+
     if (!bytes_xfer_prev) {
         bytes_xfer_prev = ram_bytes_transferred();
     }
-- 
1.7.7.6

[Qemu-devel] [PATCH 15/18] migration: adjust migration_thread for local migration

[Lei Li]

Integrate localhost migration thread to migration_thread. As the mechanism
of local migration is different than the current migration, and it does
not need to do iterate.

So the whole thread can split into two stages, it will send all the ram
pages in qemu_savevm_state_begin stage, and send the device states in
qemu_save_device_state stage.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 migration.c |   73 ++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 45 insertions(+), 28 deletions(-)

diff --git a/migration.c b/migration.c
index 2f85358..2471664 100644
--- a/migration.c
+++ b/migration.c
@@ -565,37 +565,54 @@ static void *migration_thread(void *opaque)
     while (s->state == MIG_STATE_ACTIVE) {
         int64_t current_time;
         uint64_t pending_size;
+        int ret;
 
-        if (!qemu_file_rate_limit(s->file)) {
-            DPRINTF("iterate\n");
-            pending_size = qemu_savevm_state_pending(s->file, max_size);
-            DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
-            if (pending_size && pending_size >= max_size) {
-                qemu_savevm_state_iterate(s->file);
-            } else {
-                int ret;
-
-                DPRINTF("done iterating\n");
-                qemu_mutex_lock_iothread();
-                start_time = qemu_get_clock_ms(rt_clock);
-                qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
-                old_vm_running = runstate_is_running();
-
-                ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
-                if (ret >= 0) {
-                    qemu_file_set_rate_limit(s->file, INT_MAX);
-                    qemu_savevm_state_complete(s->file);
-                }
-                qemu_mutex_unlock_iothread();
+        if (s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST]) {
+            DPRINTF("local migration start\n");
 
-                if (ret < 0) {
-                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
-                    break;
-                }
+            ret = qemu_save_device_state(s->file);
+            if (ret < 0) {
+                migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
+                break;
+            }
 
-                if (!qemu_file_get_error(s->file)) {
-                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETED);
-                    break;
+            if (!qemu_file_get_error(s->file)) {
+                DPRINTF("local migration completed\n");
+                migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETED);
+                break;
+            }
+        } else {
+            if (!qemu_file_rate_limit(s->file)) {
+                DPRINTF("iterate\n");
+                pending_size = qemu_savevm_state_pending(s->file, max_size);
+                DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
+                if (pending_size && pending_size >= max_size) {
+                    qemu_savevm_state_iterate(s->file);
+                } else {
+                    int ret;
+
+                    DPRINTF("done iterating\n");
+                    qemu_mutex_lock_iothread();
+                    start_time = qemu_get_clock_ms(rt_clock);
+                    qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
+                    old_vm_running = runstate_is_running();
+
+                    ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+                    if (ret >= 0) {
+                        qemu_file_set_rate_limit(s->file, INT_MAX);
+                        qemu_savevm_state_complete(s->file);
+                    }
+                    qemu_mutex_unlock_iothread();
+
+                    if (ret < 0) {
+                        migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
+                        break;
+                    }
+
+                    if (!qemu_file_get_error(s->file)) {
+                        migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETED);
+                        break;
+                    }
                 }
             }
         }
-- 
1.7.7.6

[Qemu-devel] [PATCH 16/18] migration-local: implementation of incoming part

[Lei Li]

Implementation of incoming part of localhost migration.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 include/migration/migration.h |    2 +
 include/migration/qemu-file.h |    8 ++--
 migration-local.c             |   99 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 4 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index a04f050..232f1d5 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -94,6 +94,8 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp);
 
 void local_start_outgoing_migration(void *opaque, const char *uri, Error **errp);
 
+void local_start_incoming_migration(const char *uri, Error **errp);
+
 void migrate_fd_error(MigrationState *s);
 
 void migrate_fd_connect(MigrationState *s);
diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 0f757fb..a34d418 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -77,10 +77,10 @@ typedef int (QEMURamHookFunc)(QEMUFile *f, void *opaque, uint64_t flags);
  * is saved (such as RDMA, for example.)
  */
 typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
-                               ram_addr_t block_offset,
-                               ram_addr_t offset,
-                               size_t size,
-                               int *bytes_sent);
+                                 ram_addr_t block_offset,
+                                 ram_addr_t offset,
+                                 size_t size,
+                                 int *bytes_sent);
 
 typedef struct QEMUFileOps {
     QEMUFilePutBufferFunc *put_buffer;
diff --git a/migration-local.c b/migration-local.c
index cf4a091..501371a 100644
--- a/migration-local.c
+++ b/migration-local.c
@@ -294,3 +294,102 @@ fail:
     g_free(local);
     migrate_fd_error(s);
 }
+
+
+/**********************************************************************
+ * Incoming part
+ */
+
+static void unix_accept_local_incoming(void *opaque)
+{
+    struct sockaddr_un addr;
+    socklen_t addrlen = sizeof(addr);
+    int s = (intptr_t)opaque;
+    QEMUFile *f;
+    int c;
+
+    do {
+        c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
+    } while (c == -1 && errno == EINTR);
+
+    qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
+    close(s);
+
+    DPRINTF("accepted migration\n");
+
+    if (c == -1) {
+        fprintf(stderr, "could not accept migration connection\n");
+        goto out;
+    }
+
+    f = qemu_fopen_local(c, "rb");
+    if (f == NULL) {
+        fprintf(stderr, "could not qemu_fopen socket\n");
+        goto out;
+    }
+
+    start_local_incoming_migration(f);
+
+    return;
+
+out:
+    close(c);
+}
+
+static void unix_local_incoming_connect(const char *path, Error **errp)
+{
+    int ret;
+
+    ret = unix_listen(path, NULL, 0, errp);
+    if (ret < 0) {
+        return;
+    }
+
+    qemu_set_fd_handler2(ret, NULL, unix_accept_local_incoming, NULL,
+                         (void *)(intptr_t)ret);
+}
+void local_start_incoming_migration(const char *uri, Error **errp)
+{
+    QEMUFileLocal *s = g_malloc0(sizeof(*s));
+    Error *local_err = NULL;
+
+    DPRINTF("Starting local incoming migration\n");
+
+    if (uri) {
+        unix_local_incoming_connect(uri, errp);
+    } else {
+        goto err;
+    }
+
+    DPRINTF("unix_listen success\n");
+
+    return;
+
+err:
+    error_propagate(errp, local_err);
+    g_free(s);
+}
+
+static void start_local_incoming_migration(QEMUFile *f)
+{
+    int ret;
+
+    ret = qemu_loadvm_state(f);
+    if (ret < 0) {
+        fprintf(stderr, "load of migration failed\n");
+        exit(EXIT_FAILURE);
+    }
+    qemu_announce_self();
+
+    DPRINTF("successfully loaded vm state\n");
+
+    bdrv_clear_incoming_migration_all();
+    /* Make sure all file formats flush their mutable metadata */
+    bdrv_invalidate_cache_all();
+
+    if (autostart) {
+        vm_start();
+    } else {
+        runstate_set(RUN_STATE_PAUSED);
+    }
+}
-- 
1.7.7.6

[Qemu-devel] [PATCH 16/18] migration-local: implementation of incoming part

[Lei Li]

Implementation of incoming part of localhost migration.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 include/migration/migration.h |    2 +
 include/migration/qemu-file.h |    8 ++--
 migration-local.c             |   99 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 4 deletions(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index a04f050..232f1d5 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -94,6 +94,8 @@ void rdma_start_incoming_migration(const char *host_port, Error **errp);
 
 void local_start_outgoing_migration(void *opaque, const char *uri, Error **errp);
 
+void local_start_incoming_migration(const char *uri, Error **errp);
+
 void migrate_fd_error(MigrationState *s);
 
 void migrate_fd_connect(MigrationState *s);
diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 0f757fb..a34d418 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -77,10 +77,10 @@ typedef int (QEMURamHookFunc)(QEMUFile *f, void *opaque, uint64_t flags);
  * is saved (such as RDMA, for example.)
  */
 typedef size_t (QEMURamSaveFunc)(QEMUFile *f, void *opaque,
-                               ram_addr_t block_offset,
-                               ram_addr_t offset,
-                               size_t size,
-                               int *bytes_sent);
+                                 ram_addr_t block_offset,
+                                 ram_addr_t offset,
+                                 size_t size,
+                                 int *bytes_sent);
 
 typedef struct QEMUFileOps {
     QEMUFilePutBufferFunc *put_buffer;
diff --git a/migration-local.c b/migration-local.c
index cf4a091..501371a 100644
--- a/migration-local.c
+++ b/migration-local.c
@@ -294,3 +294,102 @@ fail:
     g_free(local);
     migrate_fd_error(s);
 }
+
+
+/**********************************************************************
+ * Incoming part
+ */
+
+static void unix_accept_local_incoming(void *opaque)
+{
+    struct sockaddr_un addr;
+    socklen_t addrlen = sizeof(addr);
+    int s = (intptr_t)opaque;
+    QEMUFile *f;
+    int c;
+
+    do {
+        c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
+    } while (c == -1 && errno == EINTR);
+
+    qemu_set_fd_handler2(s, NULL, NULL, NULL, NULL);
+    close(s);
+
+    DPRINTF("accepted migration\n");
+
+    if (c == -1) {
+        fprintf(stderr, "could not accept migration connection\n");
+        goto out;
+    }
+
+    f = qemu_fopen_local(c, "rb");
+    if (f == NULL) {
+        fprintf(stderr, "could not qemu_fopen socket\n");
+        goto out;
+    }
+
+    start_local_incoming_migration(f);
+
+    return;
+
+out:
+    close(c);
+}
+
+static void unix_local_incoming_connect(const char *path, Error **errp)
+{
+    int ret;
+
+    ret = unix_listen(path, NULL, 0, errp);
+    if (ret < 0) {
+        return;
+    }
+
+    qemu_set_fd_handler2(ret, NULL, unix_accept_local_incoming, NULL,
+                         (void *)(intptr_t)ret);
+}
+void local_start_incoming_migration(const char *uri, Error **errp)
+{
+    QEMUFileLocal *s = g_malloc0(sizeof(*s));
+    Error *local_err = NULL;
+
+    DPRINTF("Starting local incoming migration\n");
+
+    if (uri) {
+        unix_local_incoming_connect(uri, errp);
+    } else {
+        goto err;
+    }
+
+    DPRINTF("unix_listen success\n");
+
+    return;
+
+err:
+    error_propagate(errp, local_err);
+    g_free(s);
+}
+
+static void start_local_incoming_migration(QEMUFile *f)
+{
+    int ret;
+
+    ret = qemu_loadvm_state(f);
+    if (ret < 0) {
+        fprintf(stderr, "load of migration failed\n");
+        exit(EXIT_FAILURE);
+    }
+    qemu_announce_self();
+
+    DPRINTF("successfully loaded vm state\n");
+
+    bdrv_clear_incoming_migration_all();
+    /* Make sure all file formats flush their mutable metadata */
+    bdrv_invalidate_cache_all();
+
+    if (autostart) {
+        vm_start();
+    } else {
+        runstate_set(RUN_STATE_PAUSED);
+    }
+}
-- 
1.7.7.6

[Qemu-devel] [PATCH 17/18] migration: add prefix for local migration to incoming migration

[Lei Li]

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 migration.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/migration.c b/migration.c
index 2471664..17cf2fd 100644
--- a/migration.c
+++ b/migration.c
@@ -81,6 +81,9 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
         unix_start_incoming_migration(p, errp);
     else if (strstart(uri, "fd:", &p))
         fd_start_incoming_migration(p, errp);
+    else if (strstart(uri, "local:", &p))
+        local_start_incoming_migration(p, errp);
+
 #endif
     else {
         error_setg(errp, "unknown migration protocol: %s", uri);
-- 
1.7.7.6

[Qemu-devel] [PATCH 18/18] hmp: better fomat for info migrate_capabilities

[Lei Li]

As we have more and more capabilities, better to display
in lines.

Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
---
 hmp.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index c45514b..3544219 100644
--- a/hmp.c
+++ b/hmp.c
@@ -226,13 +226,12 @@ void hmp_info_migrate_capabilities(Monitor *mon, const QDict *qdict)
     caps = qmp_query_migrate_capabilities(NULL);
 
     if (caps) {
-        monitor_printf(mon, "capabilities: ");
+        monitor_printf(mon, "Capabilities:\n");
         for (cap = caps; cap; cap = cap->next) {
-            monitor_printf(mon, "%s: %s ",
+            monitor_printf(mon, "%s: %s\n",
                            MigrationCapability_lookup[cap->value->capability],
                            cap->value->state ? "on" : "off");
         }
-        monitor_printf(mon, "\n");
     }
 
     qapi_free_MigrationCapabilityStatusList(caps);
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 06/18] bugfix: wrong error set by ram_control_load_hook()

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> It should set negative error value if there has been an error.
> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  savevm.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 1522d95..f10e031 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -649,7 +649,7 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
>  
>  void ram_control_load_hook(QEMUFile *f, uint64_t flags)
>  {
> -    int ret = 0;
> +    int ret = -1;

-EINVAL

otherwise looks good thanks!

Paolo

>      if (f->ops->hook_ram_load) {
>          ret = f->ops->hook_ram_load(f, f->opaque, flags);
> 

Re: [Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate
> limiting to QEMUFile, but set the return value for qemu_file_rate_limit
> to 1 in the case of qemu_file_get_error. It is wrong and should be negative
> compared to the original function buffered_rate_limit and the current logic
> in ram_save_iterate. As qemu_file_rate_limit is called manually to determine
> if it has to exit, add the definition of the meaning of the return values
> as well.

When there is an error it returns "time to stop" so that ultimately we
get to the migration_thread function and check qemu_file_get_error there.

Why doesn't this work for you?

Paolo

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  savevm.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 68552a7..6362275 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -904,10 +904,20 @@ int64_t qemu_ftell(QEMUFile *f)
>      return f->pos;
>  }
>  
> +/*
> + * The meaning of the return values is:
> + *   0: We can continue sending
> + *   1: Time to stop
> + *   negative: There has been an error
> + */
> +
>  int qemu_file_rate_limit(QEMUFile *f)
>  {
> -    if (qemu_file_get_error(f)) {
> -        return 1;
> +    int ret;
> +
> +    ret = qemu_file_get_error(f);
> +    if (ret) {
> +        return ret;
>      }
>      if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) {
>          return 1;
> 

Re: [Qemu-devel] [PATCH 05/18] savevm: add comments for qemu_file_get_error()

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Add comments for qemu_file_get_error(), as its return value
> is not very clear.
> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  savevm.c |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 6362275..1522d95 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -566,6 +566,13 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
>      return f;
>  }
>  
> +/*
> + * Get last error for stream f
> + *
> + * Return negtive error value if there has been an error on previous
> + * operations, return 0 no error happened.
> + *
> + */
>  int qemu_file_get_error(QEMUFile *f)
>  {
>      return f->last_error;
> 

Looks good,

Paolo

Re: [Qemu-devel] [PATCH 10/18] migration-local: implementation of outgoing part

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Implementation of outgoing part for localhost migration.
> The integration of migration thread and corresponding
> adjustment will be in coming patches.
> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  include/migration/migration.h |    2 +
>  migration-local.c             |   85 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 87 insertions(+), 0 deletions(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 5336117..d2c7eff 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -92,6 +92,8 @@ void rdma_start_outgoing_migration(void *opaque, const char *host_port, Error **
>  
>  void rdma_start_incoming_migration(const char *host_port, Error **errp);
>  
> +void local_start_outgoing_migration(void *opaque, const char *uri, Error **errp);
> +
>  void migrate_fd_error(MigrationState *s);
>  
>  void migrate_fd_connect(MigrationState *s);
> diff --git a/migration-local.c b/migration-local.c
> index 93190fd..cf4a091 100644
> --- a/migration-local.c
> +++ b/migration-local.c
> @@ -209,3 +209,88 @@ static void *qemu_fopen_local(int fd, const char *mode)
>  
>      return s->file;
>  }
> +
> +/************************************************************************
> + * Outgoing part
> + **/
> +
> +static QEMUFileLocal *local_migration_init(void)
> +{
> +    QEMUFileLocal *s = g_malloc0(sizeof(*s));
> +
> +    s->state = MIG_STATE_SETUP;
> +    trace_migrate_set_state(MIG_STATE_SETUP);
> +    s->fd = -1;
> +    s->last_block_sent = NULL;
> +
> +    return s;
> +}

It's not clear why this is needed, it looks like a violation of
encapsulation.

Paolo

Re: [Qemu-devel] [PATCH 15/18] migration: adjust migration_thread for local migration

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Integrate localhost migration thread to migration_thread. As the mechanism
> of local migration is different than the current migration, and it does
> not need to do iterate.
> 
> So the whole thread can split into two stages, it will send all the ram
> pages in qemu_savevm_state_begin stage, and send the device states in
> qemu_save_device_state stage.

This is not needed.  Skipping the iteration should happen automatically
if you stop the VM before invoking migration.

Paolo

> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  migration.c |   73 ++++++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 45 insertions(+), 28 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 2f85358..2471664 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -565,37 +565,54 @@ static void *migration_thread(void *opaque)
>      while (s->state == MIG_STATE_ACTIVE) {
>          int64_t current_time;
>          uint64_t pending_size;
> +        int ret;
>  
> -        if (!qemu_file_rate_limit(s->file)) {
> -            DPRINTF("iterate\n");
> -            pending_size = qemu_savevm_state_pending(s->file, max_size);
> -            DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
> -            if (pending_size && pending_size >= max_size) {
> -                qemu_savevm_state_iterate(s->file);
> -            } else {
> -                int ret;
> -
> -                DPRINTF("done iterating\n");
> -                qemu_mutex_lock_iothread();
> -                start_time = qemu_get_clock_ms(rt_clock);
> -                qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> -                old_vm_running = runstate_is_running();
> -
> -                ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> -                if (ret >= 0) {
> -                    qemu_file_set_rate_limit(s->file, INT_MAX);
> -                    qemu_savevm_state_complete(s->file);
> -                }
> -                qemu_mutex_unlock_iothread();
> +        if (s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST]) {
> +            DPRINTF("local migration start\n");
>  
> -                if (ret < 0) {
> -                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
> -                    break;
> -                }
> +            ret = qemu_save_device_state(s->file);
> +            if (ret < 0) {
> +                migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
> +                break;
> +            }
>  
> -                if (!qemu_file_get_error(s->file)) {
> -                    migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETED);
> -                    break;
> +            if (!qemu_file_get_error(s->file)) {
> +                DPRINTF("local migration completed\n");
> +                migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETED);
> +                break;
> +            }
> +        } else {
> +            if (!qemu_file_rate_limit(s->file)) {
> +                DPRINTF("iterate\n");
> +                pending_size = qemu_savevm_state_pending(s->file, max_size);
> +                DPRINTF("pending size %lu max %lu\n", pending_size, max_size);
> +                if (pending_size && pending_size >= max_size) {
> +                    qemu_savevm_state_iterate(s->file);
> +                } else {
> +                    int ret;
> +
> +                    DPRINTF("done iterating\n");
> +                    qemu_mutex_lock_iothread();
> +                    start_time = qemu_get_clock_ms(rt_clock);
> +                    qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER);
> +                    old_vm_running = runstate_is_running();
> +
> +                    ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
> +                    if (ret >= 0) {
> +                        qemu_file_set_rate_limit(s->file, INT_MAX);
> +                        qemu_savevm_state_complete(s->file);
> +                    }
> +                    qemu_mutex_unlock_iothread();
> +
> +                    if (ret < 0) {
> +                        migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_ERROR);
> +                        break;
> +                    }
> +
> +                    if (!qemu_file_get_error(s->file)) {
> +                        migrate_set_state(s, MIG_STATE_ACTIVE, MIG_STATE_COMPLETED);
> +                        break;
> +                    }
>                  }
>              }
>          }
> 

Re: [Qemu-devel] [PATCH 13/18] arch_init: adjust ram_save_setup() for migrate_is_localhost

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Send all the ram blocks hooked by save_page, which will copy
> ram page and MADV_DONTNEED the page just copied.

You should implement this entirely in the hook.

It will be a little less efficient because of the dirty bitmap overhead,
but you should aim at having *zero* changes in arch_init.c and migration.c.

Paolo

> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  arch_init.c |   19 +++++++++++++------
>  1 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 434a4ca..cbbb4db 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -474,7 +474,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>              /* In doubt sent page as normal */
>              bytes_sent = -1;
>              ret = ram_control_save_page(f, block->offset,
> -                               offset, TARGET_PAGE_SIZE, &bytes_sent);
> +                                        offset, TARGET_PAGE_SIZE, &bytes_sent);
>  
>              if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
>                  if (ret != RAM_SAVE_CONTROL_DELAYED) {
> @@ -613,11 +613,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      RAMBlock *block;
>      int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
>  
> -    migration_bitmap = bitmap_new(ram_pages);
> -    bitmap_set(migration_bitmap, 0, ram_pages);
> -    migration_dirty_pages = ram_pages;
> -    mig_throttle_on = false;
> -    dirty_rate_high_cnt = 0;
> +    if (!migrate_is_localhost()) {
> +        migration_bitmap = bitmap_new(ram_pages);
> +        bitmap_set(migration_bitmap, 0, ram_pages);
> +        migration_dirty_pages = ram_pages;
> +        mig_throttle_on = false;
> +        dirty_rate_high_cnt = 0;
> +    }
>  
>      if (migrate_use_xbzrle()) {
>          XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
> @@ -641,6 +643,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>      migration_bitmap_sync();
>      qemu_mutex_unlock_iothread();
>  
> +    if (migrate_is_localhost()) {
> +        ram_save_blocks(f);
> +        return 0;
> +    }
> +
>      qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
>  
>      QTAILQ_FOREACH(block, &ram_list.blocks, next) {
> 

Re: [Qemu-devel] [PATCH 07/18] arch_init: export RAM_SAVE_xxx flags

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Export RAM_SAVE_xxx flags for localhost migration.
> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  arch_init.c                   |   12 ------------
>  include/migration/migration.h |   14 ++++++++++++++
>  2 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 68a7ab7..1ea7c29 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -108,18 +108,6 @@ static bool mig_throttle_on;
>  static int dirty_rate_high_cnt;
>  static void check_guest_throttling(void);
>  
> -/***********************************************************/
> -/* ram save/restore */
> -
> -#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
> -#define RAM_SAVE_FLAG_COMPRESS 0x02
> -#define RAM_SAVE_FLAG_MEM_SIZE 0x04
> -#define RAM_SAVE_FLAG_PAGE     0x08
> -#define RAM_SAVE_FLAG_EOS      0x10
> -#define RAM_SAVE_FLAG_CONTINUE 0x20
> -#define RAM_SAVE_FLAG_XBZRLE   0x40
> -/* 0x80 is reserved in migration.h start with 0x100 next */
> -
>  
>  static struct defconfig_file {
>      const char *filename;
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 6a24e65..5336117 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -158,12 +158,26 @@ void ram_control_before_iterate(QEMUFile *f, uint64_t flags);
>  void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
>  void ram_control_load_hook(QEMUFile *f, uint64_t flags);
>  
> +
> +/***********************************************************/
> +/* ram save/restore */
> +
> +#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
> +#define RAM_SAVE_FLAG_COMPRESS 0x02
> +#define RAM_SAVE_FLAG_MEM_SIZE 0x04
> +#define RAM_SAVE_FLAG_PAGE     0x08
> +#define RAM_SAVE_FLAG_EOS      0x10
> +#define RAM_SAVE_FLAG_CONTINUE 0x20
> +#define RAM_SAVE_FLAG_XBZRLE   0x40
> +
>  /* Whenever this is found in the data stream, the flags
>   * will be passed to ram_control_load_hook in the incoming-migration
>   * side. This lets before_ram_iterate/after_ram_iterate add
>   * transport-specific sections to the RAM migration data.
>   */
>  #define RAM_SAVE_FLAG_HOOK     0x80
> +/* Start with 0x100 next */
> +
>  
>  #define RAM_SAVE_CONTROL_NOT_SUPP -1000
>  #define RAM_SAVE_CONTROL_DELAYED  -2000
> 

This also looks like an encapsulation violation.

Localhost migration is not very different in concept from RDMA (except
that it runs with the VM stopped, but that's just because you do
MADV_DONTNEED---it's not specific to the migration transport), and it
manages to do everything without touching arch_init.c and migration.c.

Paolo

Re: [Qemu-devel] [PATCH 14/18] arch_init: skip migration_bitmap_sync for local migration

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  arch_init.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index cbbb4db..e6069eb 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -375,6 +375,11 @@ static void migration_bitmap_sync(void)
>      int64_t end_time;
>      int64_t bytes_xfer_now;
>  
> +    /* No need to sync with bitmap for local migration */
> +    if (migrate_is_localhost()) {
> +        return;
> +    }
> +
>      if (!bytes_xfer_prev) {
>          bytes_xfer_prev = ram_bytes_transferred();
>      }
> 

Premature, please do this after everything is in place (including
vmsplice) and only if you find the dirty bitmap to be a significant
source of overhead.

Paolo

Re: [Qemu-devel] [PATCH 17/18] migration: add prefix for local migration to incoming migration

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  migration.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/migration.c b/migration.c
> index 2471664..17cf2fd 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -81,6 +81,9 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>          unix_start_incoming_migration(p, errp);
>      else if (strstart(uri, "fd:", &p))
>          fd_start_incoming_migration(p, errp);
> +    else if (strstart(uri, "local:", &p))
> +        local_start_incoming_migration(p, errp);
> +
>  #endif
>      else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
> 

This should not be needed.  The destination can just use the normal
unix: protocol.  The load hook will only be called if the source uses
localhost migration, and it will get the pipe via SCM_RIGHTS then
process the pipe.

Paolo

Re: [Qemu-devel] [PATCH 02/18] savevm: export qemu_save_device_state()

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
> Export qemu_save_device_state() for localhost migration.
> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  include/sysemu/sysemu.h |    1 +
>  savevm.c                |    7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index d7a77b6..f1f15f4 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -80,6 +80,7 @@ int qemu_savevm_state_iterate(QEMUFile *f);
>  void qemu_savevm_state_complete(QEMUFile *f);
>  void qemu_savevm_state_cancel(void);
>  uint64_t qemu_savevm_state_pending(QEMUFile *f, uint64_t max_size);
> +int qemu_save_device_state(QEMUFile *f);
>  int qemu_loadvm_state(QEMUFile *f);
>  
>  /* SLIRP */
> diff --git a/savevm.c b/savevm.c
> index 03fc4d9..f89e979 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2049,7 +2049,12 @@ static int qemu_savevm_state(QEMUFile *f)
>      return ret;
>  }
>  
> -static int qemu_save_device_state(QEMUFile *f)
> +/**
> + * Save all of the device states to stream QEMUFile
> + *
> + * Return negative if there has been an error
> + **/
> +int qemu_save_device_state(QEMUFile *f)
>  {
>      SaveStateEntry *se;
>  
> 

This should also not be needed.  Device state will be sent on the Unix
socket, while page data will be sent on the pipe.

I suggest you start from scratch.  Do a simple RFC series that does
normal live migration with dirty bitmap, but uses a side channel (pipe
passed via SCM_RIGHTS) for RAM.  This will ensure you get the hooks right.

Then you have two things to work on: (1) make the capability enforce
non-live migration, probably by adding another RunState and adding it to
runstate_needs_reset; (2) improve the hooks to use vmsplice.

Paolo

Re: [Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Eric Blake]

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

On 08/21/2013 01:18 AM, Lei Li wrote:
> Introduce migration capability localhost.
> 
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>  include/migration/migration.h |    3 +++
>  migration.c                   |   12 ++++++++++++
>  qapi-schema.json              |    8 +++++++-
>  3 files changed, 22 insertions(+), 1 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -629,10 +629,16 @@
>  # @auto-converge: If enabled, QEMU will automatically throttle down the guest
>  #          to speed up convergence of RAM migration. (since 1.6)
>  #
> +# @localhost: If enabled, QEMU will support localhost migration. This feature
> +#             allows live upgrade of a running QEMU instance by doing localhost
> +#             migration with page flipping. It requires the source and destination
> +#             are both on localhost. Disabled by default. (since 1.7)

Logically, ALL new migration parameters should be disabled by default,
as that provides the sanest back-compat setting.  Your last sentence is
therefore redundant.  But this is certainly a much nicer implementation
than your v1 attempt.

If I understand correctly, attempts to use 'local:...' are rejected
unless you first use migrate-set-capabilities to turn on localhost
migration support.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Paolo Bonzini]

Il 21/08/2013 09:18, Lei Li ha scritto:
>      } else if (strstart(uri, "unix:", &p)) {
> +        if (s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST]) {
> +            local_start_outgoing_migration(s, p, &local_err);
> +        }
>          unix_start_outgoing_migration(s, p, &local_err);
>      } else if (strstart(uri, "fd:", &p)) {
>          fd_start_outgoing_migration(s, p, &local_err);
> @@ -521,6 +524,15 @@ int migrate_use_xbzrle(void)
>      return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
>  }
>  
> +bool migrate_is_localhost(void)
> +{
> +    MigrationState *s;
> +
> +    s = migrate_get_current();
> +
> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST];
> +}

I think this is a bad name, too.  There is nothing more "local" in this
migration than in "unix:" migration.

Let's call the capability according to what it does, for example
unix-page-flipping.

Paolo

Re: [Qemu-devel] [PATCH 07/18] arch_init: export RAM_SAVE_xxx flags

[Michael R. Hines]

On 08/21/2013 06:49 AM, Paolo Bonzini wrote:
> Il 21/08/2013 09:18, Lei Li ha scritto:
>> Export RAM_SAVE_xxx flags for localhost migration.
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   arch_init.c                   |   12 ------------
>>   include/migration/migration.h |   14 ++++++++++++++
>>   2 files changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 68a7ab7..1ea7c29 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -108,18 +108,6 @@ static bool mig_throttle_on;
>>   static int dirty_rate_high_cnt;
>>   static void check_guest_throttling(void);
>>   
>> -/***********************************************************/
>> -/* ram save/restore */
>> -
>> -#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
>> -#define RAM_SAVE_FLAG_COMPRESS 0x02
>> -#define RAM_SAVE_FLAG_MEM_SIZE 0x04
>> -#define RAM_SAVE_FLAG_PAGE     0x08
>> -#define RAM_SAVE_FLAG_EOS      0x10
>> -#define RAM_SAVE_FLAG_CONTINUE 0x20
>> -#define RAM_SAVE_FLAG_XBZRLE   0x40
>> -/* 0x80 is reserved in migration.h start with 0x100 next */
>> -
>>   
>>   static struct defconfig_file {
>>       const char *filename;
>> diff --git a/include/migration/migration.h b/include/migration/migration.h
>> index 6a24e65..5336117 100644
>> --- a/include/migration/migration.h
>> +++ b/include/migration/migration.h
>> @@ -158,12 +158,26 @@ void ram_control_before_iterate(QEMUFile *f, uint64_t flags);
>>   void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
>>   void ram_control_load_hook(QEMUFile *f, uint64_t flags);
>>   
>> +
>> +/***********************************************************/
>> +/* ram save/restore */
>> +
>> +#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
>> +#define RAM_SAVE_FLAG_COMPRESS 0x02
>> +#define RAM_SAVE_FLAG_MEM_SIZE 0x04
>> +#define RAM_SAVE_FLAG_PAGE     0x08
>> +#define RAM_SAVE_FLAG_EOS      0x10
>> +#define RAM_SAVE_FLAG_CONTINUE 0x20
>> +#define RAM_SAVE_FLAG_XBZRLE   0x40
>> +
>>   /* Whenever this is found in the data stream, the flags
>>    * will be passed to ram_control_load_hook in the incoming-migration
>>    * side. This lets before_ram_iterate/after_ram_iterate add
>>    * transport-specific sections to the RAM migration data.
>>    */
>>   #define RAM_SAVE_FLAG_HOOK     0x80
>> +/* Start with 0x100 next */
>> +
>>   
>>   #define RAM_SAVE_CONTROL_NOT_SUPP -1000
>>   #define RAM_SAVE_CONTROL_DELAYED  -2000
>>
> This also looks like an encapsulation violation.
>
> Localhost migration is not very different in concept from RDMA (except
> that it runs with the VM stopped, but that's just because you do
> MADV_DONTNEED---it's not specific to the migration transport), and it
> manages to do everything without touching arch_init.c and migration.c.
>
> Paolo
>

I think maybe we need a private header file here. Multiple parties want 
to add
potentially add to this flag list without modifying arch_init.c.

How about something like include/migration/shared.h or common.h? or 
Something like that?

It could hold both the state machine flags from migration.c as well as 
the RAM_SAVE flags.....

- Michael

Re: [Qemu-devel] [PATCH 08/18] migration-local: introduce qemu_fopen_local()

[Michael R. Hines]

On 08/21/2013 03:18 AM, Lei Li wrote:
> Introduce read/write backend of QEMUFileLocal used by localhost
> migration. The unix domain socket will be replaced by PIPE with
> vmsplice mechanism.
>
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>   Makefile.objs     |    1 +
>   migration-local.c |  211 +++++++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 212 insertions(+), 0 deletions(-)
>   create mode 100644 migration-local.c
>
> diff --git a/Makefile.objs b/Makefile.objs
> index f46a4cd..30670cc 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -54,6 +54,7 @@ common-obj-y += migration.o migration-tcp.o
>   common-obj-$(CONFIG_RDMA) += migration-rdma.o
>   common-obj-y += qemu-char.o #aio.o
>   common-obj-y += block-migration.o
> +common-obj-y += migration-local.o
>   common-obj-y += page_cache.o xbzrle.o
>
>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o migration-fd.o
> diff --git a/migration-local.c b/migration-local.c
> new file mode 100644
> index 0000000..93190fd
> --- /dev/null
> +++ b/migration-local.c
> @@ -0,0 +1,211 @@
> +/*
> + * QEMU localhost migration
> + *
> + * Copyright IBM, Corp. 2013
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * later.
> + *
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "config-host.h"
> +#include "qemu-common.h"
> +#include "migration/migration.h"
> +#include "exec/cpu-common.h"
> +#include "config.h"
> +#include "exec/cpu-all.h"
> +#include "monitor/monitor.h"
> +#include "migration/qemu-file.h"
> +#include "qemu/iov.h"
> +#include "sysemu/arch_init.h"
> +#include "sysemu/sysemu.h"
> +#include "block/block.h"
> +#include "qemu/sockets.h"
> +#include "migration/block.h"
> +#include "qemu/thread.h"
> +#include "qmp-commands.h"
> +#include "trace.h"
> +#include "qemu/osdep.h"
> +
> +//#define DEBUG_MIGRATION_LOCAL
> +
> +#ifdef DEBUG_MIGRATION_LOCAL
> +#define DPRINTF(fmt, ...) \
> +    do { printf("migration-local: " fmt, ## __VA_ARGS__); } while (0)
> +#else
> +#define DPRINTF(fmt, ...) \
> +    do { } while (0)
> +#endif
> +
> +/*
> + * Interface for the local migration.
> + */
> +typedef struct QEMUFileLocal {
> +    QEMUFile *file;
> +    int fd;
> +    int state;
> +
> +    /*
> +     * This is the last block from where we have sent data
> +     * for local migration
> +     */
> +    RAMBlock *last_block_sent;
> +} QEMUFileLocal;
> +
> +
> +static int qemu_local_get_buffer(void *opaque, uint8_t *buf,
> +                                 int64_t pos, int size)
> +{
> +    QEMUFileLocal *s = opaque;
> +    ssize_t len;
> +
> +    for (;;) {
> +        len = qemu_recv(s->fd, buf, size, 0);
> +        if (len != -1) {
> +            break;
> +        }
> +        if (socket_error() == EAGAIN) {
> +            yield_until_fd_readable(s->fd);
> +        } else if (socket_error() != EINTR) {
> +            break;
> +        }
> +    }
> +
> +    if (len == -1) {
> +        len = -socket_error();
> +    }
> +    return len;
> +}
> +

This looks like a line-for-line copy of socket_get_buffer()......

Since you're just going to end up replacing this with vmsplice(),
could you just call socket_get_buffer() temporarily until
your next patch is ready?

> +static int qemu_local_get_fd(void *opaque)
> +{
> +    QEMUFileLocal *s = opaque;
> +
> +    return s->fd;
> +}
> +
> +static int qemu_local_close(void *opaque)
> +{
> +    QEMUFileLocal *s = opaque;
> +
> +    closesocket(s->fd);
> +    g_free(s);
> +
> +    return 0;
> +}
> +
> +static size_t qemu_local_put_buffer(void *opaque, struct iovec *iov,
> +                                    int iovcnt, int64_t pos)
> +{
> +    QEMUFileLocal *s = opaque;
> +    ssize_t len;
> +    ssize_t size = iov_size(iov, iovcnt);
> +
> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
> +    if (len < size) {
> +        len = -socket_error();
> +    }
> +
> +    return len;
> +}
> +
> +static size_t local_save_page(QEMUFile *f, RAMBlock *block,
> +                              ram_addr_t offset, int flags)
> +{
> +    MemoryRegion *mr = block->mr;
> +    uint8_t *p;
> +
> +    p = memory_region_get_ram_ptr(mr) + offset;
> +
> +    if (buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE)) {
> +        qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_COMPRESS);
> +        if (!flags) {
> +            qemu_put_byte(f, strlen(block->idstr));
> +            qemu_put_buffer(f, (uint8_t *)block->idstr,
> +                            strlen(block->idstr));
> +        }
> +        qemu_put_byte(f, *p);
> +        return 0;
> +    }
> +
> +    qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_PAGE);
> +    if (!flags) {
> +        qemu_put_byte(f, strlen(block->idstr));
> +        qemu_put_buffer(f, (uint8_t *)block->idstr,
> +                        strlen(block->idstr));
> +    }
> +    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
> +
> +    return TARGET_PAGE_SIZE;
> +}
> +
> +static size_t qemu_local_ram_save(QEMUFile *f, void *opaque,
> +                                  ram_addr_t block_offset, ram_addr_t offset,
> +                                  size_t size, int *bytes_sent)
> +{
> +    QEMUFileLocal *s = opaque;
> +    uint64_t current_addr = block_offset + offset;
> +    RAMBlock *block = qemu_get_ram_block(current_addr);
> +    MemoryRegion *mr = block->mr;

RAMBlock structs are not visible outside of exec.c and arch_init.c,
how did you do this?
> +    void *ram;
> +    int ret;
> +    int cont;
> +
> +    ret = qemu_file_get_error(f);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    qemu_fflush(f);
> +
> +    cont = (block == s->last_block_sent) ? RAM_SAVE_FLAG_CONTINUE : 0;
> +
> +    ram = memory_region_get_ram_ptr(mr) + offset;
> +    s->last_block_sent = block;
> +
> +    *bytes_sent = local_save_page(f, block, offset, cont);
> +    if (!bytes_sent || *bytes_sent < 0) {
> +        return RAM_SAVE_CONTROL_DELAYED;
> +    }
> +
RAM_SAVE_CONTROL_DELAYED is only if you have *not* finished moving the 
bytes.

If you've finished moving the bytes, then you should return zero.

> +    /* DONTNEED the RAM page that has already been copied. */
> +    qemu_madvise(ram, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
> +

This should be ram_handle_compressed().

> +    return 0;
> +}
> +
> +const QEMUFileOps local_read_ops = {
> +    .get_fd     = qemu_local_get_fd,
> +    .get_buffer = qemu_local_get_buffer,
> +    .close      = qemu_local_close,
> +};
> +
> +const QEMUFileOps local_write_ops = {
> +    .get_fd             = qemu_local_get_fd,
> +    .writev_buffer      = qemu_local_put_buffer,
> +    .close              = qemu_local_close,
> +    .save_page          = qemu_local_ram_save,
> +};
> +
> +static void *qemu_fopen_local(int fd, const char *mode)
> +{
> +    QEMUFileLocal *s;
> +
> +    if (qemu_file_mode_is_not_valid(mode)) {
> +        return NULL;
> +    }
> +
> +    s = g_malloc0(sizeof(QEMUFileLocal));
> +    s->fd = fd;
> +
> +    if (mode[0] == 'w') {
> +        qemu_set_block(s->fd);
> +        s->file = qemu_fopen_ops(s, &local_write_ops);
> +    } else {
> +        s->file = qemu_fopen_ops(s, &local_read_ops);
> +    }
> +
> +    return s->file;
> +}

Re: [Qemu-devel] [PATCH 10/18] migration-local: implementation of outgoing part

[Michael R. Hines]

On 08/21/2013 03:18 AM, Lei Li wrote:
> Implementation of outgoing part for localhost migration.
> The integration of migration thread and corresponding
> adjustment will be in coming patches.
>
> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> ---
>   include/migration/migration.h |    2 +
>   migration-local.c             |   85 +++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 87 insertions(+), 0 deletions(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 5336117..d2c7eff 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -92,6 +92,8 @@ void rdma_start_outgoing_migration(void *opaque, const char *host_port, Error **
>
>   void rdma_start_incoming_migration(const char *host_port, Error **errp);
>
> +void local_start_outgoing_migration(void *opaque, const char *uri, Error **errp);
> +
>   void migrate_fd_error(MigrationState *s);
>
>   void migrate_fd_connect(MigrationState *s);
> diff --git a/migration-local.c b/migration-local.c
> index 93190fd..cf4a091 100644
> --- a/migration-local.c
> +++ b/migration-local.c
> @@ -209,3 +209,88 @@ static void *qemu_fopen_local(int fd, const char *mode)
>
>       return s->file;
>   }
> +
> +/************************************************************************
> + * Outgoing part
> + **/
> +
> +static QEMUFileLocal *local_migration_init(void)
> +{
> +    QEMUFileLocal *s = g_malloc0(sizeof(*s));
> +
> +    s->state = MIG_STATE_SETUP;
> +    trace_migrate_set_state(MIG_STATE_SETUP);
> +    s->fd = -1;
> +    s->last_block_sent = NULL;
> +
> +    return s;
> +}
> +

migration.c already does this. Is there some reason why you need to 
access the state machine?

If you have a custom initial values, you should change 
migrate_get_current, not create your own init().


> +static void unix_local_wait_for_connect(int fd, void *opaque)
> +{
> +    MigrationState *s = opaque;
> +
> +    if (fd < 0) {
> +        DPRINTF("migrate connect error\n");
> +        s->file = NULL;
> +        migrate_fd_error(s);
> +    } else {
> +        DPRINTF("migrate connect success\n");
> +        s->file = qemu_fopen_local(fd, "wb");
> +        migrate_fd_connect(s);
> +    }
> +}
> +
> +static void unix_local_outgoing_connect(MigrationState *s, const char *path,
> +                                        Error **errp)
> +{
> +    unix_nonblocking_connect(path, unix_local_wait_for_connect, s, errp);
> +}
> +
> +void local_start_outgoing_migration(void *opaque, const char *uri,
> +                                    Error **errp)
> +{
> +    MigrationState *s = opaque;
> +    const char *path;
> +    QEMUFileLocal *local;
> +    Error *local_err = NULL;
> +    int is_vm_running;
> +    int ret;
> +
> +    local = local_migration_init();
> +    if (local == NULL) {
> +        error_setg(errp, "Failed to initialize\n");
> +    }
> +
> +    bdrv_flush_all();
> +
> +    is_vm_running = runstate_is_running();
> +
> +    /* Stop the VM first */
> +    if (is_vm_running) {
> +        ret = vm_stop(RUN_STATE_SAVE_VM);
> +        if (ret < 0) {
> +            goto fail;
> +        }
> +    }
> +

What is this for? The migration_thread() already does this work for you.....

Are you trying to avoid pre-copy? Pre-copy should not be bad for you.

> +    /* Start outgoing migration via unix socket. */
> +    if (uri) {
> +        /* XXX. Creation of a new unix_start_outgoing_migration_* is
> +         * not necessary, just for the first step. This will be replaced
> +         * by vmsplice mechanism.
> +         **/
> +        unix_local_outgoing_connect(s, path, &local_err);
> +    } else {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                  "uri", "a valid migration protocol");
> +        goto fail;
> +    }
> +
> +    return;
> +
> +fail:
> +    error_propagate(errp, local_err);
> +    g_free(local);
> +    migrate_fd_error(s);
> +}

Re: [Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Michael R. Hines]

On 08/21/2013 11:18 AM, Paolo Bonzini wrote:
> Il 21/08/2013 09:18, Lei Li ha scritto:
>>       } else if (strstart(uri, "unix:", &p)) {
>> +        if (s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST]) {
>> +            local_start_outgoing_migration(s, p, &local_err);
>> +        }
>>           unix_start_outgoing_migration(s, p, &local_err);
>>       } else if (strstart(uri, "fd:", &p)) {
>>           fd_start_outgoing_migration(s, p, &local_err);
>> @@ -521,6 +524,15 @@ int migrate_use_xbzrle(void)
>>       return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
>>   }
>>   
>> +bool migrate_is_localhost(void)
>> +{
>> +    MigrationState *s;
>> +
>> +    s = migrate_get_current();
>> +
>> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST];
>> +}
> I think this is a bad name, too.  There is nothing more "local" in this
> migration than in "unix:" migration.
>
> Let's call the capability according to what it does, for example
> unix-page-flipping.
>
> Paolo
>

Why is there a capability at all? Isn't that what the "local" URI is for?

- Michael

Re: [Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Lei Li]

On 08/21/2013 06:42 PM, Paolo Bonzini wrote:
> Il 21/08/2013 09:18, Lei Li ha scritto:
>> Commit 1964a397063967acc5ce71a2a24ed26e74824ee1 refactors rate
>> limiting to QEMUFile, but set the return value for qemu_file_rate_limit
>> to 1 in the case of qemu_file_get_error. It is wrong and should be negative
>> compared to the original function buffered_rate_limit and the current logic
>> in ram_save_iterate. As qemu_file_rate_limit is called manually to determine
>> if it has to exit, add the definition of the meaning of the return values
>> as well.
> When there is an error it returns "time to stop" so that ultimately we
> get to the migration_thread function and check qemu_file_get_error there.
>
> Why doesn't this work for you?

Hi Paolo,

Say, In ram_save_iterate(), the current logic is:

ret = qemu_file_rate_limit();
while(ret == 0) {
     save RAM blocks until no more to send.
}
if (ret < 0) {
     return ret;
}
...

And in savevm layer, qemu_savevm_state_iterate() set an error if the return
value of ram_save_iterate < 0.

Obviously the return value of qemu_file_rate_limit() should have an negative
value for there has been an error. Otherwise we need to modify the logic above.

> Paolo
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   savevm.c |   14 ++++++++++++--
>>   1 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 68552a7..6362275 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -904,10 +904,20 @@ int64_t qemu_ftell(QEMUFile *f)
>>       return f->pos;
>>   }
>>   
>> +/*
>> + * The meaning of the return values is:
>> + *   0: We can continue sending
>> + *   1: Time to stop
>> + *   negative: There has been an error
>> + */
>> +
>>   int qemu_file_rate_limit(QEMUFile *f)
>>   {
>> -    if (qemu_file_get_error(f)) {
>> -        return 1;
>> +    int ret;
>> +
>> +    ret = qemu_file_get_error(f);
>> +    if (ret) {
>> +        return ret;
>>       }
>>       if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) {
>>           return 1;
>>
>


-- 
Lei

Re: [Qemu-devel] [PATCH 06/18] bugfix: wrong error set by ram_control_load_hook()

[Lei Li]

On 08/21/2013 06:40 PM, Paolo Bonzini wrote:
> Il 21/08/2013 09:18, Lei Li ha scritto:
>> It should set negative error value if there has been an error.
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   savevm.c |    2 +-
>>   1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/savevm.c b/savevm.c
>> index 1522d95..f10e031 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -649,7 +649,7 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t flags)
>>   
>>   void ram_control_load_hook(QEMUFile *f, uint64_t flags)
>>   {
>> -    int ret = 0;
>> +    int ret = -1;
> -EINVAL

OK, thanks.

>
> otherwise looks good thanks!
>
> Paolo
>
>>       if (f->ops->hook_ram_load) {
>>           ret = f->ops->hook_ram_load(f, f->opaque, flags);
>>
>


-- 
Lei

Re: [Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Paolo Bonzini]

> Say, In ram_save_iterate(), the current logic is:
> 
> ret = qemu_file_rate_limit();
> while(ret == 0) {
>      save RAM blocks until no more to send.
> }
> if (ret < 0) {
>      return ret;
> }
> ...
> 
> And in savevm layer, qemu_savevm_state_iterate() set an error if the return
> value of ram_save_iterate < 0.

But that is to report errors *not in the QEMUFile*.  Errors in the
QEMUFile are already reported by qemu_file_get_error(), and
qemu_savevm_state_iterate() will not overwrite them.

qemu_file_rate_limit() returning 1 is enough to exit the loop,
which is all that is needed.

> Obviously the return value of qemu_file_rate_limit() should have an negative
> value for there has been an error. Otherwise we need to modify the logic
> above.

It is not obvious to me... what is, again, the bug that you're observing?
I think it is happening only because you're modifying the migration thread's
body.  If you use the normal code of the migration thread, it will just work.

Paolo

Re: [Qemu-devel] [PATCH 06/18] bugfix: wrong error set by ram_control_load_hook()

[Paolo Bonzini]

> On 08/21/2013 06:40 PM, Paolo Bonzini wrote:
> > Il 21/08/2013 09:18, Lei Li ha scritto:
> >> It should set negative error value if there has been an error.
> >>
> >> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
> >> ---
> >>   savevm.c |    2 +-
> >>   1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/savevm.c b/savevm.c
> >> index 1522d95..f10e031 100644
> >> --- a/savevm.c
> >> +++ b/savevm.c
> >> @@ -649,7 +649,7 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t
> >> flags)
> >>   
> >>   void ram_control_load_hook(QEMUFile *f, uint64_t flags)
> >>   {
> >> -    int ret = 0;
> >> +    int ret = -1;
> > -EINVAL
> 
> OK, thanks.

Can you extract the patches that I reviewed positively, plus this
one, and send them separately?

Thanks!

Paolo

Re: [Qemu-devel] [PATCH 13/18] arch_init: adjust ram_save_setup() for migrate_is_localhost

[Lei Li]

On 08/21/2013 06:48 PM, Paolo Bonzini wrote:
> Il 21/08/2013 09:18, Lei Li ha scritto:
>> Send all the ram blocks hooked by save_page, which will copy
>> ram page and MADV_DONTNEED the page just copied.
> You should implement this entirely in the hook.
>
> It will be a little less efficient because of the dirty bitmap overhead,
> but you should aim at having *zero* changes in arch_init.c and migration.c.

Yes, the reason I modify the migration_thread() to have new process that send all
the ram pages in adjusted qemu_savevm_state_begin stage and send device states in
qemu_savevm_device_state stage for localhost migration is to avoid the bitmap thing,
which is a little less efficient just like you mentioned above.

The performance assurance is very important to this feature, our goal is 100ms
of downtime for a 1TB guest.

>
> Paolo
>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   arch_init.c |   19 +++++++++++++------
>>   1 files changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 434a4ca..cbbb4db 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -474,7 +474,7 @@ static int ram_save_block(QEMUFile *f, bool last_stage)
>>               /* In doubt sent page as normal */
>>               bytes_sent = -1;
>>               ret = ram_control_save_page(f, block->offset,
>> -                               offset, TARGET_PAGE_SIZE, &bytes_sent);
>> +                                        offset, TARGET_PAGE_SIZE, &bytes_sent);
>>   
>>               if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
>>                   if (ret != RAM_SAVE_CONTROL_DELAYED) {
>> @@ -613,11 +613,13 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>       RAMBlock *block;
>>       int64_t ram_pages = last_ram_offset() >> TARGET_PAGE_BITS;
>>   
>> -    migration_bitmap = bitmap_new(ram_pages);
>> -    bitmap_set(migration_bitmap, 0, ram_pages);
>> -    migration_dirty_pages = ram_pages;
>> -    mig_throttle_on = false;
>> -    dirty_rate_high_cnt = 0;
>> +    if (!migrate_is_localhost()) {
>> +        migration_bitmap = bitmap_new(ram_pages);
>> +        bitmap_set(migration_bitmap, 0, ram_pages);
>> +        migration_dirty_pages = ram_pages;
>> +        mig_throttle_on = false;
>> +        dirty_rate_high_cnt = 0;
>> +    }
>>   
>>       if (migrate_use_xbzrle()) {
>>           XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
>> @@ -641,6 +643,11 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
>>       migration_bitmap_sync();
>>       qemu_mutex_unlock_iothread();
>>   
>> +    if (migrate_is_localhost()) {
>> +        ram_save_blocks(f);
>> +        return 0;
>> +    }
>> +
>>       qemu_put_be64(f, ram_bytes_total() | RAM_SAVE_FLAG_MEM_SIZE);
>>   
>>       QTAILQ_FOREACH(block, &ram_list.blocks, next) {
>>
>


-- 
Lei

Re: [Qemu-devel] [PATCH 06/18] bugfix: wrong error set by ram_control_load_hook()

[Lei Li]

On 08/23/2013 01:34 PM, Paolo Bonzini wrote:
>> On 08/21/2013 06:40 PM, Paolo Bonzini wrote:
>>> Il 21/08/2013 09:18, Lei Li ha scritto:
>>>> It should set negative error value if there has been an error.
>>>>
>>>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>>>> ---
>>>>    savevm.c |    2 +-
>>>>    1 files changed, 1 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/savevm.c b/savevm.c
>>>> index 1522d95..f10e031 100644
>>>> --- a/savevm.c
>>>> +++ b/savevm.c
>>>> @@ -649,7 +649,7 @@ void ram_control_after_iterate(QEMUFile *f, uint64_t
>>>> flags)
>>>>    
>>>>    void ram_control_load_hook(QEMUFile *f, uint64_t flags)
>>>>    {
>>>> -    int ret = 0;
>>>> +    int ret = -1;
>>> -EINVAL
>> OK, thanks.
> Can you extract the patches that I reviewed positively, plus this
> one, and send them separately?

Sure, I will send it later. :)

>
> Thanks!
>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 07/18] arch_init: export RAM_SAVE_xxx flags

[Paolo Bonzini]

Il 22/08/2013 22:14, Michael R. Hines ha scritto:
> On 08/21/2013 06:49 AM, Paolo Bonzini wrote:
>> Il 21/08/2013 09:18, Lei Li ha scritto:
>>> Export RAM_SAVE_xxx flags for localhost migration.
>>>
>>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>>> ---
>>>   arch_init.c                   |   12 ------------
>>>   include/migration/migration.h |   14 ++++++++++++++
>>>   2 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch_init.c b/arch_init.c
>>> index 68a7ab7..1ea7c29 100644
>>> --- a/arch_init.c
>>> +++ b/arch_init.c
>>> @@ -108,18 +108,6 @@ static bool mig_throttle_on;
>>>   static int dirty_rate_high_cnt;
>>>   static void check_guest_throttling(void);
>>>   -/***********************************************************/
>>> -/* ram save/restore */
>>> -
>>> -#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
>>> -#define RAM_SAVE_FLAG_COMPRESS 0x02
>>> -#define RAM_SAVE_FLAG_MEM_SIZE 0x04
>>> -#define RAM_SAVE_FLAG_PAGE     0x08
>>> -#define RAM_SAVE_FLAG_EOS      0x10
>>> -#define RAM_SAVE_FLAG_CONTINUE 0x20
>>> -#define RAM_SAVE_FLAG_XBZRLE   0x40
>>> -/* 0x80 is reserved in migration.h start with 0x100 next */
>>> -
>>>     static struct defconfig_file {
>>>       const char *filename;
>>> diff --git a/include/migration/migration.h
>>> b/include/migration/migration.h
>>> index 6a24e65..5336117 100644
>>> --- a/include/migration/migration.h
>>> +++ b/include/migration/migration.h
>>> @@ -158,12 +158,26 @@ void ram_control_before_iterate(QEMUFile *f,
>>> uint64_t flags);
>>>   void ram_control_after_iterate(QEMUFile *f, uint64_t flags);
>>>   void ram_control_load_hook(QEMUFile *f, uint64_t flags);
>>>   +
>>> +/***********************************************************/
>>> +/* ram save/restore */
>>> +
>>> +#define RAM_SAVE_FLAG_FULL     0x01 /* Obsolete, not used anymore */
>>> +#define RAM_SAVE_FLAG_COMPRESS 0x02
>>> +#define RAM_SAVE_FLAG_MEM_SIZE 0x04
>>> +#define RAM_SAVE_FLAG_PAGE     0x08
>>> +#define RAM_SAVE_FLAG_EOS      0x10
>>> +#define RAM_SAVE_FLAG_CONTINUE 0x20
>>> +#define RAM_SAVE_FLAG_XBZRLE   0x40
>>> +
>>>   /* Whenever this is found in the data stream, the flags
>>>    * will be passed to ram_control_load_hook in the incoming-migration
>>>    * side. This lets before_ram_iterate/after_ram_iterate add
>>>    * transport-specific sections to the RAM migration data.
>>>    */
>>>   #define RAM_SAVE_FLAG_HOOK     0x80
>>> +/* Start with 0x100 next */
>>> +
>>>     #define RAM_SAVE_CONTROL_NOT_SUPP -1000
>>>   #define RAM_SAVE_CONTROL_DELAYED  -2000
>>>
>> This also looks like an encapsulation violation.
>>
>> Localhost migration is not very different in concept from RDMA (except
>> that it runs with the VM stopped, but that's just because you do
>> MADV_DONTNEED---it's not specific to the migration transport), and it
>> manages to do everything without touching arch_init.c and migration.c.
> 
> I think maybe we need a private header file here. Multiple parties want
> to add potentially add to this flag list without modifying arch_init.c.

There is a mechanism for private flags, which is the page-load hook we
introduced for RDMA.  The existing RAM_SAVE_FLAG_* should not be needed
outside arch_init.c.

MIG_STATE_* might be needed elsewhere, but the right way to export it is
this one: change MigrationInfo's status field from a string to a QAPI
enum, and remove MIG_STATE_* altogether in favor of the QAPI enum.

Paolo

Re: [Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Paolo Bonzini]

Il 22/08/2013 22:50, Michael R. Hines ha scritto:
> On 08/21/2013 11:18 AM, Paolo Bonzini wrote:
>> Il 21/08/2013 09:18, Lei Li ha scritto:
>>>       } else if (strstart(uri, "unix:", &p)) {
>>> +        if (s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST]) {
>>> +            local_start_outgoing_migration(s, p, &local_err);
>>> +        }
>>>           unix_start_outgoing_migration(s, p, &local_err);
>>>       } else if (strstart(uri, "fd:", &p)) {
>>>           fd_start_outgoing_migration(s, p, &local_err);
>>> @@ -521,6 +524,15 @@ int migrate_use_xbzrle(void)
>>>       return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
>>>   }
>>>   +bool migrate_is_localhost(void)
>>> +{
>>> +    MigrationState *s;
>>> +
>>> +    s = migrate_get_current();
>>> +
>>> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST];
>>> +}
>> I think this is a bad name, too.  There is nothing more "local" in this
>> migration than in "unix:" migration.
>>
>> Let's call the capability according to what it does, for example
>> unix-page-flipping.
> 
> Why is there a capability at all? Isn't that what the "local" URI is for?

Because in these patches, the local URI is only present in the
destination (which is wrong: the destination should autodetect local
mode using the load-page hook).  As you can see above, a "unix" URI will
examine the capability and pick the appropriate migration method.

However, this is also the wrong place to look at the capability.  It is
the save-page hook that should examine the capability.  It will then do
nothing if it is disabled, and do page-flipping if the capability is on.

Paolo

Re: [Qemu-devel] [PATCH 08/18] migration-local: introduce qemu_fopen_local()

[Lei Li]

On 08/23/2013 04:42 AM, Michael R. Hines wrote:
> On 08/21/2013 03:18 AM, Lei Li wrote:
>> Introduce read/write backend of QEMUFileLocal used by localhost
>> migration. The unix domain socket will be replaced by PIPE with
>> vmsplice mechanism.
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   Makefile.objs     |    1 +
>>   migration-local.c |  211 
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 212 insertions(+), 0 deletions(-)
>>   create mode 100644 migration-local.c
>>
>> diff --git a/Makefile.objs b/Makefile.objs
>> index f46a4cd..30670cc 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -54,6 +54,7 @@ common-obj-y += migration.o migration-tcp.o
>>   common-obj-$(CONFIG_RDMA) += migration-rdma.o
>>   common-obj-y += qemu-char.o #aio.o
>>   common-obj-y += block-migration.o
>> +common-obj-y += migration-local.o
>>   common-obj-y += page_cache.o xbzrle.o
>>
>>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o 
>> migration-fd.o
>> diff --git a/migration-local.c b/migration-local.c
>> new file mode 100644
>> index 0000000..93190fd
>> --- /dev/null
>> +++ b/migration-local.c
>> @@ -0,0 +1,211 @@
>> +/*
>> + * QEMU localhost migration
>> + *
>> + * Copyright IBM, Corp. 2013
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>> + * later.
>> + *
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "config-host.h"
>> +#include "qemu-common.h"
>> +#include "migration/migration.h"
>> +#include "exec/cpu-common.h"
>> +#include "config.h"
>> +#include "exec/cpu-all.h"
>> +#include "monitor/monitor.h"
>> +#include "migration/qemu-file.h"
>> +#include "qemu/iov.h"
>> +#include "sysemu/arch_init.h"
>> +#include "sysemu/sysemu.h"
>> +#include "block/block.h"
>> +#include "qemu/sockets.h"
>> +#include "migration/block.h"
>> +#include "qemu/thread.h"
>> +#include "qmp-commands.h"
>> +#include "trace.h"
>> +#include "qemu/osdep.h"
>> +
>> +//#define DEBUG_MIGRATION_LOCAL
>> +
>> +#ifdef DEBUG_MIGRATION_LOCAL
>> +#define DPRINTF(fmt, ...) \
>> +    do { printf("migration-local: " fmt, ## __VA_ARGS__); } while (0)
>> +#else
>> +#define DPRINTF(fmt, ...) \
>> +    do { } while (0)
>> +#endif
>> +
>> +/*
>> + * Interface for the local migration.
>> + */
>> +typedef struct QEMUFileLocal {
>> +    QEMUFile *file;
>> +    int fd;
>> +    int state;
>> +
>> +    /*
>> +     * This is the last block from where we have sent data
>> +     * for local migration
>> +     */
>> +    RAMBlock *last_block_sent;
>> +} QEMUFileLocal;
>> +
>> +
>> +static int qemu_local_get_buffer(void *opaque, uint8_t *buf,
>> +                                 int64_t pos, int size)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +    ssize_t len;
>> +
>> +    for (;;) {
>> +        len = qemu_recv(s->fd, buf, size, 0);
>> +        if (len != -1) {
>> +            break;
>> +        }
>> +        if (socket_error() == EAGAIN) {
>> +            yield_until_fd_readable(s->fd);
>> +        } else if (socket_error() != EINTR) {
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (len == -1) {
>> +        len = -socket_error();
>> +    }
>> +    return len;
>> +}
>> +
>
> This looks like a line-for-line copy of socket_get_buffer()......
>
> Since you're just going to end up replacing this with vmsplice(),
> could you just call socket_get_buffer() temporarily until
> your next patch is ready?
>
>> +static int qemu_local_get_fd(void *opaque)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +
>> +    return s->fd;
>> +}
>> +
>> +static int qemu_local_close(void *opaque)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +
>> +    closesocket(s->fd);
>> +    g_free(s);
>> +
>> +    return 0;
>> +}
>> +
>> +static size_t qemu_local_put_buffer(void *opaque, struct iovec *iov,
>> +                                    int iovcnt, int64_t pos)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +    ssize_t len;
>> +    ssize_t size = iov_size(iov, iovcnt);
>> +
>> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
>> +    if (len < size) {
>> +        len = -socket_error();
>> +    }
>> +
>> +    return len;
>> +}
>> +
>> +static size_t local_save_page(QEMUFile *f, RAMBlock *block,
>> +                              ram_addr_t offset, int flags)
>> +{
>> +    MemoryRegion *mr = block->mr;
>> +    uint8_t *p;
>> +
>> +    p = memory_region_get_ram_ptr(mr) + offset;
>> +
>> +    if (buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE)) {
>> +        qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_COMPRESS);
>> +        if (!flags) {
>> +            qemu_put_byte(f, strlen(block->idstr));
>> +            qemu_put_buffer(f, (uint8_t *)block->idstr,
>> +                            strlen(block->idstr));
>> +        }
>> +        qemu_put_byte(f, *p);
>> +        return 0;
>> +    }
>> +
>> +    qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_PAGE);
>> +    if (!flags) {
>> +        qemu_put_byte(f, strlen(block->idstr));
>> +        qemu_put_buffer(f, (uint8_t *)block->idstr,
>> +                        strlen(block->idstr));
>> +    }
>> +    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>> +
>> +    return TARGET_PAGE_SIZE;
>> +}
>> +
>> +static size_t qemu_local_ram_save(QEMUFile *f, void *opaque,
>> +                                  ram_addr_t block_offset, 
>> ram_addr_t offset,
>> +                                  size_t size, int *bytes_sent)
>> +{
>> +    QEMUFileLocal *s = opaque;
>> +    uint64_t current_addr = block_offset + offset;
>> +    RAMBlock *block = qemu_get_ram_block(current_addr);
>> +    MemoryRegion *mr = block->mr;
>
> RAMBlock structs are not visible outside of exec.c and arch_init.c,
> how did you do this?

Hi Michael,

Good catch!
Actually this is the 'Known issue' that I listed in the cover letter, and I
planed to ask suggestions on this.

Currently the implementation of override of the RDMA hook like save_page for
localhost migration needs to have knowledge of MemoryRegion and RAMBlock, say,
when saving ram page, need to know which RAMBlock the given ram address save_page
hook passed is belong to, or which is the last ram block has been sent. But
seems that such structs can not be exported to the private code in migration-local.c.

My guess for now is that there might be two possible ways to handle this:

1) Choose another way around, like representation of a RAMBlock and MemoryRegion
    from localhost migration perspective. This might need more work to handle
    the descriptions.

2) It could be exported directly into private code like migration-local.c
    through some ways that I don't know yet. I remembered that the implementation
    of postcopy migration did this.

So I'd like to post and ask for your suggestions to make sure it's the right
and best way to handle.


>> +    void *ram;
>> +    int ret;
>> +    int cont;
>> +
>> +    ret = qemu_file_get_error(f);
>> +    if (ret < 0) {
>> +        return ret;
>> +    }
>> +
>> +    qemu_fflush(f);
>> +
>> +    cont = (block == s->last_block_sent) ? RAM_SAVE_FLAG_CONTINUE : 0;
>> +
>> +    ram = memory_region_get_ram_ptr(mr) + offset;
>> +    s->last_block_sent = block;
>> +
>> +    *bytes_sent = local_save_page(f, block, offset, cont);
>> +    if (!bytes_sent || *bytes_sent < 0) {
>> +        return RAM_SAVE_CONTROL_DELAYED;
>> +    }
>> +
> RAM_SAVE_CONTROL_DELAYED is only if you have *not* finished moving the 
> bytes.
>
> If you've finished moving the bytes, then you should return zero.

Acknowledge, thanks.

>
>> +    /* DONTNEED the RAM page that has already been copied. */
>> +    qemu_madvise(ram, TARGET_PAGE_SIZE, QEMU_MADV_DONTNEED);
>> +
>
> This should be ram_handle_compressed().
>
>> +    return 0;
>> +}
>> +
>> +const QEMUFileOps local_read_ops = {
>> +    .get_fd     = qemu_local_get_fd,
>> +    .get_buffer = qemu_local_get_buffer,
>> +    .close      = qemu_local_close,
>> +};
>> +
>> +const QEMUFileOps local_write_ops = {
>> +    .get_fd             = qemu_local_get_fd,
>> +    .writev_buffer      = qemu_local_put_buffer,
>> +    .close              = qemu_local_close,
>> +    .save_page          = qemu_local_ram_save,
>> +};
>> +
>> +static void *qemu_fopen_local(int fd, const char *mode)
>> +{
>> +    QEMUFileLocal *s;
>> +
>> +    if (qemu_file_mode_is_not_valid(mode)) {
>> +        return NULL;
>> +    }
>> +
>> +    s = g_malloc0(sizeof(QEMUFileLocal));
>> +    s->fd = fd;
>> +
>> +    if (mode[0] == 'w') {
>> +        qemu_set_block(s->fd);
>> +        s->file = qemu_fopen_ops(s, &local_write_ops);
>> +    } else {
>> +        s->file = qemu_fopen_ops(s, &local_read_ops);
>> +    }
>> +
>> +    return s->file;
>> +}
>
>


-- 
Lei

Re: [Qemu-devel] [PATCH 13/18] arch_init: adjust ram_save_setup() for migrate_is_localhost

[Paolo Bonzini]

Il 23/08/2013 08:25, Lei Li ha scritto:
> On 08/21/2013 06:48 PM, Paolo Bonzini wrote:
>> Il 21/08/2013 09:18, Lei Li ha scritto:
>>> Send all the ram blocks hooked by save_page, which will copy
>>> ram page and MADV_DONTNEED the page just copied.
>> You should implement this entirely in the hook.
>>
>> It will be a little less efficient because of the dirty bitmap overhead,
>> but you should aim at having *zero* changes in arch_init.c and
>> migration.c.
> 
> Yes, the reason I modify the migration_thread() to have new process that
> send all the ram pages in adjusted qemu_savevm_state_begin stage and send device
> states in qemu_savevm_device_state stage for localhost migration is to avoid the
> bitmap thing, which is a little less efficient just like you mentioned above.
> 
> The performance assurance is very important to this feature, our goal is
> 100ms of downtime for a 1TB guest.

Do not _start_ by introducing encapsulation violations all over the place.

Juan has been working on optimizing the dirty bitmap code.  His patches
could introduce a speedup of a factor of up to 64.  Thus it is possible
that his work will help you enough that you can work with the dirty bitmap.

Also, this feature (not looking at the dirty bitmap if the machine is
stopped) is not limited to localhost migration, add it later once the
basic vmsplice plumbing is in place.  This will also let you profile the
code and understand whether the goal is attainable.

I honestly doubt that 100ms of downtime is possible while the machine is
stopped.  A 1TB guest has 2^28 = 268*10^6 pages, which you want to
process in 100*10^6 nanoseconds.  Thus, your approach would require 0.4
nanoseconds per page, or roughly 2 clock cycles per page.  This is
impossible without _massive_ parallelization at all levels, starting
from the kernel.

As a matter of fact, 2^28 madvise system calls will take much, much
longer than 100ms.

Have you thought of using shared memory (with -mempath) instead of vmsplice?

Paolo

Re: [Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Lei Li]

On 08/23/2013 03:40 PM, Paolo Bonzini wrote:
> Il 22/08/2013 22:50, Michael R. Hines ha scritto:
>> On 08/21/2013 11:18 AM, Paolo Bonzini wrote:
>>> Il 21/08/2013 09:18, Lei Li ha scritto:
>>>>        } else if (strstart(uri, "unix:", &p)) {
>>>> +        if (s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST]) {
>>>> +            local_start_outgoing_migration(s, p, &local_err);
>>>> +        }
>>>>            unix_start_outgoing_migration(s, p, &local_err);
>>>>        } else if (strstart(uri, "fd:", &p)) {
>>>>            fd_start_outgoing_migration(s, p, &local_err);
>>>> @@ -521,6 +524,15 @@ int migrate_use_xbzrle(void)
>>>>        return s->enabled_capabilities[MIGRATION_CAPABILITY_XBZRLE];
>>>>    }
>>>>    +bool migrate_is_localhost(void)
>>>> +{
>>>> +    MigrationState *s;
>>>> +
>>>> +    s = migrate_get_current();
>>>> +
>>>> +    return s->enabled_capabilities[MIGRATION_CAPABILITY_LOCALHOST];
>>>> +}
>>> I think this is a bad name, too.  There is nothing more "local" in this
>>> migration than in "unix:" migration.
>>>
>>> Let's call the capability according to what it does, for example
>>> unix-page-flipping.
>> Why is there a capability at all? Isn't that what the "local" URI is for?
> Because in these patches, the local URI is only present in the
> destination (which is wrong: the destination should autodetect local
> mode using the load-page hook).  As you can see above, a "unix" URI will
> examine the capability and pick the appropriate migration method.

Hi Paolo,

I agree with that 'localhost' is a bad capability name. Since we just use unix
socket temporarily and will be ending up with vmsplice via pipe, how about
just page_flipping?

>
> However, this is also the wrong place to look at the capability.  It is
> the save-page hook that should examine the capability.  It will then do
> nothing if it is disabled, and do page-flipping if the capability is on.
>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 13/18] arch_init: adjust ram_save_setup() for migrate_is_localhost

[Alex Bligh]

--On 23 August 2013 09:48:42 +0200 Paolo Bonzini <pbonzini@redhat.com> 
wrote:

> As a matter of fact, 2^28 madvise system calls will take much, much
> longer than 100ms.

Probably a stupid question, but why would you need to do one call per
page? It takes a 'size_t length' parameter.

-- 
Alex Bligh

Re: [Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Paolo Bonzini]

Il 23/08/2013 09:51, Lei Li ha scritto:
>> Because in these patches, the local URI is only present in the
>> destination (which is wrong: the destination should autodetect local
>> mode using the load-page hook).  As you can see above, a "unix" URI will
>> examine the capability and pick the appropriate migration method.
> 
> Hi Paolo,
> 
> I agree with that 'localhost' is a bad capability name. Since we just
> use unix
> socket temporarily and will be ending up with vmsplice via pipe, how about
> just page_flipping?

The basic migration method is still a unix socket (and a "unix:" URI).
I suggested unix-page-flipping to make it clear that it doesn't work
with tcp or rdma migration (you might make it work with fd migration).

Paolo

Re: [Qemu-devel] [PATCH 13/18] arch_init: adjust ram_save_setup() for migrate_is_localhost

[Paolo Bonzini]

Il 23/08/2013 09:57, Alex Bligh ha scritto:
> 
> 
> --On 23 August 2013 09:48:42 +0200 Paolo Bonzini <pbonzini@redhat.com>
> wrote:
> 
>> As a matter of fact, 2^28 madvise system calls will take much, much
>> longer than 100ms.
> 
> Probably a stupid question, but why would you need to do one call per
> page? It takes a 'size_t length' parameter.

Right now migration is done a page at a time, and so is madvise AFAIU.
However, even with a larger length parameter I suspect it would alone
take more than 2 cycles per page.

So one way to do this could be to add a flag to "migrate" that would
migrate devices only, and use shared memory in both the source and the
target.

There is still a problem, because we must make sure the destination
doesn't write to memory (e.g. read firmware) when initializing the
board, because that would overwrite the memory of the running instance.
 But it looks more promising than page flipping.

Paolo

Re: [Qemu-devel] [PATCH 13/18] arch_init: adjust ram_save_setup() for migrate_is_localhost

[Lei Li]

On 08/23/2013 03:48 PM, Paolo Bonzini wrote:
> Il 23/08/2013 08:25, Lei Li ha scritto:
>> On 08/21/2013 06:48 PM, Paolo Bonzini wrote:
>>> Il 21/08/2013 09:18, Lei Li ha scritto:
>>>> Send all the ram blocks hooked by save_page, which will copy
>>>> ram page and MADV_DONTNEED the page just copied.
>>> You should implement this entirely in the hook.
>>>
>>> It will be a little less efficient because of the dirty bitmap overhead,
>>> but you should aim at having *zero* changes in arch_init.c and
>>> migration.c.
>> Yes, the reason I modify the migration_thread() to have new process that
>> send all the ram pages in adjusted qemu_savevm_state_begin stage and send device
>> states in qemu_savevm_device_state stage for localhost migration is to avoid the
>> bitmap thing, which is a little less efficient just like you mentioned above.
>>
>> The performance assurance is very important to this feature, our goal is
>> 100ms of downtime for a 1TB guest.
> Do not _start_ by introducing encapsulation violations all over the place.
>
> Juan has been working on optimizing the dirty bitmap code.  His patches
> could introduce a speedup of a factor of up to 64.  Thus it is possible
> that his work will help you enough that you can work with the dirty bitmap.
>
> Also, this feature (not looking at the dirty bitmap if the machine is
> stopped) is not limited to localhost migration, add it later once the
> basic vmsplice plumbing is in place.  This will also let you profile the
> code and understand whether the goal is attainable.
>
> I honestly doubt that 100ms of downtime is possible while the machine is
> stopped.  A 1TB guest has 2^28 = 268*10^6 pages, which you want to
> process in 100*10^6 nanoseconds.  Thus, your approach would require 0.4
> nanoseconds per page, or roughly 2 clock cycles per page.  This is
> impossible without _massive_ parallelization at all levels, starting
> from the kernel.
>
> As a matter of fact, 2^28 madvise system calls will take much, much
> longer than 100ms.
>
> Have you thought of using shared memory (with -mempath) instead of vmsplice?

Precisely!

Well, as Anthony mentioned in the version 1[1], there has been some work involved
regarding improvement of vmsplice() at kernel side by Robert Jennings[2].

And yes, shared memory is an alternative, I think the problem with shared memory is
that can't share anonymous memory. For this maybe Anthony can chime in as the original
idea him.  :-)


Reference links:

[1] Anthony's comments:
   https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg02577.html

[2] vmpslice support for zero-copy gifting of pages:
   http://comments.gmane.org/gmane.linux.kernel.mm/103998

>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Lei Li]

On 08/23/2013 01:34 PM, Paolo Bonzini wrote:
>> Say, In ram_save_iterate(), the current logic is:
>>
>> ret = qemu_file_rate_limit();
>> while(ret == 0) {
>>       save RAM blocks until no more to send.
>> }
>> if (ret < 0) {
>>       return ret;
>> }
>> ...
>>
>> And in savevm layer, qemu_savevm_state_iterate() set an error if the return
>> value of ram_save_iterate < 0.
> But that is to report errors *not in the QEMUFile*.  Errors in the
> QEMUFile are already reported by qemu_file_get_error(), and
> qemu_savevm_state_iterate() will not overwrite them.

Sorry, a little confusion.
In qemu_savevm_state_iterate(), if the return of ram_save_iterate < 0,
it will set error in QEMUFile by qemu_file_set_error(f, ret), the error
reported by qemu_file_get_error() in the QEMUFile is got from
qemu_file_set_error() by reading the f->last_error, right?

And now as qemu_file_rate_limit() never return negative value, what's the
meaning for the check: if (qemu_file_rate_limit(f) < 0) in ram_save_iterate()?

>
> qemu_file_rate_limit() returning 1 is enough to exit the loop,
> which is all that is needed.
>> Obviously the return value of qemu_file_rate_limit() should have an negative
>> value for there has been an error. Otherwise we need to modify the logic
>> above.
> It is not obvious to me... what is, again, the bug that you're observing?
> I think it is happening only because you're modifying the migration thread's
> body.  If you use the normal code of the migration thread, it will just work.
>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 13/18] arch_init: adjust ram_save_setup() for migrate_is_localhost

[Paolo Bonzini]

Il 23/08/2013 11:00, Lei Li ha scritto:
>>> The performance assurance is very important to this feature, our goal is
>>> 100ms of downtime for a 1TB guest.
>>
>> I honestly doubt that 100ms of downtime is possible while the machine is
>> stopped.  A 1TB guest has 2^28 = 268*10^6 pages, which you want to
>> process in 100*10^6 nanoseconds.  Thus, your approach would require 0.4
>> nanoseconds per page, or roughly 2 clock cycles per page.  This is
>> impossible without _massive_ parallelization at all levels, starting
>> from the kernel.
>>
>> Have you thought of using shared memory (with -mempath) instead of
>> vmsplice?
> 
> Precisely!
> 
> Well, as Anthony mentioned in the version 1[1], there has been some work involved
> regarding improvement of vmsplice() at kernel side by Robert Jennings[2].

Oh, finally!

> And yes, shared memory is an alternative, I think the problem with shared memory is
> that can't share anonymous memory. For this maybe Anthony can chime in
> as the original idea him.  :-)

You could perhaps switch from normal to shared memory while the VM is
running?  Either use the dirty bitmap for this, or mmap the shared
memory "in place" on top of the anonymous memory.  Since it needs to be
done while the VM is stopped, you can do it perhaps 1G at a time and let
the VM run for some time before doing the next 1G.

The nice part is that because the VM is running, you can do it as slow
as you want. :)  Only the final flush and device save affects downtime.

Paolo
> 
> Reference links:
> 
> [1] Anthony's comments:
>   https://lists.gnu.org/archive/html/qemu-devel/2013-06/msg02577.html
> 
> [2] vmpslice support for zero-copy gifting of pages:
>   http://comments.gmane.org/gmane.linux.kernel.mm/103998
> 
>>
>> Paolo
>>
> 
> 

Re: [Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Paolo Bonzini]

Il 23/08/2013 11:11, Lei Li ha scritto:
> 
> In qemu_savevm_state_iterate(), if the return of ram_save_iterate < 0,
> it will set error in QEMUFile by qemu_file_set_error(f, ret), the error
> reported by qemu_file_get_error() in the QEMUFile is got from
> qemu_file_set_error() by reading the f->last_error, right?
> 
> And now as qemu_file_rate_limit() never return negative value, what's the
> meaning for the check: if (qemu_file_rate_limit(f) < 0) in
> ram_save_iterate()?

I only see a "while ((ret = qemu_file_rate_limit(f)) == 0)", no
less-than-zero check.

Are we looking at the same code? :)

Paolo

Re: [Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Lei Li]

On 08/23/2013 05:14 PM, Paolo Bonzini wrote:
> Il 23/08/2013 11:11, Lei Li ha scritto:
>> In qemu_savevm_state_iterate(), if the return of ram_save_iterate < 0,
>> it will set error in QEMUFile by qemu_file_set_error(f, ret), the error
>> reported by qemu_file_get_error() in the QEMUFile is got from
>> qemu_file_set_error() by reading the f->last_error, right?
>>
>> And now as qemu_file_rate_limit() never return negative value, what's the
>> meaning for the check: if (qemu_file_rate_limit(f) < 0) in
>> ram_save_iterate()?
> I only see a "while ((ret = qemu_file_rate_limit(f)) == 0)", no
> less-than-zero check.
>
> Are we looking at the same code? :)

I think so, hehe.
You might want to look a little more. After the while(..), there is a check:

if (ret < 0) {
     bytes_transferred += total_sent;
     return ret;
}

>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Lei Li]

On 08/23/2013 04:01 PM, Paolo Bonzini wrote:
> Il 23/08/2013 09:51, Lei Li ha scritto:
>>> Because in these patches, the local URI is only present in the
>>> destination (which is wrong: the destination should autodetect local
>>> mode using the load-page hook).  As you can see above, a "unix" URI will
>>> examine the capability and pick the appropriate migration method.
>> Hi Paolo,
>>
>> I agree with that 'localhost' is a bad capability name. Since we just
>> use unix
>> socket temporarily and will be ending up with vmsplice via pipe, how about
>> just page_flipping?
> The basic migration method is still a unix socket (and a "unix:" URI).
> I suggested unix-page-flipping to make it clear that it doesn't work
> with tcp or rdma migration (you might make it work with fd migration).

OK, make sense, thanks.

>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Paolo Bonzini]

Il 23/08/2013 11:18, Lei Li ha scritto:
>>>
>>> And now as qemu_file_rate_limit() never return negative value, what's
>>> the
>>> meaning for the check: if (qemu_file_rate_limit(f) < 0) in
>>> ram_save_iterate()?
>> I only see a "while ((ret = qemu_file_rate_limit(f)) == 0)", no
>> less-than-zero check.
>>
>> Are we looking at the same code? :)
> 
> I think so, hehe.
> You might want to look a little more. After the while(..), there is a
> check:
> 
> if (ret < 0) {
>     bytes_transferred += total_sent;
>     return ret;
> }

Aha, there you are.  It's dead code, you can delete it.

Paolo

Re: [Qemu-devel] [PATCH 04/18] savevm: set right return value for qemu_file_rate_limit

[Lei Li]

On 08/23/2013 05:22 PM, Paolo Bonzini wrote:
> Il 23/08/2013 11:18, Lei Li ha scritto:
>>>> And now as qemu_file_rate_limit() never return negative value, what's
>>>> the
>>>> meaning for the check: if (qemu_file_rate_limit(f) < 0) in
>>>> ram_save_iterate()?
>>> I only see a "while ((ret = qemu_file_rate_limit(f)) == 0)", no
>>> less-than-zero check.
>>>
>>> Are we looking at the same code? :)
>> I think so, hehe.
>> You might want to look a little more. After the while(..), there is a
>> check:
>>
>> if (ret < 0) {
>>      bytes_transferred += total_sent;
>>      return ret;
>> }
> Aha, there you are.  It's dead code, you can delete it.

Sure, I will send it along with other two patches you reviewed positively. :)

>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 17/18] migration: add prefix for local migration to incoming migration

[Lei Li]

On 08/21/2013 06:52 PM, Paolo Bonzini wrote:
> Il 21/08/2013 09:18, Lei Li ha scritto:
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   migration.c |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/migration.c b/migration.c
>> index 2471664..17cf2fd 100644
>> --- a/migration.c
>> +++ b/migration.c
>> @@ -81,6 +81,9 @@ void qemu_start_incoming_migration(const char *uri, Error **errp)
>>           unix_start_incoming_migration(p, errp);
>>       else if (strstart(uri, "fd:", &p))
>>           fd_start_incoming_migration(p, errp);
>> +    else if (strstart(uri, "local:", &p))
>> +        local_start_incoming_migration(p, errp);
>> +
>>   #endif
>>       else {
>>           error_setg(errp, "unknown migration protocol: %s", uri);
>>
> This should not be needed.  The destination can just use the normal
> unix: protocol.  The load hook will only be called if the source uses
> localhost migration, and it will get the pipe via SCM_RIGHTS then
> process the pipe.

Acknowledge, thanks.

>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 08/18] migration-local: introduce qemu_fopen_local()

[Lei Li]

On 08/23/2013 03:44 PM, Lei Li wrote:
> On 08/23/2013 04:42 AM, Michael R. Hines wrote:
>> On 08/21/2013 03:18 AM, Lei Li wrote:
>>> Introduce read/write backend of QEMUFileLocal used by localhost
>>> migration. The unix domain socket will be replaced by PIPE with
>>> vmsplice mechanism.
>>>
>>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>>> ---
>>>   Makefile.objs     |    1 +
>>>   migration-local.c |  211 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 212 insertions(+), 0 deletions(-)
>>>   create mode 100644 migration-local.c
>>>
>>> diff --git a/Makefile.objs b/Makefile.objs
>>> index f46a4cd..30670cc 100644
>>> --- a/Makefile.objs
>>> +++ b/Makefile.objs
>>> @@ -54,6 +54,7 @@ common-obj-y += migration.o migration-tcp.o
>>>   common-obj-$(CONFIG_RDMA) += migration-rdma.o
>>>   common-obj-y += qemu-char.o #aio.o
>>>   common-obj-y += block-migration.o
>>> +common-obj-y += migration-local.o
>>>   common-obj-y += page_cache.o xbzrle.o
>>>
>>>   common-obj-$(CONFIG_POSIX) += migration-exec.o migration-unix.o 
>>> migration-fd.o
>>> diff --git a/migration-local.c b/migration-local.c
>>> new file mode 100644
>>> index 0000000..93190fd
>>> --- /dev/null
>>> +++ b/migration-local.c
>>> @@ -0,0 +1,211 @@
>>> +/*
>>> + * QEMU localhost migration
>>> + *
>>> + * Copyright IBM, Corp. 2013
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or
>>> + * later.
>>> + *
>>> + * See the COPYING file in the top-level directory.
>>> + *
>>> + */
>>> +
>>> +#include "config-host.h"
>>> +#include "qemu-common.h"
>>> +#include "migration/migration.h"
>>> +#include "exec/cpu-common.h"
>>> +#include "config.h"
>>> +#include "exec/cpu-all.h"
>>> +#include "monitor/monitor.h"
>>> +#include "migration/qemu-file.h"
>>> +#include "qemu/iov.h"
>>> +#include "sysemu/arch_init.h"
>>> +#include "sysemu/sysemu.h"
>>> +#include "block/block.h"
>>> +#include "qemu/sockets.h"
>>> +#include "migration/block.h"
>>> +#include "qemu/thread.h"
>>> +#include "qmp-commands.h"
>>> +#include "trace.h"
>>> +#include "qemu/osdep.h"
>>> +
>>> +//#define DEBUG_MIGRATION_LOCAL
>>> +
>>> +#ifdef DEBUG_MIGRATION_LOCAL
>>> +#define DPRINTF(fmt, ...) \
>>> +    do { printf("migration-local: " fmt, ## __VA_ARGS__); } while (0)
>>> +#else
>>> +#define DPRINTF(fmt, ...) \
>>> +    do { } while (0)
>>> +#endif
>>> +
>>> +/*
>>> + * Interface for the local migration.
>>> + */
>>> +typedef struct QEMUFileLocal {
>>> +    QEMUFile *file;
>>> +    int fd;
>>> +    int state;
>>> +
>>> +    /*
>>> +     * This is the last block from where we have sent data
>>> +     * for local migration
>>> +     */
>>> +    RAMBlock *last_block_sent;
>>> +} QEMUFileLocal;
>>> +
>>> +
>>> +static int qemu_local_get_buffer(void *opaque, uint8_t *buf,
>>> +                                 int64_t pos, int size)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +    ssize_t len;
>>> +
>>> +    for (;;) {
>>> +        len = qemu_recv(s->fd, buf, size, 0);
>>> +        if (len != -1) {
>>> +            break;
>>> +        }
>>> +        if (socket_error() == EAGAIN) {
>>> +            yield_until_fd_readable(s->fd);
>>> +        } else if (socket_error() != EINTR) {
>>> +            break;
>>> +        }
>>> +    }
>>> +
>>> +    if (len == -1) {
>>> +        len = -socket_error();
>>> +    }
>>> +    return len;
>>> +}
>>> +
>>
[...]

>>
>>> +static int qemu_local_get_fd(void *opaque)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +
>>> +    return s->fd;
>>> +}
>>> +
>>> +static int qemu_local_close(void *opaque)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +
>>> +    closesocket(s->fd);
>>> +    g_free(s);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static size_t qemu_local_put_buffer(void *opaque, struct iovec *iov,
>>> +                                    int iovcnt, int64_t pos)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +    ssize_t len;
>>> +    ssize_t size = iov_size(iov, iovcnt);
>>> +
>>> +    len = iov_send(s->fd, iov, iovcnt, 0, size);
>>> +    if (len < size) {
>>> +        len = -socket_error();
>>> +    }
>>> +
>>> +    return len;
>>> +}
>>> +
>>> +static size_t local_save_page(QEMUFile *f, RAMBlock *block,
>>> +                              ram_addr_t offset, int flags)
>>> +{
>>> +    MemoryRegion *mr = block->mr;
>>> +    uint8_t *p;
>>> +
>>> +    p = memory_region_get_ram_ptr(mr) + offset;
>>> +
>>> +    if (buffer_find_nonzero_offset(p, TARGET_PAGE_SIZE)) {
>>> +        qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_COMPRESS);
>>> +        if (!flags) {
>>> +            qemu_put_byte(f, strlen(block->idstr));
>>> +            qemu_put_buffer(f, (uint8_t *)block->idstr,
>>> +                            strlen(block->idstr));
>>> +        }
>>> +        qemu_put_byte(f, *p);
>>> +        return 0;
>>> +    }
>>> +
>>> +    qemu_put_be64(f, offset | flags | RAM_SAVE_FLAG_PAGE);
>>> +    if (!flags) {
>>> +        qemu_put_byte(f, strlen(block->idstr));
>>> +        qemu_put_buffer(f, (uint8_t *)block->idstr,
>>> +                        strlen(block->idstr));
>>> +    }
>>> +    qemu_put_buffer(f, p, TARGET_PAGE_SIZE);
>>> +
>>> +    return TARGET_PAGE_SIZE;
>>> +}
>>> +
>>> +static size_t qemu_local_ram_save(QEMUFile *f, void *opaque,
>>> +                                  ram_addr_t block_offset, 
>>> ram_addr_t offset,
>>> +                                  size_t size, int *bytes_sent)
>>> +{
>>> +    QEMUFileLocal *s = opaque;
>>> +    uint64_t current_addr = block_offset + offset;
>>> +    RAMBlock *block = qemu_get_ram_block(current_addr);
>>> +    MemoryRegion *mr = block->mr;
>>
>> RAMBlock structs are not visible outside of exec.c and arch_init.c,
>> how did you do this?
>
> Hi Michael,
>
> Good catch!
> Actually this is the 'Known issue' that I listed in the cover letter, 
> and I
> planed to ask suggestions on this.
>
> Currently the implementation of override of the RDMA hook like 
> save_page for
> localhost migration needs to have knowledge of MemoryRegion and 
> RAMBlock, say,
> when saving ram page, need to know which RAMBlock the given ram 
> address save_page
> hook passed is belong to, or which is the last ram block has been 
> sent. But
> seems that such structs can not be exported to the private code in 
> migration-local.c.
>
> My guess for now is that there might be two possible ways to handle this:
>
> 1) Choose another way around, like representation of a RAMBlock and 
> MemoryRegion
>    from localhost migration perspective. This might need more work to 
> handle
>    the descriptions.
>
> 2) It could be exported directly into private code like migration-local.c
>    through some ways that I don't know yet. I remembered that the 
> implementation
>    of postcopy migration did this.
>
> So I'd like to post and ask for your suggestions to make sure it's the 
> right
> and best way to handle.
>
Hi Paolo,

Any suggestions on this?

Thanks

>
>


-- 
Lei

Re: [Qemu-devel] [PATCH 11/18] migration: introduce capability localhost

[Lei Li]

On 08/21/2013 11:08 PM, Eric Blake wrote:
> On 08/21/2013 01:18 AM, Lei Li wrote:
>> Introduce migration capability localhost.
>>
>> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com>
>> ---
>>   include/migration/migration.h |    3 +++
>>   migration.c                   |   12 ++++++++++++
>>   qapi-schema.json              |    8 +++++++-
>>   3 files changed, 22 insertions(+), 1 deletions(-)
>>
>> +++ b/qapi-schema.json
>> @@ -629,10 +629,16 @@
>>   # @auto-converge: If enabled, QEMU will automatically throttle down the guest
>>   #          to speed up convergence of RAM migration. (since 1.6)
>>   #
>> +# @localhost: If enabled, QEMU will support localhost migration. This feature
>> +#             allows live upgrade of a running QEMU instance by doing localhost
>> +#             migration with page flipping. It requires the source and destination
>> +#             are both on localhost. Disabled by default. (since 1.7)
> Logically, ALL new migration parameters should be disabled by default,
> as that provides the sanest back-compat setting.  Your last sentence is
> therefore redundant.  But this is certainly a much nicer implementation
> than your v1 attempt.
>
> If I understand correctly, attempts to use 'local:...' are rejected
> unless you first use migrate-set-capabilities to turn on localhost
> migration support.

Hi Eric,

The attempt to use 'local' will not be rejected if we don't use
migrate-set-capabilities first. Because the destination QEMU process
have no idea whether this capability is set in MigrationState. It will
just lead to a failure of migration connection.

According to Paolo's new suggestion in patch set 17/18, IIUIC, it will get
rid of this 'local' prefix, just use 'unix' protocol URI. In this case,
the incoming part for localhost migration will only be called if the
source support it by checking if the pipe can be got via SCM_RIGHTS.
  

>


-- 
Lei

Re: [Qemu-devel] [PATCH 08/18] migration-local: introduce qemu_fopen_local()

[Paolo Bonzini]

Il 28/08/2013 05:26, Lei Li ha scritto:
>>>
>>> RAMBlock structs are not visible outside of exec.c and arch_init.c,
>>> how did you do this?
>>
>> Hi Michael,
>>
>> Good catch!
>> Actually this is the 'Known issue' that I listed in the cover
>> letter, and I planed to ask suggestions on this.
>>
>> Currently the implementation of override of the RDMA hook like 
>> save_page for localhost migration needs to have knowledge of
>> MemoryRegion and RAMBlock, say, when saving ram page, need to know
>> which RAMBlock the given ram address save_page hook passed is
>> belong to, or which is the last ram block has been sent. But seems
>> that such structs can not be exported to the private code in 
>> migration-local.c.
>>
>> My guess for now is that there might be two possible ways to handle this:
>>
>> 1) Choose another way around, like representation of a RAMBlock and
>> MemoryRegion from localhost migration perspective. This might need
>> more work to handle the descriptions.
>>
>> 2) It could be exported directly into private code like migration-local.c
>> through some ways that I don't know yet. I remembered that the 
>> implementation of postcopy migration did this.
>>
>> So I'd like to post and ask for your suggestions to make sure it's the
>> right and best way to handle.
>
> Hi Paolo,
> 
> Any suggestions on this?

Unlike the RAM constants and migration states, I think exporting
MemoryRegion (not sure about RAMBlock) is fine.  In fact, replacing the
save_page hook's block_offset argument with a MemoryRegion would be a
nice cleanup.

Paolo

Re: [Qemu-devel] [PATCH 08/18] migration-local: introduce qemu_fopen_local()

[Lei Li]

On 08/28/2013 02:37 PM, Paolo Bonzini wrote:
> Il 28/08/2013 05:26, Lei Li ha scritto:
>>>> RAMBlock structs are not visible outside of exec.c and arch_init.c,
>>>> how did you do this?
>>> Hi Michael,
>>>
>>> Good catch!
>>> Actually this is the 'Known issue' that I listed in the cover
>>> letter, and I planed to ask suggestions on this.
>>>
>>> Currently the implementation of override of the RDMA hook like
>>> save_page for localhost migration needs to have knowledge of
>>> MemoryRegion and RAMBlock, say, when saving ram page, need to know
>>> which RAMBlock the given ram address save_page hook passed is
>>> belong to, or which is the last ram block has been sent. But seems
>>> that such structs can not be exported to the private code in
>>> migration-local.c.
>>>
>>> My guess for now is that there might be two possible ways to handle this:
>>>
>>> 1) Choose another way around, like representation of a RAMBlock and
>>> MemoryRegion from localhost migration perspective. This might need
>>> more work to handle the descriptions.
>>>
>>> 2) It could be exported directly into private code like migration-local.c
>>> through some ways that I don't know yet. I remembered that the
>>> implementation of postcopy migration did this.
>>>
>>> So I'd like to post and ask for your suggestions to make sure it's the
>>> right and best way to handle.
>> Hi Paolo,
>>
>> Any suggestions on this?
> Unlike the RAM constants and migration states, I think exporting
> MemoryRegion (not sure about RAMBlock) is fine.  In fact, replacing the
> save_page hook's block_offset argument with a MemoryRegion would be a
> nice cleanup.

I'll see if could find way to export RAMBlock as well. If not, will represent
it like RDMA did.
Replacing the block_offset argument of save_page with MemoryRegion might be a
good idea, I will give it a try.

Thanks for your suggestions!

>
> Paolo
>


-- 
Lei

Re: [Qemu-devel] [PATCH 08/18] migration-local: introduce qemu_fopen_local()

[Michael R. Hines]

On 08/28/2013 02:37 AM, Paolo Bonzini wrote:
> Il 28/08/2013 05:26, Lei Li ha scritto:
>>>> RAMBlock structs are not visible outside of exec.c and arch_init.c,
>>>> how did you do this?
>>> Hi Michael,
>>>
>>> Good catch!
>>> Actually this is the 'Known issue' that I listed in the cover
>>> letter, and I planed to ask suggestions on this.
>>>
>>> Currently the implementation of override of the RDMA hook like
>>> save_page for localhost migration needs to have knowledge of
>>> MemoryRegion and RAMBlock, say, when saving ram page, need to know
>>> which RAMBlock the given ram address save_page hook passed is
>>> belong to, or which is the last ram block has been sent. But seems
>>> that such structs can not be exported to the private code in
>>> migration-local.c.
>>>
>>> My guess for now is that there might be two possible ways to handle this:
>>>
>>> 1) Choose another way around, like representation of a RAMBlock and
>>> MemoryRegion from localhost migration perspective. This might need
>>> more work to handle the descriptions.
>>>
>>> 2) It could be exported directly into private code like migration-local.c
>>> through some ways that I don't know yet. I remembered that the
>>> implementation of postcopy migration did this.
>>>
>>> So I'd like to post and ask for your suggestions to make sure it's the
>>> right and best way to handle.
>> Hi Paolo,
>>
>> Any suggestions on this?
> Unlike the RAM constants and migration states, I think exporting
> MemoryRegion (not sure about RAMBlock) is fine.  In fact, replacing the
> save_page hook's block_offset argument with a MemoryRegion would be a
> nice cleanup.
>
> Paolo
>
Sounds like a good plan to me.

Once the localhost patch is fully reviewed - I'll submit a similar patch 
to make use of the exported MemoryRegion.

- Michael