Re: [Qemu-devel] [RFC PATCH V2] qemu-char: Fix context for g_source_attach()

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

Hi~ All~~

Can you give me some feedback for this patch?

We need more comments~~

COLO project depend on this patch to work.

Because this patch colo-compare can make handler of qemu_chr_add_handlers()

run in compare thread, reduce workload of main_loop in network busy 
situation.

This idea from Jason.


Thanks
Zhang Chen




On 07/11/2016 10:12 AM, Zhang Chen wrote:
>
>
> On 07/08/2016 10:27 PM, Paolo Bonzini wrote:
>>
>> On 08/07/2016 10:54, Daniel P. Berrange wrote:
>>> On Fri, Jul 08, 2016 at 09:48:23AM +0800, Fam Zheng wrote:
>>>> On Wed, 06/22 18:49, Zhang Chen wrote:
>>>>> We want to poll and handle chardev in another thread
>>>>> other than main loop. But qemu_chr_add_handlers() can only
>>>>> work for global default context other than thread default context.
>>>>> So we use g_source_attach(xx, g_main_context_get_thread_default())
>>>>> replace g_source_attach(xx, NULL) to attach g_source.
>>>>> Comments from jason.
>>>>>
>>>>> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>
>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> ---
>>>>>   io/channel.c | 2 +-
>>>>>   qemu-char.c  | 6 +++---
>>>>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/io/channel.c b/io/channel.c
>>>>> index 692eb17..cd25677 100644
>>>>> --- a/io/channel.c
>>>>> +++ b/io/channel.c
>>>>> @@ -146,7 +146,7 @@ guint qio_channel_add_watch(QIOChannel *ioc,
>>>>>         g_source_set_callback(source, (GSourceFunc)func, 
>>>>> user_data, notify);
>>>>>   -    id = g_source_attach(source, NULL);
>>>>> +    id = g_source_attach(source, 
>>>>> g_main_context_get_thread_default());
>>>>>       g_source_unref(source);
>>>>>         return id;
>>>>> diff --git a/qemu-char.c b/qemu-char.c
>>>>> index 84f49ac..4340457 100644
>>>>> --- a/qemu-char.c
>>>>> +++ b/qemu-char.c
>>>>> @@ -859,7 +859,7 @@ static gboolean io_watch_poll_prepare(GSource 
>>>>> *source, gint *timeout_)
>>>>>           iwp->src = qio_channel_create_watch(
>>>>>               iwp->ioc, G_IO_IN | G_IO_ERR | G_IO_HUP | G_IO_NVAL);
>>>>>           g_source_set_callback(iwp->src, iwp->fd_read, 
>>>>> iwp->opaque, NULL);
>>>>> -        g_source_attach(iwp->src, NULL);
>>>>> +        g_source_attach(iwp->src, 
>>>>> g_main_context_get_thread_default());
>>>>>       } else {
>>>>>           g_source_destroy(iwp->src);
>>>>>           g_source_unref(iwp->src);
>>>>> @@ -918,7 +918,7 @@ static guint io_add_watch_poll(QIOChannel *ioc,
>>>>>       iwp->fd_read = (GSourceFunc) fd_read;
>>>>>       iwp->src = NULL;
>>>>>   -    tag = g_source_attach(&iwp->parent, NULL);
>>>>> +    tag = g_source_attach(&iwp->parent, 
>>>>> g_main_context_get_thread_default());
>>>>>       g_source_unref(&iwp->parent);
>>>>>       return tag;
>>>>>   }
>>>>> @@ -3982,7 +3982,7 @@ int qemu_chr_fe_add_watch(CharDriverState 
>>>>> *s, GIOCondition cond,
>>>>>       }
>>>>>         g_source_set_callback(src, (GSourceFunc)func, user_data, 
>>>>> NULL);
>>>>> -    tag = g_source_attach(src, NULL);
>>>>> +    tag = g_source_attach(src, g_main_context_get_thread_default());
>>>>>       g_source_unref(src);
>>>>>         return tag;
>>>>> -- 
>>>> IIRC this opens a gate for your special thread (COLO compare 
>>>> thread?) to use
>>>> QIOChannel.
>>> I've no real objection to this proposed patch, though it is fairly 
>>> pointless
>>> to take it now without seeing any following patch that actually 
>>> makes use
>>> of this added feature.
>> I agree.
>
> Should I move this patch to the "[RFC PATCH V5 0/4] Introduce 
> COLO-compare"
> patch set? that can show how it works.
>
> you can see this patch for how to use:
> http://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg06754.html
>
>

In colo_compare_thread()


>>
>>>> I think in the long run it is better to think about allowing 
>>>> integrating QIO to
>>>> AioContext, to support its usage outside main loop.  Given how 
>>>> opaque GSource
>>>> is, I'm not sure how feasible that is, or how useful it will be.  
>>>> Anyway we
>>>> should definitely hear more opinions from Daniel and Paolo.
>>> Personally I think it is preferable to stick as close to the 
>>> standard GSource
>>> model as possible, as that's widely used & well understood API, 
>>> compared to the
>>> QEMU specific AioContext.
>> AioContext is more optimized for the case where the callbacks are
>> static.  In general this is not the case for qemu-char.c.
>
> I don't sure AioContext can do this job good, but I think
> we can make qemu more flexible to do same one job.
> All roads lead to Rome.
>
> Thanks
> Zhang Chen
>
>
>> Paolo
>>
>>
>> .
>>
>

-- 
Thanks
zhangchen