Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Alberto Garcia]

On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng  wrote:

>> > Another advantage for bdrv_aio_poll() is, in main loop we will not
>> > need a separate AioContext in changes like:
>> > 
>> > http://patchwork.ozlabs.org/patch/514968/
>> > 
>> > Because nested aio_poll will automatically be limited to only
>> > process block layer events. My idea is to eventually let main loop
>> > use aio_poll
>> 
>> That would be a step back.  Using GSource is useful because it lets
>> you integrate libraries such as GTK+.
>
> Can we move GTK to a separate GSource thread?

I think that GTK should always run in the main thread, or at least the
one running the default main loop / GMainContext.

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Daniel P. Berrange]

On Fri, Sep 11, 2015 at 11:36:10AM +0200, Alberto Garcia wrote:
> On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng  wrote:
> 
> >> > Another advantage for bdrv_aio_poll() is, in main loop we will not
> >> > need a separate AioContext in changes like:
> >> > 
> >> > http://patchwork.ozlabs.org/patch/514968/
> >> > 
> >> > Because nested aio_poll will automatically be limited to only
> >> > process block layer events. My idea is to eventually let main loop
> >> > use aio_poll
> >> 
> >> That would be a step back.  Using GSource is useful because it lets
> >> you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
> 
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

In theory GTK can run from a separate thread, but my experiance with
GTK and threads is that you end up in a world of hurt if you try to
anything except use the main thread, so I'd really recommend against
it. Even if you do extensive testing locally, things still break across
different GTK versions people have, so its just not worth the pain.

Regards,
Daniel
-- 
|: http://berrange.com  -o-http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org  -o- http://virt-manager.org :|
|: http://autobuild.org   -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org   -o-   http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 11:36, Alberto Garcia wrote:
> On Fri 11 Sep 2015 11:14:33 AM CEST, Fam Zheng  wrote:
> 
> >> > Another advantage for bdrv_aio_poll() is, in main loop we will not
> >> > need a separate AioContext in changes like:
> >> > 
> >> > http://patchwork.ozlabs.org/patch/514968/
> >> > 
> >> > Because nested aio_poll will automatically be limited to only
> >> > process block layer events. My idea is to eventually let main loop
> >> > use aio_poll
> >> 
> >> That would be a step back.  Using GSource is useful because it lets
> >> you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
> 
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

Yeah it's basically GMainContext staying in the main thread and
block/net/chardev I/O put in a new AioContext thread.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 11:44, Fam Zheng wrote:
> > > > That would be a step back.  Using GSource is useful because it lets
> > > > you integrate libraries such as GTK+.
> > >
> > > Can we move GTK to a separate GSource thread?
> > 
> > I think that GTK should always run in the main thread, or at least the
> > one running the default main loop / GMainContext.
> 
> Yeah it's basically GMainContext staying in the main thread and
> block/net/chardev I/O put in a new AioContext thread.

Why?  The point of an event loop is that you can multiplex everything on
the same thread.  Unless we have specific needs (e.g. scalability) one
thread is the way to go and keep things simple.

The tool we have for scalability is AioContext + dataplane threads, and
because we _can_ integrate it into the main thread (AioContext is a
GSource) we can write code once for the main thread and for external
dataplane threads.

Using AioContext instead of duplicating the code is great.  Moving SLIRP
to Win32 would get rid of all the duplicated select() code between
aio-win32.c and main-loop.c.  A lot of main-loop.c code can go away
(probably not all because unlike glib we support nanosecond timeouts
with TimerListGroup, but who knows).

However, there is no need to deviate the event loop from the well-known,
standardized glib architecture.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 11:36, Alberto Garcia wrote:
> > > > Because nested aio_poll will automatically be limited to only
> > > > process block layer events. My idea is to eventually let main loop
> > > > use aio_poll
> > > 
> > > That would be a step back.  Using GSource is useful because it lets
> > > you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
>
> I think that GTK should always run in the main thread, or at least the
> one running the default main loop / GMainContext.

