[PATCH v3 0/6] colo-compare bugfixes

[PATCH v3 0/6] colo-compare bugfixes

[Lukas Straub]

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

Hello Everyone,
The performance-regression in patch 3 "net/colo-compare.c: Fix deadlock in
compare_chr_send", has been fixed in this version by putting the packets in a
queue instead of returning error. I also found and fixed some more bugs.

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:
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 | 238 +++++++++++++++++++++++++++++++++------------
 net/colo-compare.h |   1 +
 softmmu/vl.c       |   2 +
 4 files changed, 185 insertions(+), 63 deletions(-)

-- 
2.20.1

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

[PATCH v3 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 v3 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 v3 3/6] net/colo-compare.c: Fix deadlock in compare_chr_send

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 10252 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. Also create a new
function notify_chr_send, since that should be independend.

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

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 1de4220fe2..ff6a740284 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,20 @@ static int event_unhandled_count;
  *                    |packet  |  |packet  +    |packet  | |packet  +
  *                    +--------+  +--------+    +--------+ +--------+
  */
+
+typedef struct SendCo {
+    Coroutine *co;
+    GQueue send_list;
+    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 +108,7 @@ typedef struct CompareState {
     SocketReadState pri_rs;
     SocketReadState sec_rs;
     SocketReadState notify_rs;
+    SendCo sendco;
     bool vnet_hdr;
     uint32_t compare_timeout;
     uint32_t expired_scan_cycle;
@@ -126,8 +144,11 @@ enum {
 static int compare_chr_send(CompareState *s,
                             const uint8_t *buf,
                             uint32_t size,
-                            uint32_t vnet_hdr_len,
-                            bool notify_remote_frame);
+                            uint32_t vnet_hdr_len);
+
+static int notify_chr_send(CompareState *s,
+                           const uint8_t *buf,
+                           uint32_t size);
 
 static bool packet_matches_str(const char *str,
                                const uint8_t *buf,
@@ -145,7 +166,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
     if (ret < 0) {
         error_report("Notify Xen COLO-frame failed");
     }
@@ -271,8 +292,7 @@ static void colo_release_primary_pkt(CompareState *s, Packet *pkt)
     ret = compare_chr_send(s,
                            pkt->data,
                            pkt->size,
-                           pkt->vnet_hdr_len,
-                           false);
+                           pkt->vnet_hdr_len);
     if (ret < 0) {
         error_report("colo send primary packet failed");
     }
@@ -699,63 +719,123 @@ 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)
 {
+    CompareState *s = opaque;
+    SendCo *sendco = &s->sendco;
     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));
-    }
 
-    if (ret != sizeof(len)) {
-        goto err;
-    }
+        if (ret != sizeof(len)) {
+            g_free(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 (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,
                                         (uint8_t *)&len,
                                         sizeof(len));
+
+            if (ret != sizeof(len)) {
+                g_free(entry);
+                goto err;
+            }
         }
 
-        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;
         }
+
+        g_free(entry);
     }
 
-    if (notify_remote_frame) {
-        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
-                                    (uint8_t *)buf,
-                                    size);
-    } else {
-        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
+    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);
     }
+    sendco->ret = ret < 0 ? ret : -EIO;
+out:
+    sendco->co = NULL;
+    sendco->done = true;
+    aio_wait_kick();
+}
+
+static int compare_chr_send(CompareState *s,
+                            const uint8_t *buf,
+                            uint32_t size,
+                            uint32_t vnet_hdr_len)
+{
+    SendCo *sendco = &s->sendco;
+    SendEntry *entry;
+
+    if (!size) {
+        return 0;
+    }
+
+    entry = g_malloc(sizeof(SendEntry) + size);
+    entry->size = size;
+    entry->vnet_hdr_len = vnet_hdr_len;
+    memcpy(entry->buf, buf, size);
+    g_queue_push_head(&sendco->send_list, entry);
+
+    if (sendco->done) {
+        sendco->co = qemu_coroutine_create(_compare_chr_send, s);
+        sendco->done = false;
+        qemu_coroutine_enter(sendco->co);
+        if (sendco->done) {
+            /* report early errors */
+            return sendco->ret;
+        }
+    }
+
+    /* assume success */
+    return 0;
+}
+
+static int notify_chr_send(CompareState *s,
+                           const uint8_t *buf,
+                           uint32_t size)
+{
+    int ret = 0;
+    uint32_t len = htonl(size);
+
+    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
+                            (uint8_t *)&len,
+                            sizeof(len));
+
+    if (ret != sizeof(len)) {
+        goto err;
+    }
+
+    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
+                                (uint8_t *)buf,
+                                size);
 
     if (ret != size) {
         goto err;
     }
 
     return 0;
-
 err:
     return ret < 0 ? ret : -EIO;
 }
