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

["Daniel P. Berrange" <berrange@redhat.com>]

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.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|