Agreed.  The glib main loop is a positive thing, not a negative one.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 13:02, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 13:01, Fam Zheng wrote:
> > On Fri, 09/11 12:46, Paolo Bonzini wrote:
> >>
> >>
> >> On 11/09/2015 12:40, Fam Zheng wrote:
> >>> On Fri, 09/11 11:54, Paolo Bonzini wrote:
> 
> 
>  On 11/09/2015 11:44, Fam Zheng wrote:
>  That would be a step back.  Using GSource is useful because it lets
>  you integrate libraries such as GTK+.
> >>>
> >>> Can we move GTK to a separate GSource thread?
> >>
> >> I think that GTK should always run in the main thread, or at least the
> >> one running the default main loop / GMainContext.
> >
> > Yeah it's basically GMainContext staying in the main thread and
> > block/net/chardev I/O put in a new AioContext thread.
> 
>  Why?  The point of an event loop is that you can multiplex everything on
>  the same thread.  Unless we have specific needs (e.g. scalability) one
>  thread is the way to go and keep things simple.
> >>>
> >>> The reason is scalability. :)
> >>
> >> Scalability of what?  If virtio-net or virtio-serial needs to be more
> >> scalable, putting all of them into a non-main-loop thread will not make
> >> things more scalable, because you have a single thread anyway.  You'd
> >> need to go BQL-free and allow an arbitrary number.
> >>
> >>> Moving things to AIO isn't deviation, it's more about enabling of 
> >>> dataplane and
> >>> epoll. That's why block was moved to AioContext, and I think we can do 
> >>> similar
> >>> for net and serial, the difference is that as a start, they don't need to 
> >>> be
> >>> fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() 
> >>> loop,
> >>> they can better performance because of epoll.
> >>
> >> Isn't that what your "iohandler.c with AioHandler" already does?  True,
> >> it would be epoll-within-poll, not pure poll.  But if you need epoll,
> >> you might as well go BQL-free.
> > 
> > epoll-within-poll? Do you mean change the main event loop from:
> > 
> > qemu_poll_ns([..., ioeventfd1, ioeventfd2, ..., ioeventfd99], ...)
> > 
> > to
> > 
> > qemu_poll_ns([epollfd], ...)
> > 
> > where epollfd watches all the fds, and let the handler of epollfd do
> > epoll_wait()?
> 
> I mean that glib's main loop (or os_host_main_loop_wait in main-loop.c)
> uses poll, and then the AioContext GSource uses epoll.  Still, you have
> a constant (and low) number of file descriptors in the outer poll.
> 

OK, I think I get your idea, and it sounds good to me, thank you very much! ;-)

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 11:54, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 11:44, Fam Zheng wrote:
> > > > > That would be a step back.  Using GSource is useful because it lets
> > > > > you integrate libraries such as GTK+.
> > > >
> > > > Can we move GTK to a separate GSource thread?
> > > 
> > > I think that GTK should always run in the main thread, or at least the
> > > one running the default main loop / GMainContext.
> > 
> > Yeah it's basically GMainContext staying in the main thread and
> > block/net/chardev I/O put in a new AioContext thread.
> 
> Why?  The point of an event loop is that you can multiplex everything on
> the same thread.  Unless we have specific needs (e.g. scalability) one
> thread is the way to go and keep things simple.

The reason is scalability. :)

> 
> The tool we have for scalability is AioContext + dataplane threads, and
> because we _can_ integrate it into the main thread (AioContext is a
> GSource) we can write code once for the main thread and for external
> dataplane threads.

I don't think integrating with main thread or not is a problem, we can still
use the same code as before, because the event loop code change should be
transparent (see below).

> 
> Using AioContext instead of duplicating the code is great.  Moving SLIRP
> to Win32 would get rid of all the duplicated select() code between
> aio-win32.c and main-loop.c.  A lot of main-loop.c code can go away
> (probably not all because unlike glib we support nanosecond timeouts
> with TimerListGroup, but who knows).
> 
> However, there is no need to deviate the event loop from the well-known,
> standardized glib architecture.

Moving things to AIO isn't deviation, it's more about enabling of dataplane and
epoll. That's why block was moved to AioContext, and I think we can do similar
for net and serial, the difference is that as a start, they don't need to be
fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() loop,
they can better performance because of epoll.

I'm thinking about something like this:

Main thread (GTK):

g_main_run()


New io thread (one per process, includes block, net, serial):

while (true) {
qemu_mutex_lock_iothread();
aio_poll(qemu_aio_context);
qemu_mutex_unlock_iothread();
}


Dataplane thread (one per "-object iothread", includes dataplane enabled
devices):

while (true) {
aio_poll(iothread->ctx);
}

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 13:01, Fam Zheng wrote:
> On Fri, 09/11 12:46, Paolo Bonzini wrote:
>>
>>
>> On 11/09/2015 12:40, Fam Zheng wrote:
>>> On Fri, 09/11 11:54, Paolo Bonzini wrote:


 On 11/09/2015 11:44, Fam Zheng wrote:
 That would be a step back.  Using GSource is useful because it lets
 you integrate libraries such as GTK+.
>>>
>>> Can we move GTK to a separate GSource thread?
>>
>> I think that GTK should always run in the main thread, or at least the
>> one running the default main loop / GMainContext.
>
> Yeah it's basically GMainContext staying in the main thread and
> block/net/chardev I/O put in a new AioContext thread.

 Why?  The point of an event loop is that you can multiplex everything on
 the same thread.  Unless we have specific needs (e.g. scalability) one
 thread is the way to go and keep things simple.
