[Qemu-devel] [PATCH v3 0/4] colo-compare: fix some bugs

[Qemu-devel] [PATCH v3 0/4] colo-compare: fix some bugs

[zhanghailiang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 1098 bytes --]

This series includes two parts: codes optimization and bug fix.
patch 1 tries to move timer process into colo compare thread as 
a new coroutine.
patch 2 ~ 4 fixe some bugs of colo compare.

v2->v3:
 - change the definition of remove_fd_in_watch() instead of
   introducing a function (Marc-André Lureau's suggestion)
v1->v2:
 - Squash patch 3 of last version into patch 2. (ZhangChen's suggestion)

zhanghailiang (4):
  colo-compare: use g_timeout_source_new() to process the stale packets
  colo-compare: kick compare thread to exit after some cleanup in
    finalization
  char: remove the right fd been watched in qemu_chr_fe_set_handlers()
  colo-compare: Fix removing fds been watched incorrectly in
    finalization

 chardev/char-fd.c     |   6 +--
 chardev/char-io.c     |   8 ++--
 chardev/char-io.h     |   2 +-
 chardev/char-pty.c    |   2 +-
 chardev/char-socket.c |   4 +-
 chardev/char-udp.c    |   6 +--
 chardev/char.c        |   2 +-
 net/colo-compare.c    | 115 ++++++++++++++++++++++++++------------------------
 8 files changed, 74 insertions(+), 71 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH v3 1/4] colo-compare: use g_timeout_source_new() to process the stale packets

[zhanghailiang]

Instead of using qemu timer to process the stale packets,
We re-use the colo compare thread to process these packets
by creating a new timeout coroutine.

Besides, since we process all the same vNIC's net connection/packets
in one thread, it is safe to remove the timer_check_lock.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 net/colo-compare.c | 62 +++++++++++++++++++-----------------------------------
 1 file changed, 22 insertions(+), 40 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 162fd6a..fdde788 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -83,9 +83,6 @@ typedef struct CompareState {
     GHashTable *connection_track_table;
     /* compare thread, a thread for each NIC */
     QemuThread thread;
-    /* Timer used on the primary to find packets that are never matched */
-    QEMUTimer *timer;
-    QemuMutex timer_check_lock;
 } CompareState;
 
 typedef struct CompareClass {
@@ -374,9 +371,7 @@ static void colo_compare_connection(void *opaque, void *user_data)
 
     while (!g_queue_is_empty(&conn->primary_list) &&
            !g_queue_is_empty(&conn->secondary_list)) {
-        qemu_mutex_lock(&s->timer_check_lock);
         pkt = g_queue_pop_tail(&conn->primary_list);
-        qemu_mutex_unlock(&s->timer_check_lock);
         switch (conn->ip_proto) {
         case IPPROTO_TCP:
             result = g_queue_find_custom(&conn->secondary_list,
@@ -411,9 +406,7 @@ static void colo_compare_connection(void *opaque, void *user_data)
              * until next comparison.
              */
             trace_colo_compare_main("packet different");
-            qemu_mutex_lock(&s->timer_check_lock);
             g_queue_push_tail(&conn->primary_list, pkt);
-            qemu_mutex_unlock(&s->timer_check_lock);
             /* TODO: colo_notify_checkpoint();*/
             break;
         }
@@ -486,11 +479,26 @@ static void compare_sec_chr_in(void *opaque, const uint8_t *buf, int size)
     }
 }
 
+/*
+ * Check old packet regularly so it can watch for any packets
+ * that the secondary hasn't produced equivalents of.
+ */
+static gboolean check_old_packet_regular(void *opaque)
+{
+    CompareState *s = opaque;
+
+    /* if have old packet we will notify checkpoint */
+    colo_old_packet_check(s);
+
+    return TRUE;
+}
+
 static void *colo_compare_thread(void *opaque)
 {
     GMainContext *worker_context;
     GMainLoop *compare_loop;
     CompareState *s = opaque;
+    GSource *timeout_source;
 
     worker_context = g_main_context_new();
 
@@ -501,8 +509,15 @@ static void *colo_compare_thread(void *opaque)
 
     compare_loop = g_main_loop_new(worker_context, FALSE);
 
+    /* To kick any packets that the secondary doesn't match */
+    timeout_source = g_timeout_source_new(REGULAR_PACKET_CHECK_MS);
+    g_source_set_callback(timeout_source,
+                          (GSourceFunc)check_old_packet_regular, s, NULL);
+    g_source_attach(timeout_source, worker_context);
+
     g_main_loop_run(compare_loop);
 
+    g_source_unref(timeout_source);
     g_main_loop_unref(compare_loop);
     g_main_context_unref(worker_context);
     return NULL;
@@ -604,26 +619,6 @@ static int find_and_check_chardev(Chardev **chr,
 }
 
 /*
- * Check old packet regularly so it can watch for any packets
- * that the secondary hasn't produced equivalents of.
- */
-static void check_old_packet_regular(void *opaque)
-{
-    CompareState *s = opaque;
-
-    timer_mod(s->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
-              REGULAR_PACKET_CHECK_MS);
-    /* if have old packet we will notify checkpoint */
-    /*
-     * TODO: Make timer handler run in compare thread
-     * like qemu_chr_add_handlers_full.
-     */
-    qemu_mutex_lock(&s->timer_check_lock);
-    colo_old_packet_check(s);
-    qemu_mutex_unlock(&s->timer_check_lock);
-}
-
-/*
  * Called from the main thread on the primary
  * to setup colo-compare.
  */
@@ -665,7 +660,6 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
     net_socket_rs_init(&s->sec_rs, compare_sec_rs_finalize);
 
     g_queue_init(&s->conn_list);
-    qemu_mutex_init(&s->timer_check_lock);
 
     s->connection_track_table = g_hash_table_new_full(connection_key_hash,
                                                       connection_key_equal,
@@ -678,12 +672,6 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
                        QEMU_THREAD_JOINABLE);
     compare_id++;
 
-    /* A regular timer to kick any packets that the secondary doesn't match */
-    s->timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, /* Only when guest runs */
-                            check_old_packet_regular, s);
-    timer_mod(s->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) +
-                        REGULAR_PACKET_CHECK_MS);
-
     return;
 }
 
