Re: [Qemu-devel] [RFC PATCH 0/2] port network layer onto glib

[Anthony Liguori]

Paolo Bonzini <address@hidden> writes:

> Il 13/03/2013 13:34, Anthony Liguori ha scritto:
>> Paolo Bonzini <address@hidden> writes:
>> 
>>> Il 13/03/2013 06:59, Liu Ping Fan ha scritto:
>>>> These series aim to port network backend onto glib, and
>>>> prepare for moving towards making network layer mutlit-thread.
>>>> The brief of the whole aim and plan is documented on 
>>>> http://wiki.qemu.org/Features/network_reentrant
>>>>
>>>> In these series, attach each NetClientState with a GSource
>>>> At the first, I use AioContext instead of GSource, but after discussion,
>>>> I think with GSource, we can integrated with glib more closely.
>>>
>>> Integrating with glib by itself is pointless.  What is the *benefit*?
>>>
>>> We have a pretty good idea of how to make multithreaded device models
>>> using AioContext, since we are using it for the block layer and
>>> virtio-blk dataplane.  Doing the same work twice, on two different
>>> frameworks, doesn't seem like a very good idea.
>> 
>> Hrm, I had thought on previous threads there was clear agreement that we
>> did not want to use AioContext outside of the block layer.
>> 
>> I think we certainly all agree that moving to a thread aware event loop
>> is a necessary step toward multi-threading.  I think the only question
>> is whether to use AioContext or glib.
>> 
>> AioContext is necessary for the block layer because the block layer
>> still has synchronous I/O.  I think we should aim to replace all sync
>> I/O in the long term with coroutine based I/O.  That lets us eliminate
>> AioContextes entirely which is nice as the semantics are subtle.
>> 
>> I think that's a solid argument for glib over AioContext.  The former is
>> well understood, documented, and makes unit testing easier.
>
> I don't see anything particularly subtle in AioContext, except
> qemu_bh_schedule_idle and the flush callback.  The flush callback only
> has a naming problem really, it is a relic of qemu_aio_flush().
> qemu_bh_schedule_idle could disappear if we converted the floppy disk
> drive to AIO; patches existed for that but then the poster
> disappeared.

I think the nesting is also a bit strange.

When an nesting occurs, only the bottom halves and fd events that were
registered in the current depth or greater are invoked.  This is very
different from how nesting works with glib and any other event loop
implementation I've worked with.

Normally, all events are executed when nesting.  I think our behavior
creates a very subtle and fragile code that tries to make nesting not a
point of re-entrancy.  I'm not entirely convinced that we've even gotten
this right historically.

This is my biggest concern with AioContext and using it more throughout
QEMU.

> glib's main loop has its share of subtleness (GMainLoop vs.
> GMainContext, anyone?),

Ack.  I understand where it comes from but it's silly.

> and AioContext's code is vastly simpler than GMainLoop's.

For now.

> AioContext is also documented and unit tested, with tests
> for both standalone and GSource operation.  Unit tests for AioContext
> users are trivial to write, we have one in test-thread-pool.
>
>> Did you have a specific concern with using glib vs. AioContext?  Is it
>> about reusing code in the block layer where AioContext is required?
>
> In the short term yes, code duplication is a concern.  We already have
> two implementation of virtio.

I share your concern but in the opposite direction.  We have three main
loops today.

> I would like the dataplane virtio code to
> grow everything else that needs to be in all dataplane-style devices
> (for example, things such as setting up the guest<->host notifiers), and
> the hw/virtio.c API implemented on top of it (or dead altogether).
> Usage of AioContext is pretty much forced by the block layer.

I don't think that AioContext is the right answer because it makes it
too easy to shoot yourself in the foot.

> However, I'm more worried by overhead.  GMainLoop is great because
> everybody plugs into it.  It enabled the GTK+ front-end, it let us reuse
> GIOChannel for chardev flow control, and one can similarly think of
> integrating Avahi for example.  However, I think it's mostly useful for
> simple-minded non-performance-critical code.  QEMU has worked great in
> almost all scenarios with only one non-VCPU thread, and if we are going
> to move stuff to other threads we should only do that because we want
> performance and control.  I'm not at all confident that GMainLoop can
> provide them.

I guess my view on this is that GMainLoop is a well structure and sane
implementation.  Its got very few entry points and sane hooks.  If we
decide we need epoll or something like that, it would be very easy to
make a drop-in replacement for it.

AioContext will encourage bad habits IMHO.  I don't think it's the right
base for !block layer and I think we should aim to eliminate it within
the block layer over time too.

Regards,

Anthony Liguori

> On the contrary, AioContext really does two things (g_poll and bottom
> halves) and does them fast.  For really high-performance scenarios, such
> as the ones virtio-blk-dataplane was written for, I'd be surprised if
> glib's main loop had the same performance as AioContext.  Also,
> AioContext could be easily converted to use epoll, while we don't have
> the same level of control on glib's main loop.
>
> Of course I will easily change my mind if I see patches that show the
> contrary. :)
>
> Paolo