From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51250) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPLks-0002tE-8Q for qemu-devel@nongnu.org; Mon, 18 Jul 2016 23:31:35 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bPLko-0002Id-Pi for qemu-devel@nongnu.org; Mon, 18 Jul 2016 23:31:34 -0400 Received: from [59.151.112.132] (port=55492 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bPLkn-0002GP-Us for qemu-devel@nongnu.org; Mon, 18 Jul 2016 23:31:30 -0400 References: <1468827650-17234-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <1468827650-17234-7-git-send-email-zhangchen.fnst@cn.fujitsu.com> <20160718083702.GI1670@redhat.com> From: Zhang Chen Message-ID: <578D9F59.802@cn.fujitsu.com> Date: Tue, 19 Jul 2016 11:32:41 +0800 MIME-Version: 1.0 In-Reply-To: <20160718083702.GI1670@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RFC PATCH V7 6/7] colo-compare: introduce packet comparison thread List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: qemu devel , Jason Wang , Li Zhijian , "eddie . dong" , "Dr . David Alan Gilbert" , zhanghailiang 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 >> Signed-off-by: Li Zhijian >> Signed-off-by: Wen Congyang >> --- >> 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