@@ -723,12 +711,6 @@ static void colo_compare_finalize(Object *obj)
         qemu_thread_join(&s->thread);
     }
 
-    if (s->timer) {
-        timer_del(s->timer);
-    }
-
-    qemu_mutex_destroy(&s->timer_check_lock);
-
     g_free(s->pri_indev);
     g_free(s->sec_indev);
     g_free(s->outdev);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 1/4] colo-compare: use g_timeout_source_new() to process the stale packets

[Zhang Chen]

On 02/17/2017 10:53 AM, zhanghailiang wrote:
> Instead of using qemu timer to process the stale packets,
> We re-use the colo compare thread to process these packets
> by creating a new timeout coroutine.
>
> Besides, since we process all the same vNIC's net connection/packets
> in one thread, it is safe to remove the timer_check_lock.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>

> ---
>   net/colo-compare.c | 62 +++++++++++++++++++-----------------------------------
>   1 file changed, 22 insertions(+), 40 deletions(-)
>
>
-- 
Thanks
Zhang Chen

[Qemu-devel] [PATCH v3 2/4] colo-compare: kick compare thread to exit after some cleanup in finalization

[zhanghailiang]

We should call g_main_loop_quit() to notify colo compare thread to
exit, Or it will run in g_main_loop_run() forever.

Besides, the finalizing process can't happen in context of colo thread,
it is reasonable to remove the 'if (qemu_thread_is_self(&s->thread))'
branch.

