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

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

[zhanghailiang]

Hi,
This series fixes some bugs of colo compare.

Patch 1 is trying to move timer process into colo compare
thread as a new coroutine.

Patch 2 ~ 5 fix several bugs in colo compare, for more details,
please refer to each patch.

Please review, thanks.

zhanghailiang (5):
  colo-compare: use g_timeout_source_new() to process the stale packets
  colo-compare: kick compare thread to exit while finalize
  colo-compare: release all unhandled packets in finalize function
  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-io.c  |  13 ++++--
 chardev/char-io.h  |   2 +
 chardev/char.c     |   2 +-
 net/colo-compare.c | 115 +++++++++++++++++++++++++++--------------------------
 4 files changed, 71 insertions(+), 61 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/5] 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

[Qemu-devel] [PATCH 2/5] colo-compare: kick compare thread to exit while finalize

[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.

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

diff --git a/net/colo-compare.c b/net/colo-compare.c
index fdde788..a16e2d5 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;
 }
@@ -703,13 +704,11 @@ 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);
-    }
+
+    g_queue_free(&s->conn_list);
 
     g_free(s->pri_indev);
     g_free(s->sec_indev);
-- 
1.8.3.1

[Qemu-devel] [PATCH 3/5] colo-compare: release all unhandled packets in finalize function

[zhanghailiang]

We should release all unhandled packets before finalize colo compare.
Besides, we need to free connection_track_table, or there will be
a memory leak bug.

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

diff --git a/net/colo-compare.c b/net/colo-compare.c
index a16e2d5..809bad3 100644
--- a/net/colo-compare.c
+++ b/net/colo-compare.c
@@ -676,6 +676,23 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
     return;
 }
 
+static void colo_release_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);
@@ -707,9 +724,12 @@ static void colo_compare_finalize(Object *obj)
     g_main_loop_quit(s->compare_loop);
     qemu_thread_join(&s->thread);
 
+    /* Release all unhandled packets after compare thead exited */
+    g_queue_foreach(&s->conn_list, colo_release_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

[Qemu-devel] [PATCH 4/5] 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-io.c | 13 +++++++++----
 chardev/char-io.h |  2 ++
 chardev/char.c    |  2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/chardev/char-io.c b/chardev/char-io.c
index 7dfc3f2..a69cc61 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,14 +146,19 @@ static void io_remove_watch_poll(guint tag)
     g_source_destroy(&iwp->parent);
 }
 
-void remove_fd_in_watch(Chardev *chr)
+void qemu_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;
     }
 }
 
+void remove_fd_in_watch(Chardev *chr)
+{
+    qemu_remove_fd_in_watch(chr, NULL);
+}
+
 int io_channel_send_full(QIOChannel *ioc,
                          const void *buf, size_t len,
                          int *fds, size_t nfds)
diff --git a/chardev/char-io.h b/chardev/char-io.h
index d7ae5f1..117c888 100644
--- a/chardev/char-io.h
+++ b/chardev/char-io.h
@@ -38,6 +38,8 @@ guint io_add_watch_poll(Chardev *chr,
 
 void remove_fd_in_watch(Chardev *chr);
 
+void qemu_remove_fd_in_watch(Chardev *chr, GMainContext *context);
+
 int io_channel_send(QIOChannel *ioc, const void *buf, size_t len);
 
 int io_channel_send_full(QIOChannel *ioc, const void *buf, size_t len,
diff --git a/chardev/char.c b/chardev/char.c
index abd525f..5563375 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);
+        qemu_remove_fd_in_watch(s, context);
     } else {
         fe_open = 1;
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH 5/5] 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 809bad3..4c74b02 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 2/5] colo-compare: kick compare thread to exit while finalize

[Zhang Chen]

On 02/15/2017 04:34 PM, 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.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>   net/colo-compare.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index fdde788..a16e2d5 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;
>   }
> @@ -703,13 +704,11 @@ 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);
> -    }

Before free the 's->conn_list', you should flush all queued primary packets
and release all queued secondary packets here, so combine this patch 
with 3/5 patch as
one patch is a better choose.

Thanks
Zhang Chen

