[PATCH v4 0/6] colo-compare bugfixes

[PATCH v4 0/6] colo-compare bugfixes

[Lukas Straub]

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

Hello Everyone,
Here are fixes for bugs that I found in my tests.

Benchmark results:
Client-to-server tcp:
without patch: ~63 Mbit/s
with patch: ~66 Mbit/s
Server-to-client tcp:
without patch: ~771 Kbit/s
with patch: ~702 Kbit/s

Regards,
Lukas Straub

Version changes:
v4:
 -fix potential deadlock with notify_remote_frame
 -avoid malloc and memcpy in many cases

v3:
 -fix checkpatch.pl error

v2:
 -better wording
 -fix performance-regression in patch 3 "net/colo-compare.c: Fix deadlock in compare_chr_send"
 -add more bugfixes


Lukas Straub (6):
  net/colo-compare.c: Create event_bh with the right AioContext
  chardev/char.c: Use qemu_co_sleep_ns if in coroutine
  net/colo-compare.c: Fix deadlock in compare_chr_send
  net/colo-compare.c: Only hexdump packets if tracing is enabled
  net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active
  net/colo-compare.c: Correct ordering in complete and finalize

 chardev/char.c     |   7 +-
 net/colo-compare.c | 250 +++++++++++++++++++++++++++++++++------------
 net/colo-compare.h |   1 +
 net/colo.c         |   7 ++
 net/colo.h         |   1 +
 softmmu/vl.c       |   2 +
 6 files changed, 204 insertions(+), 64 deletions(-)

-- 
2.20.1

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

[PATCH v4 1/6] net/colo-compare.c: Create event_bh with the right AioContext

[Lukas Straub]

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

qemu_bh_new will set the bh to be executed in the main
loop. This causes crashes as colo_compare_handle_event assumes
that it has exclusive access the queues, which are also
concurrently accessed in the iothread.

Create the bh with the AioContext of the iothread to fulfill
these assumptions and fix the crashes. This is safe, because
the bh already takes the appropriate locks.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Zhang Chen <chen.zhang@intel.com>
Reviewed-by: Derek Su <dereksu@qnap.com>
Tested-by: Derek Su <dereksu@qnap.com>
---
 net/colo-compare.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 10c0239f9d..1de4220fe2 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -890,6 +890,7 @@ static void colo_compare_handle_event(void *opaque)
 
 static void colo_compare_iothread(CompareState *s)
 {
+    AioContext *ctx = iothread_get_aio_context(s->iothread);
     object_ref(OBJECT(s->iothread));
     s->worker_context = iothread_get_g_main_context(s->iothread);
 
@@ -906,7 +907,7 @@ static void colo_compare_iothread(CompareState *s)
     }
 
     colo_compare_timer_init(s);
-    s->event_bh = qemu_bh_new(colo_compare_handle_event, s);
+    s->event_bh = aio_bh_new(ctx, colo_compare_handle_event, s);
 }
 
 static char *compare_get_pri_indev(Object *obj, Error **errp)
-- 
2.20.1


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

[PATCH v4 2/6] chardev/char.c: Use qemu_co_sleep_ns if in coroutine

[Lukas Straub]

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

This will be needed in the next patch.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Zhang Chen <chen.zhang@intel.com>
---
 chardev/char.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/chardev/char.c b/chardev/char.c
index e77564060d..5c8014199f 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -38,6 +38,7 @@
 #include "qemu/module.h"
 #include "qemu/option.h"
 #include "qemu/id.h"
+#include "qemu/coroutine.h"
 
 #include "chardev/char-mux.h"
 
@@ -119,7 +120,11 @@ static int qemu_chr_write_buffer(Chardev *s,
     retry:
         res = cc->chr_write(s, buf + *offset, len - *offset);
         if (res < 0 && errno == EAGAIN && write_all) {
-            g_usleep(100);
+            if (qemu_in_coroutine()) {
+                qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
+            } else {
+                g_usleep(100);
+            }
             goto retry;
         }
 
-- 
2.20.1


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

[PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Lukas Straub]

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

The chr_out chardev is connected to a filter-redirector
running in the main loop. qemu_chr_fe_write_all might block
here in compare_chr_send if the (socket-)buffer is full.
If another filter-redirector in the main loop want's to
send data to chr_pri_in it might also block if the buffer
is full. This leads to a deadlock because both event loops
get blocked.

Fix this by converting compare_chr_send to a coroutine and
putting the packets in a send queue.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 net/colo-compare.c | 187 ++++++++++++++++++++++++++++++++++-----------
 net/colo.c         |   7 ++
 net/colo.h         |   1 +
 3 files changed, 150 insertions(+), 45 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 1de4220fe2..2a4e7f7c4e 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -32,6 +32,9 @@
 #include "migration/migration.h"
 #include "util.h"
 
+#include "block/aio-wait.h"
+#include "qemu/coroutine.h"
+
 #define TYPE_COLO_COMPARE "colo-compare"
 #define COLO_COMPARE(obj) \
     OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE)
@@ -77,6 +80,23 @@ static int event_unhandled_count;
  *                    |packet  |  |packet  +    |packet  | |packet  +
  *                    +--------+  +--------+    +--------+ +--------+
  */
+
+typedef struct SendCo {
+    Coroutine *co;
+    struct CompareState *s;
+    CharBackend *chr;
+    GQueue send_list;
+    bool notify_remote_frame;
+    bool done;
+    int ret;
+} SendCo;
+
+typedef struct SendEntry {
+    uint32_t size;
+    uint32_t vnet_hdr_len;
+    uint8_t *buf;
+} SendEntry;
+
 typedef struct CompareState {
     Object parent;
 
@@ -91,6 +111,8 @@ typedef struct CompareState {
     SocketReadState pri_rs;
     SocketReadState sec_rs;
     SocketReadState notify_rs;
+    SendCo out_sendco;
+    SendCo notify_sendco;
     bool vnet_hdr;
     uint32_t compare_timeout;
     uint32_t expired_scan_cycle;
@@ -124,10 +146,11 @@ enum {
 
 
 static int compare_chr_send(CompareState *s,
-                            const uint8_t *buf,
+                            uint8_t *buf,
                             uint32_t size,
                             uint32_t vnet_hdr_len,
-                            bool notify_remote_frame);
+                            bool notify_remote_frame,
+                            bool zero_copy);
 
 static bool packet_matches_str(const char *str,
                                const uint8_t *buf,
@@ -145,7 +168,7 @@ static void notify_remote_frame(CompareState *s)
     char msg[] = "DO_CHECKPOINT";
     int ret = 0;
 
-    ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
+    ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true, false);
     if (ret < 0) {
         error_report("Notify Xen COLO-frame failed");
     }
@@ -272,12 +295,13 @@ static void colo_release_primary_pkt(CompareState *s, Packet *pkt)
                            pkt->data,
                            pkt->size,
                            pkt->vnet_hdr_len,
-                           false);
+                           false,
+                           true);
     if (ret < 0) {
         error_report("colo send primary packet failed");
     }
     trace_colo_compare_main("packet same and release packet");
-    packet_destroy(pkt, NULL);
+    packet_destroy_partial(pkt, NULL);
 }
 
 /*
@@ -699,65 +723,115 @@ static void colo_compare_connection(void *opaque, void *user_data)
     }
 }
 
-static int compare_chr_send(CompareState *s,
-                            const uint8_t *buf,
-                            uint32_t size,
-                            uint32_t vnet_hdr_len,
-                            bool notify_remote_frame)
+static void coroutine_fn _compare_chr_send(void *opaque)
 {
+    SendCo *sendco = opaque;
+    CompareState *s = sendco->s;
     int ret = 0;
-    uint32_t len = htonl(size);
 
-    if (!size) {
-        return 0;
-    }
+    while (!g_queue_is_empty(&sendco->send_list)) {
+        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
+        uint32_t len = htonl(entry->size);
 
-    if (notify_remote_frame) {
-        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
-                                    (uint8_t *)&len,
-                                    sizeof(len));
-    } else {
-        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
-    }
+        ret = qemu_chr_fe_write_all(sendco->chr, (uint8_t *)&len, sizeof(len));
 
-    if (ret != sizeof(len)) {
-        goto err;
-    }
+        if (ret != sizeof(len)) {
+            g_free(entry->buf);
+            g_slice_free(SendEntry, entry);
+            goto err;
+        }
 
-    if (s->vnet_hdr) {
-        /*
-         * We send vnet header len make other module(like filter-redirector)
-         * know how to parse net packet correctly.
-         */
-        len = htonl(vnet_hdr_len);
+        if (!sendco->notify_remote_frame && s->vnet_hdr) {
+            /*
+             * We send vnet header len make other module(like filter-redirector)
+             * know how to parse net packet correctly.
+             */
+            len = htonl(entry->vnet_hdr_len);
 
-        if (!notify_remote_frame) {
-            ret = qemu_chr_fe_write_all(&s->chr_out,
+            ret = qemu_chr_fe_write_all(sendco->chr,
                                         (uint8_t *)&len,
                                         sizeof(len));
+
+            if (ret != sizeof(len)) {
+                g_free(entry->buf);
+                g_slice_free(SendEntry, entry);
+                goto err;
+            }
         }
 
-        if (ret != sizeof(len)) {
+        ret = qemu_chr_fe_write_all(sendco->chr,
+                                    (uint8_t *)entry->buf,
+                                    entry->size);
+
+        if (ret != entry->size) {
+            g_free(entry->buf);
+            g_slice_free(SendEntry, entry);
             goto err;
         }
+
+        g_free(entry->buf);
+        g_slice_free(SendEntry, entry);
     }
 
+    sendco->ret = 0;
+    goto out;
+
+err:
+    while (!g_queue_is_empty(&sendco->send_list)) {
+        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
+        g_free(entry->buf);
+        g_slice_free(SendEntry, entry);
+    }
+    sendco->ret = ret < 0 ? ret : -EIO;
+out:
+    sendco->co = NULL;
+    sendco->done = true;
+    aio_wait_kick();
+}
+
+static int compare_chr_send(CompareState *s,
+                            uint8_t *buf,
+                            uint32_t size,
+                            uint32_t vnet_hdr_len,
+                            bool notify_remote_frame,
+                            bool zero_copy)
+{
+    SendCo *sendco;
+    SendEntry *entry;
+
     if (notify_remote_frame) {
-        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
-                                    (uint8_t *)buf,
-                                    size);
+        sendco = &s->notify_sendco;
     } else {
-        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
+        sendco = &s->out_sendco;
     }
 
-    if (ret != size) {
-        goto err;
+    if (!size) {
+        return 0;
     }
 
-    return 0;
+    entry = g_slice_new(SendEntry);
+    entry->size = size;
+    entry->vnet_hdr_len = vnet_hdr_len;
+    if (zero_copy) {
+        entry->buf = buf;
+    } else {
+        entry->buf = g_malloc(size);
+        memcpy(entry->buf, buf, size);
+    }
+    g_queue_push_head(&sendco->send_list, entry);
+
+    if (sendco->done) {
+        sendco->co = qemu_coroutine_create(_compare_chr_send, sendco);
+        sendco->done = false;
+        qemu_coroutine_enter(sendco->co);
+        if (sendco->done) {
+            /* report early errors */
+            return sendco->ret;
+        }
+    }
 
-err:
-    return ret < 0 ? ret : -EIO;
+    /* assume success */
+    return 0;
 }
 
 static int compare_chr_can_read(void *opaque)
@@ -1063,6 +1137,7 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
                          pri_rs->buf,
                          pri_rs->packet_len,
                          pri_rs->vnet_hdr_len,
+                         false,
                          false);
     } else {
         /* compare packet in the specified connection */
@@ -1093,7 +1168,7 @@ static void compare_notify_rs_finalize(SocketReadState *notify_rs)
     if (packet_matches_str("COLO_USERSPACE_PROXY_INIT",
                            notify_rs->buf,
                            notify_rs->packet_len)) {
-        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
+        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true, false);
         if (ret < 0) {
             error_report("Notify Xen COLO-frame INIT failed");
         }
@@ -1199,6 +1274,18 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
 
     QTAILQ_INSERT_TAIL(&net_compares, s, next);
 
+    s->out_sendco.s = s;
+    s->out_sendco.chr = &s->chr_out;
+    s->out_sendco.notify_remote_frame = false;
+    s->out_sendco.done = true;
+    g_queue_init(&s->out_sendco.send_list);
+
+    s->notify_sendco.s = s;
+    s->notify_sendco.chr = &s->chr_notify_dev;
+    s->notify_sendco.notify_remote_frame = true;
+    s->notify_sendco.done = true;
+    g_queue_init(&s->notify_sendco.send_list);
+
     g_queue_init(&s->conn_list);
 
     qemu_mutex_init(&event_mtx);
@@ -1225,8 +1312,9 @@ static void colo_flush_packets(void *opaque, void *user_data)
                          pkt->data,
                          pkt->size,
                          pkt->vnet_hdr_len,
-                         false);
-        packet_destroy(pkt, NULL);
+                         false,
+                         true);
+        packet_destroy_partial(pkt, NULL);
     }
     while (!g_queue_is_empty(&conn->secondary_list)) {
         pkt = g_queue_pop_head(&conn->secondary_list);
@@ -1301,10 +1389,19 @@ static void colo_compare_finalize(Object *obj)
         }
     }
 
+    AioContext *ctx = iothread_get_aio_context(s->iothread);
+    aio_context_acquire(ctx);
+    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
+    AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
+    aio_context_release(ctx);
+
     /* Release all unhandled packets after compare thead exited */
     g_queue_foreach(&s->conn_list, colo_flush_packets, s);
+    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
 
     g_queue_clear(&s->conn_list);
+    g_queue_clear(&s->out_sendco.send_list);
+    g_queue_clear(&s->notify_sendco.send_list);
 
     if (s->connection_track_table) {
         g_hash_table_destroy(s->connection_track_table);
diff --git a/net/colo.c b/net/colo.c
index 8196b35837..a6c66d829a 100644
--- a/net/colo.c
+++ b/net/colo.c
@@ -185,6 +185,13 @@ void packet_destroy(void *opaque, void *user_data)
     g_slice_free(Packet, pkt);
 }
 
+void packet_destroy_partial(void *opaque, void *user_data)
+{
+    Packet *pkt = opaque;
+
+    g_slice_free(Packet, pkt);
+}
+
 /*
  * Clear hashtable, stop this hash growing really huge
  */
diff --git a/net/colo.h b/net/colo.h
index 679314b1ca..573ab91785 100644
--- a/net/colo.h
+++ b/net/colo.h
@@ -102,5 +102,6 @@ bool connection_has_tracked(GHashTable *connection_track_table,
 void connection_hashtable_reset(GHashTable *connection_track_table);
 Packet *packet_new(const void *data, int size, int vnet_hdr_len);
 void packet_destroy(void *opaque, void *user_data);
+void packet_destroy_partial(void *opaque, void *user_data);
 
 #endif /* NET_COLO_H */
-- 
2.20.1


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

RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Monday, May 4, 2020 6:28 PM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian 
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc- 
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini 
> <pbonzini@redhat.com>
> Subject: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in 
> compare_chr_send
> 
> The chr_out chardev is connected to a filter-redirector running in the 
> main loop. qemu_chr_fe_write_all might block here in compare_chr_send 
> if the (socket-)buffer is full.
> If another filter-redirector in the main loop want's to send data to 
> chr_pri_in it might also block if the buffer is full. This leads to a 
> deadlock because both event loops get blocked.
> 
> Fix this by converting compare_chr_send to a coroutine and putting the 
> packets in a send queue.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 187 ++++++++++++++++++++++++++++++++++-------
> ----
>  net/colo.c         |   7 ++
>  net/colo.h         |   1 +
>  3 files changed, 150 insertions(+), 45 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index 
> 1de4220fe2..2a4e7f7c4e 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -32,6 +32,9 @@
>  #include "migration/migration.h"
>  #include "util.h"
> 
> +#include "block/aio-wait.h"
> +#include "qemu/coroutine.h"
> +
>  #define TYPE_COLO_COMPARE "colo-compare"
>  #define COLO_COMPARE(obj) \
>      OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -77,6
> +80,23 @@ static int event_unhandled_count;
>   *                    |packet  |  |packet  +    |packet  | |packet  +
>   *                    +--------+  +--------+    +--------+ +--------+
>   */
> +
> +typedef struct SendCo {
> +    Coroutine *co;
> +    struct CompareState *s;
> +    CharBackend *chr;
> +    GQueue send_list;
> +    bool notify_remote_frame;
> +    bool done;
> +    int ret;
> +} SendCo;
> +
> +typedef struct SendEntry {
> +    uint32_t size;
> +    uint32_t vnet_hdr_len;
> +    uint8_t *buf;
> +} SendEntry;
> +
>  typedef struct CompareState {
>      Object parent;
> 
> @@ -91,6 +111,8 @@ typedef struct CompareState {
>      SocketReadState pri_rs;
>      SocketReadState sec_rs;
>      SocketReadState notify_rs;
> +    SendCo out_sendco;
> +    SendCo notify_sendco;
>      bool vnet_hdr;
>      uint32_t compare_timeout;
>      uint32_t expired_scan_cycle;
> @@ -124,10 +146,11 @@ enum {
> 
> 
>  static int compare_chr_send(CompareState *s,
> -                            const uint8_t *buf,
> +                            uint8_t *buf,
>                              uint32_t size,
>                              uint32_t vnet_hdr_len,
> -                            bool notify_remote_frame);
> +                            bool notify_remote_frame,
> +                            bool zero_copy);
> 
>  static bool packet_matches_str(const char *str,
>                                 const uint8_t *buf, @@ -145,7 +168,7 
> @@ static void notify_remote_frame(CompareState *s)
>      char msg[] = "DO_CHECKPOINT";
>      int ret = 0;
> 
> -    ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> +    ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true, 
> + false);
>      if (ret < 0) {
>          error_report("Notify Xen COLO-frame failed");
>      }
> @@ -272,12 +295,13 @@ static void
> colo_release_primary_pkt(CompareState *s, Packet *pkt)
>                             pkt->data,
>                             pkt->size,
>                             pkt->vnet_hdr_len,
> -                           false);
> +                           false,
> +                           true);
>      if (ret < 0) {
>          error_report("colo send primary packet failed");
>      }
>      trace_colo_compare_main("packet same and release packet");
> -    packet_destroy(pkt, NULL);
> +    packet_destroy_partial(pkt, NULL);
>  }
> 
>  /*
> @@ -699,65 +723,115 @@ static void colo_compare_connection(void 
> *opaque, void *user_data)
>      }
>  }
> 
> -static int compare_chr_send(CompareState *s,
> -                            const uint8_t *buf,
> -                            uint32_t size,
> -                            uint32_t vnet_hdr_len,
> -                            bool notify_remote_frame)
> +static void coroutine_fn _compare_chr_send(void *opaque)
>  {
> +    SendCo *sendco = opaque;
> +    CompareState *s = sendco->s;
>      int ret = 0;
> -    uint32_t len = htonl(size);
> 
> -    if (!size) {
> -        return 0;
> -    }
> +    while (!g_queue_is_empty(&sendco->send_list)) {
> +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> +        uint32_t len = htonl(entry->size);
> 
> -    if (notify_remote_frame) {
> -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> -                                    (uint8_t *)&len,
> -                                    sizeof(len));
> -    } else {
> -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> -    }
> +        ret = qemu_chr_fe_write_all(sendco->chr, (uint8_t *)&len, 
> + sizeof(len));
> 
> -    if (ret != sizeof(len)) {
> -        goto err;
> -    }
> +        if (ret != sizeof(len)) {
> +            g_free(entry->buf);
> +            g_slice_free(SendEntry, entry);
> +            goto err;
> +        }
> 
> -    if (s->vnet_hdr) {
> -        /*
> -         * We send vnet header len make other module(like filter-redirector)
> -         * know how to parse net packet correctly.
> -         */
> -        len = htonl(vnet_hdr_len);
> +        if (!sendco->notify_remote_frame && s->vnet_hdr) {
> +            /*
> +             * We send vnet header len make other module(like filter-redirector)
> +             * know how to parse net packet correctly.
> +             */
> +            len = htonl(entry->vnet_hdr_len);
> 
> -        if (!notify_remote_frame) {
> -            ret = qemu_chr_fe_write_all(&s->chr_out,
> +            ret = qemu_chr_fe_write_all(sendco->chr,
>                                          (uint8_t *)&len,
>                                          sizeof(len));
> +
> +            if (ret != sizeof(len)) {
> +                g_free(entry->buf);
> +                g_slice_free(SendEntry, entry);
> +                goto err;
> +            }
>          }
> 
> -        if (ret != sizeof(len)) {
> +        ret = qemu_chr_fe_write_all(sendco->chr,
> +                                    (uint8_t *)entry->buf,
> +                                    entry->size);
> +
> +        if (ret != entry->size) {
> +            g_free(entry->buf);
> +            g_slice_free(SendEntry, entry);
>              goto err;
>          }
> +
> +        g_free(entry->buf);
> +        g_slice_free(SendEntry, entry);
>      }
> 
> +    sendco->ret = 0;
> +    goto out;
> +
> +err:
> +    while (!g_queue_is_empty(&sendco->send_list)) {
> +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> +        g_free(entry->buf);
> +        g_slice_free(SendEntry, entry);
> +    }
> +    sendco->ret = ret < 0 ? ret : -EIO;
> +out:
> +    sendco->co = NULL;
> +    sendco->done = true;
> +    aio_wait_kick();
> +}
> +
> +static int compare_chr_send(CompareState *s,
> +                            uint8_t *buf,
> +                            uint32_t size,
> +                            uint32_t vnet_hdr_len,
> +                            bool notify_remote_frame,
> +                            bool zero_copy) {
> +    SendCo *sendco;
> +    SendEntry *entry;
> +
>      if (notify_remote_frame) {
> -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> -                                    (uint8_t *)buf,
> -                                    size);
> +        sendco = &s->notify_sendco;
>      } else {
> -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> +        sendco = &s->out_sendco;
>      }
> 
> -    if (ret != size) {
> -        goto err;
> +    if (!size) {
> +        return 0;
>      }
> 
> -    return 0;
> +    entry = g_slice_new(SendEntry);
> +    entry->size = size;
> +    entry->vnet_hdr_len = vnet_hdr_len;
> +    if (zero_copy) {
> +        entry->buf = buf;
> +    } else {
> +        entry->buf = g_malloc(size);
> +        memcpy(entry->buf, buf, size);
> +    }
> +    g_queue_push_head(&sendco->send_list, entry);
> +
> +    if (sendco->done) {
> +        sendco->co = qemu_coroutine_create(_compare_chr_send, sendco);
> +        sendco->done = false;
> +        qemu_coroutine_enter(sendco->co);
> +        if (sendco->done) {
> +            /* report early errors */
> +            return sendco->ret;
> +        }
> +    }
> 
> -err:
> -    return ret < 0 ? ret : -EIO;
> +    /* assume success */
> +    return 0;
>  }
> 
>  static int compare_chr_can_read(void *opaque) @@ -1063,6 +1137,7 @@ 
> static void compare_pri_rs_finalize(SocketReadState *pri_rs)
>                           pri_rs->buf,
>                           pri_rs->packet_len,
>                           pri_rs->vnet_hdr_len,
> +                         false,
>                           false);
>      } else {
>          /* compare packet in the specified connection */ @@ -1093,7
> +1168,7 @@ static void compare_notify_rs_finalize(SocketReadState
> +*notify_rs)
>      if (packet_matches_str("COLO_USERSPACE_PROXY_INIT",
>                             notify_rs->buf,
>                             notify_rs->packet_len)) {
> -        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> +        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, 
> + true, false);
>          if (ret < 0) {
>              error_report("Notify Xen COLO-frame INIT failed");
>          }
> @@ -1199,6 +1274,18 @@ static void
> colo_compare_complete(UserCreatable *uc, Error **errp)
> 
>      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> 
> +    s->out_sendco.s = s;
> +    s->out_sendco.chr = &s->chr_out;
> +    s->out_sendco.notify_remote_frame = false;
> +    s->out_sendco.done = true;
> +    g_queue_init(&s->out_sendco.send_list);
> +
> +    s->notify_sendco.s = s;
> +    s->notify_sendco.chr = &s->chr_notify_dev;
> +    s->notify_sendco.notify_remote_frame = true;
> +    s->notify_sendco.done = true;
> +    g_queue_init(&s->notify_sendco.send_list);
> +

No need to init the notify_sendco each time, because the notify dev just an optional parameter.
You can use the if (s->notify_dev) here. Just Xen use the chr_notify_dev.

Overall, make the chr_send job to coroutine is a good idea. It looks good for me.
And your patch inspired me, it looks we can re-use the compare_chr_send code on filter mirror/redirector too.

Tested-by: Zhang Chen <chen.zhang@intel.com>


>      g_queue_init(&s->conn_list);
> 
>      qemu_mutex_init(&event_mtx);
> @@ -1225,8 +1312,9 @@ static void colo_flush_packets(void *opaque, 
> void
> *user_data)
>                           pkt->data,
>                           pkt->size,
>                           pkt->vnet_hdr_len,
> -                         false);
> -        packet_destroy(pkt, NULL);
> +                         false,
> +                         true);
> +        packet_destroy_partial(pkt, NULL);
>      }
>      while (!g_queue_is_empty(&conn->secondary_list)) {
>          pkt = g_queue_pop_head(&conn->secondary_list);
> @@ -1301,10 +1389,19 @@ static void colo_compare_finalize(Object *obj)
>          }
>      }
> 
> +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> +    aio_context_acquire(ctx);
> +    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> +    AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);