Before compare thead exits, some cleanup works need to be
done,  All unhandled packets need to be released and connection_track_table
needs to be freed, or there will be memory leak.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 net/colo-compare.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index fdde788..37ce75c 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -83,6 +83,8 @@ typedef struct CompareState {
     GHashTable *connection_track_table;
     /* compare thread, a thread for each NIC */
     QemuThread thread;
+
+    GMainLoop *compare_loop;
 } CompareState;
 
 typedef struct CompareClass {
@@ -496,7 +498,6 @@ static gboolean check_old_packet_regular(void *opaque)
 static void *colo_compare_thread(void *opaque)
 {
     GMainContext *worker_context;
-    GMainLoop *compare_loop;
     CompareState *s = opaque;
     GSource *timeout_source;
 
@@ -507,7 +508,7 @@ static void *colo_compare_thread(void *opaque)
     qemu_chr_fe_set_handlers(&s->chr_sec_in, compare_chr_can_read,
                              compare_sec_chr_in, NULL, s, worker_context, true);
 
-    compare_loop = g_main_loop_new(worker_context, FALSE);
+    s->compare_loop = g_main_loop_new(worker_context, FALSE);
 
     /* To kick any packets that the secondary doesn't match */
     timeout_source = g_timeout_source_new(REGULAR_PACKET_CHECK_MS);
@@ -515,10 +516,10 @@ static void *colo_compare_thread(void *opaque)
                           (GSourceFunc)check_old_packet_regular, s, NULL);
     g_source_attach(timeout_source, worker_context);
 
-    g_main_loop_run(compare_loop);
+    g_main_loop_run(s->compare_loop);
 
     g_source_unref(timeout_source);
-    g_main_loop_unref(compare_loop);
+    g_main_loop_unref(s->compare_loop);
     g_main_context_unref(worker_context);
     return NULL;
 }
@@ -675,6 +676,23 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
     return;
 }
 
+static void colo_flush_packets(void *opaque, void *user_data)
+{
+    CompareState *s = user_data;
+    Connection *conn = opaque;
+    Packet *pkt = NULL;
+
+    while (!g_queue_is_empty(&conn->primary_list)) {
+        pkt = g_queue_pop_head(&conn->primary_list);
+        compare_chr_send(&s->chr_out, pkt->data, pkt->size);
+        packet_destroy(pkt, NULL);
+    }
+    while (!g_queue_is_empty(&conn->secondary_list)) {
+        pkt = g_queue_pop_head(&conn->secondary_list);
+        packet_destroy(pkt, NULL);
+    }
+}
+
 static void colo_compare_class_init(ObjectClass *oc, void *data)
 {
     UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
@@ -703,14 +721,15 @@ static void colo_compare_finalize(Object *obj)
     qemu_chr_fe_deinit(&s->chr_sec_in);
     qemu_chr_fe_deinit(&s->chr_out);
 
-    g_queue_free(&s->conn_list);
+    g_main_loop_quit(s->compare_loop);
+    qemu_thread_join(&s->thread);
 
-    if (qemu_thread_is_self(&s->thread)) {
-        /* compare connection */
-        g_queue_foreach(&s->conn_list, colo_compare_connection, s);
-        qemu_thread_join(&s->thread);
-    }
+    /* Release all unhandled packets after compare thead exited */
+    g_queue_foreach(&s->conn_list, colo_flush_packets, s);
+
+    g_queue_free(&s->conn_list);
 
+    g_hash_table_destroy(s->connection_track_table);
     g_free(s->pri_indev);
     g_free(s->sec_indev);
     g_free(s->outdev);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 2/4] colo-compare: kick compare thread to exit after some cleanup in finalization

[Zhang Chen]

On 02/17/2017 10:53 AM, zhanghailiang wrote:
> We should call g_main_loop_quit() to notify colo compare thread to
> exit, Or it will run in g_main_loop_run() forever.
>
> Besides, the finalizing process can't happen in context of colo thread,
> it is reasonable to remove the 'if (qemu_thread_is_self(&s->thread))'
> branch.
>
> Before compare thead exits, some cleanup works need to be
> done,  All unhandled packets need to be released and connection_track_table
> needs to be freed, or there will be memory leak.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>