@@ -1062,8 +1142,7 @@ static void compare_pri_rs_finalize(SocketReadState *pri_rs)
         compare_chr_send(s,
                          pri_rs->buf,
                          pri_rs->packet_len,
-                         pri_rs->vnet_hdr_len,
-                         false);
+                         pri_rs->vnet_hdr_len);
     } else {
         /* compare packet in the specified connection */
         colo_compare_connection(conn, s);
@@ -1093,7 +1172,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
         if (ret < 0) {
             error_report("Notify Xen COLO-frame INIT failed");
         }
@@ -1199,6 +1278,9 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
 
     QTAILQ_INSERT_TAIL(&net_compares, s, next);
 
+    s->sendco.done = true;
+    g_queue_init(&s->sendco.send_list);
+
     g_queue_init(&s->conn_list);
 
     qemu_mutex_init(&event_mtx);
@@ -1224,8 +1306,7 @@ static void colo_flush_packets(void *opaque, void *user_data)
         compare_chr_send(s,
                          pkt->data,
                          pkt->size,
-                         pkt->vnet_hdr_len,
-                         false);
+                         pkt->vnet_hdr_len);
         packet_destroy(pkt, NULL);
     }
     while (!g_queue_is_empty(&conn->secondary_list)) {
@@ -1281,6 +1362,11 @@ static void colo_compare_finalize(Object *obj)
     CompareState *s = COLO_COMPARE(obj);
     CompareState *tmp = NULL;
 
+    AioContext *ctx = iothread_get_aio_context(s->iothread);
+    aio_context_acquire(ctx);
+    AIO_WAIT_WHILE(ctx, !s->sendco.done);
+    aio_context_release(ctx);
+
     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);
@@ -1305,6 +1391,7 @@ static void colo_compare_finalize(Object *obj)
     g_queue_foreach(&s->conn_list, colo_flush_packets, s);
 
     g_queue_clear(&s->conn_list);
+    g_queue_clear(&s->sendco.send_list);
 
     if (s->connection_track_table) {
         g_hash_table_destroy(s->connection_track_table);
-- 
2.20.1


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

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

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Monday, April 27, 2020 5:19 AM
> 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 v3 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. Also create a new function notify_chr_send, since
> that should be independend.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  net/colo-compare.c | 173 ++++++++++++++++++++++++++++++++++-------
> ----
>  1 file changed, 130 insertions(+), 43 deletions(-)
> 
> diff --git a/net/colo-compare.c b/net/colo-compare.c index
> 1de4220fe2..ff6a740284 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,20 @@ static int event_unhandled_count;
>   *                    |packet  |  |packet  +    |packet  | |packet  +
>   *                    +--------+  +--------+    +--------+ +--------+
>   */
> +
> +typedef struct SendCo {
> +    Coroutine *co;
> +    GQueue send_list;
> +    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 +108,7 @@ typedef struct CompareState {
>      SocketReadState pri_rs;
>      SocketReadState sec_rs;
>      SocketReadState notify_rs;
> +    SendCo sendco;
>      bool vnet_hdr;
>      uint32_t compare_timeout;
>      uint32_t expired_scan_cycle;
> @@ -126,8 +144,11 @@ enum {
>  static int compare_chr_send(CompareState *s,
>                              const uint8_t *buf,
>                              uint32_t size,
> -                            uint32_t vnet_hdr_len,
> -                            bool notify_remote_frame);
> +                            uint32_t vnet_hdr_len);
> +
> +static int notify_chr_send(CompareState *s,
> +                           const uint8_t *buf,
> +                           uint32_t size);
> 
>  static bool packet_matches_str(const char *str,
>                                 const uint8_t *buf, @@ -145,7 +166,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
>      if (ret < 0) {
>          error_report("Notify Xen COLO-frame failed");
>      }
> @@ -271,8 +292,7 @@ static void colo_release_primary_pkt(CompareState
> *s, Packet *pkt)
>      ret = compare_chr_send(s,
>                             pkt->data,
>                             pkt->size,
> -                           pkt->vnet_hdr_len,
> -                           false);
> +                           pkt->vnet_hdr_len);
>      if (ret < 0) {
>          error_report("colo send primary packet failed");
>      }
> @@ -699,63 +719,123 @@ 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)
>  {
> +    CompareState *s = opaque;
> +    SendCo *sendco = &s->sendco;
>      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));
> -    }
> 
> -    if (ret != sizeof(len)) {
> -        goto err;
> -    }
> +        if (ret != sizeof(len)) {
> +            g_free(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 (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,
>                                          (uint8_t *)&len,
>                                          sizeof(len));
> +
> +            if (ret != sizeof(len)) {
> +                g_free(entry);
> +                goto err;
> +            }
>          }
> 
> -        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;
>          }
> +
> +        g_free(entry);
>      }
> 
> -    if (notify_remote_frame) {
> -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> -                                    (uint8_t *)buf,
> -                                    size);
> -    } else {
> -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> +    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);
>      }
> +    sendco->ret = ret < 0 ? ret : -EIO;
> +out:
> +    sendco->co = NULL;
> +    sendco->done = true;
> +    aio_wait_kick();
> +}
> +
> +static int compare_chr_send(CompareState *s,
> +                            const uint8_t *buf,
> +                            uint32_t size,
> +                            uint32_t vnet_hdr_len) {
> +    SendCo *sendco = &s->sendco;
> +    SendEntry *entry;
> +
> +    if (!size) {
> +        return 0;
> +    }
> +
> +    entry = g_malloc(sizeof(SendEntry) + size);
> +    entry->size = size;
> +    entry->vnet_hdr_len = vnet_hdr_len;
> +    memcpy(entry->buf, buf, size);
> +    g_queue_push_head(&sendco->send_list, entry);
> +
> +    if (sendco->done) {
> +        sendco->co = qemu_coroutine_create(_compare_chr_send, s);
> +        sendco->done = false;
> +        qemu_coroutine_enter(sendco->co);
> +        if (sendco->done) {
> +            /* report early errors */
> +            return sendco->ret;
> +        }
> +    }
> +
> +    /* assume success */
> +    return 0;
> +}
> +