Same as above.

> +    aio_context_release(ctx);
> +
>      /* Release all unhandled packets after compare thead exited */
>      g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> +    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
> 
>      g_queue_clear(&s->conn_list);
> +    g_queue_clear(&s->out_sendco.send_list);
> +    g_queue_clear(&s->notify_sendco.send_list);

Same as above.

> 
>      if (s->connection_track_table) {
>          g_hash_table_destroy(s->connection_track_table);
> diff --git a/net/colo.c b/net/colo.c
> index 8196b35837..a6c66d829a 100644
> --- a/net/colo.c
> +++ b/net/colo.c
> @@ -185,6 +185,13 @@ void packet_destroy(void *opaque, void *user_data)
>      g_slice_free(Packet, pkt);
>  }
> 
> +void packet_destroy_partial(void *opaque, void *user_data) {
> +    Packet *pkt = opaque;
> +
> +    g_slice_free(Packet, pkt);
> +}
> +
>  /*
>   * Clear hashtable, stop this hash growing really huge
>   */
> diff --git a/net/colo.h b/net/colo.h
> index 679314b1ca..573ab91785 100644
> --- a/net/colo.h
> +++ b/net/colo.h
> @@ -102,5 +102,6 @@ bool connection_has_tracked(GHashTable 
> *connection_track_table,  void connection_hashtable_reset(GHashTable
> *connection_track_table);  Packet *packet_new(const void *data, int 
> size, int vnet_hdr_len);  void packet_destroy(void *opaque, void 
> *user_data);
> +void packet_destroy_partial(void *opaque, void *user_data);
> 
>  #endif /* NET_COLO_H */
> --
> 2.20.1

Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Lukas Straub]

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

On Thu, 7 May 2020 11:00:26 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Monday, May 4, 2020 6:28 PM
> > To: qemu-devel <qemu-devel@nongnu.org>
> > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian 
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc- 
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini 
> > <pbonzini@redhat.com>
> > Subject: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in 
> > compare_chr_send
> > 
> > The chr_out chardev is connected to a filter-redirector running in the 
> > main loop. qemu_chr_fe_write_all might block here in compare_chr_send 
> > if the (socket-)buffer is full.
> > If another filter-redirector in the main loop want's to send data to 
> > chr_pri_in it might also block if the buffer is full. This leads to a 
> > deadlock because both event loops get blocked.
> > 
> > Fix this by converting compare_chr_send to a coroutine and putting the 
> > packets in a send queue.
> > 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  net/colo-compare.c | 187 ++++++++++++++++++++++++++++++++++-------
> > ----
> >  net/colo.c         |   7 ++
> >  net/colo.h         |   1 +
> >  3 files changed, 150 insertions(+), 45 deletions(-)
> > 
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index 
> > 1de4220fe2..2a4e7f7c4e 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -32,6 +32,9 @@
> >  #include "migration/migration.h"
> >  #include "util.h"
> > 
> > +#include "block/aio-wait.h"
> > +#include "qemu/coroutine.h"
> > +
> >  #define TYPE_COLO_COMPARE "colo-compare"
> >  #define COLO_COMPARE(obj) \
> >      OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -77,6
> > +80,23 @@ static int event_unhandled_count;
> >   *                    |packet  |  |packet  +    |packet  | |packet  +
> >   *                    +--------+  +--------+    +--------+ +--------+
> >   */
> > +
> > +typedef struct SendCo {
> > +    Coroutine *co;
> > +    struct CompareState *s;
> > +    CharBackend *chr;
> > +    GQueue send_list;
> > +    bool notify_remote_frame;
> > +    bool done;
> > +    int ret;
> > +} SendCo;
> > +
> > +typedef struct SendEntry {
> > +    uint32_t size;
> > +    uint32_t vnet_hdr_len;
> > +    uint8_t *buf;
> > +} SendEntry;
> > +
> >  typedef struct CompareState {
> >      Object parent;
> > 
> > @@ -91,6 +111,8 @@ typedef struct CompareState {
> >      SocketReadState pri_rs;
> >      SocketReadState sec_rs;
> >      SocketReadState notify_rs;
> > +    SendCo out_sendco;
> > +    SendCo notify_sendco;
> >      bool vnet_hdr;
> >      uint32_t compare_timeout;
> >      uint32_t expired_scan_cycle;
> > @@ -124,10 +146,11 @@ enum {
> > 
> > 
> >  static int compare_chr_send(CompareState *s,
> > -                            const uint8_t *buf,
> > +                            uint8_t *buf,
> >                              uint32_t size,
> >                              uint32_t vnet_hdr_len,
> > -                            bool notify_remote_frame);
> > +                            bool notify_remote_frame,
> > +                            bool zero_copy);
> > 
> >  static bool packet_matches_str(const char *str,
> >                                 const uint8_t *buf, @@ -145,7 +168,7 
> > @@ static void notify_remote_frame(CompareState *s)
> >      char msg[] = "DO_CHECKPOINT";
> >      int ret = 0;
> > 
> > -    ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > +    ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true, 
> > + false);
> >      if (ret < 0) {
> >          error_report("Notify Xen COLO-frame failed");
> >      }
> > @@ -272,12 +295,13 @@ static void
> > colo_release_primary_pkt(CompareState *s, Packet *pkt)
> >                             pkt->data,
> >                             pkt->size,
> >                             pkt->vnet_hdr_len,
> > -                           false);
> > +                           false,
> > +                           true);
> >      if (ret < 0) {
> >          error_report("colo send primary packet failed");
> >      }
> >      trace_colo_compare_main("packet same and release packet");
> > -    packet_destroy(pkt, NULL);
> > +    packet_destroy_partial(pkt, NULL);
> >  }
> > 
> >  /*
> > @@ -699,65 +723,115 @@ static void colo_compare_connection(void 
> > *opaque, void *user_data)
> >      }
> >  }
> > 
> > -static int compare_chr_send(CompareState *s,
> > -                            const uint8_t *buf,
> > -                            uint32_t size,
> > -                            uint32_t vnet_hdr_len,
> > -                            bool notify_remote_frame)
> > +static void coroutine_fn _compare_chr_send(void *opaque)
> >  {
> > +    SendCo *sendco = opaque;
> > +    CompareState *s = sendco->s;
> >      int ret = 0;
> > -    uint32_t len = htonl(size);
> > 
> > -    if (!size) {
> > -        return 0;
> > -    }
> > +    while (!g_queue_is_empty(&sendco->send_list)) {
> > +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > +        uint32_t len = htonl(entry->size);
> > 
> > -    if (notify_remote_frame) {
> > -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > -                                    (uint8_t *)&len,
> > -                                    sizeof(len));
> > -    } else {
> > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> > -    }
> > +        ret = qemu_chr_fe_write_all(sendco->chr, (uint8_t *)&len, 
> > + sizeof(len));
> > 
> > -    if (ret != sizeof(len)) {
> > -        goto err;
> > -    }
> > +        if (ret != sizeof(len)) {
> > +            g_free(entry->buf);
> > +            g_slice_free(SendEntry, entry);
> > +            goto err;
> > +        }
> > 
> > -    if (s->vnet_hdr) {
> > -        /*
> > -         * We send vnet header len make other module(like filter-redirector)
> > -         * know how to parse net packet correctly.
> > -         */
> > -        len = htonl(vnet_hdr_len);
> > +        if (!sendco->notify_remote_frame && s->vnet_hdr) {
> > +            /*
> > +             * We send vnet header len make other module(like filter-redirector)
> > +             * know how to parse net packet correctly.
> > +             */
> > +            len = htonl(entry->vnet_hdr_len);
> > 
> > -        if (!notify_remote_frame) {
> > -            ret = qemu_chr_fe_write_all(&s->chr_out,
> > +            ret = qemu_chr_fe_write_all(sendco->chr,
> >                                          (uint8_t *)&len,
> >                                          sizeof(len));
> > +
> > +            if (ret != sizeof(len)) {
> > +                g_free(entry->buf);
> > +                g_slice_free(SendEntry, entry);
> > +                goto err;
> > +            }
> >          }
> > 
> > -        if (ret != sizeof(len)) {
> > +        ret = qemu_chr_fe_write_all(sendco->chr,
> > +                                    (uint8_t *)entry->buf,
> > +                                    entry->size);
> > +
> > +        if (ret != entry->size) {
> > +            g_free(entry->buf);
> > +            g_slice_free(SendEntry, entry);
> >              goto err;
> >          }
> > +
> > +        g_free(entry->buf);
> > +        g_slice_free(SendEntry, entry);
> >      }
> > 
> > +    sendco->ret = 0;
> > +    goto out;
> > +
> > +err:
> > +    while (!g_queue_is_empty(&sendco->send_list)) {
> > +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > +        g_free(entry->buf);
> > +        g_slice_free(SendEntry, entry);
> > +    }
> > +    sendco->ret = ret < 0 ? ret : -EIO;
> > +out:
> > +    sendco->co = NULL;
> > +    sendco->done = true;
> > +    aio_wait_kick();
> > +}
> > +
> > +static int compare_chr_send(CompareState *s,
> > +                            uint8_t *buf,
> > +                            uint32_t size,
> > +                            uint32_t vnet_hdr_len,
> > +                            bool notify_remote_frame,
> > +                            bool zero_copy) {
> > +    SendCo *sendco;
> > +    SendEntry *entry;
> > +
> >      if (notify_remote_frame) {
> > -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > -                                    (uint8_t *)buf,
> > -                                    size);
> > +        sendco = &s->notify_sendco;
> >      } else {
> > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> > +        sendco = &s->out_sendco;
> >      }
> > 
> > -    if (ret != size) {
> > -        goto err;
> > +    if (!size) {
> > +        return 0;
> >      }
> > 
> > -    return 0;
> > +    entry = g_slice_new(SendEntry);
> > +    entry->size = size;
> > +    entry->vnet_hdr_len = vnet_hdr_len;
> > +    if (zero_copy) {
> > +        entry->buf = buf;
> > +    } else {
> > +        entry->buf = g_malloc(size);
> > +        memcpy(entry->buf, buf, size);
> > +    }
> > +    g_queue_push_head(&sendco->send_list, entry);
> > +
> > +    if (sendco->done) {
> > +        sendco->co = qemu_coroutine_create(_compare_chr_send, sendco);
> > +        sendco->done = false;
> > +        qemu_coroutine_enter(sendco->co);
> > +        if (sendco->done) {
> > +            /* report early errors */
> > +            return sendco->ret;
> > +        }
> > +    }
> > 
> > -err:
> > -    return ret < 0 ? ret : -EIO;
> > +    /* assume success */
> > +    return 0;
> >  }
> > 
> >  static int compare_chr_can_read(void *opaque) @@ -1063,6 +1137,7 @@ 
> > static void compare_pri_rs_finalize(SocketReadState *pri_rs)
> >                           pri_rs->buf,
> >                           pri_rs->packet_len,
> >                           pri_rs->vnet_hdr_len,
> > +                         false,
> >                           false);
> >      } else {
> >          /* compare packet in the specified connection */ @@ -1093,7
> > +1168,7 @@ static void compare_notify_rs_finalize(SocketReadState
> > +*notify_rs)
> >      if (packet_matches_str("COLO_USERSPACE_PROXY_INIT",
> >                             notify_rs->buf,
> >                             notify_rs->packet_len)) {
> > -        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > +        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, 
> > + true, false);
> >          if (ret < 0) {
> >              error_report("Notify Xen COLO-frame INIT failed");
> >          }
> > @@ -1199,6 +1274,18 @@ static void
> > colo_compare_complete(UserCreatable *uc, Error **errp)
> > 
> >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > 
> > +    s->out_sendco.s = s;
> > +    s->out_sendco.chr = &s->chr_out;
> > +    s->out_sendco.notify_remote_frame = false;
> > +    s->out_sendco.done = true;
> > +    g_queue_init(&s->out_sendco.send_list);
> > +
> > +    s->notify_sendco.s = s;
> > +    s->notify_sendco.chr = &s->chr_notify_dev;
> > +    s->notify_sendco.notify_remote_frame = true;
> > +    s->notify_sendco.done = true;
> > +    g_queue_init(&s->notify_sendco.send_list);
> > +  
> 
> No need to init the notify_sendco each time, because the notify dev just an optional parameter.
> You can use the if (s->notify_dev) here. Just Xen use the chr_notify_dev.

Ok, I will change that and the code below in the next version.

> Overall, make the chr_send job to coroutine is a good idea. It looks good for me.
> And your patch inspired me, it looks we can re-use the compare_chr_send code on filter mirror/redirector too.

I already have patch for that, but I don't think it is a good idea, because the guest then can send packets faster than colo-compare can process. This leads bufferbloat and the performance drops in my tests:
Client-to-server tcp:
without patch: ~66 Mbit/s
with patch: ~59 Mbit/s
Server-to-client tcp:
without patch: ~702 Kbit/s
with patch: ~328 Kbit/s

Regards,
Lukas Straub

> Tested-by: Zhang Chen <chen.zhang@intel.com>
> 
> 
> >      g_queue_init(&s->conn_list);
> > 
> >      qemu_mutex_init(&event_mtx);
> > @@ -1225,8 +1312,9 @@ static void colo_flush_packets(void *opaque, 
> > void
> > *user_data)
> >                           pkt->data,
> >                           pkt->size,
> >                           pkt->vnet_hdr_len,
> > -                         false);
> > -        packet_destroy(pkt, NULL);
> > +                         false,
> > +                         true);
> > +        packet_destroy_partial(pkt, NULL);
> >      }
> >      while (!g_queue_is_empty(&conn->secondary_list)) {
> >          pkt = g_queue_pop_head(&conn->secondary_list);
> > @@ -1301,10 +1389,19 @@ static void colo_compare_finalize(Object *obj)
> >          }
> >      }
> > 
> > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > +    aio_context_acquire(ctx);
> > +    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > +    AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);  
> 
> Same as above.
> 
> > +    aio_context_release(ctx);
> > +
> >      /* Release all unhandled packets after compare thead exited */
> >      g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> > +    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
> > 
> >      g_queue_clear(&s->conn_list);
> > +    g_queue_clear(&s->out_sendco.send_list);
> > +    g_queue_clear(&s->notify_sendco.send_list);  
> 
> Same as above.
> 
> > 
> >      if (s->connection_track_table) {
> >          g_hash_table_destroy(s->connection_track_table);
> > diff --git a/net/colo.c b/net/colo.c
> > index 8196b35837..a6c66d829a 100644
> > --- a/net/colo.c
> > +++ b/net/colo.c
> > @@ -185,6 +185,13 @@ void packet_destroy(void *opaque, void *user_data)
> >      g_slice_free(Packet, pkt);
> >  }
> > 
> > +void packet_destroy_partial(void *opaque, void *user_data) {
> > +    Packet *pkt = opaque;
> > +
> > +    g_slice_free(Packet, pkt);
> > +}
> > +
> >  /*
> >   * Clear hashtable, stop this hash growing really huge
> >   */
> > diff --git a/net/colo.h b/net/colo.h
> > index 679314b1ca..573ab91785 100644
> > --- a/net/colo.h
> > +++ b/net/colo.h
> > @@ -102,5 +102,6 @@ bool connection_has_tracked(GHashTable 
> > *connection_track_table,  void connection_hashtable_reset(GHashTable
> > *connection_track_table);  Packet *packet_new(const void *data, int 
> > size, int vnet_hdr_len);  void packet_destroy(void *opaque, void 
> > *user_data);
> > +void packet_destroy_partial(void *opaque, void *user_data);
> > 
> >  #endif /* NET_COLO_H */
> > --
> > 2.20.1  
> 


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

RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Thursday, May 7, 2020 11:51 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
> 
> On Thu, 7 May 2020 11:00:26 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel <qemu-devel@nongnu.org>
> > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> > > compare_chr_send
> > >
> > > The chr_out chardev is connected to a filter-redirector running in
> > > the main loop. qemu_chr_fe_write_all might block here in
> > > compare_chr_send if the (socket-)buffer is full.
> > > If another filter-redirector in the main loop want's to send data to
> > > chr_pri_in it might also block if the buffer is full. This leads to
> > > a deadlock because both event loops get blocked.
> > >
> > > Fix this by converting compare_chr_send to a coroutine and putting
> > > the packets in a send queue.
> > >
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  net/colo-compare.c | 187 ++++++++++++++++++++++++++++++++++---
> ----
> > > ----
> > >  net/colo.c         |   7 ++
> > >  net/colo.h         |   1 +
> > >  3 files changed, 150 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > 1de4220fe2..2a4e7f7c4e 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -32,6 +32,9 @@
> > >  #include "migration/migration.h"
> > >  #include "util.h"
> > >
> > > +#include "block/aio-wait.h"
> > > +#include "qemu/coroutine.h"
> > > +
> > >  #define TYPE_COLO_COMPARE "colo-compare"
> > >  #define COLO_COMPARE(obj) \
> > >      OBJECT_CHECK(CompareState, (obj), TYPE_COLO_COMPARE) @@ -
> 77,6
> > > +80,23 @@ static int event_unhandled_count;
> > >   *                    |packet  |  |packet  +    |packet  | |packet  +
> > >   *                    +--------+  +--------+    +--------+ +--------+
> > >   */
> > > +
> > > +typedef struct SendCo {
> > > +    Coroutine *co;
> > > +    struct CompareState *s;
> > > +    CharBackend *chr;
> > > +    GQueue send_list;
> > > +    bool notify_remote_frame;
> > > +    bool done;
> > > +    int ret;
> > > +} SendCo;
> > > +
> > > +typedef struct SendEntry {
> > > +    uint32_t size;
> > > +    uint32_t vnet_hdr_len;
> > > +    uint8_t *buf;
> > > +} SendEntry;
> > > +
> > >  typedef struct CompareState {
> > >      Object parent;
> > >
> > > @@ -91,6 +111,8 @@ typedef struct CompareState {
> > >      SocketReadState pri_rs;
> > >      SocketReadState sec_rs;
> > >      SocketReadState notify_rs;
> > > +    SendCo out_sendco;
> > > +    SendCo notify_sendco;
> > >      bool vnet_hdr;
> > >      uint32_t compare_timeout;
> > >      uint32_t expired_scan_cycle;
> > > @@ -124,10 +146,11 @@ enum {
> > >
> > >
> > >  static int compare_chr_send(CompareState *s,
> > > -                            const uint8_t *buf,
> > > +                            uint8_t *buf,
> > >                              uint32_t size,
> > >                              uint32_t vnet_hdr_len,
> > > -                            bool notify_remote_frame);
> > > +                            bool notify_remote_frame,
> > > +                            bool zero_copy);
> > >
> > >  static bool packet_matches_str(const char *str,
> > >                                 const uint8_t *buf, @@ -145,7 +168,7
> > > @@ static void notify_remote_frame(CompareState *s)
> > >      char msg[] = "DO_CHECKPOINT";
> > >      int ret = 0;
> > >
> > > -    ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > > +    ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true,
> > > + false);
> > >      if (ret < 0) {
> > >          error_report("Notify Xen COLO-frame failed");
> > >      }
> > > @@ -272,12 +295,13 @@ static void
> > > colo_release_primary_pkt(CompareState *s, Packet *pkt)
> > >                             pkt->data,
> > >                             pkt->size,
> > >                             pkt->vnet_hdr_len,
> > > -                           false);
> > > +                           false,
> > > +                           true);
> > >      if (ret < 0) {
> > >          error_report("colo send primary packet failed");
> > >      }
> > >      trace_colo_compare_main("packet same and release packet");
> > > -    packet_destroy(pkt, NULL);
> > > +    packet_destroy_partial(pkt, NULL);
> > >  }
> > >
> > >  /*
> > > @@ -699,65 +723,115 @@ static void colo_compare_connection(void
> > > *opaque, void *user_data)
> > >      }
> > >  }
> > >
> > > -static int compare_chr_send(CompareState *s,
> > > -                            const uint8_t *buf,
> > > -                            uint32_t size,
> > > -                            uint32_t vnet_hdr_len,
> > > -                            bool notify_remote_frame)
> > > +static void coroutine_fn _compare_chr_send(void *opaque)
> > >  {
> > > +    SendCo *sendco = opaque;
> > > +    CompareState *s = sendco->s;
> > >      int ret = 0;
> > > -    uint32_t len = htonl(size);
> > >
> > > -    if (!size) {
> > > -        return 0;
> > > -    }
> > > +    while (!g_queue_is_empty(&sendco->send_list)) {
> > > +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > > +        uint32_t len = htonl(entry->size);
> > >
> > > -    if (notify_remote_frame) {
> > > -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > > -                                    (uint8_t *)&len,
> > > -                                    sizeof(len));
> > > -    } else {
> > > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len,
> sizeof(len));
> > > -    }
> > > +        ret = qemu_chr_fe_write_all(sendco->chr, (uint8_t *)&len,
> > > + sizeof(len));
> > >
> > > -    if (ret != sizeof(len)) {
> > > -        goto err;
> > > -    }
> > > +        if (ret != sizeof(len)) {
> > > +            g_free(entry->buf);
> > > +            g_slice_free(SendEntry, entry);
> > > +            goto err;
> > > +        }
> > >
> > > -    if (s->vnet_hdr) {
> > > -        /*
> > > -         * We send vnet header len make other module(like filter-redirector)
> > > -         * know how to parse net packet correctly.
> > > -         */
> > > -        len = htonl(vnet_hdr_len);
> > > +        if (!sendco->notify_remote_frame && s->vnet_hdr) {
> > > +            /*
> > > +             * We send vnet header len make other module(like filter-
> redirector)
> > > +             * know how to parse net packet correctly.
> > > +             */
> > > +            len = htonl(entry->vnet_hdr_len);
> > >
> > > -        if (!notify_remote_frame) {
> > > -            ret = qemu_chr_fe_write_all(&s->chr_out,
> > > +            ret = qemu_chr_fe_write_all(sendco->chr,
> > >                                          (uint8_t *)&len,
> > >                                          sizeof(len));
> > > +
> > > +            if (ret != sizeof(len)) {
> > > +                g_free(entry->buf);
> > > +                g_slice_free(SendEntry, entry);
> > > +                goto err;
> > > +            }
> > >          }
> > >
> > > -        if (ret != sizeof(len)) {
> > > +        ret = qemu_chr_fe_write_all(sendco->chr,
> > > +                                    (uint8_t *)entry->buf,
> > > +                                    entry->size);
> > > +
> > > +        if (ret != entry->size) {
> > > +            g_free(entry->buf);
> > > +            g_slice_free(SendEntry, entry);
> > >              goto err;
> > >          }
> > > +
> > > +        g_free(entry->buf);
> > > +        g_slice_free(SendEntry, entry);
> > >      }
> > >
> > > +    sendco->ret = 0;
> > > +    goto out;
> > > +
> > > +err:
> > > +    while (!g_queue_is_empty(&sendco->send_list)) {
> > > +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > > +        g_free(entry->buf);
> > > +        g_slice_free(SendEntry, entry);
> > > +    }
> > > +    sendco->ret = ret < 0 ? ret : -EIO;
> > > +out:
> > > +    sendco->co = NULL;
> > > +    sendco->done = true;
> > > +    aio_wait_kick();
> > > +}
> > > +
> > > +static int compare_chr_send(CompareState *s,
> > > +                            uint8_t *buf,
> > > +                            uint32_t size,
> > > +                            uint32_t vnet_hdr_len,
> > > +                            bool notify_remote_frame,
> > > +                            bool zero_copy) {
> > > +    SendCo *sendco;
> > > +    SendEntry *entry;
> > > +
> > >      if (notify_remote_frame) {
> > > -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > > -                                    (uint8_t *)buf,
> > > -                                    size);
> > > +        sendco = &s->notify_sendco;
> > >      } else {
> > > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> > > +        sendco = &s->out_sendco;
> > >      }
> > >
> > > -    if (ret != size) {
> > > -        goto err;
> > > +    if (!size) {
> > > +        return 0;
> > >      }
> > >
> > > -    return 0;
> > > +    entry = g_slice_new(SendEntry);
> > > +    entry->size = size;
> > > +    entry->vnet_hdr_len = vnet_hdr_len;
> > > +    if (zero_copy) {
> > > +        entry->buf = buf;
> > > +    } else {
> > > +        entry->buf = g_malloc(size);
> > > +        memcpy(entry->buf, buf, size);
> > > +    }
> > > +    g_queue_push_head(&sendco->send_list, entry);
> > > +
> > > +    if (sendco->done) {
> > > +        sendco->co = qemu_coroutine_create(_compare_chr_send,
> sendco);
> > > +        sendco->done = false;
> > > +        qemu_coroutine_enter(sendco->co);
> > > +        if (sendco->done) {
> > > +            /* report early errors */
> > > +            return sendco->ret;
> > > +        }
> > > +    }
> > >
> > > -err:
> > > -    return ret < 0 ? ret : -EIO;
> > > +    /* assume success */
> > > +    return 0;
> > >  }
> > >
> > >  static int compare_chr_can_read(void *opaque) @@ -1063,6 +1137,7
> @@
> > > static void compare_pri_rs_finalize(SocketReadState *pri_rs)
> > >                           pri_rs->buf,
> > >                           pri_rs->packet_len,
> > >                           pri_rs->vnet_hdr_len,
> > > +                         false,
> > >                           false);
> > >      } else {
> > >          /* compare packet in the specified connection */ @@ -1093,7
> > > +1168,7 @@ static void compare_notify_rs_finalize(SocketReadState
> > > +*notify_rs)
> > >      if (packet_matches_str("COLO_USERSPACE_PROXY_INIT",
> > >                             notify_rs->buf,
> > >                             notify_rs->packet_len)) {
> > > -        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0, true);
> > > +        ret = compare_chr_send(s, (uint8_t *)msg, strlen(msg), 0,
> > > + true, false);
> > >          if (ret < 0) {
> > >              error_report("Notify Xen COLO-frame INIT failed");
> > >          }
> > > @@ -1199,6 +1274,18 @@ static void
> > > colo_compare_complete(UserCreatable *uc, Error **errp)
> > >
> > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > >
> > > +    s->out_sendco.s = s;
> > > +    s->out_sendco.chr = &s->chr_out;
> > > +    s->out_sendco.notify_remote_frame = false;
> > > +    s->out_sendco.done = true;
> > > +    g_queue_init(&s->out_sendco.send_list);
> > > +
> > > +    s->notify_sendco.s = s;
> > > +    s->notify_sendco.chr = &s->chr_notify_dev;
> > > +    s->notify_sendco.notify_remote_frame = true;
> > > +    s->notify_sendco.done = true;
> > > +    g_queue_init(&s->notify_sendco.send_list);
> > > +
> >
> > No need to init the notify_sendco each time, because the notify dev just
> an optional parameter.
> > You can use the if (s->notify_dev) here. Just Xen use the chr_notify_dev.
> 
> Ok, I will change that and the code below in the next version.
> 
> > Overall, make the chr_send job to coroutine is a good idea. It looks good
> for me.
> > And your patch inspired me, it looks we can re-use the compare_chr_send
> code on filter mirror/redirector too.
> 
> I already have patch for that, but I don't think it is a good idea, because the
> guest then can send packets faster than colo-compare can process. This leads
> bufferbloat and the performance drops in my tests:
> Client-to-server tcp:
> without patch: ~66 Mbit/s
> with patch: ~59 Mbit/s
> Server-to-client tcp:
> without patch: ~702 Kbit/s
> with patch: ~328 Kbit/s

Oh, a big performance drop, is that caused by memcpy/zero_copy parts ? 

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Tested-by: Zhang Chen <chen.zhang@intel.com>
> >
> >
> > >      g_queue_init(&s->conn_list);
> > >
> > >      qemu_mutex_init(&event_mtx);
> > > @@ -1225,8 +1312,9 @@ static void colo_flush_packets(void *opaque,
> > > void
> > > *user_data)
> > >                           pkt->data,
> > >                           pkt->size,
> > >                           pkt->vnet_hdr_len,
> > > -                         false);
> > > -        packet_destroy(pkt, NULL);
> > > +                         false,
> > > +                         true);
> > > +        packet_destroy_partial(pkt, NULL);
> > >      }
> > >      while (!g_queue_is_empty(&conn->secondary_list)) {
> > >          pkt = g_queue_pop_head(&conn->secondary_list);
> > > @@ -1301,10 +1389,19 @@ static void colo_compare_finalize(Object *obj)
> > >          }
> > >      }
> > >
> > > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > +    aio_context_acquire(ctx);
> > > +    AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > > +    AIO_WAIT_WHILE(ctx, !s->notify_sendco.done);
> >
> > Same as above.
> >
> > > +    aio_context_release(ctx);
> > > +
> > >      /* Release all unhandled packets after compare thead exited */
> > >      g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> > > +    AIO_WAIT_WHILE(NULL, !s->out_sendco.done);
> > >
> > >      g_queue_clear(&s->conn_list);
> > > +    g_queue_clear(&s->out_sendco.send_list);
> > > +    g_queue_clear(&s->notify_sendco.send_list);
> >
> > Same as above.
> >
> > >
> > >      if (s->connection_track_table) {
> > >          g_hash_table_destroy(s->connection_track_table);
> > > diff --git a/net/colo.c b/net/colo.c index 8196b35837..a6c66d829a
> > > 100644
> > > --- a/net/colo.c
> > > +++ b/net/colo.c
> > > @@ -185,6 +185,13 @@ void packet_destroy(void *opaque, void
> *user_data)
> > >      g_slice_free(Packet, pkt);
> > >  }
> > >
> > > +void packet_destroy_partial(void *opaque, void *user_data) {
> > > +    Packet *pkt = opaque;
> > > +
> > > +    g_slice_free(Packet, pkt);
> > > +}
> > > +
> > >  /*
> > >   * Clear hashtable, stop this hash growing really huge
> > >   */
> > > diff --git a/net/colo.h b/net/colo.h index 679314b1ca..573ab91785
> > > 100644
> > > --- a/net/colo.h
> > > +++ b/net/colo.h
> > > @@ -102,5 +102,6 @@ bool connection_has_tracked(GHashTable
> > > *connection_track_table,  void connection_hashtable_reset(GHashTable
> > > *connection_track_table);  Packet *packet_new(const void *data, int
> > > size, int vnet_hdr_len);  void packet_destroy(void *opaque, void
> > > *user_data);
> > > +void packet_destroy_partial(void *opaque, void *user_data);
> > >
> > >  #endif /* NET_COLO_H */
> > > --
> > > 2.20.1
> >

Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Lukas Straub]

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

