[PATCH v2 0/6] multifd: Add colo support

[PATCH v2 0/6] multifd: Add colo support

[Lukas Straub]

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

Hello Everyone,
These patches add support for colo to multifd.

-v2:
 - Split out addition of p->block 
 - Add more comments

Lukas Straub (6):
  ram: Add public helper to set colo bitmap
  ram: Let colo_flush_ram_cache take the bitmap_mutex
  multifd: Introduce multifd-internal.h
  multifd: Introduce a overridable revc_pages method
  multifd: Add the ramblock to MultiFDRecvParams
  multifd: Add colo support

 migration/meson.build        |  1 +
 migration/multifd-colo.c     | 67 ++++++++++++++++++++++++++++++++
 migration/multifd-internal.h | 39 +++++++++++++++++++
 migration/multifd.c          | 74 +++++++++++++++++++++++-------------
 migration/multifd.h          |  2 +
 migration/ram.c              | 19 +++++++--
 migration/ram.h              |  1 +
 7 files changed, 173 insertions(+), 30 deletions(-)
 create mode 100644 migration/multifd-colo.c
 create mode 100644 migration/multifd-internal.h

-- 
2.39.2

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

[PATCH v2 1/6] ram: Add public helper to set colo bitmap

[Lukas Straub]

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

The overhead of the mutex in non-multifd mode is negligible,
because in that case its just the single thread taking the mutex.

This will be used in the next commits to add colo support to multifd.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 migration/ram.c | 17 ++++++++++++++---
 migration/ram.h |  1 +
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 5e7bf20ca5..2d3fd2112a 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -3633,6 +3633,18 @@ static ram_addr_t host_page_offset_from_ram_block_offset(RAMBlock *block,
     return ((uintptr_t)block->host + offset) & (block->page_size - 1);
 }
 
+void colo_record_bitmap(RAMBlock *block, ram_addr_t *normal, uint normal_num)
+{
+    qemu_mutex_lock(&ram_state->bitmap_mutex);
+    for (int i = 0; i < normal_num; i++) {
+        ram_addr_t offset = normal[i];
+        ram_state->migration_dirty_pages += !test_and_set_bit(
+                                                offset >> TARGET_PAGE_BITS,
+                                                block->bmap);
+    }
+    qemu_mutex_unlock(&ram_state->bitmap_mutex);
+}
+
 static inline void *colo_cache_from_block_offset(RAMBlock *block,
                              ram_addr_t offset, bool record_bitmap)
 {
@@ -3650,9 +3662,8 @@ static inline void *colo_cache_from_block_offset(RAMBlock *block,
     * It help us to decide which pages in ram cache should be flushed
     * into VM's RAM later.
     */
-    if (record_bitmap &&
-        !test_and_set_bit(offset >> TARGET_PAGE_BITS, block->bmap)) {
-        ram_state->migration_dirty_pages++;
+    if (record_bitmap) {
+        colo_record_bitmap(block, &offset, 1);
     }
     return block->colo_cache + offset;
 }
diff --git a/migration/ram.h b/migration/ram.h
index 6fffbeb5f1..887d1fbae6 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -82,6 +82,7 @@ int colo_init_ram_cache(void);
 void colo_flush_ram_cache(void);
 void colo_release_ram_cache(void);
 void colo_incoming_start_dirty_log(void);
+void colo_record_bitmap(RAMBlock *block, ram_addr_t *normal, uint normal_num);
 
 /* Background snapshot */
 bool ram_write_tracking_available(void);
-- 
2.39.2


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

Re: [PATCH v2 1/6] ram: Add public helper to set colo bitmap

[Juan Quintela]

Lukas Straub <lukasstraub2@web.de> wrote:
> The overhead of the mutex in non-multifd mode is negligible,
> because in that case its just the single thread taking the mutex.
>
> This will be used in the next commits to add colo support to multifd.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>

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

queued.

[PATCH v2 2/6] ram: Let colo_flush_ram_cache take the bitmap_mutex

[Lukas Straub]

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

This is not required, colo_flush_ram_cache does not run concurrently
with the multifd threads since the cache is only flushed after
everything has been received. But it makes me more comfortable.

This will be used in the next commits to add colo support to multifd.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 migration/ram.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/migration/ram.c b/migration/ram.c
index 2d3fd2112a..f9e7aeda12 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -4230,6 +4230,7 @@ void colo_flush_ram_cache(void)
     unsigned long offset = 0;
 
     memory_global_dirty_log_sync();
+    qemu_mutex_lock(&ram_state->bitmap_mutex);
     WITH_RCU_READ_LOCK_GUARD() {
         RAMBLOCK_FOREACH_NOT_IGNORED(block) {
             ramblock_sync_dirty_bitmap(ram_state, block);
@@ -4264,6 +4265,7 @@ void colo_flush_ram_cache(void)
             }
         }
     }
+    qemu_mutex_unlock(&ram_state->bitmap_mutex);
     trace_colo_flush_ram_cache_end();
 }
 