Why not make notify_chr_send same as compare_chr_send?

Thanks
Zhang Chen

> +static int notify_chr_send(CompareState *s,
> +                           const uint8_t *buf,
> +                           uint32_t size) {
> +    int ret = 0;
> +    uint32_t len = htonl(size);
> +
> +    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> +                            (uint8_t *)&len,
> +                            sizeof(len));
> +
> +    if (ret != sizeof(len)) {
> +        goto err;
> +    }
> +
> +    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> +                                (uint8_t *)buf,
> +                                size);
> 
>      if (ret != size) {
>          goto err;
>      }
> 
>      return 0;
> -
>  err:
>      return ret < 0 ? ret : -EIO;
>  }
> @@ -1062,8 +1142,7 @@ static void
> compare_pri_rs_finalize(SocketReadState *pri_rs)
>          compare_chr_send(s,
>                           pri_rs->buf,
>                           pri_rs->packet_len,
> -                         pri_rs->vnet_hdr_len,
> -                         false);
> +                         pri_rs->vnet_hdr_len);
>      } else {
>          /* compare packet in the specified connection */
>          colo_compare_connection(conn, s); @@ -1093,7 +1172,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
>          if (ret < 0) {
>              error_report("Notify Xen COLO-frame INIT failed");
>          }
> @@ -1199,6 +1278,9 @@ static void colo_compare_complete(UserCreatable
> *uc, Error **errp)
> 
>      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> 
> +    s->sendco.done = true;
> +    g_queue_init(&s->sendco.send_list);
> +
>      g_queue_init(&s->conn_list);
> 
>      qemu_mutex_init(&event_mtx);
> @@ -1224,8 +1306,7 @@ static void colo_flush_packets(void *opaque, void
> *user_data)
>          compare_chr_send(s,
>                           pkt->data,
>                           pkt->size,
> -                         pkt->vnet_hdr_len,
> -                         false);
> +                         pkt->vnet_hdr_len);
>          packet_destroy(pkt, NULL);
>      }
>      while (!g_queue_is_empty(&conn->secondary_list)) { @@ -1281,6
> +1362,11 @@ static void colo_compare_finalize(Object *obj)
>      CompareState *s = COLO_COMPARE(obj);
>      CompareState *tmp = NULL;
> 
> +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> +    aio_context_acquire(ctx);
> +    AIO_WAIT_WHILE(ctx, !s->sendco.done);
> +    aio_context_release(ctx);
> +
>      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); @@ -1305,6 +1391,7 @@ static
> void colo_compare_finalize(Object *obj)
>      g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> 
>      g_queue_clear(&s->conn_list);
> +    g_queue_clear(&s->sendco.send_list);
> 
>      if (s->connection_track_table) {
>          g_hash_table_destroy(s->connection_track_table);
> --
> 2.20.1

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

[Lukas Straub]

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

On Mon, 27 Apr 2020 03:36:57 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Monday, April 27, 2020 5:19 AM
> > 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 v3 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. Also create a new function notify_chr_send, since
> > that should be independend.
> > 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  net/colo-compare.c | 173 ++++++++++++++++++++++++++++++++++-------
> > ----
> >  1 file changed, 130 insertions(+), 43 deletions(-)
> > 
> > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > 1de4220fe2..ff6a740284 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,20 @@ static int event_unhandled_count;
> >   *                    |packet  |  |packet  +    |packet  | |packet  +
> >   *                    +--------+  +--------+    +--------+ +--------+
> >   */
> > +
> > +typedef struct SendCo {
> > +    Coroutine *co;
> > +    GQueue send_list;
> > +    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 +108,7 @@ typedef struct CompareState {
> >      SocketReadState pri_rs;
> >      SocketReadState sec_rs;
> >      SocketReadState notify_rs;
> > +    SendCo sendco;
> >      bool vnet_hdr;
> >      uint32_t compare_timeout;
> >      uint32_t expired_scan_cycle;
> > @@ -126,8 +144,11 @@ enum {
> >  static int compare_chr_send(CompareState *s,
> >                              const uint8_t *buf,
> >                              uint32_t size,
> > -                            uint32_t vnet_hdr_len,
> > -                            bool notify_remote_frame);
> > +                            uint32_t vnet_hdr_len);
> > +
> > +static int notify_chr_send(CompareState *s,
> > +                           const uint8_t *buf,
> > +                           uint32_t size);
> > 
> >  static bool packet_matches_str(const char *str,
> >                                 const uint8_t *buf, @@ -145,7 +166,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
> >      if (ret < 0) {
> >          error_report("Notify Xen COLO-frame failed");
> >      }
> > @@ -271,8 +292,7 @@ static void colo_release_primary_pkt(CompareState
> > *s, Packet *pkt)
> >      ret = compare_chr_send(s,
> >                             pkt->data,
> >                             pkt->size,
> > -                           pkt->vnet_hdr_len,
> > -                           false);
> > +                           pkt->vnet_hdr_len);
> >      if (ret < 0) {
> >          error_report("colo send primary packet failed");
> >      }
> > @@ -699,63 +719,123 @@ 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)
> >  {
> > +    CompareState *s = opaque;
> > +    SendCo *sendco = &s->sendco;
> >      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));
> > -    }
> > 
> > -    if (ret != sizeof(len)) {
> > -        goto err;
> > -    }
> > +        if (ret != sizeof(len)) {
> > +            g_free(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 (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,
> >                                          (uint8_t *)&len,
> >                                          sizeof(len));
> > +
> > +            if (ret != sizeof(len)) {
> > +                g_free(entry);
> > +                goto err;
> > +            }
> >          }
> > 
> > -        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;
> >          }
> > +
> > +        g_free(entry);
> >      }
> > 
> > -    if (notify_remote_frame) {
> > -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > -                                    (uint8_t *)buf,
> > -                                    size);
> > -    } else {
> > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> > +    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);
> >      }
> > +    sendco->ret = ret < 0 ? ret : -EIO;
> > +out:
> > +    sendco->co = NULL;
> > +    sendco->done = true;
> > +    aio_wait_kick();
> > +}
> > +
> > +static int compare_chr_send(CompareState *s,
> > +                            const uint8_t *buf,
> > +                            uint32_t size,
> > +                            uint32_t vnet_hdr_len) {
> > +    SendCo *sendco = &s->sendco;
> > +    SendEntry *entry;
> > +
> > +    if (!size) {
> > +        return 0;
> > +    }
> > +
> > +    entry = g_malloc(sizeof(SendEntry) + size);
> > +    entry->size = size;
> > +    entry->vnet_hdr_len = vnet_hdr_len;
> > +    memcpy(entry->buf, buf, size);
> > +    g_queue_push_head(&sendco->send_list, entry);
> > +
> > +    if (sendco->done) {
> > +        sendco->co = qemu_coroutine_create(_compare_chr_send, s);
> > +        sendco->done = false;
> > +        qemu_coroutine_enter(sendco->co);
> > +        if (sendco->done) {
> > +            /* report early errors */
> > +            return sendco->ret;
> > +        }
> > +    }
> > +
> > +    /* assume success */
> > +    return 0;
> > +}
> > +  
> 
> Why not make notify_chr_send same as compare_chr_send?