On Fri, 8 May 2020 02:19:00 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:
> > > No need to init the notify_sendco each time, because the notify dev just  
> > an optional parameter.  
> > > You can use the if (s->notify_dev) here. Just Xen use the chr_notify_dev.  
> > 
> > Ok, I will change that and the code below in the next version.
> >   
> > > Overall, make the chr_send job to coroutine is a good idea. It looks good  
> > for me.  
> > > And your patch inspired me, it looks we can re-use the compare_chr_send  
> > code on filter mirror/redirector too.
> > 
> > I already have patch for that, but I don't think it is a good idea, because the
> > guest then can send packets faster than colo-compare can process. This leads
> > bufferbloat and the performance drops in my tests:
> > Client-to-server tcp:
> > without patch: ~66 Mbit/s
> > with patch: ~59 Mbit/s
> > Server-to-client tcp:
> > without patch: ~702 Kbit/s
> > with patch: ~328 Kbit/s  
> 
> Oh, a big performance drop, is that caused by memcpy/zero_copy parts ? 
> 
> Thanks
> Zhang Chen

No, there is no memcpy overhead with this patch, see below.

Regards,
Lukas Straub

---
 net/filter-mirror.c | 142 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 106 insertions(+), 36 deletions(-)

diff --git a/net/filter-mirror.c b/net/filter-mirror.c
index d83e815545..6bcd317502 100644
--- a/net/filter-mirror.c
+++ b/net/filter-mirror.c
@@ -20,6 +20,8 @@
 #include "chardev/char-fe.h"
 #include "qemu/iov.h"
 #include "qemu/sockets.h"
+#include "block/aio-wait.h"
+#include "qemu/coroutine.h"
 
 #define FILTER_MIRROR(obj) \
     OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR)
@@ -31,6 +33,18 @@
 #define TYPE_FILTER_REDIRECTOR "filter-redirector"
 #define REDIRECTOR_MAX_LEN NET_BUFSIZE
 
+typedef struct SendCo {
+    Coroutine *co;
+    GQueue send_list;
+    bool done;
+    int ret;
+} SendCo;
+
+typedef struct SendEntry {
+    ssize_t size;
+    uint8_t buf[];
+} SendEntry;
+
 typedef struct MirrorState {
     NetFilterState parent_obj;
     char *indev;
@@ -38,59 +52,101 @@ typedef struct MirrorState {
     CharBackend chr_in;
     CharBackend chr_out;
     SocketReadState rs;
+    SendCo sendco;
     bool vnet_hdr;
 } MirrorState;
 
-static int filter_send(MirrorState *s,
-                       const struct iovec *iov,
-                       int iovcnt)
+static void coroutine_fn _filter_send(void *opaque)
 {
+    MirrorState *s = opaque;
+    SendCo *sendco = &s->sendco;
     NetFilterState *nf = NETFILTER(s);
     int ret = 0;
-    ssize_t size = 0;
-    uint32_t len = 0;
-    char *buf;
-
-    size = iov_size(iov, iovcnt);
-    if (!size) {
-        return 0;
-    }
 
-    len = htonl(size);
-    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
-    if (ret != sizeof(len)) {
-        goto err;
-    }
+    while (!g_queue_is_empty(&sendco->send_list)) {
+        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
+        uint32_t len = htonl(entry->size);
 
-    if (s->vnet_hdr) {
-        /*
-         * If vnet_hdr = on, we send vnet header len to make other
-         * module(like colo-compare) know how to parse net
-         * packet correctly.
-         */
-        ssize_t vnet_hdr_len;
+        ret = qemu_chr_fe_write_all(&s->chr_out,
+                                    (uint8_t *)&len,
+                                    sizeof(len));
+        if (ret != sizeof(len)) {
+            g_free(entry);
+            goto err;
+        }
 
-        vnet_hdr_len = nf->netdev->vnet_hdr_len;
+        if (s->vnet_hdr) {
+            /*
+             * If vnet_hdr = on, we send vnet header len to make other
+             * module(like colo-compare) know how to parse net
+             * packet correctly.
+             */
+
+            len = htonl(nf->netdev->vnet_hdr_len);
+            ret = qemu_chr_fe_write_all(&s->chr_out,
+                                        (uint8_t *)&len,
+                                        sizeof(len));
+            if (ret != sizeof(len)) {
+                g_free(entry);
+                goto err;
+            }
+        }
 
-        len = htonl(vnet_hdr_len);
-        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
-        if (ret != sizeof(len)) {
+        ret = qemu_chr_fe_write_all(&s->chr_out,
+                                    (uint8_t *)entry->buf,
+                                    entry->size);
+        if (ret != entry->size) {
+            g_free(entry);
             goto err;
         }
-    }
 
-    buf = g_malloc(size);
-    iov_to_buf(iov, iovcnt, 0, buf, size);
-    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
-    g_free(buf);
-    if (ret != size) {
-        goto err;
+        g_free(entry);
     }
 
-    return 0;
+    sendco->ret = 0;
+    goto out;
 
 err:
-    return ret < 0 ? ret : -EIO;
+    while (!g_queue_is_empty(&sendco->send_list)) {
+        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
+        g_free(entry);
+    }
+    sendco->ret = ret < 0 ? ret : -EIO;
+out:
+    sendco->co = NULL;
+    sendco->done = true;
+    aio_wait_kick();
+}
+
+static int filter_send(MirrorState *s,
+                       const struct iovec *iov,
+                       int iovcnt)
+{
+    SendCo *sendco = &s->sendco;
+    SendEntry *entry;
+
+    ssize_t size = iov_size(iov, iovcnt);
+    if (!size) {
+        return 0;
+    }
+
+    entry = g_malloc(sizeof(SendEntry) + size);
+    entry->size = size;
+    iov_to_buf(iov, iovcnt, 0, entry->buf, size);
+    g_queue_push_head(&sendco->send_list, entry);
+
+    if (sendco->done) {
+        sendco->co = qemu_coroutine_create(_filter_send, s);
+        sendco->done = false;
+        qemu_coroutine_enter(sendco->co);
+        if (sendco->done) {
+            /* report early errors */
+            return sendco->ret;
+        }
+    }
+
+    /* assume success */
+    return 0;
 }
 
 static void redirector_to_filter(NetFilterState *nf,
@@ -194,6 +250,10 @@ static void filter_mirror_cleanup(NetFilterState *nf)
 {
     MirrorState *s = FILTER_MIRROR(nf);
 
+    AIO_WAIT_WHILE(NULL, !s->sendco.done);
+
+    g_queue_clear(&s->sendco.send_list);
+
     qemu_chr_fe_deinit(&s->chr_out, false);
 }
 
@@ -201,6 +261,10 @@ static void filter_redirector_cleanup(NetFilterState *nf)
 {
     MirrorState *s = FILTER_REDIRECTOR(nf);
 
+    AIO_WAIT_WHILE(NULL, !s->sendco.done);
+
+    g_queue_clear(&s->sendco.send_list);
+
     qemu_chr_fe_deinit(&s->chr_in, false);
     qemu_chr_fe_deinit(&s->chr_out, false);
 }
@@ -224,6 +288,9 @@ static void filter_mirror_setup(NetFilterState *nf, Error **errp)
     }
 
     qemu_chr_fe_init(&s->chr_out, chr, errp);
+
+    s->sendco.done = true;
+    g_queue_init(&s->sendco.send_list);
 }
 
 static void redirector_rs_finalize(SocketReadState *rs)
@@ -281,6 +348,9 @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
             return;
         }
     }
+
+    s->sendco.done = true;
+    g_queue_init(&s->sendco.send_list);
 }
 
 static void filter_mirror_class_init(ObjectClass *oc, void *data)
-- 
2.20.1


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

RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, May 8, 2020 2:08 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
> 
> On Fri, 8 May 2020 02:19:00 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > > > No need to init the notify_sendco each time, because the notify
> > > > dev just
> > > an optional parameter.
> > > > You can use the if (s->notify_dev) here. Just Xen use the
> chr_notify_dev.
> > >
> > > Ok, I will change that and the code below in the next version.
> > >
> > > > Overall, make the chr_send job to coroutine is a good idea. It
> > > > looks good
> > > for me.
> > > > And your patch inspired me, it looks we can re-use the
> > > > compare_chr_send
> > > code on filter mirror/redirector too.
> > >
> > > I already have patch for that, but I don't think it is a good idea,
> > > because the guest then can send packets faster than colo-compare can
> > > process. This leads bufferbloat and the performance drops in my tests:
> > > Client-to-server tcp:
> > > without patch: ~66 Mbit/s
> > > with patch: ~59 Mbit/s
> > > Server-to-client tcp:
> > > without patch: ~702 Kbit/s
> > > with patch: ~328 Kbit/s
> >
> > Oh, a big performance drop, is that caused by memcpy/zero_copy parts ?
> >
> > Thanks
> > Zhang Chen
> 
> No, there is no memcpy overhead with this patch, see below.

I means for the filter mirror/redirector parts why coroutine will lead huge performance drop?

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> ---
>  net/filter-mirror.c | 142 +++++++++++++++++++++++++++++++++-----------
>  1 file changed, 106 insertions(+), 36 deletions(-)
> 
> diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> d83e815545..6bcd317502 100644
> --- a/net/filter-mirror.c
> +++ b/net/filter-mirror.c
> @@ -20,6 +20,8 @@
>  #include "chardev/char-fe.h"
>  #include "qemu/iov.h"
>  #include "qemu/sockets.h"
> +#include "block/aio-wait.h"
> +#include "qemu/coroutine.h"
> 
>  #define FILTER_MIRROR(obj) \
>      OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR) @@ -31,6
> +33,18 @@  #define TYPE_FILTER_REDIRECTOR "filter-redirector"
>  #define REDIRECTOR_MAX_LEN NET_BUFSIZE
> 
> +typedef struct SendCo {
> +    Coroutine *co;
> +    GQueue send_list;
> +    bool done;
> +    int ret;
> +} SendCo;
> +
> +typedef struct SendEntry {
> +    ssize_t size;
> +    uint8_t buf[];
> +} SendEntry;
> +
>  typedef struct MirrorState {
>      NetFilterState parent_obj;
>      char *indev;
> @@ -38,59 +52,101 @@ typedef struct MirrorState {
>      CharBackend chr_in;
>      CharBackend chr_out;
>      SocketReadState rs;
> +    SendCo sendco;
>      bool vnet_hdr;
>  } MirrorState;
> 
> -static int filter_send(MirrorState *s,
> -                       const struct iovec *iov,
> -                       int iovcnt)
> +static void coroutine_fn _filter_send(void *opaque)
>  {
> +    MirrorState *s = opaque;
> +    SendCo *sendco = &s->sendco;
>      NetFilterState *nf = NETFILTER(s);
>      int ret = 0;
> -    ssize_t size = 0;
> -    uint32_t len = 0;
> -    char *buf;
> -
> -    size = iov_size(iov, iovcnt);
> -    if (!size) {
> -        return 0;
> -    }
> 
> -    len = htonl(size);
> -    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> -    if (ret != sizeof(len)) {
> -        goto err;
> -    }
> +    while (!g_queue_is_empty(&sendco->send_list)) {
> +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> +        uint32_t len = htonl(entry->size);
> 
> -    if (s->vnet_hdr) {
> -        /*
> -         * If vnet_hdr = on, we send vnet header len to make other
> -         * module(like colo-compare) know how to parse net
> -         * packet correctly.
> -         */
> -        ssize_t vnet_hdr_len;
> +        ret = qemu_chr_fe_write_all(&s->chr_out,
> +                                    (uint8_t *)&len,
> +                                    sizeof(len));
> +        if (ret != sizeof(len)) {
> +            g_free(entry);
> +            goto err;
> +        }
> 
> -        vnet_hdr_len = nf->netdev->vnet_hdr_len;
> +        if (s->vnet_hdr) {
> +            /*
> +             * If vnet_hdr = on, we send vnet header len to make other
> +             * module(like colo-compare) know how to parse net
> +             * packet correctly.
> +             */
> +
> +            len = htonl(nf->netdev->vnet_hdr_len);
> +            ret = qemu_chr_fe_write_all(&s->chr_out,
> +                                        (uint8_t *)&len,
> +                                        sizeof(len));
> +            if (ret != sizeof(len)) {
> +                g_free(entry);
> +                goto err;
> +            }
> +        }
> 
> -        len = htonl(vnet_hdr_len);
> -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> -        if (ret != sizeof(len)) {
> +        ret = qemu_chr_fe_write_all(&s->chr_out,
> +                                    (uint8_t *)entry->buf,
> +                                    entry->size);
> +        if (ret != entry->size) {
> +            g_free(entry);
>              goto err;
>          }
> -    }
> 
> -    buf = g_malloc(size);
> -    iov_to_buf(iov, iovcnt, 0, buf, size);
> -    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> -    g_free(buf);
> -    if (ret != size) {
> -        goto err;
> +        g_free(entry);
>      }
> 
> -    return 0;
> +    sendco->ret = 0;
> +    goto out;
> 
>  err:
> -    return ret < 0 ? ret : -EIO;
> +    while (!g_queue_is_empty(&sendco->send_list)) {
> +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> +        g_free(entry);
> +    }
> +    sendco->ret = ret < 0 ? ret : -EIO;
> +out:
> +    sendco->co = NULL;
> +    sendco->done = true;
> +    aio_wait_kick();
> +}
> +
> +static int filter_send(MirrorState *s,
> +                       const struct iovec *iov,
> +                       int iovcnt)
> +{
> +    SendCo *sendco = &s->sendco;
> +    SendEntry *entry;
> +
> +    ssize_t size = iov_size(iov, iovcnt);
> +    if (!size) {
> +        return 0;
> +    }
> +
> +    entry = g_malloc(sizeof(SendEntry) + size);
> +    entry->size = size;
> +    iov_to_buf(iov, iovcnt, 0, entry->buf, size);
> +    g_queue_push_head(&sendco->send_list, entry);
> +
> +    if (sendco->done) {
> +        sendco->co = qemu_coroutine_create(_filter_send, s);
> +        sendco->done = false;
> +        qemu_coroutine_enter(sendco->co);
> +        if (sendco->done) {
> +            /* report early errors */
> +            return sendco->ret;
> +        }
> +    }
> +
> +    /* assume success */
> +    return 0;
>  }
> 
>  static void redirector_to_filter(NetFilterState *nf, @@ -194,6 +250,10 @@
> static void filter_mirror_cleanup(NetFilterState *nf)  {
>      MirrorState *s = FILTER_MIRROR(nf);
> 
> +    AIO_WAIT_WHILE(NULL, !s->sendco.done);
> +
> +    g_queue_clear(&s->sendco.send_list);
> +
>      qemu_chr_fe_deinit(&s->chr_out, false);  }
> 
> @@ -201,6 +261,10 @@ static void filter_redirector_cleanup(NetFilterState
> *nf)  {
>      MirrorState *s = FILTER_REDIRECTOR(nf);
> 
> +    AIO_WAIT_WHILE(NULL, !s->sendco.done);
> +
> +    g_queue_clear(&s->sendco.send_list);
> +
>      qemu_chr_fe_deinit(&s->chr_in, false);
>      qemu_chr_fe_deinit(&s->chr_out, false);  } @@ -224,6 +288,9 @@ static
> void filter_mirror_setup(NetFilterState *nf, Error **errp)
>      }
> 
>      qemu_chr_fe_init(&s->chr_out, chr, errp);
> +
> +    s->sendco.done = true;
> +    g_queue_init(&s->sendco.send_list);
>  }
> 
>  static void redirector_rs_finalize(SocketReadState *rs) @@ -281,6 +348,9
> @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
>              return;
>          }
>      }
> +
> +    s->sendco.done = true;
> +    g_queue_init(&s->sendco.send_list);
>  }
> 
>  static void filter_mirror_class_init(ObjectClass *oc, void *data)
> --
> 2.20.1

Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Lukas Straub]

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

On Fri, 8 May 2020 06:28:45 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Friday, May 8, 2020 2:08 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> > compare_chr_send
> > 
> > On Fri, 8 May 2020 02:19:00 +0000
> > "Zhang, Chen" <chen.zhang@intel.com> wrote:  
> > > > > No need to init the notify_sendco each time, because the notify
> > > > > dev just  
> > > > an optional parameter.  
> > > > > You can use the if (s->notify_dev) here. Just Xen use the  
> > chr_notify_dev.  
> > > >
> > > > Ok, I will change that and the code below in the next version.
> > > >  
> > > > > Overall, make the chr_send job to coroutine is a good idea. It
> > > > > looks good  
> > > > for me.  
> > > > > And your patch inspired me, it looks we can re-use the
> > > > > compare_chr_send  
> > > > code on filter mirror/redirector too.
> > > >
> > > > I already have patch for that, but I don't think it is a good idea,
> > > > because the guest then can send packets faster than colo-compare can
> > > > process. This leads bufferbloat and the performance drops in my tests:
> > > > Client-to-server tcp:
> > > > without patch: ~66 Mbit/s
> > > > with patch: ~59 Mbit/s
> > > > Server-to-client tcp:
> > > > without patch: ~702 Kbit/s
> > > > with patch: ~328 Kbit/s  
> > >
> > > Oh, a big performance drop, is that caused by memcpy/zero_copy parts ?
> > >
> > > Thanks
> > > Zhang Chen  
> > 
> > No, there is no memcpy overhead with this patch, see below.  
> 
> I means for the filter mirror/redirector parts why coroutine will lead huge performance drop?