-- 
2.39.2


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

Re: [PATCH v2 2/6] ram: Let colo_flush_ram_cache take the bitmap_mutex

[Juan Quintela]

Lukas Straub <lukasstraub2@web.de> wrote:
> This is not required, colo_flush_ram_cache does not run concurrently
> with the multifd threads since the cache is only flushed after
> everything has been received. But it makes me more comfortable.
>
> This will be used in the next commits to add colo support to multifd.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>

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

[PATCH v2 3/6] multifd: Introduce multifd-internal.h

[Lukas Straub]

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

Introduce multifd-internal.h so code that would normally go into
multifd.c can go into an extra file. This way, multifd.c hopefully
won't grow to 4000 lines like ram.c

This will be used in the next commits to add colo support to multifd.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 migration/multifd-internal.h | 34 ++++++++++++++++++++++++++++++++++
 migration/multifd.c          | 15 ++++-----------
 2 files changed, 38 insertions(+), 11 deletions(-)
 create mode 100644 migration/multifd-internal.h

diff --git a/migration/multifd-internal.h b/migration/multifd-internal.h
new file mode 100644
index 0000000000..6eeaa028e7
--- /dev/null
+++ b/migration/multifd-internal.h
@@ -0,0 +1,34 @@
+/*
+ * Internal Multifd header
+ *
+ * Copyright (c) 2019-2020 Red Hat Inc
+ *
+ * Authors:
+ *  Juan Quintela <quintela@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifdef QEMU_MIGRATION_MULTIFD_INTERNAL_H
+#error Only include this header directly
+#endif
+#define QEMU_MIGRATION_MULTIFD_INTERNAL_H
+
+#ifndef MULTIFD_INTERNAL
+#error This header is internal to multifd
+#endif
+
+struct MultiFDRecvState {
+    MultiFDRecvParams *params;
+    /* number of created threads */
+    int count;
+    /* syncs main thread and channels */
+    QemuSemaphore sem_sync;
+    /* global number of generated multifd packets */
+    uint64_t packet_num;
+    /* multifd ops */
+    MultiFDMethods *ops;
+};
+
+extern struct MultiFDRecvState *multifd_recv_state;
diff --git a/migration/multifd.c b/migration/multifd.c
index 4e71c19292..f6bad69b6c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -31,6 +31,9 @@
 #include "io/channel-socket.h"
 #include "yank_functions.h"
 
+#define MULTIFD_INTERNAL
+#include "multifd-internal.h"
+
 /* Multiple fd's */
 
 #define MULTIFD_MAGIC 0x11223344U
@@ -967,17 +970,7 @@ int multifd_save_setup(Error **errp)
     return 0;
 }
 