> +
> +    g_queue_free(&s->conn_list);
>   
>       g_free(s->pri_indev);
>       g_free(s->sec_indev);

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH 3/5] colo-compare: release all unhandled packets in finalize function

[Zhang Chen]

On 02/15/2017 04:34 PM, zhanghailiang wrote:
> We should release all unhandled packets before finalize colo compare.
> Besides, we need to free connection_track_table, or there will be
> a memory leak bug.
>
> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
> ---
>   net/colo-compare.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index a16e2d5..809bad3 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -676,6 +676,23 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>       return;
>   }
>   

This function in my patch "colo-compare and filter-rewriter work with 
colo-frame "
Named 'colo_flush_connection', I think use 'flush' instead of 'release' 
is better,

Thanks
Zhang Chen


> +static void colo_release_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);
> @@ -707,9 +724,12 @@ static void colo_compare_finalize(Object *obj)
>       g_main_loop_quit(s->compare_loop);
>       qemu_thread_join(&s->thread);
>   
> +    /* Release all unhandled packets after compare thead exited */
> +    g_queue_foreach(&s->conn_list, colo_release_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);

-- 
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH 3/5] colo-compare: release all unhandled packets in finalize function

[Jason Wang]

On 2017年02月15日 16:34, zhanghailiang wrote:
> We should release all unhandled packets before finalize colo compare.
> Besides, we need to free connection_track_table, or there will be
> a memory leak bug.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>   net/colo-compare.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
>
> diff --git a/net/colo-compare.c b/net/colo-compare.c
> index a16e2d5..809bad3 100644
> --- a/net/colo-compare.c
> +++ b/net/colo-compare.c
> @@ -676,6 +676,23 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>       return;
>   }
>   
> +static void colo_release_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);

Any reason to send packets here?

Thanks

> +        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);
> @@ -707,9 +724,12 @@ static void colo_compare_finalize(Object *obj)
>       g_main_loop_quit(s->compare_loop);
>       qemu_thread_join(&s->thread);
>   
> +    /* Release all unhandled packets after compare thead exited */
> +    g_queue_foreach(&s->conn_list, colo_release_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);

Re: [Qemu-devel] [PATCH 2/5] colo-compare: kick compare thread to exit while finalize

[Jason Wang]

On 2017年02月16日 10:25, Zhang Chen wrote:
>> @@ -703,13 +704,11 @@ 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);
>> -    }
>
> Before free the 's->conn_list', you should flush all queued primary 
> packets
> and release all queued secondary packets here, so combine this patch 
> with 3/5 patch as
> one patch is a better choose.
>
> Thanks
> Zhang Chen 

Yes, agree.

Thanks

Re: [Qemu-devel] [PATCH 3/5] colo-compare: release all unhandled packets in finalize function

[Hailiang Zhang]

On 2017/2/16 10:34, Jason Wang wrote:
>
>
> On 2017年02月15日 16:34, zhanghailiang wrote:
>> We should release all unhandled packets before finalize colo compare.
>> Besides, we need to free connection_track_table, or there will be
>> a memory leak bug.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>    net/colo-compare.c | 20 ++++++++++++++++++++
>>    1 file changed, 20 insertions(+)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index a16e2d5..809bad3 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -676,6 +676,23 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>        return;
>>    }
>>
>> +static void colo_release_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);
>
> Any reason to send packets here?
>

Yes, considering the usage case which we shut COLO for
the VM to make it as a normal VM without FT.
We need to remove all the filter objects. In this case,
IMHO, it is necessary to release the unhandled packets.


Thanks.

> Thanks
>
>> +        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);
>> @@ -707,9 +724,12 @@ static void colo_compare_finalize(Object *obj)
>>        g_main_loop_quit(s->compare_loop);
>>        qemu_thread_join(&s->thread);
>>
>> +    /* Release all unhandled packets after compare thead exited */
>> +    g_queue_foreach(&s->conn_list, colo_release_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);
>
>
> .
>

Re: [Qemu-devel] [PATCH 2/5] colo-compare: kick compare thread to exit while finalize

[Hailiang Zhang]

