Re: [Qemu-devel] [RFC PATCH V4 0/4] Introduce COLO-compare

[Zhang Chen]

On 05/31/2016 07:06 PM, Jason Wang wrote:
> On 2016年05月31日 16:28, Zhang Chen wrote:
> > On 05/31/2016 02:16 PM, Jason Wang wrote:
> > > On 2016年05月31日 11:54, Zhang Chen wrote:
> > > > On 05/30/2016 11:19 AM, Jason Wang wrote:
> > > > > On 2016年05月25日 20:50, Zhang Chen wrote:
> > > > > > COLO-compare is a part of COLO project. It is used
> > > > > > to compare the network package to help COLO decide
> > > > > > whether to do checkpoint.
> > > > > >
> > > > > > the full version in this github:
> > > > > > https://github.com/zhangckid/qemu/tree/colo-v2.7-proxy-mode-compare-with-colo-base-may25 
> > > > > >
> > > > > >
> > > > > >
> > > > > > v4:
> > > > > >   p4:
> > > > > >      - add some comments
> > > > > >      - fix some trace-events
> > > > > >      - fix tcp compare error
> > > > > >   p3:
> > > > > >      - add rcu_read_lock().
> > > > > >      - fix trace name
> > > > > >      - fix jason's other comments
> > > > > >      - rebase some Dave's branch function
> > > > > >   p2:
> > > > > >      - colo_compare_connection() change g_queue_push_head() to
> > > > > >      - g_queue_push_tail() match to sorted order.
> > > > > >      - remove QemuMutex conn_list_lock
> > > > >
> > > > > Looks like conn_list lock is still there. I still prefer to do all 
> > > > > thing in the comparing thread. Have you tried Fam's suggestion to 
> > > > > use g_main_context_push_thread_default()? If it does not work, 
> > > > > does it work simply by replacing all:
> > > > >
> > > > > g_source_attach(x, NULL);
> > > > >
> > > > > with
> > > > >
> > > > > g_souce_attach(x, g_main_context_get_thread_default());
> > > > >
> > > > > after call g_main_context_push_thread_default()?
> > > > >
> > > > > Thanks
> > > >
> > > > I have tried fam's suggestion it does not work.
> > > >
> > > > so I tried what you suggestion like that:
> > > >
> > > > static void *colo_compare_thread(void *opaque)
> > > > {
> > > >     CompareState *s = opaque;
> > > >     GSource *source;
> > > >     GMainContext *worker_context;
> > > >
> > > >     source = g_source_new(&source_funcs, sizeof(DemoSource));
> > > >     worker_context = g_main_context_new ();
> > > >
> > > >     g_source_attach(source, g_main_context_get_thread_default());
> > > >     g_source_set_callback(source, NULL, NULL, NULL);
> > > >     g_main_context_push_thread_default (worker_context);
> > +    g_source_unref(source);
> > > > 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);
> > > >     g_main_context_pop_thread_default (worker_context);
> > >
> > > g_source_attach() should be done after 
> > > g_main_context_push_thread_default()
> >
> > Like that? (+    g_source_unref(source);)
> > But compare_pri_chr_in always run in main loop.
> >
> > Thanks
> > Zhang Chen
>
> Not very sure just from the above codes, since I don't see any 
> connection between source and compare_pri_chr_in. The point is to 
> attach the source of compare_pri_chr_in to thread default context 
> instead of global default context I think.

Hi~ jason.

I tried a lot of ways to make it run in compare thread, but it not work.

Because that:
void g_main_context_push_thread_default (GMainContext *context);
Acquires context and sets it as the thread-default context for the 
current thread.

So, this function g_main_context_push_thread_default() just can set 
thread-default context,
not the global default context. and I can't find a function to set 
global default context in glib.
The qemu's QIOChannel uses g_source_attach(source, NULL); to attach GSource.
g_source_attach (GSource *source,
                 GMainContext *context);

context
a GMainContext (if NULL, the default context will be used).

But he not say "the default context" is global default context or 
thread-default context.
So I read glib codes find that:
guint
g_source_attach (GSource *source,
                 GMainContext *context)
{
.....
if (!context)
    context = g_main_context_default ();
.....
}

g_main_context_default ()
Returns the global default main context.

So...I think we can't make qemu_chr_add_handlers() run in colo thread in 
this way.
Do you have any comments about that?



> > > > but it does not work too. Do you mean like this?
> > >
> > > Not exactly the same, but should be worth to try also. My suggestion 
> > > is to replace all g_source_attach(src, NULL) in qemu source 
> > > (especially the ones in qemu-char.c) with g_source_attach(src, 
> > > g_main_context_get_thread_default()). (Maybe not that easy, but 
> > > better to have a try).
> > >
> > > You may refer 
> > > https://developer.gnome.org/glib/unstable/glib-The-Main-Event-Loop.html 
> > > for more docs on this. I believe there should have some solutions on 
> > > this.
> > >
> > > > Thanks
> > > > Zhang Chen
> > > >
> > > > > >      - remove pkt->s
> > > > > >      - move data structure to colo-base.h
> > > > > >      - add colo-base.c reuse codes for filter-rewriter
> > > > > >      - add some filter-rewriter needs struct
> > > > > >      - depends on previous SocketReadState patch
> > > > > >   p1:
> > > > > >      - except move qemu_chr_add_handlers()
> > > > > >        to colo thread
> > > > > >      - remove class_finalize
> > > > > >      - remove secondary arp codes
> > > > > >      - depends on previous SocketReadState patch
> > > > > >
> > > > > > v3:
> > > > > >    - rebase colo-compare to colo-frame v2.7
> > > > > >    - fix most of Dave's comments
> > > > > >      (except RCU)
> > > > > >    - add TCP,UDP,ICMP and other packet comparison
> > > > > >    - add trace-event
> > > > > >    - add some comments
> > > > > >    - other bug fix
> > > > > >    - add RFC index
> > > > > >    - add usage in patch 1/4
> > > > > >
> > > > > > v2:
> > > > > >    - add jhash.h
> > > > > >
> > > > > > v1:
> > > > > >    - initial patch
> > > > > >
> > > > > >
> > > > > > Zhang Chen (4):
> > > > > >    colo-compare: introduce colo compare initialization
> > > > > >    colo-compare: track connection and enqueue packet
> > > > > >    colo-compare: introduce packet comparison thread
> > > > > >    colo-compare: add TCP,UDP,ICMP packet comparison
> > > > > >
> > > > > >   include/qemu/jhash.h |  61 +++++
> > > > > >   net/Makefile.objs    |   2 +
> > > > > >   net/colo-base.c      | 183 +++++++++++++
> > > > > >   net/colo-base.h      |  92 +++++++
> > > > > >   net/colo-compare.c   | 745 
> > > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >   qemu-options.hx      |  34 +++
> > > > > >   trace-events         |  11 +
> > > > > >   vl.c                 |   3 +-
> > > > > >   8 files changed, 1130 insertions(+), 1 deletion(-)
> > > > > >   create mode 100644 include/qemu/jhash.h
> > > > > >   create mode 100644 net/colo-base.c
> > > > > >   create mode 100644 net/colo-base.h
> > > > > >   create mode 100644 net/colo-compare.c
> > > > >
> > > > >
> > > > >
> > > > > .
> > >
> > >
> > >
> > > .
>
>
>
> .

--
Thanks
zhangchen