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

[mdroth <mdroth@linux.vnet.ibm.com>]

On Thu, Mar 28, 2013 at 03:55:52PM +0100, Stefan Hajnoczi wrote:
> On Thu, Mar 28, 2013 at 03:55:51PM +0800, Liu Ping Fan wrote:
> > From: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> > 
> > These series aim to make the whole network re-entrant, here only apply backend and frontend,
> > and for the netcore, separated patches have been sent out.  All of these will prepare us for
> > moving towards making network layer mutlit-thread.
> > Finally it would be omething like
> >    qemu -object io-thread,id=thread0 \
> >      -device virtio-net,rx[0]=thread0,tx[0]=thread0
> > 
> > The brief of the whole aim and plan is documented on
> >   http://wiki.qemu.org/Features/network_reentrant
> > 
> > The main issue is about GSource or AioContext,
> >   http://marc.info/?t=136315453300002&r=1&w=3
> > And I sumary the main points:
> >   disadvantage for current AioContext
> >    1st. need to define and expand interface for other fd events, while glib open this interface for user *
> >    2nd. need to add support for IOCanReadHandler, while gsource provide prepare, check method to allow more flexible control
> >    3rd. block layer's AioContext will block other AioContexts on the same thread.
> >    4th. need more document
> >  disadvantage for glib
> >    1st. if more than one fds on the same GSource, need re-implement something like aio_set_file_handler
> > 
> > Since I have successed to port frontend on glib, there is no obstale to use glib.
> > 
> > 
> > v1->v2:
> >   1.NetClientState can associate with up to 2 GSource, for virtio net, one for tx, one for rx, 
> >     so vq can run on different threads.
> >   2.make network front-end onto glib, currently virtio net dataplane
> > 
> > 
> > Liu Ping Fan (4):
> >   net: port tap onto glib
> >   net: resolve race of tap backend and its peer
> >   net: port hub onto glib
> >   net: port virtio net onto glib
> > 
> >  hw/qdev-properties-system.c |    1 +
> >  hw/virtio-net.c             |  165 +++++++++++++++++++++++++++++++++++++++++++
> >  hw/virtio.c                 |    6 ++
> >  hw/virtio.h                 |    2 +
> >  include/net/net.h           |   27 +++++++
> >  include/net/queue.h         |   14 ++++
> >  net/hub.c                   |   34 ++++++++-
> >  net/net.c                   |   97 +++++++++++++++++++++++++
> >  net/queue.c                 |    4 +-
> >  net/tap.c                   |   62 +++++++++++++---
> >  10 files changed, 397 insertions(+), 15 deletions(-)
> 
> It seems the AioContext vs glib issue hasn't been settled yet.  My take
> is that glib is preferrable *if* we don't need to write too many
> helpers/wrappers on top (then we're basically back to rolling our own,
> AioContext).
> 
> I was surprised by the amount of code required to listen on a file
> descriptor.  Are you sure there isn't a glib way of doing this that
> avoids rolling our own GSource?

GIOChannel provides a pre-baked GSource you can pass around, but this
might add more confusion to the mix when you consider things like
Slirp which will likely require a custom GSource. It also assumes a
different signature than GSourceFunc for the callback which further adds
to the confusion.

Keeping the interfaces centered around normal GSources I think would help
to avoid the proliferation of more event-handling registration
mechanisms in the future, and make conversions easier if we do need to
change things.

A generic GSource for handling FDs that we can re-use for basic use
cases would help curtail some of the boilerplate later for common fd
handlers. Probably doesn't make sense to generalize until we reach a
decision on glib vs. aiocontext though.

> 
> In the next series, please drop the hub re-entrancy stuff and virtio-net
> data plane.  Instead just focus on systematically moving existing net
> clients onto the event loop (net/*.c and NICs).  The only controversial
> issue there is AioContext vs glib, and once that's settled we can merge
> the patches.
> 
> Please avoid layering violations - for example a comment about
> virtio-net in net.h, a comment about vhost in tap, or putting
> net_source_funcs in net.h.  I think converting all existing net clients
> will help make the code changes appropriate and eliminate these kinds of
> hacks which are because you're focussing just on virtio, tap, and hub
> here.
> 
> Stefan
>