Hello,
The notify chardev_dev is not affected from this deadlock issue and is independent from the outdev chardev. So it wouldn't make sense for notify messages to wait in the queue if the outdev chardev is blocked. Also, the code is easier to understand this way.

Regards,
Lukas Straub

> Thanks
> Zhang Chen
> 
> > +static int notify_chr_send(CompareState *s,
> > +                           const uint8_t *buf,
> > +                           uint32_t size) {
> > +    int ret = 0;
> > +    uint32_t len = htonl(size);
> > +
> > +    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > +                            (uint8_t *)&len,
> > +                            sizeof(len));
> > +
> > +    if (ret != sizeof(len)) {
> > +        goto err;
> > +    }
> > +
> > +    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > +                                (uint8_t *)buf,
> > +                                size);
> > 
> >      if (ret != size) {
> >          goto err;
> >      }
> > 
> >      return 0;
> > -
> >  err:
> >      return ret < 0 ? ret : -EIO;
> >  }
> > @@ -1062,8 +1142,7 @@ static void
> > compare_pri_rs_finalize(SocketReadState *pri_rs)
> >          compare_chr_send(s,
> >                           pri_rs->buf,
> >                           pri_rs->packet_len,
> > -                         pri_rs->vnet_hdr_len,
> > -                         false);
> > +                         pri_rs->vnet_hdr_len);
> >      } else {
> >          /* compare packet in the specified connection */
> >          colo_compare_connection(conn, s); @@ -1093,7 +1172,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
> >          if (ret < 0) {
> >              error_report("Notify Xen COLO-frame INIT failed");
> >          }
> > @@ -1199,6 +1278,9 @@ static void colo_compare_complete(UserCreatable
> > *uc, Error **errp)
> > 
> >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > 
> > +    s->sendco.done = true;
> > +    g_queue_init(&s->sendco.send_list);
> > +
> >      g_queue_init(&s->conn_list);
> > 
> >      qemu_mutex_init(&event_mtx);
> > @@ -1224,8 +1306,7 @@ static void colo_flush_packets(void *opaque, void
> > *user_data)
> >          compare_chr_send(s,
> >                           pkt->data,
> >                           pkt->size,
> > -                         pkt->vnet_hdr_len,
> > -                         false);
> > +                         pkt->vnet_hdr_len);
> >          packet_destroy(pkt, NULL);
> >      }
> >      while (!g_queue_is_empty(&conn->secondary_list)) { @@ -1281,6
> > +1362,11 @@ static void colo_compare_finalize(Object *obj)
> >      CompareState *s = COLO_COMPARE(obj);
> >      CompareState *tmp = NULL;
> > 
> > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > +    aio_context_acquire(ctx);
> > +    AIO_WAIT_WHILE(ctx, !s->sendco.done);
> > +    aio_context_release(ctx);
> > +
> >      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); @@ -1305,6 +1391,7 @@ static
> > void colo_compare_finalize(Object *obj)
> >      g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> > 
> >      g_queue_clear(&s->conn_list);
> > +    g_queue_clear(&s->sendco.send_list);
> > 
> >      if (s->connection_track_table) {
> >          g_hash_table_destroy(s->connection_track_table);
> > --
> > 2.20.1  
> 


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

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

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Monday, April 27, 2020 3:22 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 v3 3/6] net/colo-compare.c: Fix deadlock in
> compare_chr_send
> 
> On Mon, 27 Apr 2020 03:36:57 +0000
> "Zhang, Chen" <chen.zhang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <lukasstraub2@web.de>
> > > Sent: Monday, April 27, 2020 5:19 AM
> > > 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 v3 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. Also create a new function
> > > notify_chr_send, since that should be independend.
> > >
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  net/colo-compare.c | 173 ++++++++++++++++++++++++++++++++++---
> ----
> > > ----
> > >  1 file changed, 130 insertions(+), 43 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > 1de4220fe2..ff6a740284 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,20 @@ static int event_unhandled_count;
> > >   *                    |packet  |  |packet  +    |packet  | |packet  +
> > >   *                    +--------+  +--------+    +--------+ +--------+
> > >   */
> > > +
> > > +typedef struct SendCo {
> > > +    Coroutine *co;
> > > +    GQueue send_list;
> > > +    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 +108,7 @@ typedef struct CompareState {
> > >      SocketReadState pri_rs;
> > >      SocketReadState sec_rs;
> > >      SocketReadState notify_rs;
> > > +    SendCo sendco;
> > >      bool vnet_hdr;
> > >      uint32_t compare_timeout;
> > >      uint32_t expired_scan_cycle;
> > > @@ -126,8 +144,11 @@ enum {
> > >  static int compare_chr_send(CompareState *s,
> > >                              const uint8_t *buf,
> > >                              uint32_t size,
> > > -                            uint32_t vnet_hdr_len,
> > > -                            bool notify_remote_frame);
> > > +                            uint32_t vnet_hdr_len);
> > > +
> > > +static int notify_chr_send(CompareState *s,
> > > +                           const uint8_t *buf,
> > > +                           uint32_t size);
> > >
> > >  static bool packet_matches_str(const char *str,
> > >                                 const uint8_t *buf, @@ -145,7 +166,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
> > >      if (ret < 0) {
> > >          error_report("Notify Xen COLO-frame failed");
> > >      }
> > > @@ -271,8 +292,7 @@ static void
> > > colo_release_primary_pkt(CompareState
> > > *s, Packet *pkt)
> > >      ret = compare_chr_send(s,
> > >                             pkt->data,
> > >                             pkt->size,
> > > -                           pkt->vnet_hdr_len,
> > > -                           false);
> > > +                           pkt->vnet_hdr_len);
> > >      if (ret < 0) {
> > >          error_report("colo send primary packet failed");
> > >      }
> > > @@ -699,63 +719,123 @@ 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)
> > >  {
> > > +    CompareState *s = opaque;
> > > +    SendCo *sendco = &s->sendco;
> > >      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));
> > > -    }
> > >
> > > -    if (ret != sizeof(len)) {
> > > -        goto err;
> > > -    }
> > > +        if (ret != sizeof(len)) {
> > > +            g_free(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 (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,
> > >                                          (uint8_t *)&len,
> > >                                          sizeof(len));
> > > +
> > > +            if (ret != sizeof(len)) {
> > > +                g_free(entry);
> > > +                goto err;
> > > +            }
> > >          }
> > >
> > > -        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;
> > >          }
> > > +
> > > +        g_free(entry);
> > >      }
> > >
> > > -    if (notify_remote_frame) {
> > > -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > > -                                    (uint8_t *)buf,
> > > -                                    size);
> > > -    } else {
> > > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> > > +    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);
> > >      }
> > > +    sendco->ret = ret < 0 ? ret : -EIO;
> > > +out:
> > > +    sendco->co = NULL;
> > > +    sendco->done = true;
> > > +    aio_wait_kick();
> > > +}
> > > +
> > > +static int compare_chr_send(CompareState *s,
> > > +                            const uint8_t *buf,
> > > +                            uint32_t size,
> > > +                            uint32_t vnet_hdr_len) {
> > > +    SendCo *sendco = &s->sendco;
> > > +    SendEntry *entry;
> > > +
> > > +    if (!size) {
> > > +        return 0;
> > > +    }
> > > +
> > > +    entry = g_malloc(sizeof(SendEntry) + size);
> > > +    entry->size = size;
> > > +    entry->vnet_hdr_len = vnet_hdr_len;
> > > +    memcpy(entry->buf, buf, size);
> > > +    g_queue_push_head(&sendco->send_list, entry);
> > > +
> > > +    if (sendco->done) {
> > > +        sendco->co = qemu_coroutine_create(_compare_chr_send, s);
> > > +        sendco->done = false;
> > > +        qemu_coroutine_enter(sendco->co);
> > > +        if (sendco->done) {
> > > +            /* report early errors */
> > > +            return sendco->ret;
> > > +        }
> > > +    }
> > > +
> > > +    /* assume success */
> > > +    return 0;
> > > +}
> > > +
> >
> > Why not make notify_chr_send same as compare_chr_send?
> 
> Hello,
> The notify chardev_dev is not affected from this deadlock issue and is
> independent from the outdev chardev. So it wouldn't make sense for notify
> messages to wait in the queue if the outdev chardev is blocked. Also, the
> code is easier to understand this way.
> 