It's because having a additional buffer before the network bottleneck (colo-compare in our case) confuses TCP's congestion-control:
TCP will speed up the data transfer until packets start to drop (or the network interface is blocked). This feedback has to be quick so TCP can select a suitable transfer speed. But with the patch, the guest will fill the buffer as fast as it can (it does not "see" the slow bandwidth of colo-compare behind the buffer) until it it hits against the TCP congestion window. At this point TCP drastically reduces its transfer speed and it stays low because the full buffer delays the packets so it doesn't receive ACK's so it can't speed up the transfer again. Until the buffer is empty again (can take up to a second in my tests). Then this cycle repeats.

Regards,
Lukas Straub

> Thanks
> Zhang Chen
> 
> > 
> > Regards,
> > Lukas Straub
> > 
> > ---
> >  net/filter-mirror.c | 142 +++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 106 insertions(+), 36 deletions(-)
> > 
> > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > d83e815545..6bcd317502 100644
> > --- a/net/filter-mirror.c
> > +++ b/net/filter-mirror.c
> > @@ -20,6 +20,8 @@
> >  #include "chardev/char-fe.h"
> >  #include "qemu/iov.h"
> >  #include "qemu/sockets.h"
> > +#include "block/aio-wait.h"
> > +#include "qemu/coroutine.h"
> > 
> >  #define FILTER_MIRROR(obj) \
> >      OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR) @@ -31,6
> > +33,18 @@  #define TYPE_FILTER_REDIRECTOR "filter-redirector"
> >  #define REDIRECTOR_MAX_LEN NET_BUFSIZE
> > 
> > +typedef struct SendCo {
> > +    Coroutine *co;
> > +    GQueue send_list;
> > +    bool done;
> > +    int ret;
> > +} SendCo;
> > +
> > +typedef struct SendEntry {
> > +    ssize_t size;
> > +    uint8_t buf[];
> > +} SendEntry;
> > +
> >  typedef struct MirrorState {
> >      NetFilterState parent_obj;
> >      char *indev;
> > @@ -38,59 +52,101 @@ typedef struct MirrorState {
> >      CharBackend chr_in;
> >      CharBackend chr_out;
> >      SocketReadState rs;
> > +    SendCo sendco;
> >      bool vnet_hdr;
> >  } MirrorState;
> > 
> > -static int filter_send(MirrorState *s,
> > -                       const struct iovec *iov,
> > -                       int iovcnt)
> > +static void coroutine_fn _filter_send(void *opaque)
> >  {
> > +    MirrorState *s = opaque;
> > +    SendCo *sendco = &s->sendco;
> >      NetFilterState *nf = NETFILTER(s);
> >      int ret = 0;
> > -    ssize_t size = 0;
> > -    uint32_t len = 0;
> > -    char *buf;
> > -
> > -    size = iov_size(iov, iovcnt);
> > -    if (!size) {
> > -        return 0;
> > -    }
> > 
> > -    len = htonl(size);
> > -    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> > -    if (ret != sizeof(len)) {
> > -        goto err;
> > -    }
> > +    while (!g_queue_is_empty(&sendco->send_list)) {
> > +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > +        uint32_t len = htonl(entry->size);
> > 
> > -    if (s->vnet_hdr) {
> > -        /*
> > -         * If vnet_hdr = on, we send vnet header len to make other
> > -         * module(like colo-compare) know how to parse net
> > -         * packet correctly.
> > -         */
> > -        ssize_t vnet_hdr_len;
> > +        ret = qemu_chr_fe_write_all(&s->chr_out,
> > +                                    (uint8_t *)&len,
> > +                                    sizeof(len));
> > +        if (ret != sizeof(len)) {
> > +            g_free(entry);
> > +            goto err;
> > +        }
> > 
> > -        vnet_hdr_len = nf->netdev->vnet_hdr_len;
> > +        if (s->vnet_hdr) {
> > +            /*
> > +             * If vnet_hdr = on, we send vnet header len to make other
> > +             * module(like colo-compare) know how to parse net
> > +             * packet correctly.
> > +             */
> > +
> > +            len = htonl(nf->netdev->vnet_hdr_len);
> > +            ret = qemu_chr_fe_write_all(&s->chr_out,
> > +                                        (uint8_t *)&len,
> > +                                        sizeof(len));
> > +            if (ret != sizeof(len)) {
> > +                g_free(entry);
> > +                goto err;
> > +            }
> > +        }
> > 
> > -        len = htonl(vnet_hdr_len);
> > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len, sizeof(len));
> > -        if (ret != sizeof(len)) {
> > +        ret = qemu_chr_fe_write_all(&s->chr_out,
> > +                                    (uint8_t *)entry->buf,
> > +                                    entry->size);
> > +        if (ret != entry->size) {
> > +            g_free(entry);
> >              goto err;
> >          }
> > -    }
> > 
> > -    buf = g_malloc(size);
> > -    iov_to_buf(iov, iovcnt, 0, buf, size);
> > -    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> > -    g_free(buf);
> > -    if (ret != size) {
> > -        goto err;
> > +        g_free(entry);
> >      }
> > 
> > -    return 0;
> > +    sendco->ret = 0;
> > +    goto out;
> > 
> >  err:
> > -    return ret < 0 ? ret : -EIO;
> > +    while (!g_queue_is_empty(&sendco->send_list)) {
> > +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > +        g_free(entry);
> > +    }
> > +    sendco->ret = ret < 0 ? ret : -EIO;
> > +out:
> > +    sendco->co = NULL;
> > +    sendco->done = true;
> > +    aio_wait_kick();
> > +}
> > +
> > +static int filter_send(MirrorState *s,
> > +                       const struct iovec *iov,
> > +                       int iovcnt)
> > +{
> > +    SendCo *sendco = &s->sendco;
> > +    SendEntry *entry;
> > +
> > +    ssize_t size = iov_size(iov, iovcnt);
> > +    if (!size) {
> > +        return 0;
> > +    }
> > +
> > +    entry = g_malloc(sizeof(SendEntry) + size);
> > +    entry->size = size;
> > +    iov_to_buf(iov, iovcnt, 0, entry->buf, size);
> > +    g_queue_push_head(&sendco->send_list, entry);
> > +
> > +    if (sendco->done) {
> > +        sendco->co = qemu_coroutine_create(_filter_send, s);
> > +        sendco->done = false;
> > +        qemu_coroutine_enter(sendco->co);
> > +        if (sendco->done) {
> > +            /* report early errors */
> > +            return sendco->ret;
> > +        }
> > +    }
> > +
> > +    /* assume success */
> > +    return 0;
> >  }
> > 
> >  static void redirector_to_filter(NetFilterState *nf, @@ -194,6 +250,10 @@
> > static void filter_mirror_cleanup(NetFilterState *nf)  {
> >      MirrorState *s = FILTER_MIRROR(nf);
> > 
> > +    AIO_WAIT_WHILE(NULL, !s->sendco.done);
> > +
> > +    g_queue_clear(&s->sendco.send_list);
> > +
> >      qemu_chr_fe_deinit(&s->chr_out, false);  }
> > 
> > @@ -201,6 +261,10 @@ static void filter_redirector_cleanup(NetFilterState
> > *nf)  {
> >      MirrorState *s = FILTER_REDIRECTOR(nf);
> > 
> > +    AIO_WAIT_WHILE(NULL, !s->sendco.done);
> > +
> > +    g_queue_clear(&s->sendco.send_list);
> > +
> >      qemu_chr_fe_deinit(&s->chr_in, false);
> >      qemu_chr_fe_deinit(&s->chr_out, false);  } @@ -224,6 +288,9 @@ static
> > void filter_mirror_setup(NetFilterState *nf, Error **errp)
> >      }
> > 
> >      qemu_chr_fe_init(&s->chr_out, chr, errp);
> > +
> > +    s->sendco.done = true;
> > +    g_queue_init(&s->sendco.send_list);
> >  }
> > 
> >  static void redirector_rs_finalize(SocketReadState *rs) @@ -281,6 +348,9
> > @@ static void filter_redirector_setup(NetFilterState *nf, Error **errp)
> >              return;
> >          }
> >      }
> > +
> > +    s->sendco.done = true;
> > +    g_queue_init(&s->sendco.send_list);
> >  }
> > 
> >  static void filter_mirror_class_init(ObjectClass *oc, void *data)
> > --
> > 2.20.1  
> 


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

RE: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, May 8, 2020 3:56 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
> 
> On Fri, 8 May 2020 06:28:45 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Friday, May 8, 2020 2:08 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: Re: [PATCH v4 3/6] net/colo-compare.c: Fix deadlock in
> > > compare_chr_send
> > >
> > > On Fri, 8 May 2020 02:19:00 +0000
> > > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > > > > > No need to init the notify_sendco each time, because the
> > > > > > notify dev just
> > > > > an optional parameter.
> > > > > > You can use the if (s->notify_dev) here. Just Xen use the
> > > chr_notify_dev.
> > > > >
> > > > > Ok, I will change that and the code below in the next version.
> > > > >
> > > > > > Overall, make the chr_send job to coroutine is a good idea. It
> > > > > > looks good
> > > > > for me.
> > > > > > And your patch inspired me, it looks we can re-use the
> > > > > > compare_chr_send
> > > > > code on filter mirror/redirector too.
> > > > >
> > > > > I already have patch for that, but I don't think it is a good
> > > > > idea, because the guest then can send packets faster than
> > > > > colo-compare can process. This leads bufferbloat and the
> performance drops in my tests:
> > > > > Client-to-server tcp:
> > > > > without patch: ~66 Mbit/s
> > > > > with patch: ~59 Mbit/s
> > > > > Server-to-client tcp:
> > > > > without patch: ~702 Kbit/s
> > > > > with patch: ~328 Kbit/s
> > > >
> > > > Oh, a big performance drop, is that caused by memcpy/zero_copy
> parts ?
> > > >
> > > > Thanks
> > > > Zhang Chen
> > >
> > > No, there is no memcpy overhead with this patch, see below.
> >
> > I means for the filter mirror/redirector parts why coroutine will lead huge
> performance drop?
> 
> It's because having a additional buffer before the network bottleneck (colo-
> compare in our case) confuses TCP's congestion-control:
> TCP will speed up the data transfer until packets start to drop (or the
> network interface is blocked). This feedback has to be quick so TCP can select
> a suitable transfer speed. But with the patch, the guest will fill the buffer as
> fast as it can (it does not "see" the slow bandwidth of colo-compare behind
> the buffer) until it it hits against the TCP congestion window. At this point TCP
> drastically reduces its transfer speed and it stays low because the full buffer
> delays the packets so it doesn't receive ACK's so it can't speed up the
> transfer again. Until the buffer is empty again (can take up to a second in my
> tests). Then this cycle repeats.

Make sense!
After fix above issue:
Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > ---
> > >  net/filter-mirror.c | 142
> > > +++++++++++++++++++++++++++++++++-----------
> > >  1 file changed, 106 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/net/filter-mirror.c b/net/filter-mirror.c index
> > > d83e815545..6bcd317502 100644
> > > --- a/net/filter-mirror.c
> > > +++ b/net/filter-mirror.c
> > > @@ -20,6 +20,8 @@
> > >  #include "chardev/char-fe.h"
> > >  #include "qemu/iov.h"
> > >  #include "qemu/sockets.h"
> > > +#include "block/aio-wait.h"
> > > +#include "qemu/coroutine.h"
> > >
> > >  #define FILTER_MIRROR(obj) \
> > >      OBJECT_CHECK(MirrorState, (obj), TYPE_FILTER_MIRROR) @@ -31,6
> > > +33,18 @@  #define TYPE_FILTER_REDIRECTOR "filter-redirector"
> > >  #define REDIRECTOR_MAX_LEN NET_BUFSIZE
> > >
> > > +typedef struct SendCo {
> > > +    Coroutine *co;
> > > +    GQueue send_list;
> > > +    bool done;
> > > +    int ret;
> > > +} SendCo;
> > > +
> > > +typedef struct SendEntry {
> > > +    ssize_t size;
> > > +    uint8_t buf[];
> > > +} SendEntry;
> > > +
> > >  typedef struct MirrorState {
> > >      NetFilterState parent_obj;
> > >      char *indev;
> > > @@ -38,59 +52,101 @@ typedef struct MirrorState {
> > >      CharBackend chr_in;
> > >      CharBackend chr_out;
> > >      SocketReadState rs;
> > > +    SendCo sendco;
> > >      bool vnet_hdr;
> > >  } MirrorState;
> > >
> > > -static int filter_send(MirrorState *s,
> > > -                       const struct iovec *iov,
> > > -                       int iovcnt)
> > > +static void coroutine_fn _filter_send(void *opaque)
> > >  {
> > > +    MirrorState *s = opaque;
> > > +    SendCo *sendco = &s->sendco;
> > >      NetFilterState *nf = NETFILTER(s);
> > >      int ret = 0;
> > > -    ssize_t size = 0;
> > > -    uint32_t len = 0;
> > > -    char *buf;
> > > -
> > > -    size = iov_size(iov, iovcnt);
> > > -    if (!size) {
> > > -        return 0;
> > > -    }
> > >
> > > -    len = htonl(size);
> > > -    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len,
> sizeof(len));
> > > -    if (ret != sizeof(len)) {
> > > -        goto err;
> > > -    }
> > > +    while (!g_queue_is_empty(&sendco->send_list)) {
> > > +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > > +        uint32_t len = htonl(entry->size);
> > >
> > > -    if (s->vnet_hdr) {
> > > -        /*
> > > -         * If vnet_hdr = on, we send vnet header len to make other
> > > -         * module(like colo-compare) know how to parse net
> > > -         * packet correctly.
> > > -         */
> > > -        ssize_t vnet_hdr_len;
> > > +        ret = qemu_chr_fe_write_all(&s->chr_out,
> > > +                                    (uint8_t *)&len,
> > > +                                    sizeof(len));
> > > +        if (ret != sizeof(len)) {
> > > +            g_free(entry);
> > > +            goto err;
> > > +        }
> > >
> > > -        vnet_hdr_len = nf->netdev->vnet_hdr_len;
> > > +        if (s->vnet_hdr) {
> > > +            /*
> > > +             * If vnet_hdr = on, we send vnet header len to make other
> > > +             * module(like colo-compare) know how to parse net
> > > +             * packet correctly.
> > > +             */
> > > +
> > > +            len = htonl(nf->netdev->vnet_hdr_len);
> > > +            ret = qemu_chr_fe_write_all(&s->chr_out,
> > > +                                        (uint8_t *)&len,
> > > +                                        sizeof(len));
> > > +            if (ret != sizeof(len)) {
> > > +                g_free(entry);
> > > +                goto err;
> > > +            }
> > > +        }
> > >
> > > -        len = htonl(vnet_hdr_len);
> > > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)&len,
> sizeof(len));
> > > -        if (ret != sizeof(len)) {
> > > +        ret = qemu_chr_fe_write_all(&s->chr_out,
> > > +                                    (uint8_t *)entry->buf,
> > > +                                    entry->size);
> > > +        if (ret != entry->size) {
> > > +            g_free(entry);
> > >              goto err;
> > >          }
> > > -    }
> > >
> > > -    buf = g_malloc(size);
> > > -    iov_to_buf(iov, iovcnt, 0, buf, size);
> > > -    ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> > > -    g_free(buf);
> > > -    if (ret != size) {
> > > -        goto err;
> > > +        g_free(entry);
> > >      }
> > >
> > > -    return 0;
> > > +    sendco->ret = 0;
> > > +    goto out;
> > >
> > >  err:
> > > -    return ret < 0 ? ret : -EIO;
> > > +    while (!g_queue_is_empty(&sendco->send_list)) {
> > > +        SendEntry *entry = g_queue_pop_tail(&sendco->send_list);
> > > +        g_free(entry);
> > > +    }
> > > +    sendco->ret = ret < 0 ? ret : -EIO;
> > > +out:
> > > +    sendco->co = NULL;
> > > +    sendco->done = true;
> > > +    aio_wait_kick();
> > > +}
> > > +
> > > +static int filter_send(MirrorState *s,
> > > +                       const struct iovec *iov,
> > > +                       int iovcnt)
> > > +{
> > > +    SendCo *sendco = &s->sendco;
> > > +    SendEntry *entry;
> > > +
> > > +    ssize_t size = iov_size(iov, iovcnt);
> > > +    if (!size) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    entry = g_malloc(sizeof(SendEntry) + size);
> > > +    entry->size = size;
> > > +    iov_to_buf(iov, iovcnt, 0, entry->buf, size);
> > > +    g_queue_push_head(&sendco->send_list, entry);
> > > +
> > > +    if (sendco->done) {
> > > +        sendco->co = qemu_coroutine_create(_filter_send, s);
> > > +        sendco->done = false;
> > > +        qemu_coroutine_enter(sendco->co);
> > > +        if (sendco->done) {
> > > +            /* report early errors */
> > > +            return sendco->ret;
> > > +        }
> > > +    }
> > > +
> > > +    /* assume success */
> > > +    return 0;
> > >  }
> > >
> > >  static void redirector_to_filter(NetFilterState *nf, @@ -194,6
> > > +250,10 @@ static void filter_mirror_cleanup(NetFilterState *nf)  {
> > >      MirrorState *s = FILTER_MIRROR(nf);
> > >
> > > +    AIO_WAIT_WHILE(NULL, !s->sendco.done);
> > > +
> > > +    g_queue_clear(&s->sendco.send_list);
> > > +
> > >      qemu_chr_fe_deinit(&s->chr_out, false);  }
> > >
> > > @@ -201,6 +261,10 @@ static void
> > > filter_redirector_cleanup(NetFilterState
> > > *nf)  {
> > >      MirrorState *s = FILTER_REDIRECTOR(nf);
> > >
> > > +    AIO_WAIT_WHILE(NULL, !s->sendco.done);
> > > +
> > > +    g_queue_clear(&s->sendco.send_list);
> > > +
> > >      qemu_chr_fe_deinit(&s->chr_in, false);
> > >      qemu_chr_fe_deinit(&s->chr_out, false);  } @@ -224,6 +288,9 @@
> > > static void filter_mirror_setup(NetFilterState *nf, Error **errp)
> > >      }
> > >
> > >      qemu_chr_fe_init(&s->chr_out, chr, errp);
> > > +
> > > +    s->sendco.done = true;
> > > +    g_queue_init(&s->sendco.send_list);
> > >  }
> > >
> > >  static void redirector_rs_finalize(SocketReadState *rs) @@ -281,6
> > > +348,9 @@ static void filter_redirector_setup(NetFilterState *nf, Error
> **errp)
> > >              return;
> > >          }
> > >      }
> > > +
> > > +    s->sendco.done = true;
> > > +    g_queue_init(&s->sendco.send_list);
> > >  }
> > >
> > >  static void filter_mirror_class_init(ObjectClass *oc, void *data)
> > > --
> > > 2.20.1
> >

[PATCH v4 4/6] net/colo-compare.c: Only hexdump packets if tracing is enabled

[Lukas Straub]

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

Else the log will be flooded if there is a lot of network
traffic.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Zhang Chen <chen.zhang@intel.com>
---
 net/colo-compare.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 2a4e7f7c4e..56db3d3bfc 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -483,10 +483,12 @@ sec:
         g_queue_push_head(&conn->primary_list, ppkt);
         g_queue_push_head(&conn->secondary_list, spkt);
 
-        qemu_hexdump((char *)ppkt->data, stderr,
-                     "colo-compare ppkt", ppkt->size);
-        qemu_hexdump((char *)spkt->data, stderr,
-                     "colo-compare spkt", spkt->size);
+        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
+            qemu_hexdump((char *)ppkt->data, stderr,
+                        "colo-compare ppkt", ppkt->size);
+            qemu_hexdump((char *)spkt->data, stderr,
+                        "colo-compare spkt", spkt->size);
+        }
 
         colo_compare_inconsistency_notify(s);
     }
-- 
2.20.1


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

Re: [PATCH v4 4/6] net/colo-compare.c: Only hexdump packets if tracing is enabled

[Philippe Mathieu-Daudé]

On 5/4/20 12:28 PM, Lukas Straub wrote:
> Else the log will be flooded if there is a lot of network
> traffic.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> Reviewed-by: Zhang Chen <chen.zhang@intel.com>
> ---
>   net/colo-compare.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index 2a4e7f7c4e..56db3d3bfc 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -483,10 +483,12 @@ sec:
>           g_queue_push_head(&conn->primary_list, ppkt);
>           g_queue_push_head(&conn->secondary_list, spkt);
>   
> -        qemu_hexdump((char *)ppkt->data, stderr,
> -                     "colo-compare ppkt", ppkt->size);
> -        qemu_hexdump((char *)spkt->data, stderr,
> -                     "colo-compare spkt", spkt->size);
> +        if (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +            qemu_hexdump((char *)ppkt->data, stderr,
> +                        "colo-compare ppkt", ppkt->size);
> +            qemu_hexdump((char *)spkt->data, stderr,
> +                        "colo-compare spkt", spkt->size);
> +        }
>   
>           colo_compare_inconsistency_notify(s);
>       }
> 

Re: [PATCH v4 4/6] net/colo-compare.c: Only hexdump packets if tracing is enabled

[Philippe Mathieu-Daudé]

On 5/4/20 1:27 PM, Philippe Mathieu-Daudé wrote:
> On 5/4/20 12:28 PM, Lukas Straub wrote:
>> Else the log will be flooded if there is a lot of network
>> traffic.
>>
>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>> Reviewed-by: Zhang Chen <chen.zhang@intel.com>
>> ---
>>   net/colo-compare.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index 2a4e7f7c4e..56db3d3bfc 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -483,10 +483,12 @@ sec:
>>           g_queue_push_head(&conn->primary_list, ppkt);
>>           g_queue_push_head(&conn->secondary_list, spkt);
>> -        qemu_hexdump((char *)ppkt->data, stderr,
>> -                     "colo-compare ppkt", ppkt->size);
>> -        qemu_hexdump((char *)spkt->data, stderr,
>> -                     "colo-compare spkt", spkt->size);
>> +        if 
>> (trace_event_get_state_backends(TRACE_COLO_COMPARE_MISCOMPARE)) {
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
>> +            qemu_hexdump((char *)ppkt->data, stderr,
>> +                        "colo-compare ppkt", ppkt->size);
>> +            qemu_hexdump((char *)spkt->data, stderr,
>> +                        "colo-compare spkt", spkt->size);
>> +        }
>>           colo_compare_inconsistency_notify(s);
>>       }
>>

[PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Lukas Straub]

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

If the colo-compare object is removed before failover and a
checkpoint happens, qemu crashes because it tries to lock
the destroyed event_mtx in colo_notify_compares_event.

Fix this by checking if everything is initialized by
introducing a new variable colo_compare_active which
is protected by a new mutex colo_compare_mutex. The new mutex
also protects against concurrent access of the net_compares
list and makes sure that colo_notify_compares_event isn't
active while we destroy event_mtx and event_complete_cond.

With this it also is again possible to use colo without
colo-compare (periodic mode) and to use multiple colo-compare
for multiple network interfaces.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
 net/colo-compare.h |  1 +
 softmmu/vl.c       |  2 ++
 3 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 56db3d3bfc..c7572d75e9 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
 #define REGULAR_PACKET_CHECK_MS 3000
 #define DEFAULT_TIME_OUT_MS 3000
 
+static QemuMutex colo_compare_mutex;
+static bool colo_compare_active;
 static QemuMutex event_mtx;
 static QemuCond event_complete_cond;
 static int event_unhandled_count;
@@ -906,6 +908,12 @@ static void check_old_packet_regular(void *opaque)
 void colo_notify_compares_event(void *opaque, int event, Error **errp)
 {
     CompareState *s;
+    qemu_mutex_lock(&colo_compare_mutex);
+
+    if (!colo_compare_active) {
+        qemu_mutex_unlock(&colo_compare_mutex);
+        return;
+    }
 
     qemu_mutex_lock(&event_mtx);
     QTAILQ_FOREACH(s, &net_compares, next) {
@@ -919,6 +927,7 @@ void colo_notify_compares_event(void *opaque, int event, Error **errp)
     }
 
     qemu_mutex_unlock(&event_mtx);
+    qemu_mutex_unlock(&colo_compare_mutex);
 }
 
 static void colo_compare_timer_init(CompareState *s)
