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

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

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