Yes, I means maybe the deadlock issue will also occur in Xen COLO side, we can resolve the potential problem here.

Thanks
Zhang Chen

> Regards,
> Lukas Straub
> 
> > Thanks
> > Zhang Chen
> >
> > > +static int notify_chr_send(CompareState *s,
> > > +                           const uint8_t *buf,
> > > +                           uint32_t size) {
> > > +    int ret = 0;
> > > +    uint32_t len = htonl(size);
> > > +
> > > +    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > > +                            (uint8_t *)&len,
> > > +                            sizeof(len));
> > > +
> > > +    if (ret != sizeof(len)) {
> > > +        goto err;
> > > +    }
> > > +
> > > +    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > > +                                (uint8_t *)buf,
> > > +                                size);
> > >
> > >      if (ret != size) {
> > >          goto err;
> > >      }
> > >
> > >      return 0;
> > > -
> > >  err:
> > >      return ret < 0 ? ret : -EIO;
> > >  }
> > > @@ -1062,8 +1142,7 @@ static void
> > > compare_pri_rs_finalize(SocketReadState *pri_rs)
> > >          compare_chr_send(s,
> > >                           pri_rs->buf,
> > >                           pri_rs->packet_len,
> > > -                         pri_rs->vnet_hdr_len,
> > > -                         false);
> > > +                         pri_rs->vnet_hdr_len);
> > >      } else {
> > >          /* compare packet in the specified connection */
> > >          colo_compare_connection(conn, s); @@ -1093,7 +1172,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
> > >          if (ret < 0) {
> > >              error_report("Notify Xen COLO-frame INIT failed");
> > >          }
> > > @@ -1199,6 +1278,9 @@ static void
> > > colo_compare_complete(UserCreatable
> > > *uc, Error **errp)
> > >
> > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > >
> > > +    s->sendco.done = true;
> > > +    g_queue_init(&s->sendco.send_list);
> > > +
> > >      g_queue_init(&s->conn_list);
> > >
> > >      qemu_mutex_init(&event_mtx);
> > > @@ -1224,8 +1306,7 @@ static void colo_flush_packets(void *opaque,
> > > void
> > > *user_data)
> > >          compare_chr_send(s,
> > >                           pkt->data,
> > >                           pkt->size,
> > > -                         pkt->vnet_hdr_len,
> > > -                         false);
> > > +                         pkt->vnet_hdr_len);
> > >          packet_destroy(pkt, NULL);
> > >      }
> > >      while (!g_queue_is_empty(&conn->secondary_list)) { @@ -1281,6
> > > +1362,11 @@ static void colo_compare_finalize(Object *obj)
> > >      CompareState *s = COLO_COMPARE(obj);
> > >      CompareState *tmp = NULL;
> > >
> > > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > +    aio_context_acquire(ctx);
> > > +    AIO_WAIT_WHILE(ctx, !s->sendco.done);
> > > +    aio_context_release(ctx);
> > > +
> > >      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); @@ -1305,6 +1391,7 @@
> > > static void colo_compare_finalize(Object *obj)
> > >      g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> > >
> > >      g_queue_clear(&s->conn_list);
> > > +    g_queue_clear(&s->sendco.send_list);
> > >
> > >      if (s->connection_track_table) {
> > >          g_hash_table_destroy(s->connection_track_table);
> > > --
> > > 2.20.1
> >

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

[Lukas Straub]

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

On Wed, 29 Apr 2020 05:37:17 +0000
"Zhang, Chen" <chen.zhang@intel.com> wrote:

> > -----Original Message-----
> > From: Lukas Straub <lukasstraub2@web.de>
> > Sent: Monday, April 27, 2020 3:22 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 v3 3/6] net/colo-compare.c: Fix deadlock in
> > compare_chr_send
> > 
> > On Mon, 27 Apr 2020 03:36:57 +0000
> > "Zhang, Chen" <chen.zhang@intel.com> wrote:
> >   
> > > > -----Original Message-----
> > > > From: Lukas Straub <lukasstraub2@web.de>
> > > > Sent: Monday, April 27, 2020 5:19 AM
> > > > 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 v3 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. Also create a new function
> > > > notify_chr_send, since that should be independend.
> > > >
> > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > > ---
> > > >  net/colo-compare.c | 173 ++++++++++++++++++++++++++++++++++---  
> > ----  
> > > > ----
> > > >  1 file changed, 130 insertions(+), 43 deletions(-)
> > > >
> > > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > > 1de4220fe2..ff6a740284 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,20 @@ static int event_unhandled_count;
> > > >   *                    |packet  |  |packet  +    |packet  | |packet  +
> > > >   *                    +--------+  +--------+    +--------+ +--------+
> > > >   */
> > > > +
> > > > +typedef struct SendCo {
> > > > +    Coroutine *co;
> > > > +    GQueue send_list;
> > > > +    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 +108,7 @@ typedef struct CompareState {
> > > >      SocketReadState pri_rs;
> > > >      SocketReadState sec_rs;
> > > >      SocketReadState notify_rs;
> > > > +    SendCo sendco;
> > > >      bool vnet_hdr;
> > > >      uint32_t compare_timeout;
> > > >      uint32_t expired_scan_cycle;
> > > > @@ -126,8 +144,11 @@ enum {
> > > >  static int compare_chr_send(CompareState *s,
> > > >                              const uint8_t *buf,
> > > >                              uint32_t size,
> > > > -                            uint32_t vnet_hdr_len,
> > > > -                            bool notify_remote_frame);
> > > > +                            uint32_t vnet_hdr_len);
> > > > +
> > > > +static int notify_chr_send(CompareState *s,
> > > > +                           const uint8_t *buf,
> > > > +                           uint32_t size);
> > > >
> > > >  static bool packet_matches_str(const char *str,
> > > >                                 const uint8_t *buf, @@ -145,7 +166,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
> > > >      if (ret < 0) {
> > > >          error_report("Notify Xen COLO-frame failed");
> > > >      }
> > > > @@ -271,8 +292,7 @@ static void
> > > > colo_release_primary_pkt(CompareState
> > > > *s, Packet *pkt)
> > > >      ret = compare_chr_send(s,
> > > >                             pkt->data,
> > > >                             pkt->size,
> > > > -                           pkt->vnet_hdr_len,
> > > > -                           false);
> > > > +                           pkt->vnet_hdr_len);
> > > >      if (ret < 0) {
> > > >          error_report("colo send primary packet failed");
> > > >      }
> > > > @@ -699,63 +719,123 @@ 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)
> > > >  {
> > > > +    CompareState *s = opaque;
> > > > +    SendCo *sendco = &s->sendco;
> > > >      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));  
> > > > -    }
> > > >
> > > > -    if (ret != sizeof(len)) {
> > > > -        goto err;
> > > > -    }
> > > > +        if (ret != sizeof(len)) {
> > > > +            g_free(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 (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,
> > > >                                          (uint8_t *)&len,
> > > >                                          sizeof(len));
> > > > +
> > > > +            if (ret != sizeof(len)) {
> > > > +                g_free(entry);
> > > > +                goto err;
> > > > +            }
> > > >          }
> > > >
> > > > -        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;
> > > >          }
> > > > +
> > > > +        g_free(entry);
> > > >      }
> > > >
> > > > -    if (notify_remote_frame) {
> > > > -        ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > > > -                                    (uint8_t *)buf,
> > > > -                                    size);
> > > > -    } else {
> > > > -        ret = qemu_chr_fe_write_all(&s->chr_out, (uint8_t *)buf, size);
> > > > +    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);
> > > >      }
> > > > +    sendco->ret = ret < 0 ? ret : -EIO;
> > > > +out:
> > > > +    sendco->co = NULL;
> > > > +    sendco->done = true;
> > > > +    aio_wait_kick();
> > > > +}
> > > > +
> > > > +static int compare_chr_send(CompareState *s,
> > > > +                            const uint8_t *buf,
> > > > +                            uint32_t size,
> > > > +                            uint32_t vnet_hdr_len) {
> > > > +    SendCo *sendco = &s->sendco;
> > > > +    SendEntry *entry;
> > > > +
> > > > +    if (!size) {
> > > > +        return 0;
> > > > +    }
> > > > +
> > > > +    entry = g_malloc(sizeof(SendEntry) + size);
> > > > +    entry->size = size;
> > > > +    entry->vnet_hdr_len = vnet_hdr_len;
> > > > +    memcpy(entry->buf, buf, size);
> > > > +    g_queue_push_head(&sendco->send_list, entry);
> > > > +
> > > > +    if (sendco->done) {
> > > > +        sendco->co = qemu_coroutine_create(_compare_chr_send, s);
> > > > +        sendco->done = false;
> > > > +        qemu_coroutine_enter(sendco->co);
> > > > +        if (sendco->done) {
> > > > +            /* report early errors */
> > > > +            return sendco->ret;
> > > > +        }
> > > > +    }
> > > > +
> > > > +    /* assume success */
> > > > +    return 0;
> > > > +}
> > > > +  
> > >
> > > Why not make notify_chr_send same as compare_chr_send?  
> > 
> > Hello,
> > The notify chardev_dev is not affected from this deadlock issue and is
> > independent from the outdev chardev. So it wouldn't make sense for notify
> > messages to wait in the queue if the outdev chardev is blocked. Also, the
> > code is easier to understand this way.
> >   
> 
> Yes, I means maybe the deadlock issue will also occur in Xen COLO side, we can resolve the potential problem here.