> ---
>   net/colo-compare.c | 39 +++++++++++++++++++++++++++++----------
>   1 file changed, 29 insertions(+), 10 deletions(-)

-- 
Thanks
Zhang Chen

[Qemu-devel] [PATCH v3 3/4] char: remove the right fd been watched in qemu_chr_fe_set_handlers()

[zhanghailiang]

We can call qemu_chr_fe_set_handlers() to add/remove fd been watched
in 'context' which can be either default main context or other explicit
context. But the original logic is not correct, we didn't remove
the right fd because we call g_main_context_find_source_by_id(NULL, tag)
which always try to find the Gsource from default context.

Fix it by passing the right context to g_main_context_find_source_by_id().

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 chardev/char-fd.c     | 6 +++---
 chardev/char-io.c     | 8 ++++----
 chardev/char-io.h     | 2 +-
 chardev/char-pty.c    | 2 +-
 chardev/char-socket.c | 4 ++--
 chardev/char-udp.c    | 6 +++---
 chardev/char.c        | 2 +-
 7 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index fb51ab4..548dd4c 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -58,7 +58,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     ret = qio_channel_read(
         chan, (gchar *)buf, len, NULL);
     if (ret == 0) {
-        remove_fd_in_watch(chr);
+        remove_fd_in_watch(chr, NULL);
         qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
         return FALSE;
     }
