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

[Daniel P. Berrange]

On Tue, Jul 19, 2016 at 11:32:41AM +0800, Zhang Chen wrote:
> 
> 
> 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?

I really prefer to see the GMainContext passed in with the API(s)
which register callbacks, not provided via the backdoor via the
glib thread default global state, or via chardev global state.

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 :|