@@ -1274,7 +1283,14 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
                            s->vnet_hdr);
     }
 
+    qemu_mutex_lock(&colo_compare_mutex);
+    if (!colo_compare_active) {
+        qemu_mutex_init(&event_mtx);
+        qemu_cond_init(&event_complete_cond);
+        colo_compare_active = true;
+    }
     QTAILQ_INSERT_TAIL(&net_compares, s, next);
+    qemu_mutex_unlock(&colo_compare_mutex);
 
     s->out_sendco.s = s;
     s->out_sendco.chr = &s->chr_out;
@@ -1290,9 +1306,6 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
 
     g_queue_init(&s->conn_list);
 
-    qemu_mutex_init(&event_mtx);
-    qemu_cond_init(&event_complete_cond);
-
     s->connection_track_table = g_hash_table_new_full(connection_key_hash,
                                                       connection_key_equal,
                                                       g_free,
@@ -1384,12 +1397,19 @@ static void colo_compare_finalize(Object *obj)
 
     qemu_bh_delete(s->event_bh);
 
+    qemu_mutex_lock(&colo_compare_mutex);
     QTAILQ_FOREACH(tmp, &net_compares, next) {
         if (tmp == s) {
             QTAILQ_REMOVE(&net_compares, s, next);
             break;
         }
     }
+    if (QTAILQ_EMPTY(&net_compares)) {
+        colo_compare_active = false;
+        qemu_mutex_destroy(&event_mtx);
+        qemu_cond_destroy(&event_complete_cond);
+    }
+    qemu_mutex_unlock(&colo_compare_mutex);
 
     AioContext *ctx = iothread_get_aio_context(s->iothread);
     aio_context_acquire(ctx);
@@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object *obj)
         object_unref(OBJECT(s->iothread));
     }
 
-    qemu_mutex_destroy(&event_mtx);
-    qemu_cond_destroy(&event_complete_cond);
-
     g_free(s->pri_indev);
     g_free(s->sec_indev);
     g_free(s->outdev);
     g_free(s->notify_dev);
 }
 
+void colo_compare_init_globals(void)
+{
+    colo_compare_active = false;
+    qemu_mutex_init(&colo_compare_mutex);
+}
+
 static const TypeInfo colo_compare_info = {
     .name = TYPE_COLO_COMPARE,
     .parent = TYPE_OBJECT,
diff --git a/net/colo-compare.h b/net/colo-compare.h
index 22ddd512e2..eb483ac586 100644
--- a/net/colo-compare.h
+++ b/net/colo-compare.h
@@ -17,6 +17,7 @@
 #ifndef QEMU_COLO_COMPARE_H
 #define QEMU_COLO_COMPARE_H
 
+void colo_compare_init_globals(void);
 void colo_notify_compares_event(void *opaque, int event, Error **errp);
 void colo_compare_register_notifier(Notifier *notify);
 void colo_compare_unregister_notifier(Notifier *notify);
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 32c0047889..a913ed5469 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -112,6 +112,7 @@
 #include "qapi/qmp/qerror.h"
 #include "sysemu/iothread.h"
 #include "qemu/guest-random.h"
+#include "net/colo-compare.h"
 
 #define MAX_VIRTIO_CONSOLES 1
 
@@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char **envp)
     precopy_infrastructure_init();
     postcopy_infrastructure_init();
     monitor_init_globals();
+    colo_compare_init_globals();
 
     if (qcrypto_init(&err) < 0) {
         error_reportf_err(err, "cannot initialize crypto: ");
-- 
2.20.1


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

RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Monday, May 4, 2020 6:28 PM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-
> compare is active
> 
> If the colo-compare object is removed before failover and a checkpoint
> happens, qemu crashes because it tries to lock the destroyed event_mtx in
> colo_notify_compares_event.
> 
> Fix this by checking if everything is initialized by introducing a new variable
> colo_compare_active which is protected by a new mutex
> colo_compare_mutex. The new mutex also protects against concurrent
> access of the net_compares list and makes sure that
> colo_notify_compares_event isn't active while we destroy event_mtx and
> event_complete_cond.
> 
> With this it also is again possible to use colo without colo-compare (periodic
> mode) and to use multiple colo-compare for multiple network interfaces.
> 

Hi Lukas,

For this case I think we don't need to touch vl.c code, we can solve this issue from another perspective:
How to remove colo-compare?
User will use qemu-monitor or QMP command to disable an object, so we just need return operation failed
When user try to remove colo-compare object while COLO is running.

Thanks
Zhang Chen

> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
>  net/colo-compare.h |  1 +
>  softmmu/vl.c       |  2 ++
>  3 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 56db3d3bfc..c7572d75e9 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =  #define
> REGULAR_PACKET_CHECK_MS 3000  #define DEFAULT_TIME_OUT_MS 3000
> 
> +static QemuMutex colo_compare_mutex;
> +static bool colo_compare_active;
>  static QemuMutex event_mtx;
>  static QemuCond event_complete_cond;
>  static int event_unhandled_count;
> @@ -906,6 +908,12 @@ static void check_old_packet_regular(void *opaque)
> void colo_notify_compares_event(void *opaque, int event, Error **errp)  {
>      CompareState *s;
> +    qemu_mutex_lock(&colo_compare_mutex);
> +
> +    if (!colo_compare_active) {
> +        qemu_mutex_unlock(&colo_compare_mutex);
> +        return;
> +    }
> 
>      qemu_mutex_lock(&event_mtx);
>      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7 @@ void
> colo_notify_compares_event(void *opaque, int event, Error **errp)
>      }
> 
>      qemu_mutex_unlock(&event_mtx);
> +    qemu_mutex_unlock(&colo_compare_mutex);
>  }
> 
>  static void colo_compare_timer_init(CompareState *s) @@ -1274,7 +1283,14
> @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>                             s->vnet_hdr);
>      }
> 
> +    qemu_mutex_lock(&colo_compare_mutex);
> +    if (!colo_compare_active) {
> +        qemu_mutex_init(&event_mtx);
> +        qemu_cond_init(&event_complete_cond);
> +        colo_compare_active = true;
> +    }
>      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> +    qemu_mutex_unlock(&colo_compare_mutex);
> 
>      s->out_sendco.s = s;
>      s->out_sendco.chr = &s->chr_out;
> @@ -1290,9 +1306,6 @@ static void colo_compare_complete(UserCreatable
> *uc, Error **errp)
> 
>      g_queue_init(&s->conn_list);
> 
> -    qemu_mutex_init(&event_mtx);
> -    qemu_cond_init(&event_complete_cond);
> -
>      s->connection_track_table =
> g_hash_table_new_full(connection_key_hash,
>                                                        connection_key_equal,
>                                                        g_free, @@ -1384,12 +1397,19 @@ static void
> colo_compare_finalize(Object *obj)
> 
>      qemu_bh_delete(s->event_bh);
> 
> +    qemu_mutex_lock(&colo_compare_mutex);
>      QTAILQ_FOREACH(tmp, &net_compares, next) {
>          if (tmp == s) {
>              QTAILQ_REMOVE(&net_compares, s, next);
>              break;
>          }
>      }
> +    if (QTAILQ_EMPTY(&net_compares)) {
> +        colo_compare_active = false;
> +        qemu_mutex_destroy(&event_mtx);
> +        qemu_cond_destroy(&event_complete_cond);
> +    }
> +    qemu_mutex_unlock(&colo_compare_mutex);
> 
>      AioContext *ctx = iothread_get_aio_context(s->iothread);
>      aio_context_acquire(ctx);
> @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object *obj)
>          object_unref(OBJECT(s->iothread));
>      }
> 
> -    qemu_mutex_destroy(&event_mtx);
> -    qemu_cond_destroy(&event_complete_cond);
> -
>      g_free(s->pri_indev);
>      g_free(s->sec_indev);
>      g_free(s->outdev);
>      g_free(s->notify_dev);
>  }
> 
> +void colo_compare_init_globals(void)
> +{
> +    colo_compare_active = false;
> +    qemu_mutex_init(&colo_compare_mutex);
> +}
> +
>  static const TypeInfo colo_compare_info = {
>      .name = TYPE_COLO_COMPARE,
>      .parent = TYPE_OBJECT,
> diff --git a/net/colo-compare.h b/net/colo-compare.h index
> 22ddd512e2..eb483ac586 100644
> --- a/net/colo-compare.h
> +++ b/net/colo-compare.h
> @@ -17,6 +17,7 @@
>  #ifndef QEMU_COLO_COMPARE_H
>  #define QEMU_COLO_COMPARE_H
> 
> +void colo_compare_init_globals(void);
>  void colo_notify_compares_event(void *opaque, int event, Error **errp);
> void colo_compare_register_notifier(Notifier *notify);  void
> colo_compare_unregister_notifier(Notifier *notify); diff --git a/softmmu/vl.c
> b/softmmu/vl.c index 32c0047889..a913ed5469 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -112,6 +112,7 @@
>  #include "qapi/qmp/qerror.h"
>  #include "sysemu/iothread.h"
>  #include "qemu/guest-random.h"
> +#include "net/colo-compare.h"
> 
>  #define MAX_VIRTIO_CONSOLES 1
> 
> @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char **envp)
>      precopy_infrastructure_init();
>      postcopy_infrastructure_init();
>      monitor_init_globals();
> +    colo_compare_init_globals();
> 
>      if (qcrypto_init(&err) < 0) {
>          error_reportf_err(err, "cannot initialize crypto: ");
> --
> 2.20.1

Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Lukas Straub]

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

On Thu, 7 May 2020 11:38:04 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Monday, May 4, 2020 6:28 PM
> > To: qemu-devel <qemu-devel@nongnu.org>
> > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-
> > compare is active
> > 
> > If the colo-compare object is removed before failover and a checkpoint
> > happens, qemu crashes because it tries to lock the destroyed event_mtx in
> > colo_notify_compares_event.
> > 
> > Fix this by checking if everything is initialized by introducing a new variable
> > colo_compare_active which is protected by a new mutex
> > colo_compare_mutex. The new mutex also protects against concurrent
> > access of the net_compares list and makes sure that
> > colo_notify_compares_event isn't active while we destroy event_mtx and
> > event_complete_cond.
> > 
> > With this it also is again possible to use colo without colo-compare (periodic
> > mode) and to use multiple colo-compare for multiple network interfaces.
> >   
> 
> Hi Lukas,
> 
> For this case I think we don't need to touch vl.c code, we can solve this issue from another perspective:
> How to remove colo-compare?
> User will use qemu-monitor or QMP command to disable an object, so we just need return operation failed
> When user try to remove colo-compare object while COLO is running.

Yeah, but that still leaves the other problem that colo can't be used without colo-compare (qemu crashes then).

Regards,
Lukas Straub

> Thanks
> Zhang Chen
> 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
> >  net/colo-compare.h |  1 +
> >  softmmu/vl.c       |  2 ++
> >  3 files changed, 32 insertions(+), 6 deletions(-)
> > 
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 56db3d3bfc..c7572d75e9 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =  #define
> > REGULAR_PACKET_CHECK_MS 3000  #define DEFAULT_TIME_OUT_MS 3000
> > 
> > +static QemuMutex colo_compare_mutex;
> > +static bool colo_compare_active;
> >  static QemuMutex event_mtx;
> >  static QemuCond event_complete_cond;
> >  static int event_unhandled_count;
> > @@ -906,6 +908,12 @@ static void check_old_packet_regular(void *opaque)
> > void colo_notify_compares_event(void *opaque, int event, Error **errp)  {
> >      CompareState *s;
> > +    qemu_mutex_lock(&colo_compare_mutex);
> > +
> > +    if (!colo_compare_active) {
> > +        qemu_mutex_unlock(&colo_compare_mutex);
> > +        return;
> > +    }
> > 
> >      qemu_mutex_lock(&event_mtx);
> >      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7 @@ void
> > colo_notify_compares_event(void *opaque, int event, Error **errp)
> >      }
> > 
> >      qemu_mutex_unlock(&event_mtx);
> > +    qemu_mutex_unlock(&colo_compare_mutex);
> >  }
> > 
> >  static void colo_compare_timer_init(CompareState *s) @@ -1274,7 +1283,14
> > @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> >                             s->vnet_hdr);
> >      }
> > 
> > +    qemu_mutex_lock(&colo_compare_mutex);
> > +    if (!colo_compare_active) {
> > +        qemu_mutex_init(&event_mtx);
> > +        qemu_cond_init(&event_complete_cond);
> > +        colo_compare_active = true;
> > +    }
> >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > +    qemu_mutex_unlock(&colo_compare_mutex);
> > 
> >      s->out_sendco.s = s;
> >      s->out_sendco.chr = &s->chr_out;
> > @@ -1290,9 +1306,6 @@ static void colo_compare_complete(UserCreatable
> > *uc, Error **errp)
> > 
> >      g_queue_init(&s->conn_list);
> > 
> > -    qemu_mutex_init(&event_mtx);
> > -    qemu_cond_init(&event_complete_cond);
> > -
> >      s->connection_track_table =
> > g_hash_table_new_full(connection_key_hash,
> >                                                        connection_key_equal,
> >                                                        g_free, @@ -1384,12 +1397,19 @@ static void
> > colo_compare_finalize(Object *obj)
> > 
> >      qemu_bh_delete(s->event_bh);
> > 
> > +    qemu_mutex_lock(&colo_compare_mutex);
> >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> >          if (tmp == s) {
> >              QTAILQ_REMOVE(&net_compares, s, next);
> >              break;
> >          }
> >      }
> > +    if (QTAILQ_EMPTY(&net_compares)) {
> > +        colo_compare_active = false;
> > +        qemu_mutex_destroy(&event_mtx);
> > +        qemu_cond_destroy(&event_complete_cond);
> > +    }
> > +    qemu_mutex_unlock(&colo_compare_mutex);
> > 
> >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> >      aio_context_acquire(ctx);
> > @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object *obj)
> >          object_unref(OBJECT(s->iothread));
> >      }
> > 
> > -    qemu_mutex_destroy(&event_mtx);
> > -    qemu_cond_destroy(&event_complete_cond);
> > -
> >      g_free(s->pri_indev);
> >      g_free(s->sec_indev);
> >      g_free(s->outdev);
> >      g_free(s->notify_dev);
> >  }
> > 
> > +void colo_compare_init_globals(void)
> > +{
> > +    colo_compare_active = false;
> > +    qemu_mutex_init(&colo_compare_mutex);
> > +}
> > +
> >  static const TypeInfo colo_compare_info = {
> >      .name = TYPE_COLO_COMPARE,
> >      .parent = TYPE_OBJECT,
> > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > 22ddd512e2..eb483ac586 100644
> > --- a/net/colo-compare.h
> > +++ b/net/colo-compare.h
> > @@ -17,6 +17,7 @@
> >  #ifndef QEMU_COLO_COMPARE_H
> >  #define QEMU_COLO_COMPARE_H
> > 
> > +void colo_compare_init_globals(void);
> >  void colo_notify_compares_event(void *opaque, int event, Error **errp);
> > void colo_compare_register_notifier(Notifier *notify);  void
> > colo_compare_unregister_notifier(Notifier *notify); diff --git a/softmmu/vl.c
> > b/softmmu/vl.c index 32c0047889..a913ed5469 100644
> > --- a/softmmu/vl.c
> > +++ b/softmmu/vl.c
> > @@ -112,6 +112,7 @@
> >  #include "qapi/qmp/qerror.h"
> >  #include "sysemu/iothread.h"
> >  #include "qemu/guest-random.h"
> > +#include "net/colo-compare.h"
> > 
> >  #define MAX_VIRTIO_CONSOLES 1
> > 
> > @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char **envp)
> >      precopy_infrastructure_init();
> >      postcopy_infrastructure_init();
> >      monitor_init_globals();
> > +    colo_compare_init_globals();
> > 
> >      if (qcrypto_init(&err) < 0) {
> >          error_reportf_err(err, "cannot initialize crypto: ");
> > --
> > 2.20.1  
> 


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

RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Thursday, May 7, 2020 11:54 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> colo-compare is active
> 
> On Thu, 7 May 2020 11:38:04 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel <qemu-devel@nongnu.org>
> > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> > > colo- compare is active
> > >
> > > If the colo-compare object is removed before failover and a
> > > checkpoint happens, qemu crashes because it tries to lock the
> > > destroyed event_mtx in colo_notify_compares_event.
> > >
> > > Fix this by checking if everything is initialized by introducing a
> > > new variable colo_compare_active which is protected by a new mutex
> > > colo_compare_mutex. The new mutex also protects against concurrent
> > > access of the net_compares list and makes sure that
> > > colo_notify_compares_event isn't active while we destroy event_mtx
> > > and event_complete_cond.
> > >
> > > With this it also is again possible to use colo without colo-compare
> > > (periodic
> > > mode) and to use multiple colo-compare for multiple network interfaces.
> > >
> >
> > Hi Lukas,
> >
> > For this case I think we don't need to touch vl.c code, we can solve this
> issue from another perspective:
> > How to remove colo-compare?
> > User will use qemu-monitor or QMP command to disable an object, so we
> > just need return operation failed When user try to remove colo-compare
> object while COLO is running.
> 
> Yeah, but that still leaves the other problem that colo can't be used without
> colo-compare (qemu crashes then).

Yes, the COLO-compare is necessary module in COLO original design.
At most cases, user need it do dynamic sync.
For rare cases, maybe we can add a new colo-compare parameter to bypass all the network workload.

Thanks
Zhang Chen 

> 
> Regards,
> Lukas Straub
> 
> > Thanks
> > Zhang Chen
> >
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
> > >  net/colo-compare.h |  1 +
> > >  softmmu/vl.c       |  2 ++
> > >  3 files changed, 32 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > 56db3d3bfc..c7572d75e9 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
> > > #define REGULAR_PACKET_CHECK_MS 3000  #define
> DEFAULT_TIME_OUT_MS
> > > 3000
> > >
> > > +static QemuMutex colo_compare_mutex; static bool
> > > +colo_compare_active;
> > >  static QemuMutex event_mtx;
> > >  static QemuCond event_complete_cond;  static int
> > > event_unhandled_count; @@ -906,6 +908,12 @@ static void
> > > check_old_packet_regular(void *opaque) void
> > > colo_notify_compares_event(void *opaque, int event, Error **errp)  {
> > >      CompareState *s;
> > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > +
> > > +    if (!colo_compare_active) {
> > > +        qemu_mutex_unlock(&colo_compare_mutex);
> > > +        return;
> > > +    }
> > >
> > >      qemu_mutex_lock(&event_mtx);
> > >      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7 @@
> > > void colo_notify_compares_event(void *opaque, int event, Error **errp)
> > >      }
> > >
> > >      qemu_mutex_unlock(&event_mtx);
> > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > >  }
> > >
> > >  static void colo_compare_timer_init(CompareState *s) @@ -1274,7
> > > +1283,14 @@ static void colo_compare_complete(UserCreatable *uc,
> Error **errp)
> > >                             s->vnet_hdr);
> > >      }
> > >
> > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > +    if (!colo_compare_active) {
> > > +        qemu_mutex_init(&event_mtx);
> > > +        qemu_cond_init(&event_complete_cond);
> > > +        colo_compare_active = true;
> > > +    }
> > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > >
> > >      s->out_sendco.s = s;
> > >      s->out_sendco.chr = &s->chr_out; @@ -1290,9 +1306,6 @@ static
> > > void colo_compare_complete(UserCreatable
> > > *uc, Error **errp)
> > >
> > >      g_queue_init(&s->conn_list);
> > >
> > > -    qemu_mutex_init(&event_mtx);
> > > -    qemu_cond_init(&event_complete_cond);
> > > -
> > >      s->connection_track_table =
> > > g_hash_table_new_full(connection_key_hash,
> > >                                                        connection_key_equal,
> > >                                                        g_free, @@
> > > -1384,12 +1397,19 @@ static void colo_compare_finalize(Object *obj)
> > >
> > >      qemu_bh_delete(s->event_bh);
> > >
> > > +    qemu_mutex_lock(&colo_compare_mutex);
> > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > >          if (tmp == s) {
> > >              QTAILQ_REMOVE(&net_compares, s, next);
> > >              break;
> > >          }
> > >      }
> > > +    if (QTAILQ_EMPTY(&net_compares)) {
> > > +        colo_compare_active = false;
> > > +        qemu_mutex_destroy(&event_mtx);
> > > +        qemu_cond_destroy(&event_complete_cond);
> > > +    }
> > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > >
> > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > >      aio_context_acquire(ctx);
> > > @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object *obj)
> > >          object_unref(OBJECT(s->iothread));
> > >      }
> > >
> > > -    qemu_mutex_destroy(&event_mtx);
> > > -    qemu_cond_destroy(&event_complete_cond);
> > > -
> > >      g_free(s->pri_indev);
> > >      g_free(s->sec_indev);
> > >      g_free(s->outdev);
> > >      g_free(s->notify_dev);
> > >  }
> > >
> > > +void colo_compare_init_globals(void) {
> > > +    colo_compare_active = false;
> > > +    qemu_mutex_init(&colo_compare_mutex);
> > > +}
> > > +
> > >  static const TypeInfo colo_compare_info = {
> > >      .name = TYPE_COLO_COMPARE,
> > >      .parent = TYPE_OBJECT,
> > > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > > 22ddd512e2..eb483ac586 100644
> > > --- a/net/colo-compare.h
> > > +++ b/net/colo-compare.h
> > > @@ -17,6 +17,7 @@
> > >  #ifndef QEMU_COLO_COMPARE_H
> > >  #define QEMU_COLO_COMPARE_H
> > >
> > > +void colo_compare_init_globals(void);
> > >  void colo_notify_compares_event(void *opaque, int event, Error
> > > **errp); void colo_compare_register_notifier(Notifier *notify);
> > > void colo_compare_unregister_notifier(Notifier *notify); diff --git
> > > a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..a913ed5469 100644
> > > --- a/softmmu/vl.c
> > > +++ b/softmmu/vl.c
> > > @@ -112,6 +112,7 @@
> > >  #include "qapi/qmp/qerror.h"
> > >  #include "sysemu/iothread.h"
> > >  #include "qemu/guest-random.h"
> > > +#include "net/colo-compare.h"
> > >
> > >  #define MAX_VIRTIO_CONSOLES 1
> > >
> > > @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char
> **envp)
> > >      precopy_infrastructure_init();
> > >      postcopy_infrastructure_init();
> > >      monitor_init_globals();
> > > +    colo_compare_init_globals();
> > >
> > >      if (qcrypto_init(&err) < 0) {
> > >          error_reportf_err(err, "cannot initialize crypto: ");
> > > --
> > > 2.20.1
> >

Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Derek Su]

On 2020/5/8 上午10:26, Zhang, Chen wrote:
> 
> 
>> -----Original Message-----
>> From: Lukas Straub <lukasstraub2@web.de>
>> Sent: Thursday, May 7, 2020 11:54 PM
>> To: Zhang, Chen <chen.zhang@intel.com>
>> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
>> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
>> <pbonzini@redhat.com>
>> Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
>> colo-compare is active
>>
>> On Thu, 7 May 2020 11:38:04 +0000
>> "Zhang, Chen" <chen.zhang@intel.com> wrote:
>>
>>>> -----Original Message-----
>>>> From: Lukas Straub <lukasstraub2@web.de>
>>>> Sent: Monday, May 4, 2020 6:28 PM
>>>> To: qemu-devel <qemu-devel@nongnu.org>
>>>> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
>>>> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
>>>> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
>>>> <pbonzini@redhat.com>
>>>> Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
>>>> colo- compare is active
>>>>
>>>> If the colo-compare object is removed before failover and a
>>>> checkpoint happens, qemu crashes because it tries to lock the
>>>> destroyed event_mtx in colo_notify_compares_event.
>>>>
>>>> Fix this by checking if everything is initialized by introducing a
>>>> new variable colo_compare_active which is protected by a new mutex
>>>> colo_compare_mutex. The new mutex also protects against concurrent
>>>> access of the net_compares list and makes sure that
>>>> colo_notify_compares_event isn't active while we destroy event_mtx
>>>> and event_complete_cond.
>>>>
>>>> With this it also is again possible to use colo without colo-compare
>>>> (periodic
>>>> mode) and to use multiple colo-compare for multiple network interfaces.
>>>>
>>>
>>> Hi Lukas,
>>>
>>> For this case I think we don't need to touch vl.c code, we can solve this
>> issue from another perspective:
>>> How to remove colo-compare?
>>> User will use qemu-monitor or QMP command to disable an object, so we
>>> just need return operation failed When user try to remove colo-compare
>> object while COLO is running.
>>
>> Yeah, but that still leaves the other problem that colo can't be used without
>> colo-compare (qemu crashes then).
> 
> Yes, the COLO-compare is necessary module in COLO original design.
> At most cases, user need it do dynamic sync.
> For rare cases, maybe we can add a new colo-compare parameter to bypass all the network workload.