@@ -89,7 +89,7 @@ static void fd_chr_update_read_handler(Chardev *chr,
 {
     FDChardev *s = FD_CHARDEV(chr);
 
-    remove_fd_in_watch(chr);
+    remove_fd_in_watch(chr, NULL);
     if (s->ioc_in) {
         chr->fd_in_tag = io_add_watch_poll(chr, s->ioc_in,
                                            fd_chr_read_poll,
@@ -103,7 +103,7 @@ static void char_fd_finalize(Object *obj)
     Chardev *chr = CHARDEV(obj);
     FDChardev *s = FD_CHARDEV(obj);
 
-    remove_fd_in_watch(chr);
+    remove_fd_in_watch(chr, NULL);
     if (s->ioc_in) {
         object_unref(OBJECT(s->ioc_in));
     }
diff --git a/chardev/char-io.c b/chardev/char-io.c
index 7dfc3f2..b4bb094 100644
--- a/chardev/char-io.c
+++ b/chardev/char-io.c
@@ -127,14 +127,14 @@ guint io_add_watch_poll(Chardev *chr,
     return tag;
 }
 
-static void io_remove_watch_poll(guint tag)
+static void io_remove_watch_poll(guint tag, GMainContext *context)
 {
     GSource *source;
     IOWatchPoll *iwp;
 
     g_return_if_fail(tag > 0);
 
-    source = g_main_context_find_source_by_id(NULL, tag);
+    source = g_main_context_find_source_by_id(context, tag);
     g_return_if_fail(source != NULL);
 
     iwp = io_watch_poll_from_source(source);
@@ -146,10 +146,10 @@ static void io_remove_watch_poll(guint tag)
     g_source_destroy(&iwp->parent);
 }
 
-void remove_fd_in_watch(Chardev *chr)
+void remove_fd_in_watch(Chardev *chr, GMainContext *context)
 {
     if (chr->fd_in_tag) {
-        io_remove_watch_poll(chr->fd_in_tag);
+        io_remove_watch_poll(chr->fd_in_tag, context);
         chr->fd_in_tag = 0;
     }
 }
diff --git a/chardev/char-io.h b/chardev/char-io.h
index d7ae5f1..842be56 100644
--- a/chardev/char-io.h
+++ b/chardev/char-io.h
@@ -36,7 +36,7 @@ guint io_add_watch_poll(Chardev *chr,
                         gpointer user_data,
                         GMainContext *context);
 
-void remove_fd_in_watch(Chardev *chr);
+void remove_fd_in_watch(Chardev *chr, GMainContext *context);
 
 int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
 
diff --git a/chardev/char-pty.c b/chardev/char-pty.c
index ecf2c7a..a6337be 100644
--- a/chardev/char-pty.c
+++ b/chardev/char-pty.c
@@ -199,7 +199,7 @@ static void pty_chr_state(Chardev *chr, int connected)
             g_source_remove(s->open_tag);
             s->open_tag = 0;
         }
-        remove_fd_in_watch(chr);
+        remove_fd_in_watch(chr, NULL);
         s->connected = 0;
         /* (re-)connect poll interval for idle guests: once per second.
          * We check more frequently in case the guests sends data to
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 865c527..d7e92e1 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -328,7 +328,7 @@ static void tcp_chr_free_connection(Chardev *chr)
     }
 
     tcp_set_msgfds(chr, NULL, 0);
-    remove_fd_in_watch(chr);
+    remove_fd_in_watch(chr, NULL);
     object_unref(OBJECT(s->sioc));
     s->sioc = NULL;
     object_unref(OBJECT(s->ioc));
@@ -498,7 +498,7 @@ static void tcp_chr_update_read_handler(Chardev *chr,
         return;
     }
 
-    remove_fd_in_watch(chr);
+    remove_fd_in_watch(chr, NULL);
     if (s->ioc) {
         chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
                                            tcp_chr_read_poll,
diff --git a/chardev/char-udp.c b/chardev/char-udp.c
index 2c6c7dd..804bd22 100644
--- a/chardev/char-udp.c
+++ b/chardev/char-udp.c
@@ -81,7 +81,7 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
     ret = qio_channel_read(
         s->ioc, (char *)s->buf, sizeof(s->buf), NULL);
     if (ret <= 0) {
-        remove_fd_in_watch(chr);
+        remove_fd_in_watch(chr, NULL);
         return FALSE;
     }
     s->bufcnt = ret;
@@ -101,7 +101,7 @@ static void udp_chr_update_read_handler(Chardev *chr,
 {
     UdpChardev *s = UDP_CHARDEV(chr);
 
-    remove_fd_in_watch(chr);
+    remove_fd_in_watch(chr, NULL);
     if (s->ioc) {
         chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
                                            udp_chr_read_poll,
@@ -115,7 +115,7 @@ static void char_udp_finalize(Object *obj)
     Chardev *chr = CHARDEV(obj);
     UdpChardev *s = UDP_CHARDEV(obj);
 
-    remove_fd_in_watch(chr);
+    remove_fd_in_watch(chr, NULL);
     if (s->ioc) {
         object_unref(OBJECT(s->ioc));
     }
diff --git a/chardev/char.c b/chardev/char.c
index abd525f..8af3634 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -560,7 +560,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
     cc = CHARDEV_GET_CLASS(s);
     if (!opaque && !fd_can_read && !fd_read && !fd_event) {
         fe_open = 0;
-        remove_fd_in_watch(s);
+        remove_fd_in_watch(s, context);
     } else {
         fe_open = 1;
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 3/4] char: remove the right fd been watched in qemu_chr_fe_set_handlers()

[Marc-André Lureau]

Hi

----- Original Message -----
> We can call qemu_chr_fe_set_handlers() to add/remove fd been watched
> in 'context' which can be either default main context or other explicit
> context. But the original logic is not correct, we didn't remove
> the right fd because we call g_main_context_find_source_by_id(NULL, tag)
> which always try to find the Gsource from default context.
> 
> Fix it by passing the right context to g_main_context_find_source_by_id().
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-fd.c     | 6 +++---
>  chardev/char-io.c     | 8 ++++----
>  chardev/char-io.h     | 2 +-
>  chardev/char-pty.c    | 2 +-
>  chardev/char-socket.c | 4 ++--
>  chardev/char-udp.c    | 6 +++---
>  chardev/char.c        | 2 +-
>  7 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/chardev/char-fd.c b/chardev/char-fd.c
> index fb51ab4..548dd4c 100644
> --- a/chardev/char-fd.c
> +++ b/chardev/char-fd.c
> @@ -58,7 +58,7 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition
> cond, void *opaque)
>      ret = qio_channel_read(
>          chan, (gchar *)buf, len, NULL);
>      if (ret == 0) {
> -        remove_fd_in_watch(chr);
> +        remove_fd_in_watch(chr, NULL);
>          qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
>          return FALSE;
>      }
> @@ -89,7 +89,7 @@ static void fd_chr_update_read_handler(Chardev *chr,
>  {
>      FDChardev *s = FD_CHARDEV(chr);
>  
> -    remove_fd_in_watch(chr);
> +    remove_fd_in_watch(chr, NULL);
>      if (s->ioc_in) {
>          chr->fd_in_tag = io_add_watch_poll(chr, s->ioc_in,
>                                             fd_chr_read_poll,
> @@ -103,7 +103,7 @@ static void char_fd_finalize(Object *obj)
>      Chardev *chr = CHARDEV(obj);
>      FDChardev *s = FD_CHARDEV(obj);
>  
> -    remove_fd_in_watch(chr);
> +    remove_fd_in_watch(chr, NULL);
>      if (s->ioc_in) {
>          object_unref(OBJECT(s->ioc_in));
>      }
> diff --git a/chardev/char-io.c b/chardev/char-io.c
> index 7dfc3f2..b4bb094 100644
> --- a/chardev/char-io.c
> +++ b/chardev/char-io.c
> @@ -127,14 +127,14 @@ guint io_add_watch_poll(Chardev *chr,
>      return tag;
>  }
>  
> -static void io_remove_watch_poll(guint tag)
> +static void io_remove_watch_poll(guint tag, GMainContext *context)
>  {
>      GSource *source;
>      IOWatchPoll *iwp;
>  
>      g_return_if_fail(tag > 0);
>  
> -    source = g_main_context_find_source_by_id(NULL, tag);
> +    source = g_main_context_find_source_by_id(context, tag);
>      g_return_if_fail(source != NULL);
>  
>      iwp = io_watch_poll_from_source(source);
> @@ -146,10 +146,10 @@ static void io_remove_watch_poll(guint tag)
>      g_source_destroy(&iwp->parent);
>  }
>  
> -void remove_fd_in_watch(Chardev *chr)
> +void remove_fd_in_watch(Chardev *chr, GMainContext *context)
>  {
>      if (chr->fd_in_tag) {
> -        io_remove_watch_poll(chr->fd_in_tag);
> +        io_remove_watch_poll(chr->fd_in_tag, context);
>          chr->fd_in_tag = 0;
>      }
>  }
> diff --git a/chardev/char-io.h b/chardev/char-io.h
> index d7ae5f1..842be56 100644
> --- a/chardev/char-io.h
> +++ b/chardev/char-io.h
> @@ -36,7 +36,7 @@ guint io_add_watch_poll(Chardev *chr,
>                          gpointer user_data,
>                          GMainContext *context);
>  
> -void remove_fd_in_watch(Chardev *chr);
> +void remove_fd_in_watch(Chardev *chr, GMainContext *context);
>  
>  int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
>  
> diff --git a/chardev/char-pty.c b/chardev/char-pty.c
> index ecf2c7a..a6337be 100644
> --- a/chardev/char-pty.c
> +++ b/chardev/char-pty.c
> @@ -199,7 +199,7 @@ static void pty_chr_state(Chardev *chr, int connected)
>              g_source_remove(s->open_tag);
>              s->open_tag = 0;
>          }
> -        remove_fd_in_watch(chr);
> +        remove_fd_in_watch(chr, NULL);
>          s->connected = 0;
>          /* (re-)connect poll interval for idle guests: once per second.
>           * We check more frequently in case the guests sends data to
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 865c527..d7e92e1 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -328,7 +328,7 @@ static void tcp_chr_free_connection(Chardev *chr)
>      }
>  
>      tcp_set_msgfds(chr, NULL, 0);
> -    remove_fd_in_watch(chr);
> +    remove_fd_in_watch(chr, NULL);
>      object_unref(OBJECT(s->sioc));
>      s->sioc = NULL;
>      object_unref(OBJECT(s->ioc));
> @@ -498,7 +498,7 @@ static void tcp_chr_update_read_handler(Chardev *chr,
>          return;
>      }
>  
> -    remove_fd_in_watch(chr);
> +    remove_fd_in_watch(chr, NULL);
>      if (s->ioc) {
>          chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>                                             tcp_chr_read_poll,
> diff --git a/chardev/char-udp.c b/chardev/char-udp.c
> index 2c6c7dd..804bd22 100644
> --- a/chardev/char-udp.c
> +++ b/chardev/char-udp.c
> @@ -81,7 +81,7 @@ static gboolean udp_chr_read(QIOChannel *chan, GIOCondition
> cond, void *opaque)
>      ret = qio_channel_read(
>          s->ioc, (char *)s->buf, sizeof(s->buf), NULL);
>      if (ret <= 0) {
> -        remove_fd_in_watch(chr);
> +        remove_fd_in_watch(chr, NULL);
>          return FALSE;
>      }
>      s->bufcnt = ret;
> @@ -101,7 +101,7 @@ static void udp_chr_update_read_handler(Chardev *chr,
>  {
>      UdpChardev *s = UDP_CHARDEV(chr);
>  
> -    remove_fd_in_watch(chr);
> +    remove_fd_in_watch(chr, NULL);
>      if (s->ioc) {
>          chr->fd_in_tag = io_add_watch_poll(chr, s->ioc,
>                                             udp_chr_read_poll,
> @@ -115,7 +115,7 @@ static void char_udp_finalize(Object *obj)
>      Chardev *chr = CHARDEV(obj);
>      UdpChardev *s = UDP_CHARDEV(obj);
>  
> -    remove_fd_in_watch(chr);
> +    remove_fd_in_watch(chr, NULL);
>      if (s->ioc) {
>          object_unref(OBJECT(s->ioc));
>      }
> diff --git a/chardev/char.c b/chardev/char.c
> index abd525f..8af3634 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -560,7 +560,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>      cc = CHARDEV_GET_CLASS(s);
>      if (!opaque && !fd_can_read && !fd_read && !fd_event) {
>          fe_open = 0;
> -        remove_fd_in_watch(s);
> +        remove_fd_in_watch(s, context);
>      } else {
>          fe_open = 1;
>      }
> --
> 1.8.3.1
> 
> 
> 

[Qemu-devel] [PATCH v3 4/4] colo-compare: Fix removing fds been watched incorrectly in finalization

[zhanghailiang]

We will catch the bellow error report while try to delete compare object
by qmp command:
chardev/char-io.c:91: io_watch_poll_finalize: Assertion `iwp->src == ((void *)0)' failed.

This is caused by failing to remove the right fd been watched while
call qemu_chr_fe_set_handlers();

Fix it by pass the worker_context parameter to qemu_chr_fe_set_handlers().

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 net/colo-compare.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/net/colo-compare.c b/net/colo-compare.c
index 37ce75c..a6fc2ff 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -84,6 +84,7 @@ typedef struct CompareState {
     /* compare thread, a thread for each NIC */
     QemuThread thread;
 
+    GMainContext *worker_context;
     GMainLoop *compare_loop;
 } CompareState;
 
@@ -497,30 +498,29 @@ static gboolean check_old_packet_regular(void *opaque)
 
 static void *colo_compare_thread(void *opaque)
 {
-    GMainContext *worker_context;
     CompareState *s = opaque;
     GSource *timeout_source;
 
-    worker_context = g_main_context_new();
+    s->worker_context = g_main_context_new();
 
     qemu_chr_fe_set_handlers(&s->chr_pri_in, compare_chr_can_read,
-                             compare_pri_chr_in, NULL, s, worker_context, true);
+                          compare_pri_chr_in, NULL, s, s->worker_context, true);
     qemu_chr_fe_set_handlers(&s->chr_sec_in, compare_chr_can_read,
-                             compare_sec_chr_in, NULL, s, worker_context, true);
+                          compare_sec_chr_in, NULL, s, s->worker_context, true);
 
-    s->compare_loop = g_main_loop_new(worker_context, FALSE);
+    s->compare_loop = g_main_loop_new(s->worker_context, FALSE);
 
     /* To kick any packets that the secondary doesn't match */
     timeout_source = g_timeout_source_new(REGULAR_PACKET_CHECK_MS);
     g_source_set_callback(timeout_source,
                           (GSourceFunc)check_old_packet_regular, s, NULL);
-    g_source_attach(timeout_source, worker_context);
+    g_source_attach(timeout_source, s->worker_context);
 
     g_main_loop_run(s->compare_loop);
 
     g_source_unref(timeout_source);
     g_main_loop_unref(s->compare_loop);
-    g_main_context_unref(worker_context);
+    g_main_context_unref(s->worker_context);
     return NULL;
 }
 