-struct {
-    MultiFDRecvParams *params;
-    /* number of created threads */
-    int count;
-    /* syncs main thread and channels */
-    QemuSemaphore sem_sync;
-    /* global number of generated multifd packets */
-    uint64_t packet_num;
-    /* multifd ops */
-    MultiFDMethods *ops;
-} *multifd_recv_state;
+struct MultiFDRecvState *multifd_recv_state;
 
 static void multifd_recv_terminate_threads(Error *err)
 {
-- 
2.39.2


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

[PATCH v2 4/6] multifd: Introduce a overridable revc_pages method

[Lukas Straub]

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

This allows to override the behaviour around recv_pages. Think of
it like a "multifd_colo" child class of multifd.

This will be used in the next commits to add colo support to multifd.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 migration/meson.build        |  1 +
 migration/multifd-colo.c     | 39 +++++++++++++++++++++++++++++
 migration/multifd-internal.h |  5 ++++
 migration/multifd.c          | 48 ++++++++++++++++++++++++++++--------
 4 files changed, 83 insertions(+), 10 deletions(-)
 create mode 100644 migration/multifd-colo.c

diff --git a/migration/meson.build b/migration/meson.build
index da1897fadf..22ab6c6d73 100644
--- a/migration/meson.build
+++ b/migration/meson.build
@@ -23,6 +23,7 @@ softmmu_ss.add(files(
   'migration.c',
   'multifd.c',
   'multifd-zlib.c',
+  'multifd-colo.c',
   'options.c',
   'postcopy-ram.c',
   'savevm.c',
diff --git a/migration/multifd-colo.c b/migration/multifd-colo.c
new file mode 100644
index 0000000000..c035d15e87
--- /dev/null
+++ b/migration/multifd-colo.c
@@ -0,0 +1,39 @@
+/*
+ * multifd colo implementation
+ *
+ * Copyright (c) Lukas Straub <lukasstraub2@web.de>
+ *
+ * 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 "qemu/osdep.h"
+#include "exec/target_page.h"
+#include "exec/ramblock.h"
+#include "qemu/error-report.h"
+#include "qapi/error.h"
+#include "ram.h"
+#include "multifd.h"
+#include "io/channel-socket.h"
+
+#define MULTIFD_INTERNAL
+#include "multifd-internal.h"
+
+static int multifd_colo_recv_pages(MultiFDRecvParams *p, Error **errp)
+{
+    return multifd_recv_state->ops->recv_pages(p, errp);
+}
+
+int multifd_colo_load_setup(Error **errp)
+{
+    int ret;
+
+    ret = _multifd_load_setup(errp);
+    if (ret) {
+        return ret;
+    }
+
+    multifd_recv_state->recv_pages = multifd_colo_recv_pages;
+
+    return 0;
+}
diff --git a/migration/multifd-internal.h b/migration/multifd-internal.h
index 6eeaa028e7..82357f1d88 100644
--- a/migration/multifd-internal.h
+++ b/migration/multifd-internal.h
@@ -29,6 +29,11 @@ struct MultiFDRecvState {
     uint64_t packet_num;
     /* multifd ops */
     MultiFDMethods *ops;
+    /* overridable recv method */
+    int (*recv_pages)(MultiFDRecvParams *p, Error **errp);
 };
 
 extern struct MultiFDRecvState *multifd_recv_state;
+
+int _multifd_load_setup(Error **errp);
+int multifd_colo_load_setup(Error **errp);
diff --git a/migration/multifd.c b/migration/multifd.c
index f6bad69b6c..fb5e8859de 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1126,7 +1126,7 @@ static void *multifd_recv_thread(void *opaque)
         qemu_mutex_unlock(&p->mutex);
 
         if (p->normal_num) {
-            ret = multifd_recv_state->ops->recv_pages(p, &local_err);
+            ret = multifd_recv_state->recv_pages(p, &local_err);
             if (ret != 0) {
                 break;
             }
@@ -1152,20 +1152,12 @@ static void *multifd_recv_thread(void *opaque)
     return NULL;
 }
 
-int multifd_load_setup(Error **errp)
+int _multifd_load_setup(Error **errp)
 {
     int thread_count;
     uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size();
     uint8_t i;
 
-    /*
-     * Return successfully if multiFD recv state is already initialised
-     * or multiFD is not enabled.
-     */
-    if (multifd_recv_state || !migrate_multifd()) {
-        return 0;
-    }
-
     thread_count = migrate_multifd_channels();
     multifd_recv_state = g_malloc0(sizeof(*multifd_recv_state));
     multifd_recv_state->params = g_new0(MultiFDRecvParams, thread_count);
@@ -1204,6 +1196,42 @@ int multifd_load_setup(Error **errp)
     return 0;
 }
 
+static int multifd_normal_recv_pages(MultiFDRecvParams *p, Error **errp)
+{
+    return multifd_recv_state->ops->recv_pages(p, errp);
+}
+
+static int multifd_normal_load_setup(Error **errp)
+{
+    int ret;
+
+    ret = _multifd_load_setup(errp);
+    if (ret) {
+        return ret;
+    }
+
+    multifd_recv_state->recv_pages = multifd_normal_recv_pages;
+
+    return 0;
+}
+
+int multifd_load_setup(Error **errp)
+{
+    /*
+     * Return successfully if multiFD recv state is already initialised
+     * or multiFD is not enabled.
+     */
+    if (multifd_recv_state || !migrate_multifd()) {
+        return 0;
+    }
+
+    if (migrate_colo()) {
+        return multifd_colo_load_setup(errp);
+    } else {
+        return multifd_normal_load_setup(errp);
+    }
+}
+
 bool multifd_recv_all_channels_created(void)
 {
     int thread_count = migrate_multifd_channels();
-- 
2.39.2


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

[PATCH v2 5/6] multifd: Add the ramblock to MultiFDRecvParams

[Lukas Straub]

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

This will be used in the next commits to add colo support to multifd.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 migration/multifd.c | 11 +++++------
 migration/multifd.h |  2 ++
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index fb5e8859de..fddbf86596 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -284,7 +284,6 @@ static void multifd_send_fill_packet(MultiFDSendParams *p)
 static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
 {
     MultiFDPacket_t *packet = p->packet;
-    RAMBlock *block;
     int i;
 
     packet->magic = be32_to_cpu(packet->magic);
@@ -334,21 +333,21 @@ static int multifd_recv_unfill_packet(MultiFDRecvParams *p, Error **errp)
 
     /* make sure that ramblock is 0 terminated */
     packet->ramblock[255] = 0;
-    block = qemu_ram_block_by_name(packet->ramblock);
-    if (!block) {
+    p->block = qemu_ram_block_by_name(packet->ramblock);
+    if (!p->block) {
         error_setg(errp, "multifd: unknown ram block %s",
                    packet->ramblock);
         return -1;
     }
 
-    p->host = block->host;
+    p->host = p->block->host;
     for (i = 0; i < p->normal_num; i++) {
         uint64_t offset = be64_to_cpu(packet->offset[i]);
 
-        if (offset > (block->used_length - p->page_size)) {
+        if (offset > (p->block->used_length - p->page_size)) {
             error_setg(errp, "multifd: offset too long %" PRIu64
                        " (max " RAM_ADDR_FMT ")",
-                       offset, block->used_length);
+                       offset, p->block->used_length);
             return -1;
         }
         p->normal[i] = offset;
diff --git a/migration/multifd.h b/migration/multifd.h
index 7cfc265148..a835643b48 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -175,6 +175,8 @@ typedef struct {
     uint32_t next_packet_size;
     /* packets sent through this channel */
     uint64_t num_packets;
+    /* ramblock */
+    RAMBlock *block;
     /* ramblock host address */
     uint8_t *host;
     /* non zero pages recv through this channel */
-- 
2.39.2


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

Re: [PATCH v2 5/6] multifd: Add the ramblock to MultiFDRecvParams

[Juan Quintela]

Lukas Straub <lukasstraub2@web.de> wrote:
> This will be used in the next commits to add colo support to multifd.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>

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

queued.

[PATCH v2 6/6] multifd: Add colo support

[Lukas Straub]

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

Like in the normal ram_load() path, put the received pages into the
colo cache and mark the pages in the bitmap so that they will be
flushed to the guest later.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 migration/multifd-colo.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/migration/multifd-colo.c b/migration/multifd-colo.c
index c035d15e87..305a1b7000 100644
--- a/migration/multifd-colo.c
+++ b/migration/multifd-colo.c
@@ -15,13 +15,41 @@
 #include "ram.h"
 #include "multifd.h"
 #include "io/channel-socket.h"
+#include "migration/colo.h"
 
 #define MULTIFD_INTERNAL
 #include "multifd-internal.h"
 
 static int multifd_colo_recv_pages(MultiFDRecvParams *p, Error **errp)
 {
-    return multifd_recv_state->ops->recv_pages(p, errp);
+    int ret = 0;
+
+    /*
+     * While we're still in precopy mode, we copy received pages to both guest
+     * and cache. No need to set dirty bits, since guest and cache memory are
+     * in sync.
+     */
+    if (migration_incoming_in_colo_state()) {
+        colo_record_bitmap(p->block, p->normal, p->normal_num);
+    }
+
+    p->host = p->block->colo_cache;
+    ret = multifd_recv_state->ops->recv_pages(p, errp);
+    if (ret != 0) {
+        p->host = p->block->host;
+        return ret;
+    }
+
+    if (!migration_incoming_in_colo_state()) {
+        for (int i = 0; i < p->normal_num; i++) {
+            void *guest = p->block->host + p->normal[i];
+            void *cache = p->host + p->normal[i];
+            memcpy(guest, cache, p->page_size);
+        }
+    }
+
+    p->host = p->block->host;
+    return ret;
 }
 
 int multifd_colo_load_setup(Error **errp)
-- 
2.39.2

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