Ok,
I will change it in the next version.

> Thanks
> Zhang Chen
> 
> > Regards,
> > Lukas Straub
> >   
> > > Thanks
> > > Zhang Chen
> > >  
> > > > +static int notify_chr_send(CompareState *s,
> > > > +                           const uint8_t *buf,
> > > > +                           uint32_t size) {
> > > > +    int ret = 0;
> > > > +    uint32_t len = htonl(size);
> > > > +
> > > > +    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > > > +                            (uint8_t *)&len,
> > > > +                            sizeof(len));
> > > > +
> > > > +    if (ret != sizeof(len)) {
> > > > +        goto err;
> > > > +    }
> > > > +
> > > > +    ret = qemu_chr_fe_write_all(&s->chr_notify_dev,
> > > > +                                (uint8_t *)buf,
> > > > +                                size);
> > > >
> > > >      if (ret != size) {
> > > >          goto err;
> > > >      }
> > > >
> > > >      return 0;
> > > > -
> > > >  err:
> > > >      return ret < 0 ? ret : -EIO;
> > > >  }
> > > > @@ -1062,8 +1142,7 @@ static void
> > > > compare_pri_rs_finalize(SocketReadState *pri_rs)
> > > >          compare_chr_send(s,
> > > >                           pri_rs->buf,
> > > >                           pri_rs->packet_len,
> > > > -                         pri_rs->vnet_hdr_len,
> > > > -                         false);
> > > > +                         pri_rs->vnet_hdr_len);
> > > >      } else {
> > > >          /* compare packet in the specified connection */
> > > >          colo_compare_connection(conn, s); @@ -1093,7 +1172,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 = notify_chr_send(s, (uint8_t *)msg, strlen(msg));
> > > >          if (ret < 0) {
> > > >              error_report("Notify Xen COLO-frame INIT failed");
> > > >          }
> > > > @@ -1199,6 +1278,9 @@ static void
> > > > colo_compare_complete(UserCreatable
> > > > *uc, Error **errp)
> > > >
> > > >      QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > >
> > > > +    s->sendco.done = true;
> > > > +    g_queue_init(&s->sendco.send_list);
> > > > +
> > > >      g_queue_init(&s->conn_list);
> > > >
> > > >      qemu_mutex_init(&event_mtx);
> > > > @@ -1224,8 +1306,7 @@ static void colo_flush_packets(void *opaque,
> > > > void
> > > > *user_data)
> > > >          compare_chr_send(s,
> > > >                           pkt->data,
> > > >                           pkt->size,
> > > > -                         pkt->vnet_hdr_len,
> > > > -                         false);
> > > > +                         pkt->vnet_hdr_len);
> > > >          packet_destroy(pkt, NULL);
> > > >      }
> > > >      while (!g_queue_is_empty(&conn->secondary_list)) { @@ -1281,6
> > > > +1362,11 @@ static void colo_compare_finalize(Object *obj)
> > > >      CompareState *s = COLO_COMPARE(obj);
> > > >      CompareState *tmp = NULL;
> > > >
> > > > +    AioContext *ctx = iothread_get_aio_context(s->iothread);
> > > > +    aio_context_acquire(ctx);
> > > > +    AIO_WAIT_WHILE(ctx, !s->sendco.done);
> > > > +    aio_context_release(ctx);
> > > > +
> > > >      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); @@ -1305,6 +1391,7 @@
> > > > static void colo_compare_finalize(Object *obj)
> > > >      g_queue_foreach(&s->conn_list, colo_flush_packets, s);
> > > >
> > > >      g_queue_clear(&s->conn_list);
> > > > +    g_queue_clear(&s->sendco.send_list);
> > > >
> > > >      if (s->connection_track_table) {
> > > >          g_hash_table_destroy(s->connection_track_table);
> > > > --
> > > > 2.20.1  
> > >  
> 


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

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

[Lukas Straub]

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

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

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 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 ff6a740284..6634911770 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -479,10 +479,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 v3 4/6] net/colo-compare.c: Only hexdump packets if tracing is enabled

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <lukasstraub2@web.de>
> Sent: Monday, April 27, 2020 5:19 AM
> 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 v3 4/6] net/colo-compare.c: Only hexdump packets if tracing
> is enabled
> 
> Else the log will be flooded if there is a lot of network traffic.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> 