Hi, Chen

In our application, we only need "periodical mode" because of the 
performance issue, and have internal patch now. Is it OK to send the 
internal patch for review?

Thanks.

Regards,
Derek

> 
> Thanks
> Zhang Chen
> 
>>
>> Regards,
>> Lukas Straub
>>
>>> Thanks
>>> Zhang Chen
>>>
>>>> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
>>>> ---
>>>>   net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
>>>>   net/colo-compare.h |  1 +
>>>>   softmmu/vl.c       |  2 ++
>>>>   3 files changed, 32 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/net/colo-compare.c b/net/colo-compare.c index
>>>> 56db3d3bfc..c7572d75e9 100644
>>>> --- a/net/colo-compare.c
>>>> +++ b/net/colo-compare.c
>>>> @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
>>>> #define REGULAR_PACKET_CHECK_MS 3000  #define
>> DEFAULT_TIME_OUT_MS
>>>> 3000
>>>>
>>>> +static QemuMutex colo_compare_mutex; static bool
>>>> +colo_compare_active;
>>>>   static QemuMutex event_mtx;
>>>>   static QemuCond event_complete_cond;  static int
>>>> event_unhandled_count; @@ -906,6 +908,12 @@ static void
>>>> check_old_packet_regular(void *opaque) void
>>>> colo_notify_compares_event(void *opaque, int event, Error **errp)  {
>>>>       CompareState *s;
>>>> +    qemu_mutex_lock(&colo_compare_mutex);
>>>> +
>>>> +    if (!colo_compare_active) {
>>>> +        qemu_mutex_unlock(&colo_compare_mutex);
>>>> +        return;
>>>> +    }
>>>>
>>>>       qemu_mutex_lock(&event_mtx);
>>>>       QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7 @@
>>>> void colo_notify_compares_event(void *opaque, int event, Error **errp)
>>>>       }
>>>>
>>>>       qemu_mutex_unlock(&event_mtx);
>>>> +    qemu_mutex_unlock(&colo_compare_mutex);
>>>>   }
>>>>
>>>>   static void colo_compare_timer_init(CompareState *s) @@ -1274,7
>>>> +1283,14 @@ static void colo_compare_complete(UserCreatable *uc,
>> Error **errp)
>>>>                              s->vnet_hdr);
>>>>       }
>>>>
>>>> +    qemu_mutex_lock(&colo_compare_mutex);
>>>> +    if (!colo_compare_active) {
>>>> +        qemu_mutex_init(&event_mtx);
>>>> +        qemu_cond_init(&event_complete_cond);
>>>> +        colo_compare_active = true;
>>>> +    }
>>>>       QTAILQ_INSERT_TAIL(&net_compares, s, next);
>>>> +    qemu_mutex_unlock(&colo_compare_mutex);
>>>>
>>>>       s->out_sendco.s = s;
>>>>       s->out_sendco.chr = &s->chr_out; @@ -1290,9 +1306,6 @@ static
>>>> void colo_compare_complete(UserCreatable
>>>> *uc, Error **errp)
>>>>
>>>>       g_queue_init(&s->conn_list);
>>>>
>>>> -    qemu_mutex_init(&event_mtx);
>>>> -    qemu_cond_init(&event_complete_cond);
>>>> -
>>>>       s->connection_track_table =
>>>> g_hash_table_new_full(connection_key_hash,
>>>>                                                         connection_key_equal,
>>>>                                                         g_free, @@
>>>> -1384,12 +1397,19 @@ static void colo_compare_finalize(Object *obj)
>>>>
>>>>       qemu_bh_delete(s->event_bh);
>>>>
>>>> +    qemu_mutex_lock(&colo_compare_mutex);
>>>>       QTAILQ_FOREACH(tmp, &net_compares, next) {
>>>>           if (tmp == s) {
>>>>               QTAILQ_REMOVE(&net_compares, s, next);
>>>>               break;
>>>>           }
>>>>       }
>>>> +    if (QTAILQ_EMPTY(&net_compares)) {
>>>> +        colo_compare_active = false;
>>>> +        qemu_mutex_destroy(&event_mtx);
>>>> +        qemu_cond_destroy(&event_complete_cond);
>>>> +    }
>>>> +    qemu_mutex_unlock(&colo_compare_mutex);
>>>>
>>>>       AioContext *ctx = iothread_get_aio_context(s->iothread);
>>>>       aio_context_acquire(ctx);
>>>> @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object *obj)
>>>>           object_unref(OBJECT(s->iothread));
>>>>       }
>>>>
>>>> -    qemu_mutex_destroy(&event_mtx);
>>>> -    qemu_cond_destroy(&event_complete_cond);
>>>> -
>>>>       g_free(s->pri_indev);
>>>>       g_free(s->sec_indev);
>>>>       g_free(s->outdev);
>>>>       g_free(s->notify_dev);
>>>>   }
>>>>
>>>> +void colo_compare_init_globals(void) {
>>>> +    colo_compare_active = false;
>>>> +    qemu_mutex_init(&colo_compare_mutex);
>>>> +}
>>>> +
>>>>   static const TypeInfo colo_compare_info = {
>>>>       .name = TYPE_COLO_COMPARE,
>>>>       .parent = TYPE_OBJECT,
>>>> diff --git a/net/colo-compare.h b/net/colo-compare.h index
>>>> 22ddd512e2..eb483ac586 100644
>>>> --- a/net/colo-compare.h
>>>> +++ b/net/colo-compare.h
>>>> @@ -17,6 +17,7 @@
>>>>   #ifndef QEMU_COLO_COMPARE_H
>>>>   #define QEMU_COLO_COMPARE_H
>>>>
>>>> +void colo_compare_init_globals(void);
>>>>   void colo_notify_compares_event(void *opaque, int event, Error
>>>> **errp); void colo_compare_register_notifier(Notifier *notify);
>>>> void colo_compare_unregister_notifier(Notifier *notify); diff --git
>>>> a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..a913ed5469 100644
>>>> --- a/softmmu/vl.c
>>>> +++ b/softmmu/vl.c
>>>> @@ -112,6 +112,7 @@
>>>>   #include "qapi/qmp/qerror.h"
>>>>   #include "sysemu/iothread.h"
>>>>   #include "qemu/guest-random.h"
>>>> +#include "net/colo-compare.h"
>>>>
>>>>   #define MAX_VIRTIO_CONSOLES 1
>>>>
>>>> @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char
>> **envp)
>>>>       precopy_infrastructure_init();
>>>>       postcopy_infrastructure_init();
>>>>       monitor_init_globals();
>>>> +    colo_compare_init_globals();
>>>>
>>>>       if (qcrypto_init(&err) < 0) {
>>>>           error_reportf_err(err, "cannot initialize crypto: ");
>>>> --
>>>> 2.20.1
>>>
> 

Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Lukas Straub]

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

On Fri, 8 May 2020 02:26:21 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Thursday, May 7, 2020 11:54 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> > colo-compare is active
> > 
> > On Thu, 7 May 2020 11:38:04 +0000
> > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > Sent: Monday, May 4, 2020 6:28 PM
> > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > <pbonzini@redhat.com>
> > > > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> > > > colo- compare is active
> > > >
> > > > If the colo-compare object is removed before failover and a
> > > > checkpoint happens, qemu crashes because it tries to lock the
> > > > destroyed event_mtx in colo_notify_compares_event.
> > > >
> > > > Fix this by checking if everything is initialized by introducing a
> > > > new variable colo_compare_active which is protected by a new mutex
> > > > colo_compare_mutex. The new mutex also protects against concurrent
> > > > access of the net_compares list and makes sure that
> > > > colo_notify_compares_event isn't active while we destroy event_mtx
> > > > and event_complete_cond.
> > > >
> > > > With this it also is again possible to use colo without colo-compare
> > > > (periodic
> > > > mode) and to use multiple colo-compare for multiple network interfaces.
> > > >  
> > >
> > > Hi Lukas,
> > >
> > > For this case I think we don't need to touch vl.c code, we can solve this  
> > issue from another perspective:  
> > > How to remove colo-compare?
> > > User will use qemu-monitor or QMP command to disable an object, so we
> > > just need return operation failed When user try to remove colo-compare  
> > object while COLO is running.
> > 
> > Yeah, but that still leaves the other problem that colo can't be used without
> > colo-compare (qemu crashes then).  
> 
> Yes, the COLO-compare is necessary module in COLO original design.
> At most cases, user need it do dynamic sync.
> For rare cases, maybe we can add a new colo-compare parameter to bypass all the network workload.

I think such an parameter would only be a workaround instead of a real solution like this patch.

Regards,
Lukas Straub

> Thanks
> Zhang Chen 
> 
> > 
> > Regards,
> > Lukas Straub
> >   
> > > Thanks
> > > Zhang Chen
> > >  
> > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > ---
> > > >  net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
> > > >  net/colo-compare.h |  1 +
> > > >  softmmu/vl.c       |  2 ++
> > > >  3 files changed, 32 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > 56db3d3bfc..c7572d75e9 100644
> > > > --- a/net/colo-compare.c
> > > > +++ b/net/colo-compare.c
> > > > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
> > > > #define REGULAR_PACKET_CHECK_MS 3000  #define  
> > DEFAULT_TIME_OUT_MS  
> > > > 3000
> > > >
> > > > +static QemuMutex colo_compare_mutex; static bool
> > > > +colo_compare_active;
> > > >  static QemuMutex event_mtx;
> > > >  static QemuCond event_complete_cond;  static int
> > > > event_unhandled_count; @@ -906,6 +908,12 @@ static void
> > > > check_old_packet_regular(void *opaque) void
> > > > colo_notify_compares_event(void *opaque, int event, Error **errp)  {
> > > >      CompareState *s;
> > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > +
> > > > +    if (!colo_compare_active) {
> > > > +        qemu_mutex_unlock(&colo_compare_mutex);
> > > > +        return;
> > > > +    }
> > > >
> > > >      qemu_mutex_lock(&event_mtx);
> > > >      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7 @@
> > > > void colo_notify_compares_event(void *opaque, int event, Error **errp)
> > > >      }
> > > >
> > > >      qemu_mutex_unlock(&event_mtx);
> > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > >  }
> > > >
> > > >  static void colo_compare_timer_init(CompareState *s) @@ -1274,7
> > > > +1283,14 @@ static void colo_compare_complete(UserCreatable *uc,  
> > Error **errp)  
> > > >                             s->vnet_hdr);
> > > >      }
> > > >
> > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > +    if (!colo_compare_active) {
> > > > +        qemu_mutex_init(&event_mtx);
> > > > +        qemu_cond_init(&event_complete_cond);
> > > > +        colo_compare_active = true;
> > > > +    }
> > > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > >
> > > >      s->out_sendco.s = s;
> > > >      s->out_sendco.chr = &s->chr_out; @@ -1290,9 +1306,6 @@ static
> > > > void colo_compare_complete(UserCreatable
> > > > *uc, Error **errp)
> > > >
> > > >      g_queue_init(&s->conn_list);
> > > >
> > > > -    qemu_mutex_init(&event_mtx);
> > > > -    qemu_cond_init(&event_complete_cond);
> > > > -
> > > >      s->connection_track_table =
> > > > g_hash_table_new_full(connection_key_hash,
> > > >                                                        connection_key_equal,
> > > >                                                        g_free, @@
> > > > -1384,12 +1397,19 @@ static void colo_compare_finalize(Object *obj)
> > > >
> > > >      qemu_bh_delete(s->event_bh);
> > > >
> > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > > >          if (tmp == s) {
> > > >              QTAILQ_REMOVE(&net_compares, s, next);
> > > >              break;
> > > >          }
> > > >      }
> > > > +    if (QTAILQ_EMPTY(&net_compares)) {
> > > > +        colo_compare_active = false;
> > > > +        qemu_mutex_destroy(&event_mtx);
> > > > +        qemu_cond_destroy(&event_complete_cond);
> > > > +    }
> > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > >
> > > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > >      aio_context_acquire(ctx);
> > > > @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object *obj)
> > > >          object_unref(OBJECT(s->iothread));
> > > >      }
> > > >
> > > > -    qemu_mutex_destroy(&event_mtx);
> > > > -    qemu_cond_destroy(&event_complete_cond);
> > > > -
> > > >      g_free(s->pri_indev);
> > > >      g_free(s->sec_indev);
> > > >      g_free(s->outdev);
> > > >      g_free(s->notify_dev);
> > > >  }
> > > >
> > > > +void colo_compare_init_globals(void) {
> > > > +    colo_compare_active = false;
> > > > +    qemu_mutex_init(&colo_compare_mutex);
> > > > +}
> > > > +
> > > >  static const TypeInfo colo_compare_info = {
> > > >      .name = TYPE_COLO_COMPARE,
> > > >      .parent = TYPE_OBJECT,
> > > > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > > > 22ddd512e2..eb483ac586 100644
> > > > --- a/net/colo-compare.h
> > > > +++ b/net/colo-compare.h
> > > > @@ -17,6 +17,7 @@
> > > >  #ifndef QEMU_COLO_COMPARE_H
> > > >  #define QEMU_COLO_COMPARE_H
> > > >
> > > > +void colo_compare_init_globals(void);
> > > >  void colo_notify_compares_event(void *opaque, int event, Error
> > > > **errp); void colo_compare_register_notifier(Notifier *notify);
> > > > void colo_compare_unregister_notifier(Notifier *notify); diff --git
> > > > a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..a913ed5469 100644
> > > > --- a/softmmu/vl.c
> > > > +++ b/softmmu/vl.c
> > > > @@ -112,6 +112,7 @@
> > > >  #include "qapi/qmp/qerror.h"
> > > >  #include "sysemu/iothread.h"
> > > >  #include "qemu/guest-random.h"
> > > > +#include "net/colo-compare.h"
> > > >
> > > >  #define MAX_VIRTIO_CONSOLES 1
> > > >
> > > > @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char  
> > **envp)  
> > > >      precopy_infrastructure_init();
> > > >      postcopy_infrastructure_init();
> > > >      monitor_init_globals();
> > > > +    colo_compare_init_globals();
> > > >
> > > >      if (qcrypto_init(&err) < 0) {
> > > >          error_reportf_err(err, "cannot initialize crypto: ");
> > > > --
> > > > 2.20.1  
> > >  
> 


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

RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, May 8, 2020 2:11 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> colo-compare is active
> 
> On Fri, 8 May 2020 02:26:21 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Thursday, May 7, 2020 11:54 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > that colo-compare is active
> > >
> > > On Thu, 7 May 2020 11:38:04 +0000
> > > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > > Sent: Monday, May 4, 2020 6:28 PM
> > > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> > > > > Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > > <pbonzini@redhat.com>
> > > > > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > > > that
> > > > > colo- compare is active
> > > > >
> > > > > If the colo-compare object is removed before failover and a
> > > > > checkpoint happens, qemu crashes because it tries to lock the
> > > > > destroyed event_mtx in colo_notify_compares_event.
> > > > >
> > > > > Fix this by checking if everything is initialized by introducing
> > > > > a new variable colo_compare_active which is protected by a new
> > > > > mutex colo_compare_mutex. The new mutex also protects against
> > > > > concurrent access of the net_compares list and makes sure that
> > > > > colo_notify_compares_event isn't active while we destroy
> > > > > event_mtx and event_complete_cond.
> > > > >
> > > > > With this it also is again possible to use colo without
> > > > > colo-compare (periodic
> > > > > mode) and to use multiple colo-compare for multiple network
> interfaces.
> > > > >
> > > >
> > > > Hi Lukas,
> > > >
> > > > For this case I think we don't need to touch vl.c code, we can
> > > > solve this
> > > issue from another perspective:
> > > > How to remove colo-compare?
> > > > User will use qemu-monitor or QMP command to disable an object, so
> > > > we just need return operation failed When user try to remove
> > > > colo-compare
> > > object while COLO is running.
> > >
> > > Yeah, but that still leaves the other problem that colo can't be
> > > used without colo-compare (qemu crashes then).
> >
> > Yes, the COLO-compare is necessary module in COLO original design.
> > At most cases, user need it do dynamic sync.
> > For rare cases, maybe we can add a new colo-compare parameter to
> bypass all the network workload.
> 
> I think such an parameter would only be a workaround instead of a real
> solution like this patch.

The root problem is why COLO-compare is necessary.
Yes, maybe someone want to use pure periodic synchronization mode,
But it means it will lost all guest network support(without colo-compare/filter-mirror/filter-redirector/filter-rewriter).
The secondary guest just a solid backup for the primary guest, when occur failover the new build stateful connection (like TCP)
will crashed, need userspace to handle this status. It lost the original meaning for COLO FT/HA solution, no need use do HA in application layer.
it looks like normal/remote periodic VM snapshot here. 
Dave or Jason have any comments here? 

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > > Thanks
> > > > Zhang Chen
> > > >
> > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > > ---
> > > > >  net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
> > > > >  net/colo-compare.h |  1 +
> > > > >  softmmu/vl.c       |  2 ++
> > > > >  3 files changed, 32 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > > 56db3d3bfc..c7572d75e9 100644
> > > > > --- a/net/colo-compare.c
> > > > > +++ b/net/colo-compare.c
> > > > > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
> > > > > #define REGULAR_PACKET_CHECK_MS 3000  #define
> > > DEFAULT_TIME_OUT_MS
> > > > > 3000
> > > > >
> > > > > +static QemuMutex colo_compare_mutex; static bool
> > > > > +colo_compare_active;
> > > > >  static QemuMutex event_mtx;
> > > > >  static QemuCond event_complete_cond;  static int
> > > > > event_unhandled_count; @@ -906,6 +908,12 @@ static void
> > > > > check_old_packet_regular(void *opaque) void
> > > > > colo_notify_compares_event(void *opaque, int event, Error **errp)
> {
> > > > >      CompareState *s;
> > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > +
> > > > > +    if (!colo_compare_active) {
> > > > > +        qemu_mutex_unlock(&colo_compare_mutex);
> > > > > +        return;
> > > > > +    }
> > > > >
> > > > >      qemu_mutex_lock(&event_mtx);
> > > > >      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7
> > > > > @@ void colo_notify_compares_event(void *opaque, int event,
> Error **errp)
> > > > >      }
> > > > >
> > > > >      qemu_mutex_unlock(&event_mtx);
> > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > >  }
> > > > >
> > > > >  static void colo_compare_timer_init(CompareState *s) @@ -1274,7
> > > > > +1283,14 @@ static void colo_compare_complete(UserCreatable *uc,
> > > Error **errp)
> > > > >                             s->vnet_hdr);
> > > > >      }
> > > > >
> > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > +    if (!colo_compare_active) {
> > > > > +        qemu_mutex_init(&event_mtx);
> > > > > +        qemu_cond_init(&event_complete_cond);
> > > > > +        colo_compare_active = true;
> > > > > +    }
> > > > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > >
> > > > >      s->out_sendco.s = s;
> > > > >      s->out_sendco.chr = &s->chr_out; @@ -1290,9 +1306,6 @@
> > > > > static void colo_compare_complete(UserCreatable
> > > > > *uc, Error **errp)
> > > > >
> > > > >      g_queue_init(&s->conn_list);
> > > > >
> > > > > -    qemu_mutex_init(&event_mtx);
> > > > > -    qemu_cond_init(&event_complete_cond);
> > > > > -
> > > > >      s->connection_track_table =
> > > > > g_hash_table_new_full(connection_key_hash,
> > > > >                                                        connection_key_equal,
> > > > >                                                        g_free,
> > > > > @@
> > > > > -1384,12 +1397,19 @@ static void colo_compare_finalize(Object
> > > > > *obj)
> > > > >
> > > > >      qemu_bh_delete(s->event_bh);
> > > > >
> > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > > > >          if (tmp == s) {
> > > > >              QTAILQ_REMOVE(&net_compares, s, next);
> > > > >              break;
> > > > >          }
> > > > >      }
> > > > > +    if (QTAILQ_EMPTY(&net_compares)) {
> > > > > +        colo_compare_active = false;
> > > > > +        qemu_mutex_destroy(&event_mtx);
> > > > > +        qemu_cond_destroy(&event_complete_cond);
> > > > > +    }
> > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > >
> > > > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > > >      aio_context_acquire(ctx);
> > > > > @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object
> *obj)
> > > > >          object_unref(OBJECT(s->iothread));
> > > > >      }
> > > > >
> > > > > -    qemu_mutex_destroy(&event_mtx);
> > > > > -    qemu_cond_destroy(&event_complete_cond);
> > > > > -
> > > > >      g_free(s->pri_indev);
> > > > >      g_free(s->sec_indev);
> > > > >      g_free(s->outdev);
> > > > >      g_free(s->notify_dev);
> > > > >  }
> > > > >
> > > > > +void colo_compare_init_globals(void) {
> > > > > +    colo_compare_active = false;
> > > > > +    qemu_mutex_init(&colo_compare_mutex);
> > > > > +}
> > > > > +
> > > > >  static const TypeInfo colo_compare_info = {
> > > > >      .name = TYPE_COLO_COMPARE,
> > > > >      .parent = TYPE_OBJECT,
> > > > > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > > > > 22ddd512e2..eb483ac586 100644
> > > > > --- a/net/colo-compare.h
> > > > > +++ b/net/colo-compare.h
> > > > > @@ -17,6 +17,7 @@
> > > > >  #ifndef QEMU_COLO_COMPARE_H
> > > > >  #define QEMU_COLO_COMPARE_H
> > > > >
> > > > > +void colo_compare_init_globals(void);
> > > > >  void colo_notify_compares_event(void *opaque, int event, Error
> > > > > **errp); void colo_compare_register_notifier(Notifier *notify);
> > > > > void colo_compare_unregister_notifier(Notifier *notify); diff
> > > > > --git a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..a913ed5469
> > > > > 100644
> > > > > --- a/softmmu/vl.c
> > > > > +++ b/softmmu/vl.c
> > > > > @@ -112,6 +112,7 @@
> > > > >  #include "qapi/qmp/qerror.h"
> > > > >  #include "sysemu/iothread.h"
> > > > >  #include "qemu/guest-random.h"
> > > > > +#include "net/colo-compare.h"
> > > > >
> > > > >  #define MAX_VIRTIO_CONSOLES 1
> > > > >
> > > > > @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char
> > > **envp)
> > > > >      precopy_infrastructure_init();
> > > > >      postcopy_infrastructure_init();
> > > > >      monitor_init_globals();
> > > > > +    colo_compare_init_globals();
> > > > >
> > > > >      if (qcrypto_init(&err) < 0) {
> > > > >          error_reportf_err(err, "cannot initialize crypto: ");
> > > > > --
> > > > > 2.20.1
> > > >
> >

Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Lukas Straub]

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

