qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH V7 6/7] colo-compare: introduce packet compa


From: Zhang Chen
Subject: Re: [Qemu-devel] [RFC PATCH V7 6/7] colo-compare: introduce packet comparison thread
Date: Tue, 19 Jul 2016 11:32:41 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0



On 07/18/2016 04:37 PM, Daniel P. Berrange wrote:
On Mon, Jul 18, 2016 at 03:40:49PM +0800, Zhang Chen wrote:
If primary packet is same with secondary packet,
we will send primary packet and drop secondary
packet, otherwise notify COLO frame to do checkpoint.
If primary packet comes and secondary packet not,
after REGULAR_PACKET_CHECK_MS milliseconds we set
the primary packet as old_packet,then do a checkpoint.

Signed-off-by: Zhang Chen <address@hidden>
Signed-off-by: Li Zhijian <address@hidden>
Signed-off-by: Wen Congyang <address@hidden>
---
  net/colo-base.c    |   1 +
  net/colo-base.h    |   3 +
  net/colo-compare.c | 216 +++++++++++++++++++++++++++++++++++++++++++++++++++++
  trace-events       |   2 +
  4 files changed, 222 insertions(+)

+static void *colo_compare_thread(void *opaque)
+{
+    GMainContext *worker_context;
+    GMainLoop *compare_loop;
+    CompareState *s = opaque;
+
+    worker_context = g_main_context_new();
+    g_assert(g_main_context_get_thread_default() == NULL);
+    g_main_context_push_thread_default(worker_context);
+    g_assert(g_main_context_get_thread_default() == worker_context);
+
+    qemu_chr_add_handlers(s->chr_pri_in, compare_chr_can_read,
+                          compare_pri_chr_in, NULL, s);
+    qemu_chr_add_handlers(s->chr_sec_in, compare_chr_can_read,
+                          compare_sec_chr_in, NULL, s);
Looking at this I see you've used g_main_context_push_thread_default
in order to avoid having to modify the qemu_chr_add_handlers() method
to accept a GMainContext. Personally I think I'd favour explicit
passing of a GMainContext, rather than modifying Qemu Char code &
QIOChannel to magically use per-thread main context.  In particular
I'm concerned that one day other code may use the
g_main_context_push_thread_default method for some other purpose
but still want their char devs handled by the main thread. Your
solution makes this impossible and adds magic which could surprise
us in bad ways.

So I think I'd rather see qemu_chr_add_handlers() modified to accept
a GMainContext, or perhaps add a qemu_chr_add_handlers_full() method
which takes a GMainContext, so we can avoid modifying all existing
callers ofo qemu_chr_add_handlers.  There should be no need to modify
QIOChannel at all, since you can use qio_channel_create_watch instead
of qio_channel_add_watch and simply attach to the context you want
directly.

I get your point.
Considering current qemu code, I think add a flag
in CharDriverState to decide whether or not to
run g_source_attach(xxx, g_main_context_get_thread_default());
is a easy way for this.
How about this way?

Thanks for your review.
Zhang Chen


Regards,
Daniel

--
Thanks
zhangchen






reply via email to

[Prev in Thread] Current Thread [Next in Thread]