Re: [Qemu-devel] [PATCH 0/9] introduce virtio net dataplane

[Michael S. Tsirkin]

On Thu, Feb 21, 2013 at 06:45:54PM +0200, Michael S. Tsirkin wrote:
> On Thu, Feb 21, 2013 at 10:15:58AM -0600, Anthony Liguori wrote:
> > "Michael S. Tsirkin" <address@hidden> writes:
> > 
> > > On Thu, Feb 21, 2013 at 08:54:44PM +0800, Liu Ping Fan wrote:
> > >> This is a emulation to virtio-blk dataplane, which push the data
> > >> handling out of biglock. And it is a try to implement this process
> > >> in userspace, while vhost-net in kernel.
> > >
> > > What's the motivation for doing this?
> > 
> > I haven't looked carefully at the patches, but I don't think we should
> > "cheat" when it comes to virtio-net.  I think it's useful to have a
> > baseline proof of concept but I think we should focus on making the
> > network layer re-entrant.
> > 
> > In terms of why even bother, if we can make virtio-net a viable
> > alternative to vhost-net, it's a huge win.  Being in userspace is a huge
> > advantage.
> > 
> > I understand the challenges with zero-copy but we really need a proper
> > userspace implementation to determine how much it really matters.
> > Zero-copy tap is also not entirely outside the realm of possibility.
> 
> This is exactly what we have, vhost-net is the interface we use
> for asynchronous communication. If you want to add yet another
> asynchronous interface in kernel, it's certainly doable but then what's
> the point?
> 
> > >> The iperf's result show it improving the perfermance of base line,
> > >> but still slower than vhost-net (maybe the rx path need optimized).
> > >> 
> > >> Todo:
> > >> implement fine lock for net-subsystem
> > >
> > > vhost-net is currently the only way to get zero copy transmit
> > > on linux (and zero copy receive in the future)
> > > so at least in theory it'll always be faster.
> > 
> > It might always be faster.  But that doesn't mean we should limit the
> > performance of virtio-net in userspace.  Some people may be willing to
> > take a small performance hit in order to obtain the security offered by
> > being in userspace.
> > 
> > Regards,
> > 
> > Anthony Liguori
> 
> By 'being in userspace' I presume you mean the ring processing code.
> Ring processing can be done in userspace if you are ready
> to use the synchronous read/write operations, skipping
> this bunch of code might improve security. But it requires a data
> copy almost by definition.
> 

BTW I think we do have a problem at the moment,
and that is that virtio net can process the same ring both in userspace
(for level interrupts) and in kernel (for edge interrupts).
And which one is selected is under guest control.
This is twice the number of potential security issues.
A solution, in my opinion, is to improve handling of level
interrupts to the point where we can make vhost-net
the default for level as well.
The new support for level IRQFD in kvm should make
this possible.

Unfortunately, this prototype moves in exactly the reverse
direction.

> > >
> > >> Liu Ping Fan (9):
> > >>   vring: split the modification and publish of used ring
> > >>   vring: introduce vring_restore() to restore from img
> > >>   event poll: make epoll work for normal fd
> > >>   event poll: pass event type to event callback
> > >>   event poll: enable event poll handle more than one event each time
> > >>   virtio net: introduce dataplane for virtio net
> > >>   tap: make tap peer work on dedicated data-plane thread
> > >>   virtio net: enable dataplane for virtio net
> > >>   configure: make virtio net dataplane configurable
> > >> 
> > >>  configure                  |   12 ++
> > >>  hw/dataplane/Makefile.objs |    4 +-
> > >>  hw/dataplane/event-poll.c  |   69 +++++--
> > >>  hw/dataplane/event-poll.h  |   14 ++-
> > >>  hw/dataplane/virtio-blk.c  |    8 +-
> > >>  hw/dataplane/virtio-net.c  |  444 
> > >> ++++++++++++++++++++++++++++++++++++++++++++
> > >>  hw/dataplane/virtio-net.h  |   32 ++++
> > >>  hw/dataplane/vring.c       |   37 ++++
> > >>  hw/dataplane/vring.h       |    3 +
> > >>  hw/virtio-net.c            |   94 +++++-----
> > >>  hw/virtio-net.h            |   64 +++++++
> > >>  hw/virtio-pci.c            |    2 +-
> > >>  include/net/tap.h          |    6 +
> > >>  net/tap.c                  |   92 +++++++++-
> > >>  14 files changed, 806 insertions(+), 75 deletions(-)
> > >>  create mode 100644 hw/dataplane/virtio-net.c
> > >>  create mode 100644 hw/dataplane/virtio-net.h
> > >> 
> > >> -- 
> > >> 1.7.4.4
> > >>