>>>
>>> The reason is scalability. :)
>>
>> Scalability of what?  If virtio-net or virtio-serial needs to be more
>> scalable, putting all of them into a non-main-loop thread will not make
>> things more scalable, because you have a single thread anyway.  You'd
>> need to go BQL-free and allow an arbitrary number.
>>
>>> Moving things to AIO isn't deviation, it's more about enabling of dataplane 
>>> and
>>> epoll. That's why block was moved to AioContext, and I think we can do 
>>> similar
>>> for net and serial, the difference is that as a start, they don't need to be
>>> fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() 
>>> loop,
>>> they can better performance because of epoll.
>>
>> Isn't that what your "iohandler.c with AioHandler" already does?  True,
>> it would be epoll-within-poll, not pure poll.  But if you need epoll,
>> you might as well go BQL-free.
> 
> epoll-within-poll? Do you mean change the main event loop from:
> 
> qemu_poll_ns([..., ioeventfd1, ioeventfd2, ..., ioeventfd99], ...)
> 
> to
> 
> qemu_poll_ns([epollfd], ...)
> 
> where epollfd watches all the fds, and let the handler of epollfd do
> epoll_wait()?

I mean that glib's main loop (or os_host_main_loop_wait in main-loop.c)
uses poll, and then the AioContext GSource uses epoll.  Still, you have
a constant (and low) number of file descriptors in the outer poll.

Paolo

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Fam Zheng]

On Fri, 09/11 12:46, Paolo Bonzini wrote:
> 
> 
> On 11/09/2015 12:40, Fam Zheng wrote:
> > On Fri, 09/11 11:54, Paolo Bonzini wrote:
> >>
> >>
> >> On 11/09/2015 11:44, Fam Zheng wrote:
> >> That would be a step back.  Using GSource is useful because it lets
> >> you integrate libraries such as GTK+.
> >
> > Can we move GTK to a separate GSource thread?
> 
>  I think that GTK should always run in the main thread, or at least the
>  one running the default main loop / GMainContext.
> >>>
> >>> Yeah it's basically GMainContext staying in the main thread and
> >>> block/net/chardev I/O put in a new AioContext thread.
> >>
> >> Why?  The point of an event loop is that you can multiplex everything on
> >> the same thread.  Unless we have specific needs (e.g. scalability) one
> >> thread is the way to go and keep things simple.
> > 
> > The reason is scalability. :)
> 
> Scalability of what?  If virtio-net or virtio-serial needs to be more
> scalable, putting all of them into a non-main-loop thread will not make
> things more scalable, because you have a single thread anyway.  You'd
> need to go BQL-free and allow an arbitrary number.
> 
> > Moving things to AIO isn't deviation, it's more about enabling of dataplane 
> > and
> > epoll. That's why block was moved to AioContext, and I think we can do 
> > similar
> > for net and serial, the difference is that as a start, they don't need to be
> > fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() 
> > loop,
> > they can better performance because of epoll.
> 
> Isn't that what your "iohandler.c with AioHandler" already does?  True,
> it would be epoll-within-poll, not pure poll.  But if you need epoll,
> you might as well go BQL-free.

epoll-within-poll? Do you mean change the main event loop from:

qemu_poll_ns([..., ioeventfd1, ioeventfd2, ..., ioeventfd99], ...)

to

qemu_poll_ns([epollfd], ...)

where epollfd watches all the fds, and let the handler of epollfd do
epoll_wait()?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/11] block: Only poll block layer fds in bdrv_aio_poll

[Paolo Bonzini]

On 11/09/2015 12:40, Fam Zheng wrote:
> On Fri, 09/11 11:54, Paolo Bonzini wrote:
>>
>>
>> On 11/09/2015 11:44, Fam Zheng wrote:
>> That would be a step back.  Using GSource is useful because it lets
>> you integrate libraries such as GTK+.
>
> Can we move GTK to a separate GSource thread?

 I think that GTK should always run in the main thread, or at least the
 one running the default main loop / GMainContext.
>>>
>>> Yeah it's basically GMainContext staying in the main thread and
>>> block/net/chardev I/O put in a new AioContext thread.
>>
>> Why?  The point of an event loop is that you can multiplex everything on
>> the same thread.  Unless we have specific needs (e.g. scalability) one
>> thread is the way to go and keep things simple.
> 
> The reason is scalability. :)

Scalability of what?  If virtio-net or virtio-serial needs to be more
scalable, putting all of them into a non-main-loop thread will not make
things more scalable, because you have a single thread anyway.  You'd
need to go BQL-free and allow an arbitrary number.

> Moving things to AIO isn't deviation, it's more about enabling of dataplane 
> and
> epoll. That's why block was moved to AioContext, and I think we can do similar
> for net and serial, the difference is that as a start, they don't need to be
> fully BQL-free like virtio-blk and scsi. But by running in an aio_poll() loop,
> they can better performance because of epoll.

Isn't that what your "iohandler.c with AioHandler" already does?  True,
it would be epoll-within-poll, not pure poll.  But if you need epoll,
you might as well go BQL-free.

Paolo