On Fri, 8 May 2020 06:50:39 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Friday, May 8, 2020 2:11 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> > colo-compare is active
> > 
> > On Fri, 8 May 2020 02:26:21 +0000
> > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > Sent: Thursday, May 7, 2020 11:54 PM
> > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > <pbonzini@redhat.com>
> > > > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > > that colo-compare is active
> > > >
> > > > On Thu, 7 May 2020 11:38:04 +0000
> > > > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > > >  
> > > > > > -----Original Message-----
> > > > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > > > Sent: Monday, May 4, 2020 6:28 PM
> > > > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> > > > > > Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > > > <pbonzini@redhat.com>
> > > > > > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > > > > that
> > > > > > colo- compare is active
> > > > > >
> > > > > > If the colo-compare object is removed before failover and a
> > > > > > checkpoint happens, qemu crashes because it tries to lock the
> > > > > > destroyed event_mtx in colo_notify_compares_event.
> > > > > >
> > > > > > Fix this by checking if everything is initialized by introducing
> > > > > > a new variable colo_compare_active which is protected by a new
> > > > > > mutex colo_compare_mutex. The new mutex also protects against
> > > > > > concurrent access of the net_compares list and makes sure that
> > > > > > colo_notify_compares_event isn't active while we destroy
> > > > > > event_mtx and event_complete_cond.
> > > > > >
> > > > > > With this it also is again possible to use colo without
> > > > > > colo-compare (periodic
> > > > > > mode) and to use multiple colo-compare for multiple network  
> > interfaces.  
> > > > > >  
> > > > >
> > > > > Hi Lukas,
> > > > >
> > > > > For this case I think we don't need to touch vl.c code, we can
> > > > > solve this  
> > > > issue from another perspective:  
> > > > > How to remove colo-compare?
> > > > > User will use qemu-monitor or QMP command to disable an object, so
> > > > > we just need return operation failed When user try to remove
> > > > > colo-compare  
> > > > object while COLO is running.
> > > >
> > > > Yeah, but that still leaves the other problem that colo can't be
> > > > used without colo-compare (qemu crashes then).  
> > >
> > > Yes, the COLO-compare is necessary module in COLO original design.
> > > At most cases, user need it do dynamic sync.
> > > For rare cases, maybe we can add a new colo-compare parameter to  
> > bypass all the network workload.
> > 
> > I think such an parameter would only be a workaround instead of a real
> > solution like this patch.  
> 
> The root problem is why COLO-compare is necessary.
> Yes, maybe someone want to use pure periodic synchronization mode,
> But it means it will lost all guest network support(without colo-compare/filter-mirror/filter-redirector/filter-rewriter).
> The secondary guest just a solid backup for the primary guest, when occur failover the new build stateful connection (like TCP)
> will crashed, need userspace to handle this status. It lost the original meaning for COLO FT/HA solution, no need use do HA in application layer.
> it looks like normal/remote periodic VM snapshot here. 

Sure, but maybe the user doesn't need (reliable) network on failover. Also proper network support with periodic mode can easily be implemented by modifying filter-buffer to buffer packets until checkpoint. Being able to use colo without colo-compare gives more flexibility to the user.

Regards,
Lukas Straub

> Dave or Jason have any comments here? 
> 
> Thanks
> Zhang Chen
> 
> > 
> > Regards,
> > Lukas Straub
> >   
> > > Thanks
> > > Zhang Chen
> > >  
> > > >
> > > > Regards,
> > > > Lukas Straub
> > > >  
> > > > > Thanks
> > > > > Zhang Chen
> > > > >  
> > > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > > > ---
> > > > > >  net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
> > > > > >  net/colo-compare.h |  1 +
> > > > > >  softmmu/vl.c       |  2 ++
> > > > > >  3 files changed, 32 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > > > 56db3d3bfc..c7572d75e9 100644
> > > > > > --- a/net/colo-compare.c
> > > > > > +++ b/net/colo-compare.c
> > > > > > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
> > > > > > #define REGULAR_PACKET_CHECK_MS 3000  #define  
> > > > DEFAULT_TIME_OUT_MS  
> > > > > > 3000
> > > > > >
> > > > > > +static QemuMutex colo_compare_mutex; static bool
> > > > > > +colo_compare_active;
> > > > > >  static QemuMutex event_mtx;
> > > > > >  static QemuCond event_complete_cond;  static int
> > > > > > event_unhandled_count; @@ -906,6 +908,12 @@ static void
> > > > > > check_old_packet_regular(void *opaque) void
> > > > > > colo_notify_compares_event(void *opaque, int event, Error **errp)  
> > {  
> > > > > >      CompareState *s;
> > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > > +
> > > > > > +    if (!colo_compare_active) {
> > > > > > +        qemu_mutex_unlock(&colo_compare_mutex);
> > > > > > +        return;
> > > > > > +    }
> > > > > >
> > > > > >      qemu_mutex_lock(&event_mtx);
> > > > > >      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7
> > > > > > @@ void colo_notify_compares_event(void *opaque, int event,  
> > Error **errp)  
> > > > > >      }
> > > > > >
> > > > > >      qemu_mutex_unlock(&event_mtx);
> > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > >  }
> > > > > >
> > > > > >  static void colo_compare_timer_init(CompareState *s) @@ -1274,7
> > > > > > +1283,14 @@ static void colo_compare_complete(UserCreatable *uc,  
> > > > Error **errp)  
> > > > > >                             s->vnet_hdr);
> > > > > >      }
> > > > > >
> > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > > +    if (!colo_compare_active) {
> > > > > > +        qemu_mutex_init(&event_mtx);
> > > > > > +        qemu_cond_init(&event_complete_cond);
> > > > > > +        colo_compare_active = true;
> > > > > > +    }
> > > > > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > >
> > > > > >      s->out_sendco.s = s;
> > > > > >      s->out_sendco.chr = &s->chr_out; @@ -1290,9 +1306,6 @@
> > > > > > static void colo_compare_complete(UserCreatable
> > > > > > *uc, Error **errp)
> > > > > >
> > > > > >      g_queue_init(&s->conn_list);
> > > > > >
> > > > > > -    qemu_mutex_init(&event_mtx);
> > > > > > -    qemu_cond_init(&event_complete_cond);
> > > > > > -
> > > > > >      s->connection_track_table =
> > > > > > g_hash_table_new_full(connection_key_hash,
> > > > > >                                                        connection_key_equal,
> > > > > >                                                        g_free,
> > > > > > @@
> > > > > > -1384,12 +1397,19 @@ static void colo_compare_finalize(Object
> > > > > > *obj)
> > > > > >
> > > > > >      qemu_bh_delete(s->event_bh);
> > > > > >
> > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > > > > >          if (tmp == s) {
> > > > > >              QTAILQ_REMOVE(&net_compares, s, next);
> > > > > >              break;
> > > > > >          }
> > > > > >      }
> > > > > > +    if (QTAILQ_EMPTY(&net_compares)) {
> > > > > > +        colo_compare_active = false;
> > > > > > +        qemu_mutex_destroy(&event_mtx);
> > > > > > +        qemu_cond_destroy(&event_complete_cond);
> > > > > > +    }
> > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > >
> > > > > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > > > >      aio_context_acquire(ctx);
> > > > > > @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object  
> > *obj)  
> > > > > >          object_unref(OBJECT(s->iothread));
> > > > > >      }
> > > > > >
> > > > > > -    qemu_mutex_destroy(&event_mtx);
> > > > > > -    qemu_cond_destroy(&event_complete_cond);
> > > > > > -
> > > > > >      g_free(s->pri_indev);
> > > > > >      g_free(s->sec_indev);
> > > > > >      g_free(s->outdev);
> > > > > >      g_free(s->notify_dev);
> > > > > >  }
> > > > > >
> > > > > > +void colo_compare_init_globals(void) {
> > > > > > +    colo_compare_active = false;
> > > > > > +    qemu_mutex_init(&colo_compare_mutex);
> > > > > > +}
> > > > > > +
> > > > > >  static const TypeInfo colo_compare_info = {
> > > > > >      .name = TYPE_COLO_COMPARE,
> > > > > >      .parent = TYPE_OBJECT,
> > > > > > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > > > > > 22ddd512e2..eb483ac586 100644
> > > > > > --- a/net/colo-compare.h
> > > > > > +++ b/net/colo-compare.h
> > > > > > @@ -17,6 +17,7 @@
> > > > > >  #ifndef QEMU_COLO_COMPARE_H
> > > > > >  #define QEMU_COLO_COMPARE_H
> > > > > >
> > > > > > +void colo_compare_init_globals(void);
> > > > > >  void colo_notify_compares_event(void *opaque, int event, Error
> > > > > > **errp); void colo_compare_register_notifier(Notifier *notify);
> > > > > > void colo_compare_unregister_notifier(Notifier *notify); diff
> > > > > > --git a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..a913ed5469
> > > > > > 100644
> > > > > > --- a/softmmu/vl.c
> > > > > > +++ b/softmmu/vl.c
> > > > > > @@ -112,6 +112,7 @@
> > > > > >  #include "qapi/qmp/qerror.h"
> > > > > >  #include "sysemu/iothread.h"
> > > > > >  #include "qemu/guest-random.h"
> > > > > > +#include "net/colo-compare.h"
> > > > > >
> > > > > >  #define MAX_VIRTIO_CONSOLES 1
> > > > > >
> > > > > > @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char  
> > > > **envp)  
> > > > > >      precopy_infrastructure_init();
> > > > > >      postcopy_infrastructure_init();
> > > > > >      monitor_init_globals();
> > > > > > +    colo_compare_init_globals();
> > > > > >
> > > > > >      if (qcrypto_init(&err) < 0) {
> > > > > >          error_reportf_err(err, "cannot initialize crypto: ");
> > > > > > --
> > > > > > 2.20.1  
> > > > >  
> > >  
> 


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

RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Saturday, May 9, 2020 8:21 PM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>; Dr . David Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> colo-compare is active
> 
> On Fri, 8 May 2020 06:50:39 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Friday, May 8, 2020 2:11 PM
> > > To: Zhang, Chen <chen.zhang@intel.com>
> > > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > that colo-compare is active
> > >
> > > On Fri, 8 May 2020 02:26:21 +0000
> > > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > >
> > > > > -----Original Message-----
> > > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > > Sent: Thursday, May 7, 2020 11:54 PM
> > > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> > > > > Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > > <pbonzini@redhat.com>
> > > > > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c:
> > > > > Check that colo-compare is active
> > > > >
> > > > > On Thu, 7 May 2020 11:38:04 +0000 "Zhang, Chen"
> > > > > <chen.zhang@intel.com> wrote:
> > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > > > > Sent: Monday, May 4, 2020 6:28 PM
> > > > > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > > > > <lizhijian@cn.fujitsu.com>; Jason Wang
> > > > > > > <jasowang@redhat.com>;
> > > > > > > Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo
> > > > > > > Bonzini <pbonzini@redhat.com>
> > > > > > > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c:
> > > > > > > Check that
> > > > > > > colo- compare is active
> > > > > > >
> > > > > > > If the colo-compare object is removed before failover and a
> > > > > > > checkpoint happens, qemu crashes because it tries to lock
> > > > > > > the destroyed event_mtx in colo_notify_compares_event.
> > > > > > >
> > > > > > > Fix this by checking if everything is initialized by
> > > > > > > introducing a new variable colo_compare_active which is
> > > > > > > protected by a new mutex colo_compare_mutex. The new mutex
> > > > > > > also protects against concurrent access of the net_compares
> > > > > > > list and makes sure that colo_notify_compares_event isn't
> > > > > > > active while we destroy event_mtx and event_complete_cond.
> > > > > > >
> > > > > > > With this it also is again possible to use colo without
> > > > > > > colo-compare (periodic
> > > > > > > mode) and to use multiple colo-compare for multiple network
> > > interfaces.
> > > > > > >
> > > > > >
> > > > > > Hi Lukas,
> > > > > >
> > > > > > For this case I think we don't need to touch vl.c code, we can
> > > > > > solve this
> > > > > issue from another perspective:
> > > > > > How to remove colo-compare?
> > > > > > User will use qemu-monitor or QMP command to disable an
> > > > > > object, so we just need return operation failed When user try
> > > > > > to remove colo-compare
> > > > > object while COLO is running.
> > > > >
> > > > > Yeah, but that still leaves the other problem that colo can't be
> > > > > used without colo-compare (qemu crashes then).
> > > >
> > > > Yes, the COLO-compare is necessary module in COLO original design.
> > > > At most cases, user need it do dynamic sync.
> > > > For rare cases, maybe we can add a new colo-compare parameter to
> > > bypass all the network workload.
> > >
> > > I think such an parameter would only be a workaround instead of a
> > > real solution like this patch.
> >
> > The root problem is why COLO-compare is necessary.
> > Yes, maybe someone want to use pure periodic synchronization mode, But
> > it means it will lost all guest network support(without colo-compare/filter-
> mirror/filter-redirector/filter-rewriter).
> > The secondary guest just a solid backup for the primary guest, when
> > occur failover the new build stateful connection (like TCP) will crashed,
> need userspace to handle this status. It lost the original meaning for COLO
> FT/HA solution, no need use do HA in application layer.
> > it looks like normal/remote periodic VM snapshot here.
> 
> Sure, but maybe the user doesn't need (reliable) network on failover. Also
> proper network support with periodic mode can easily be implemented by
> modifying filter-buffer to buffer packets until checkpoint. Being able to use
> colo without colo-compare gives more flexibility to the user.

OK, use the filter-buffer instead of colo-compare is a new use case here.
If other maintainers have no comments about touch the vl.c for COLO , I think it's OK here.

Thanks
Zhang Chen 


> 
> Regards,
> Lukas Straub
> 
> > Dave or Jason have any comments here?
> >
> > Thanks
> > Zhang Chen
> >
> > >
> > > Regards,
> > > Lukas Straub
> > >
> > > > Thanks
> > > > Zhang Chen
> > > >
> > > > >
> > > > > Regards,
> > > > > Lukas Straub
> > > > >
> > > > > > Thanks
> > > > > > Zhang Chen
> > > > > >
> > > > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > > > > ---
> > > > > > >  net/colo-compare.c | 35 +++++++++++++++++++++++++++++---
> ---
> > > > > > >  net/colo-compare.h |  1 +
> > > > > > >  softmmu/vl.c       |  2 ++
> > > > > > >  3 files changed, 32 insertions(+), 6 deletions(-)
> > > > > > >
> > > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > > > > 56db3d3bfc..c7572d75e9 100644
> > > > > > > --- a/net/colo-compare.c
> > > > > > > +++ b/net/colo-compare.c
> > > > > > > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers
> > > > > > > = #define REGULAR_PACKET_CHECK_MS 3000  #define
> > > > > DEFAULT_TIME_OUT_MS
> > > > > > > 3000
> > > > > > >
> > > > > > > +static QemuMutex colo_compare_mutex; static bool
> > > > > > > +colo_compare_active;
> > > > > > >  static QemuMutex event_mtx;  static QemuCond
> > > > > > > event_complete_cond;  static int event_unhandled_count; @@
> > > > > > > -906,6 +908,12 @@ static void check_old_packet_regular(void
> > > > > > > *opaque) void colo_notify_compares_event(void *opaque, int
> > > > > > > event, Error **errp)
> > > {
> > > > > > >      CompareState *s;
> > > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > > > +
> > > > > > > +    if (!colo_compare_active) {
> > > > > > > +        qemu_mutex_unlock(&colo_compare_mutex);
> > > > > > > +        return;
> > > > > > > +    }
> > > > > > >
> > > > > > >      qemu_mutex_lock(&event_mtx);
> > > > > > >      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6
> > > > > > > +927,7 @@ void colo_notify_compares_event(void *opaque, int
> > > > > > > event,
> > > Error **errp)
> > > > > > >      }
> > > > > > >
> > > > > > >      qemu_mutex_unlock(&event_mtx);
> > > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > > >  }
> > > > > > >
> > > > > > >  static void colo_compare_timer_init(CompareState *s) @@
> > > > > > > -1274,7
> > > > > > > +1283,14 @@ static void colo_compare_complete(UserCreatable
> > > > > > > +*uc,
> > > > > Error **errp)
> > > > > > >                             s->vnet_hdr);
> > > > > > >      }
> > > > > > >
> > > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > > > +    if (!colo_compare_active) {
> > > > > > > +        qemu_mutex_init(&event_mtx);
> > > > > > > +        qemu_cond_init(&event_complete_cond);
> > > > > > > +        colo_compare_active = true;
> > > > > > > +    }
> > > > > > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > > >
> > > > > > >      s->out_sendco.s = s;
> > > > > > >      s->out_sendco.chr = &s->chr_out; @@ -1290,9 +1306,6 @@
> > > > > > > static void colo_compare_complete(UserCreatable
> > > > > > > *uc, Error **errp)
> > > > > > >
> > > > > > >      g_queue_init(&s->conn_list);
> > > > > > >
> > > > > > > -    qemu_mutex_init(&event_mtx);
> > > > > > > -    qemu_cond_init(&event_complete_cond);
> > > > > > > -
> > > > > > >      s->connection_track_table =
> > > > > > > g_hash_table_new_full(connection_key_hash,
> > > > > > >                                                        connection_key_equal,
> > > > > > >
> > > > > > > g_free, @@
> > > > > > > -1384,12 +1397,19 @@ static void
> > > > > > > colo_compare_finalize(Object
> > > > > > > *obj)
> > > > > > >
> > > > > > >      qemu_bh_delete(s->event_bh);
> > > > > > >
> > > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > > > > > >          if (tmp == s) {
> > > > > > >              QTAILQ_REMOVE(&net_compares, s, next);
> > > > > > >              break;
> > > > > > >          }
> > > > > > >      }
> > > > > > > +    if (QTAILQ_EMPTY(&net_compares)) {
> > > > > > > +        colo_compare_active = false;
> > > > > > > +        qemu_mutex_destroy(&event_mtx);
> > > > > > > +        qemu_cond_destroy(&event_complete_cond);
> > > > > > > +    }
> > > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > > >
> > > > > > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > > > > >      aio_context_acquire(ctx); @@ -1413,15 +1433,18 @@
> > > > > > > static void colo_compare_finalize(Object
> > > *obj)
> > > > > > >          object_unref(OBJECT(s->iothread));
> > > > > > >      }
> > > > > > >
> > > > > > > -    qemu_mutex_destroy(&event_mtx);
> > > > > > > -    qemu_cond_destroy(&event_complete_cond);
> > > > > > > -
> > > > > > >      g_free(s->pri_indev);
> > > > > > >      g_free(s->sec_indev);
> > > > > > >      g_free(s->outdev);
> > > > > > >      g_free(s->notify_dev);
> > > > > > >  }
> > > > > > >
> > > > > > > +void colo_compare_init_globals(void) {
> > > > > > > +    colo_compare_active = false;
> > > > > > > +    qemu_mutex_init(&colo_compare_mutex);
> > > > > > > +}
> > > > > > > +
> > > > > > >  static const TypeInfo colo_compare_info = {
> > > > > > >      .name = TYPE_COLO_COMPARE,
> > > > > > >      .parent = TYPE_OBJECT,
> > > > > > > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > > > > > > 22ddd512e2..eb483ac586 100644
> > > > > > > --- a/net/colo-compare.h
> > > > > > > +++ b/net/colo-compare.h
> > > > > > > @@ -17,6 +17,7 @@
> > > > > > >  #ifndef QEMU_COLO_COMPARE_H  #define
> QEMU_COLO_COMPARE_H
> > > > > > >
> > > > > > > +void colo_compare_init_globals(void);
> > > > > > >  void colo_notify_compares_event(void *opaque, int event,
> > > > > > > Error **errp); void colo_compare_register_notifier(Notifier
> > > > > > > *notify); void colo_compare_unregister_notifier(Notifier
> > > > > > > *notify); diff --git a/softmmu/vl.c b/softmmu/vl.c index
> > > > > > > 32c0047889..a913ed5469
> > > > > > > 100644
> > > > > > > --- a/softmmu/vl.c
> > > > > > > +++ b/softmmu/vl.c
> > > > > > > @@ -112,6 +112,7 @@
> > > > > > >  #include "qapi/qmp/qerror.h"
> > > > > > >  #include "sysemu/iothread.h"
> > > > > > >  #include "qemu/guest-random.h"
> > > > > > > +#include "net/colo-compare.h"
> > > > > > >
> > > > > > >  #define MAX_VIRTIO_CONSOLES 1
> > > > > > >
> > > > > > > @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv,
> > > > > > > char
> > > > > **envp)
> > > > > > >      precopy_infrastructure_init();
> > > > > > >      postcopy_infrastructure_init();
> > > > > > >      monitor_init_globals();
> > > > > > > +    colo_compare_init_globals();
> > > > > > >
> > > > > > >      if (qcrypto_init(&err) < 0) {
> > > > > > >          error_reportf_err(err, "cannot initialize crypto:
> > > > > > > ");
> > > > > > > --
> > > > > > > 2.20.1
> > > > > >
> > > >
> >

Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active

[Dr. David Alan Gilbert]

* Zhang, Chen (chen.zhang@intel.com) wrote:
> 
> 
> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Friday, May 8, 2020 2:11 PM
> > To: Zhang, Chen <chen.zhang@intel.com>
> > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that
> > colo-compare is active
> > 
> > On Fri, 8 May 2020 02:26:21 +0000
> > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > 
> > > > -----Original Message-----
> > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > Sent: Thursday, May 7, 2020 11:54 PM
> > > > To: Zhang, Chen <chen.zhang@intel.com>
> > > > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > <pbonzini@redhat.com>
> > > > Subject: Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > > that colo-compare is active
> > > >
> > > > On Thu, 7 May 2020 11:38:04 +0000
> > > > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> > > >
> > > > > > -----Original Message-----
> > > > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > > > Sent: Monday, May 4, 2020 6:28 PM
> > > > > > To: qemu-devel <qemu-devel@nongnu.org>
> > > > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>;
> > > > > > Marc- André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > > > > <pbonzini@redhat.com>
> > > > > > Subject: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check
> > > > > > that
> > > > > > colo- compare is active
> > > > > >
> > > > > > If the colo-compare object is removed before failover and a
> > > > > > checkpoint happens, qemu crashes because it tries to lock the
> > > > > > destroyed event_mtx in colo_notify_compares_event.
> > > > > >
> > > > > > Fix this by checking if everything is initialized by introducing
> > > > > > a new variable colo_compare_active which is protected by a new
> > > > > > mutex colo_compare_mutex. The new mutex also protects against
> > > > > > concurrent access of the net_compares list and makes sure that
> > > > > > colo_notify_compares_event isn't active while we destroy
> > > > > > event_mtx and event_complete_cond.
> > > > > >
> > > > > > With this it also is again possible to use colo without
> > > > > > colo-compare (periodic
> > > > > > mode) and to use multiple colo-compare for multiple network
> > interfaces.
> > > > > >
> > > > >
> > > > > Hi Lukas,
> > > > >
> > > > > For this case I think we don't need to touch vl.c code, we can
> > > > > solve this
> > > > issue from another perspective:
> > > > > How to remove colo-compare?
> > > > > User will use qemu-monitor or QMP command to disable an object, so
> > > > > we just need return operation failed When user try to remove
> > > > > colo-compare
> > > > object while COLO is running.
> > > >
> > > > Yeah, but that still leaves the other problem that colo can't be
> > > > used without colo-compare (qemu crashes then).
> > >
> > > Yes, the COLO-compare is necessary module in COLO original design.
> > > At most cases, user need it do dynamic sync.
> > > For rare cases, maybe we can add a new colo-compare parameter to
> > bypass all the network workload.
> > 
> > I think such an parameter would only be a workaround instead of a real
> > solution like this patch.
> 
> The root problem is why COLO-compare is necessary.
> Yes, maybe someone want to use pure periodic synchronization mode,
> But it means it will lost all guest network support(without colo-compare/filter-mirror/filter-redirector/filter-rewriter).
> The secondary guest just a solid backup for the primary guest, when occur failover the new build stateful connection (like TCP)
> will crashed, need userspace to handle this status. It lost the original meaning for COLO FT/HA solution, no need use do HA in application layer.
> it looks like normal/remote periodic VM snapshot here. 
> Dave or Jason have any comments here? 

People have done fixed-rate VM synchronisation in the past;
and there are workloads where the packet stream is particularly random
so you almost always get comparison miscompares.

But in those setups, the rest of the configuration is very different -
since the destination isn't running at the same time as the source, the
block mirroring also gets simpler in those scenarious.

One thing we played with in the past was dynamically turning
comparison on and off; switching back to simpler synchronisation
if the workload turns out to suit it.

Dave

> Thanks
> Zhang Chen
> 
> > 
> > Regards,
> > Lukas Straub
> > 
> > > Thanks
> > > Zhang Chen
> > >
> > > >
> > > > Regards,
> > > > Lukas Straub
> > > >
> > > > > Thanks
> > > > > Zhang Chen
> > > > >
> > > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > > > ---
> > > > > >  net/colo-compare.c | 35 +++++++++++++++++++++++++++++------
> > > > > >  net/colo-compare.h |  1 +
> > > > > >  softmmu/vl.c       |  2 ++
> > > > > >  3 files changed, 32 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > > > 56db3d3bfc..c7572d75e9 100644
> > > > > > --- a/net/colo-compare.c
> > > > > > +++ b/net/colo-compare.c
> > > > > > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers =
> > > > > > #define REGULAR_PACKET_CHECK_MS 3000  #define
> > > > DEFAULT_TIME_OUT_MS
> > > > > > 3000
> > > > > >
> > > > > > +static QemuMutex colo_compare_mutex; static bool
> > > > > > +colo_compare_active;
> > > > > >  static QemuMutex event_mtx;
> > > > > >  static QemuCond event_complete_cond;  static int
> > > > > > event_unhandled_count; @@ -906,6 +908,12 @@ static void
> > > > > > check_old_packet_regular(void *opaque) void
> > > > > > colo_notify_compares_event(void *opaque, int event, Error **errp)
> > {
> > > > > >      CompareState *s;
> > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > > +
> > > > > > +    if (!colo_compare_active) {
> > > > > > +        qemu_mutex_unlock(&colo_compare_mutex);
> > > > > > +        return;
> > > > > > +    }
> > > > > >
> > > > > >      qemu_mutex_lock(&event_mtx);
> > > > > >      QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7
> > > > > > @@ void colo_notify_compares_event(void *opaque, int event,
> > Error **errp)
> > > > > >      }
> > > > > >
> > > > > >      qemu_mutex_unlock(&event_mtx);
> > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > >  }
> > > > > >
> > > > > >  static void colo_compare_timer_init(CompareState *s) @@ -1274,7
> > > > > > +1283,14 @@ static void colo_compare_complete(UserCreatable *uc,
> > > > Error **errp)
> > > > > >                             s->vnet_hdr);
> > > > > >      }
> > > > > >
> > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > > +    if (!colo_compare_active) {
> > > > > > +        qemu_mutex_init(&event_mtx);
> > > > > > +        qemu_cond_init(&event_complete_cond);
> > > > > > +        colo_compare_active = true;
> > > > > > +    }
> > > > > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > >
> > > > > >      s->out_sendco.s = s;
> > > > > >      s->out_sendco.chr = &s->chr_out; @@ -1290,9 +1306,6 @@
> > > > > > static void colo_compare_complete(UserCreatable
> > > > > > *uc, Error **errp)
> > > > > >
> > > > > >      g_queue_init(&s->conn_list);
> > > > > >
> > > > > > -    qemu_mutex_init(&event_mtx);
> > > > > > -    qemu_cond_init(&event_complete_cond);
> > > > > > -
> > > > > >      s->connection_track_table =
> > > > > > g_hash_table_new_full(connection_key_hash,
> > > > > >                                                        connection_key_equal,
> > > > > >                                                        g_free,
> > > > > > @@
> > > > > > -1384,12 +1397,19 @@ static void colo_compare_finalize(Object
> > > > > > *obj)
> > > > > >
> > > > > >      qemu_bh_delete(s->event_bh);
> > > > > >
> > > > > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > > > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > > > > >          if (tmp == s) {
> > > > > >              QTAILQ_REMOVE(&net_compares, s, next);
> > > > > >              break;
> > > > > >          }
> > > > > >      }
> > > > > > +    if (QTAILQ_EMPTY(&net_compares)) {
> > > > > > +        colo_compare_active = false;
> > > > > > +        qemu_mutex_destroy(&event_mtx);
> > > > > > +        qemu_cond_destroy(&event_complete_cond);
> > > > > > +    }
> > > > > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > > > >
> > > > > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > > > >      aio_context_acquire(ctx);
> > > > > > @@ -1413,15 +1433,18 @@ static void colo_compare_finalize(Object
> > *obj)
> > > > > >          object_unref(OBJECT(s->iothread));
> > > > > >      }
> > > > > >
> > > > > > -    qemu_mutex_destroy(&event_mtx);
> > > > > > -    qemu_cond_destroy(&event_complete_cond);
> > > > > > -
> > > > > >      g_free(s->pri_indev);
> > > > > >      g_free(s->sec_indev);
> > > > > >      g_free(s->outdev);
> > > > > >      g_free(s->notify_dev);
> > > > > >  }
> > > > > >
> > > > > > +void colo_compare_init_globals(void) {
> > > > > > +    colo_compare_active = false;
> > > > > > +    qemu_mutex_init(&colo_compare_mutex);
> > > > > > +}
> > > > > > +
> > > > > >  static const TypeInfo colo_compare_info = {
> > > > > >      .name = TYPE_COLO_COMPARE,
> > > > > >      .parent = TYPE_OBJECT,
> > > > > > diff --git a/net/colo-compare.h b/net/colo-compare.h index
> > > > > > 22ddd512e2..eb483ac586 100644
> > > > > > --- a/net/colo-compare.h
> > > > > > +++ b/net/colo-compare.h
> > > > > > @@ -17,6 +17,7 @@
> > > > > >  #ifndef QEMU_COLO_COMPARE_H
> > > > > >  #define QEMU_COLO_COMPARE_H
> > > > > >
> > > > > > +void colo_compare_init_globals(void);
> > > > > >  void colo_notify_compares_event(void *opaque, int event, Error
> > > > > > **errp); void colo_compare_register_notifier(Notifier *notify);
> > > > > > void colo_compare_unregister_notifier(Notifier *notify); diff
> > > > > > --git a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..a913ed5469
> > > > > > 100644
> > > > > > --- a/softmmu/vl.c
> > > > > > +++ b/softmmu/vl.c
> > > > > > @@ -112,6 +112,7 @@
> > > > > >  #include "qapi/qmp/qerror.h"
> > > > > >  #include "sysemu/iothread.h"
> > > > > >  #include "qemu/guest-random.h"
> > > > > > +#include "net/colo-compare.h"
> > > > > >
> > > > > >  #define MAX_VIRTIO_CONSOLES 1
> > > > > >
> > > > > > @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char
> > > > **envp)
> > > > > >      precopy_infrastructure_init();
> > > > > >      postcopy_infrastructure_init();
> > > > > >      monitor_init_globals();
> > > > > > +    colo_compare_init_globals();
> > > > > >
> > > > > >      if (qcrypto_init(&err) < 0) {
> > > > > >          error_reportf_err(err, "cannot initialize crypto: ");
> > > > > > --
> > > > > > 2.20.1
> > > > >
> > >
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize

[Lukas Straub]

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

In colo_compare_complete, insert CompareState into net_compares
only after everything has been initialized.
In colo_compare_finalize, remove CompareState from net_compares
before anything is deinitialized.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 net/colo-compare.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index c7572d75e9..6f80bcece6 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -1283,15 +1283,6 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
                            s->vnet_hdr);
     }
 
