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

[Zhang Chen <zhangchen.fnst@cn.fujitsu.com>]

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 <zhangchen.fnst@cn.fujitsu.com>
>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> ---
>>   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