On 2017/2/16 10:25, Zhang Chen wrote:
>
>
> On 02/15/2017 04:34 PM, 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.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>    net/colo-compare.c | 19 +++++++++----------
>>    1 file changed, 9 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index fdde788..a16e2d5 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;
>>    }
>> @@ -703,13 +704,11 @@ 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);
>> -    }
>
> Before free the 's->conn_list', you should flush all queued primary packets
> and release all queued secondary packets here, so combine this patch
> with 3/5 patch as
> one patch is a better choose.
>

Make sense, will fix it in next version, thanks.

> Thanks
> Zhang Chen
>
>> +
>> +    g_queue_free(&s->conn_list);
>>
>>        g_free(s->pri_indev);
>>        g_free(s->sec_indev);
>

Re: [Qemu-devel] [PATCH 3/5] colo-compare: release all unhandled packets in finalize function

[Jason Wang]

On 2017年02月16日 10:43, Hailiang Zhang wrote:
> On 2017/2/16 10:34, Jason Wang wrote:
>>
>>
>> On 2017年02月15日 16:34, zhanghailiang wrote:
>>> We should release all unhandled packets before finalize colo compare.
>>> Besides, we need to free connection_track_table, or there will be
>>> a memory leak bug.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>    net/colo-compare.c | 20 ++++++++++++++++++++
>>>    1 file changed, 20 insertions(+)
>>>
>>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>>> index a16e2d5..809bad3 100644
>>> --- a/net/colo-compare.c
>>> +++ b/net/colo-compare.c
>>> @@ -676,6 +676,23 @@ static void colo_compare_complete(UserCreatable 
>>> *uc, Error **errp)
>>>        return;
>>>    }
>>>
>>> +static void colo_release_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);
>>
>> Any reason to send packets here?
>>
>
> Yes, considering the usage case which we shut COLO for
> the VM to make it as a normal VM without FT.
> We need to remove all the filter objects. In this case,
> IMHO, it is necessary to release the unhandled packets.
>
>
> Thanks.

Right, I see. All other patches looks good let's squash this into 2.

Thanks

>
>> Thanks
>>
>>> +        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);
>>> @@ -707,9 +724,12 @@ static void colo_compare_finalize(Object *obj)
>>>        g_main_loop_quit(s->compare_loop);
>>>        qemu_thread_join(&s->thread);
>>>
>>> +    /* Release all unhandled packets after compare thead exited */
>>> +    g_queue_foreach(&s->conn_list, colo_release_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);
>>
>>
>> .
>>
>

Re: [Qemu-devel] [PATCH 3/5] colo-compare: release all unhandled packets in finalize function

[Hailiang Zhang]

On 2017/2/16 10:27, Zhang Chen wrote:
>
>
> On 02/15/2017 04:34 PM, zhanghailiang wrote:
>> We should release all unhandled packets before finalize colo compare.
>> Besides, we need to free connection_track_table, or there will be
>> a memory leak bug.
>>
>> Signed-off-by: zhanghailiang<zhang.zhanghailiang@huawei.com>
>> ---
>>    net/colo-compare.c | 20 ++++++++++++++++++++
>>    1 file changed, 20 insertions(+)
>>
>> diff --git a/net/colo-compare.c b/net/colo-compare.c
>> index a16e2d5..809bad3 100644
>> --- a/net/colo-compare.c
>> +++ b/net/colo-compare.c
>> @@ -676,6 +676,23 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
>>        return;
>>    }
>>
>
> This function in my patch "colo-compare and filter-rewriter work with
> colo-frame "
> Named 'colo_flush_connection', I think use 'flush' instead of 'release'
> is better,
>

OK, i will fix it in next version, thanks.

> Thanks
> Zhang Chen
>
>
>> +static void colo_release_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);
>> @@ -707,9 +724,12 @@ static void colo_compare_finalize(Object *obj)
>>        g_main_loop_quit(s->compare_loop);
>>        qemu_thread_join(&s->thread);
>>
>> +    /* Release all unhandled packets after compare thead exited */
>> +    g_queue_foreach(&s->conn_list, colo_release_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);
>