@@ -717,8 +717,10 @@ static void colo_compare_finalize(Object *obj)
 {
     CompareState *s = COLO_COMPARE(obj);
 
-    qemu_chr_fe_deinit(&s->chr_pri_in);
-    qemu_chr_fe_deinit(&s->chr_sec_in);
+    qemu_chr_fe_set_handlers(&s->chr_pri_in, NULL, NULL, NULL, NULL,
+                             s->worker_context, true);
+    qemu_chr_fe_set_handlers(&s->chr_sec_in, NULL, NULL, NULL, NULL,
+                             s->worker_context, true);
     qemu_chr_fe_deinit(&s->chr_out);
 
     g_main_loop_quit(s->compare_loop);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v3 4/4] colo-compare: Fix removing fds been watched incorrectly in finalization

[Zhang Chen]

On 02/17/2017 10:53 AM, zhanghailiang wrote:
> We will catch the bellow error report while try to delete compare object
> by qmp command:
> chardev/char-io.c:91: io_watch_poll_finalize: Assertion `iwp->src == ((void *)0)' failed.
>
> This is caused by failing to remove the right fd been watched while
> call qemu_chr_fe_set_handlers();
>
> Fix it by pass the worker_context parameter to qemu_chr_fe_set_handlers().
>
> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>

Reviewed-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>

> ---
>   net/colo-compare.c | 20 +++++++++++---------
>   1 file changed, 11 insertions(+), 9 deletions(-)

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH v3 0/4] colo-compare: fix some bugs

[Jason Wang]

On 2017年02月17日 10:53, zhanghailiang wrote:
> This series includes two parts: codes optimization and bug fix.
> patch 1 tries to move timer process into colo compare thread as
> a new coroutine.
> patch 2 ~ 4 fixe some bugs of colo compare.
>
> v2->v3:
>   - change the definition of remove_fd_in_watch() instead of
>     introducing a function (Marc-André Lureau's suggestion)
> v1->v2:
>   - Squash patch 3 of last version into patch 2. (ZhangChen's suggestion)
>
> zhanghailiang (4):
>    colo-compare: use g_timeout_source_new() to process the stale packets
>    colo-compare: kick compare thread to exit after some cleanup in
>      finalization
>    char: remove the right fd been watched in qemu_chr_fe_set_handlers()
>    colo-compare: Fix removing fds been watched incorrectly in
>      finalization
>
>   chardev/char-fd.c     |   6 +--
>   chardev/char-io.c     |   8 ++--
>   chardev/char-io.h     |   2 +-
>   chardev/char-pty.c    |   2 +-
>   chardev/char-socket.c |   4 +-
>   chardev/char-udp.c    |   6 +--
>   chardev/char.c        |   2 +-
>   net/colo-compare.c    | 115 ++++++++++++++++++++++++++------------------------
>   8 files changed, 74 insertions(+), 71 deletions(-)
>

Applied, thanks.