-    qemu_mutex_lock(&colo_compare_mutex);
-    if (!colo_compare_active) {
-        qemu_mutex_init(&event_mtx);
-        qemu_cond_init(&event_complete_cond);
-        colo_compare_active = true;
-    }
-    QTAILQ_INSERT_TAIL(&net_compares, s, next);
-    qemu_mutex_unlock(&colo_compare_mutex);
-
     s->out_sendco.s = s;
     s->out_sendco.chr = &s->chr_out;
     s->out_sendco.notify_remote_frame = false;
@@ -1312,6 +1303,16 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
                                                       connection_destroy);
 
     colo_compare_iothread(s);
+
+    qemu_mutex_lock(&colo_compare_mutex);
+    if (!colo_compare_active) {
+        qemu_mutex_init(&event_mtx);
+        qemu_cond_init(&event_complete_cond);
+        colo_compare_active = true;
+    }
+    QTAILQ_INSERT_TAIL(&net_compares, s, next);
+    qemu_mutex_unlock(&colo_compare_mutex);
+
     return;
 }
 
@@ -1384,19 +1385,6 @@ static void colo_compare_finalize(Object *obj)
     CompareState *s = COLO_COMPARE(obj);
     CompareState *tmp = NULL;
 
-    qemu_chr_fe_deinit(&s->chr_pri_in, false);
-    qemu_chr_fe_deinit(&s->chr_sec_in, false);
-    qemu_chr_fe_deinit(&s->chr_out, false);
-    if (s->notify_dev) {
-        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
-    }
-
-    if (s->iothread) {
-        colo_compare_timer_del(s);
-    }
-
-    qemu_bh_delete(s->event_bh);
-
     qemu_mutex_lock(&colo_compare_mutex);
     QTAILQ_FOREACH(tmp, &net_compares, next) {
         if (tmp == s) {
@@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
     }
     qemu_mutex_unlock(&colo_compare_mutex);
 
+    qemu_chr_fe_deinit(&s->chr_pri_in, false);
+    qemu_chr_fe_deinit(&s->chr_sec_in, false);
+    qemu_chr_fe_deinit(&s->chr_out, false);
+    if (s->notify_dev) {
+        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
+    }
+
+    if (s->iothread) {
+        colo_compare_timer_del(s);
+    }
+
+    qemu_bh_delete(s->event_bh);
+
     AioContext *ctx = iothread_get_aio_context(s->iothread);
     aio_context_acquire(ctx);
     AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
-- 
2.20.1

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

RE: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Monday, May 4, 2020 6:28 PM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete
> and finalize
> 
> In colo_compare_complete, insert CompareState into net_compares only
> after everything has been initialized.
> In colo_compare_finalize, remove CompareState from net_compares before
> anything is deinitialized.

S/deinitialized/finalized

It looks no dependences on each step on initialization and finalization.
Do you means we just need add/remove each colo-compare module at last in logic?
Or current code have some issue?

Thanks
Zhang Chen

> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> c7572d75e9..6f80bcece6 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -1283,15 +1283,6 @@ static void
> colo_compare_complete(UserCreatable *uc, Error **errp)
>                             s->vnet_hdr);
>      }
> 
> -    qemu_mutex_lock(&colo_compare_mutex);
> -    if (!colo_compare_active) {
> -        qemu_mutex_init(&event_mtx);
> -        qemu_cond_init(&event_complete_cond);
> -        colo_compare_active = true;
> -    }
> -    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> -    qemu_mutex_unlock(&colo_compare_mutex);
> -
>      s->out_sendco.s = s;
>      s->out_sendco.chr = &s->chr_out;
>      s->out_sendco.notify_remote_frame = false; @@ -1312,6 +1303,16 @@
> static void colo_compare_complete(UserCreatable *uc, Error **errp)
>                                                        connection_destroy);
> 
>      colo_compare_iothread(s);
> +
> +    qemu_mutex_lock(&colo_compare_mutex);
> +    if (!colo_compare_active) {
> +        qemu_mutex_init(&event_mtx);
> +        qemu_cond_init(&event_complete_cond);
> +        colo_compare_active = true;
> +    }
> +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> +    qemu_mutex_unlock(&colo_compare_mutex);
> +
>      return;
>  }
> 
> @@ -1384,19 +1385,6 @@ static void colo_compare_finalize(Object *obj)
>      CompareState *s = COLO_COMPARE(obj);
>      CompareState *tmp = NULL;
> 
> -    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> -    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> -    qemu_chr_fe_deinit(&s->chr_out, false);
> -    if (s->notify_dev) {
> -        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> -    }
> -
> -    if (s->iothread) {
> -        colo_compare_timer_del(s);
> -    }
> -
> -    qemu_bh_delete(s->event_bh);
> -
>      qemu_mutex_lock(&colo_compare_mutex);
>      QTAILQ_FOREACH(tmp, &net_compares, next) {
>          if (tmp == s) {
> @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
>      }
>      qemu_mutex_unlock(&colo_compare_mutex);
> 
> +    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> +    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> +    qemu_chr_fe_deinit(&s->chr_out, false);
> +    if (s->notify_dev) {
> +        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> +    }
> +
> +    if (s->iothread) {
> +        colo_compare_timer_del(s);
> +    }
> +
> +    qemu_bh_delete(s->event_bh);
> +
>      AioContext *ctx = iothread_get_aio_context(s->iothread);
>      aio_context_acquire(ctx);
>      AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> --
> 2.20.1

Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize

[Lukas Straub]

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

On Thu, 7 May 2020 13:26:11 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Monday, May 4, 2020 6:28 PM
> > To: qemu-devel <qemu-devel@nongnu.org>
> > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>
> > Subject: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete
> > and finalize
> > 
> > In colo_compare_complete, insert CompareState into net_compares only
> > after everything has been initialized.
> > In colo_compare_finalize, remove CompareState from net_compares before
> > anything is deinitialized.  
> 
> S/deinitialized/finalized
> 
> It looks no dependences on each step on initialization and finalization.
> Do you means we just need add/remove each colo-compare module at last in logic?

Yes. While I didn't see any crashes here, there is the possibility that if colo-compare is removed during checkpoint, the destroyed event_bh is called from colo_notify_compares_event. Same with colo_compare_complete (very unlikely) if colo-compare is created while colo is running, colo_notify_compares_event may call the uninitialized event_bh.

Regards,
Lukas Straub

> Or current code have some issue?
> 
> Thanks
> Zhang Chen
> 
> > 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  net/colo-compare.c | 45 +++++++++++++++++++++++----------------------
> >  1 file changed, 23 insertions(+), 22 deletions(-)
> > 
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > c7572d75e9..6f80bcece6 100644
> > --- a/net/colo-compare.c
> > +++ b/net/colo-compare.c
> > @@ -1283,15 +1283,6 @@ static void
> > colo_compare_complete(UserCreatable *uc, Error **errp)
> >                             s->vnet_hdr);
> >      }
> > 
> > -    qemu_mutex_lock(&colo_compare_mutex);
> > -    if (!colo_compare_active) {
> > -        qemu_mutex_init(&event_mtx);
> > -        qemu_cond_init(&event_complete_cond);
> > -        colo_compare_active = true;
> > -    }
> > -    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > -    qemu_mutex_unlock(&colo_compare_mutex);
> > -
> >      s->out_sendco.s = s;
> >      s->out_sendco.chr = &s->chr_out;
> >      s->out_sendco.notify_remote_frame = false; @@ -1312,6 +1303,16 @@
> > static void colo_compare_complete(UserCreatable *uc, Error **errp)
> >                                                        connection_destroy);
> > 
> >      colo_compare_iothread(s);
> > +
> > +    qemu_mutex_lock(&colo_compare_mutex);
> > +    if (!colo_compare_active) {
> > +        qemu_mutex_init(&event_mtx);
> > +        qemu_cond_init(&event_complete_cond);
> > +        colo_compare_active = true;
> > +    }
> > +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > +    qemu_mutex_unlock(&colo_compare_mutex);
> > +
> >      return;
> >  }
> > 
> > @@ -1384,19 +1385,6 @@ static void colo_compare_finalize(Object *obj)
> >      CompareState *s = COLO_COMPARE(obj);
> >      CompareState *tmp = NULL;
> > 
> > -    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > -    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > -    qemu_chr_fe_deinit(&s->chr_out, false);
> > -    if (s->notify_dev) {
> > -        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > -    }
> > -
> > -    if (s->iothread) {
> > -        colo_compare_timer_del(s);
> > -    }
> > -
> > -    qemu_bh_delete(s->event_bh);
> > -
> >      qemu_mutex_lock(&colo_compare_mutex);
> >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> >          if (tmp == s) {
> > @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> >      }
> >      qemu_mutex_unlock(&colo_compare_mutex);
> > 
> > +    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > +    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > +    qemu_chr_fe_deinit(&s->chr_out, false);
> > +    if (s->notify_dev) {
> > +        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > +    }
> > +
> > +    if (s->iothread) {
> > +        colo_compare_timer_del(s);
> > +    }
> > +
> > +    qemu_bh_delete(s->event_bh);
> > +
> >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> >      aio_context_acquire(ctx);
> >      AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > --
> > 2.20.1  


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

RE: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, May 8, 2020 12:09 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> complete and finalize
> 
> On Thu, 7 May 2020 13:26:11 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel <qemu-devel@nongnu.org>
> > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> > > complete and finalize
> > >
> > > In colo_compare_complete, insert CompareState into net_compares
> only
> > > after everything has been initialized.
> > > In colo_compare_finalize, remove CompareState from net_compares
> > > before anything is deinitialized.
> >
> > S/deinitialized/finalized
> >
> > It looks no dependences on each step on initialization and finalization.
> > Do you means we just need add/remove each colo-compare module at last
> in logic?
> 
> Yes. While I didn't see any crashes here, there is the possibility that if colo-
> compare is removed during checkpoint, the destroyed event_bh is called
> from colo_notify_compares_event. Same with colo_compare_complete
> (very unlikely) if colo-compare is created while colo is running,
> colo_notify_compares_event may call the uninitialized event_bh.

As we discussed on 5/6, remove colo-compare during checkpoint is an illegal operation.

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Or current code have some issue?
> >
> > Thanks
> > Zhang Chen
> >
> > >
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  net/colo-compare.c | 45
> > > +++++++++++++++++++++++----------------------
> > >  1 file changed, 23 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > c7572d75e9..6f80bcece6 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -1283,15 +1283,6 @@ static void
> > > colo_compare_complete(UserCreatable *uc, Error **errp)
> > >                             s->vnet_hdr);
> > >      }
> > >
> > > -    qemu_mutex_lock(&colo_compare_mutex);
> > > -    if (!colo_compare_active) {
> > > -        qemu_mutex_init(&event_mtx);
> > > -        qemu_cond_init(&event_complete_cond);
> > > -        colo_compare_active = true;
> > > -    }
> > > -    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > -    qemu_mutex_unlock(&colo_compare_mutex);
> > > -
> > >      s->out_sendco.s = s;
> > >      s->out_sendco.chr = &s->chr_out;
> > >      s->out_sendco.notify_remote_frame = false; @@ -1312,6 +1303,16
> > > @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > >
> > > connection_destroy);
> > >
> > >      colo_compare_iothread(s);
> > > +
> > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > +    if (!colo_compare_active) {
> > > +        qemu_mutex_init(&event_mtx);
> > > +        qemu_cond_init(&event_complete_cond);
> > > +        colo_compare_active = true;
> > > +    }
> > > +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > +
> > >      return;
> > >  }
> > >
> > > @@ -1384,19 +1385,6 @@ static void colo_compare_finalize(Object *obj)
> > >      CompareState *s = COLO_COMPARE(obj);
> > >      CompareState *tmp = NULL;
> > >
> > > -    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > > -    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > > -    qemu_chr_fe_deinit(&s->chr_out, false);
> > > -    if (s->notify_dev) {
> > > -        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > > -    }
> > > -
> > > -    if (s->iothread) {
> > > -        colo_compare_timer_del(s);
> > > -    }
> > > -
> > > -    qemu_bh_delete(s->event_bh);
> > > -
> > >      qemu_mutex_lock(&colo_compare_mutex);
> > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > >          if (tmp == s) {
> > > @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> > >      }
> > >      qemu_mutex_unlock(&colo_compare_mutex);
> > >
> > > +    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > > +    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > > +    qemu_chr_fe_deinit(&s->chr_out, false);
> > > +    if (s->notify_dev) {
> > > +        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > > +    }
> > > +
> > > +    if (s->iothread) {
> > > +        colo_compare_timer_del(s);
> > > +    }
> > > +
> > > +    qemu_bh_delete(s->event_bh);
> > > +
> > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > >      aio_context_acquire(ctx);
> > >      AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > > --
> > > 2.20.1

RE: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Friday, May 8, 2020 12:09 AM
> To: Zhang, Chen <chen.zhang@intel.com>
> Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> complete and finalize
> 
> On Thu, 7 May 2020 13:26:11 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel <qemu-devel@nongnu.org>
> > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> > > <pbonzini@redhat.com>
> > > Subject: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> > > complete and finalize
> > >
> > > In colo_compare_complete, insert CompareState into net_compares
> only
> > > after everything has been initialized.
> > > In colo_compare_finalize, remove CompareState from net_compares
> > > before anything is deinitialized.
> >
> > S/deinitialized/finalized
> >
> > It looks no dependences on each step on initialization and finalization.
> > Do you means we just need add/remove each colo-compare module at last
> in logic?
> 
> Yes. While I didn't see any crashes here, there is the possibility that if colo-
> compare is removed during checkpoint, the destroyed event_bh is called
> from colo_notify_compares_event. Same with colo_compare_complete
> (very unlikely) if colo-compare is created while colo is running,
> colo_notify_compares_event may call the uninitialized event_bh.
> 

Reviewed-by: Zhang Chen <chen.zhang@intel.com>

Thanks
Zhang Chen

> Regards,
> Lukas Straub
> 
> > Or current code have some issue?
> >
> > Thanks
> > Zhang Chen
> >
> > >
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  net/colo-compare.c | 45
> > > +++++++++++++++++++++++----------------------
> > >  1 file changed, 23 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > c7572d75e9..6f80bcece6 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -1283,15 +1283,6 @@ static void
> > > colo_compare_complete(UserCreatable *uc, Error **errp)
> > >                             s->vnet_hdr);
> > >      }
> > >
> > > -    qemu_mutex_lock(&colo_compare_mutex);
> > > -    if (!colo_compare_active) {
> > > -        qemu_mutex_init(&event_mtx);
> > > -        qemu_cond_init(&event_complete_cond);
> > > -        colo_compare_active = true;
> > > -    }
> > > -    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > -    qemu_mutex_unlock(&colo_compare_mutex);
> > > -
> > >      s->out_sendco.s = s;
> > >      s->out_sendco.chr = &s->chr_out;
> > >      s->out_sendco.notify_remote_frame = false; @@ -1312,6 +1303,16
> > > @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > >
> > > connection_destroy);
> > >
> > >      colo_compare_iothread(s);
> > > +
> > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > +    if (!colo_compare_active) {
> > > +        qemu_mutex_init(&event_mtx);
> > > +        qemu_cond_init(&event_complete_cond);
> > > +        colo_compare_active = true;
> > > +    }
> > > +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > +
> > >      return;
> > >  }
> > >
> > > @@ -1384,19 +1385,6 @@ static void colo_compare_finalize(Object *obj)
> > >      CompareState *s = COLO_COMPARE(obj);
> > >      CompareState *tmp = NULL;
> > >
> > > -    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > > -    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > > -    qemu_chr_fe_deinit(&s->chr_out, false);
> > > -    if (s->notify_dev) {
> > > -        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > > -    }
> > > -
> > > -    if (s->iothread) {
> > > -        colo_compare_timer_del(s);
> > > -    }
> > > -
> > > -    qemu_bh_delete(s->event_bh);
> > > -
> > >      qemu_mutex_lock(&colo_compare_mutex);
> > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > >          if (tmp == s) {
> > > @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> > >      }
> > >      qemu_mutex_unlock(&colo_compare_mutex);
> > >
> > > +    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > > +    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > > +    qemu_chr_fe_deinit(&s->chr_out, false);
> > > +    if (s->notify_dev) {
> > > +        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > > +    }
> > > +
> > > +    if (s->iothread) {
> > > +        colo_compare_timer_del(s);
> > > +    }
> > > +
> > > +    qemu_bh_delete(s->event_bh);
> > > +
> > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > >      aio_context_acquire(ctx);
> > >      AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > > --
> > > 2.20.1

RE: [PATCH v4 0/6] colo-compare bugfixes

[Zhang, Chen]

Please update this series, I will queue it to COLO branch.

Thanks
Zhang Chen

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Monday, May 4, 2020 6:28 PM
> To: qemu-devel <qemu-devel@nongnu.org>
> Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian
> <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc-
> André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>
> Subject: [PATCH v4 0/6] colo-compare bugfixes
> 
> Hello Everyone,
> Here are fixes for bugs that I found in my tests.
> 
> Benchmark results:
> Client-to-server tcp:
> without patch: ~63 Mbit/s
> with patch: ~66 Mbit/s
> Server-to-client tcp:
> without patch: ~771 Kbit/s
> with patch: ~702 Kbit/s
> 
> Regards,
> Lukas Straub
> 
> Version changes:
> v4:
>  -fix potential deadlock with notify_remote_frame  -avoid malloc and
> memcpy in many cases
> 
> v3:
>  -fix checkpatch.pl error
> 
> v2:
>  -better wording
>  -fix performance-regression in patch 3 "net/colo-compare.c: Fix deadlock in
> compare_chr_send"
>  -add more bugfixes
> 
> 
> Lukas Straub (6):
>   net/colo-compare.c: Create event_bh with the right AioContext
>   chardev/char.c: Use qemu_co_sleep_ns if in coroutine
>   net/colo-compare.c: Fix deadlock in compare_chr_send
>   net/colo-compare.c: Only hexdump packets if tracing is enabled
>   net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active
>   net/colo-compare.c: Correct ordering in complete and finalize
> 
>  chardev/char.c     |   7 +-
>  net/colo-compare.c | 250 +++++++++++++++++++++++++++++++++---------
> ---
>  net/colo-compare.h |   1 +
>  net/colo.c         |   7 ++
>  net/colo.h         |   1 +
>  softmmu/vl.c       |   2 +
>  6 files changed, 204 insertions(+), 64 deletions(-)
> 
> --
> 2.20.1