Looks good for me.

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
> ff6a740284..6634911770 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -479,10 +479,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

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

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 5259 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 6634911770..f3074ee3ff 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;
@@ -912,6 +914,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) {
@@ -925,6 +933,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)
@@ -1278,16 +1287,20 @@ 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->sendco.done = true;
     g_queue_init(&s->sendco.send_list);
 
     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,
@@ -1382,12 +1395,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);
 
     /* Release all unhandled packets after compare thead exited */
     g_queue_foreach(&s->conn_list, colo_flush_packets, s);
@@ -1403,15 +1423,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 --]

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

[Lukas Straub]

[-- Attachment #1: Type: text/plain, Size: 3036 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 | 47 +++++++++++++++++++++++-----------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index f3074ee3ff..e86dd72f02 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -1287,15 +1287,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->sendco.done = true;
     g_queue_init(&s->sendco.send_list);
 
@@ -1307,6 +1298,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;
 }
 
@@ -1377,6 +1378,20 @@ static void colo_compare_finalize(Object *obj)
     CompareState *s = COLO_COMPARE(obj);
     CompareState *tmp = NULL;
 
+    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);
     AIO_WAIT_WHILE(ctx, !s->sendco.done);
@@ -1395,20 +1410,6 @@ 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);
-
     /* Release all unhandled packets after compare thead exited */
     g_queue_foreach(&s->conn_list, colo_flush_packets, s);
 
-- 
2.20.1

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