Re: [Qemu-devel] [RFC PATCH] main-loop: Unconditionally unlock iothread

[Anthony Liguori <aliguori@us.ibm.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 04/04/2013 18:59, Anthony Liguori ha scritto:
>>> >
>>> > The right thing to use would be g_source_add_child_source() and
>>> > g_source_remove_child_source(), but that is only present since glib 2.28
>>> > and we currently require 2.12 (2.20 on Windows).
>> I don't think a child source fixes the problem.  The backend definitely
>> has work to do.  What we don't know is whether the front end is capable
>> of processing the work.
>> 
>> The problem here is that we use polling on the front-end.  IOW:
>> 
>> 1) Char backend has data and is ready to write.
>> 2) Asks front end if it can write
>> 3) Front end says no
>> 4) Goto (1)
>
> What we used to do is this, however:
>
> 1) Char backend asks front end if it can write
> 2) Front end says no

You're missing a step here.  Previously we would drop the data silently.

> 3) poll() goes on without char backend's descriptor

Which is what enabled this.

> 4) Goto (1), then:
> 5) Front end is now available, calls qemu_notify_event()

This still exists FWIW.

> 6) Char backend asks front end if it can write
> 7) Front end says yes
>
> No busy waiting here.  Though we don't really do (5) everywhere, because
> for example this patch slipped through the cracks:

Ack.

> http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg03208.html (but
> for the monitor and every consumer running in iothread context it's ok).
>
> You could achieve the same thing by making the io watch a child source
> of the QEMU wrapper.  All the wrapper does is add/remove the io watch
> source from its children, depending on can_read.

I don't understand how this solves the problem.  There aren't two
sources.  There is only a single source (the GIOChannel source).
We override the function pointers to basically do monkey patching of the
GSource.

If I understand your suggestion, it's to add the source as a child of
itself.

>
> Perhaps it's possible without child sources though, by setting the
> callbacks on the glib source directly.

That's what we're doing...

    src = g_io_create_watch(channel, G_IO_IN | G_IO_ERR | G_IO_HUP);
    g_source_set_funcs(src, &io_watch_poll_funcs);
    g_source_set_callback(src, (GSourceFunc)fd_read, user_data, NULL);

There is only one GSource active here.

Regards,

Anthony Liguori

> Paolo