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

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

On 06/22/2016 02:48 PM, Jason Wang wrote:
>
>
> On 2016年06月22日 14:24, Zhang Chen wrote:
>>
>>
>> On 06/20/2016 01:24 PM, Jason Wang wrote:
>>>
>>>
>>> On 2016年06月20日 11:27, Zhang Chen wrote:
>>>>
>>>>
>>>> On 06/20/2016 11:03 AM, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 2016年06月17日 10:25, Zhang Chen wrote:
>>>>>> 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? 
>>>>>
>>>>> How about (like I suggest previously) just use
>>>>>
>>>>> g_souce_attach(x, g_main_context_get_thread_default());
>>>>>
>>>>> in qemu's QIOChannel code?
>>>>>
>>>>>
>>>>>
>>>>
>>>> I feel it seems can work, and will try it.
>>>> but I don't know how this change in qemu's QIOChannel code
>>>> will affect the other code. needs other people confirm it?
>>>
>>> Yes, we need convince them. I believe we don't do this in the past 
>>> is because there's no users.
>>>
>>>> If no affect and works good,I will send a independent patch
>>>> for this.
>>>>
>>>
>>> Right, if it works, please send a RFC and explain why it is needed.
>>>
>>
>> Hi~ Jason.
>> It work! I have send a patch for this and signed-off-by you:
>>
>> [RFC PATCH] Change g_source_attach(xx, NULL) to g_souce_attach(xx, 
>> g_main_context_get_thread_default())
>>
>> But I find a problem when fix colo-compare.
>> if we want poll and handle chardev in compare thread,
>> we must use g_main_loop_run(compare_loop). and this function
>> block here, we can not run other job in compare thread...
>> like that:
>>
>> static void *colo_compare_thread(void *opaque)
>> {
>> .......
>>
>>     worker_context = g_main_context_new();
>>     g_main_context_push_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);
>>
>>     compare_loop = g_main_loop_new(worker_context, FALSE);
>>
>>     g_main_loop_run(compare_loop);
>>
>>     //will block here...
>>
>>     while (s->thread_status == COMPARE_THREAD_RUNNING) {
>>         qemu_event_wait(&s->event);
>>         qemu_event_reset(&s->event);
>>         rcu_read_lock();
>>         g_queue_foreach(&s->conn_list, colo_compare_connection, s);
>>         rcu_read_unlock();
>>     }
>>
>> .....
>>
>>
>> Have any comments?
>
> Why not just trigger the comparing in compare_{pri|sec}_chr_in() ?

Make sence!

Thanks
Zhang Chen

>
>>
>>
>> Thanks
>> Zhang Chen
>>
>>
>>> Thanks
>>>
>>>>
>>>> Thanks
>>>> Zhang Chen
>>>
>>>
>>>
>>> .
>>>
>>
>
>
>
> .
>

-- 
Thanks
zhangchen