[PATCH 2/2] vhost_net: a kernel-level virtio server

Previous thread: [PATCH 1/2] export cpu_tlbstate to modules by Michael S. Tsirkin on Monday, August 10, 2009 - 11:53 am. (5 messages)

Next thread: module loading permissions and request_module permission inconsistencies by Eric Paris on Monday, August 10, 2009 - 12:45 pm. (6 messages)
From: Michael S. Tsirkin
Date: Monday, August 10, 2009 - 11:53 am

What it is: vhost net is a character device that can be used to reduce
the number of system calls involved in virtio networking.
Existing virtio net code is used in the guest without modification.

There's similarity with vringfd, with some differences and reduced scope
- uses eventfd for signalling
- structures can be moved around in memory at any time (good for migration)
- support memory table and not just an offset (needed for kvm)

common virtio related code has been put in a separate file vhost.c and
can be made into a separate module if/when more backend appear.  I used
Rusty's lguest.c as the source for developing this part : this supplied
me with witty comments I wouldn't be able to write myself.

What it is not: vhost net is not a bus, and not a generic new system
call. No assumptions are made on how guest performs hypercalls.
Userspace hypervisors are supported as well as kvm.

How it works: Basically, we connect virtio frontend (configured by
userspace) to a backend. The backend could be a network device, or a
tun-like device. In this version I only support raw socket as a backend,
which can be bound to e.g. SR IOV, or to macvlan device.  Backend is
also configured by userspace, including vlan/mac etc.

Status:
This works for me, and I haven't see any crashes.
I have not run any benchmarks yet, compared to userspace, I expect to
see improved latency (as I save up to 4 system calls per packet) but not
yet bandwidth/CPU (as TSO and interrupt mitigation are not yet supported).

Features that I plan to look at in the future:
- TSO
- interrupt mitigation
- zero copy

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---
 MAINTAINERS                |   10 +
 arch/x86/kvm/Kconfig       |    1 +
 drivers/Makefile           |    1 +
 drivers/block/virtio_blk.c |    3 +
 drivers/vhost/Kconfig      |   11 +
 drivers/vhost/Makefile     |    2 +
 drivers/vhost/net.c        |  462 ++++++++++++++++++++++++++++++
 drivers/vhost/vhost.c      |  663 ...
From: Arnd Bergmann
Date: Monday, August 10, 2009 - 12:51 pm

Very nice, I loved reading it. It's getting rather late in my time
zone, so this comments only on the network driver. I'll go through


[style] I think the code gets more readable if you reorder it

what is the difference between vhost_net_reset_owner(n)

This is missing a compat_ioctl pointer. It should simply be

static long vhost_net_compat_ioctl(struct file *f,
			unsigned int ioctl, unsigned long arg)
{
	return f, ioctl, (unsigned long)compat_ptr(arg);

Why do you need a kernel thread here? If the data transfer functions
all get called from a guest intercept, shouldn't you already be

I guess that this is where one could plug into macvlan directly, using
sock_alloc_send_skb/memcpy_fromiovec/dev_queue_xmit directly,
instead of filling a msghdr for each, if we want to combine this
with the work I did on that.

	Arnd <><
--

From: Michael S. Tsirkin
Date: Monday, August 10, 2009 - 1:10 pm

set socket to -1 will only stop the device.

reset owner will let another process take over the device.
It also needs to reset all parameters to make it safe for that
other process, so in particular the device is stopped.

I tried explaining this in the header vhost.h - does the comment

I had the impression that if there's no compat_ioctl,

several reasons :)
- I get called under lock, so can't block
- eventfd can be passed to another process, and I won't be in guest context at all
- this also gets called outside guest context from socket poll
- vcpu is blocked while it's doing i/o. it is better to free it up

--

From: Arnd Bergmann
Date: Monday, August 10, 2009 - 3:16 pm

It will issue a kernel warning but not call unlocked_ioctl,
so you need either a compat_ioctl method or list the numbers


Right, that works as well, but may get more complicated once we
try to add zero-copy or other optimizations.

	Arnd <><
--

From: Arnd Bergmann
Date: Wednesday, August 12, 2009 - 10:03 am

We discussed this before, and I still think this could be directly derived
from struct virtqueue, in the same way that vring_virtqueue is derived from
struct virtqueue. That would make it possible for simple device drivers
to use the same driver in both host and guest, similar to how Ira Snyder
used virtqueues to make virtio_net run between two hosts running the
same code [1].

Ideally, I guess you should be able to even make virtio_net work in the
host if you do that, but that could bring other complexities.

	Arnd <><

[1] http://lkml.org/lkml/2009/2/23/353
--

From: Ira W. Snyder
Date: Wednesday, August 12, 2009 - 10:19 am

I have no comments about the vhost code itself, I haven't reviewed it.

It might be interesting to try using a virtio-net in the host kernel to
communicate with the virtio-net running in the guest kernel. The lack of
a management interface is the biggest problem you will face (setting MAC
addresses, negotiating features, etc. doesn't work intuitively). Getting
the network interfaces talking is relatively easy.

Ira
--

From: Michael S. Tsirkin
Date: Wednesday, August 12, 2009 - 10:31 am

That was one of the reasons I decided to move most of code out to
userspace. My kernel driver only handles datapath,

Tried this, but
- guest memory isn't pinned, so copy_to_user
  to access it, errors need to be handled in a sane way
- used/available roles are reversed
- kick/interrupt roles are reversed

So most of the code then looks like

	if (host) {
	} else {
	}
	return


The only common part is walking the descriptor list,
but that's like 10 lines of code.

At which point it's better to keep host/guest code separate, IMO.

-- 
MST
--

From: Ira W. Snyder
Date: Wednesday, August 12, 2009 - 10:48 am

Ok, that makes sense. Let me see if I understand the concept of the
driver. Here's a picture of what makes sense to me:

guest system
---------------------------------
| userspace applications        |
---------------------------------
| kernel network stack          |
---------------------------------
| virtio-net                    |
---------------------------------
| transport (virtio-ring, etc.) |
---------------------------------
               |
               |
---------------------------------
| transport (virtio-ring, etc.) |
---------------------------------
| some driver (maybe vhost?)    | <-- [1]
---------------------------------
| kernel network stack          |
---------------------------------
host system

From the host's network stack, packets can be forwarded out to the
physical network, or be consumed by a normal userspace application on
the host. Just as if this were any other network interface.

In my patch, [1] was the virtio-net driver, completely unmodified.

So, does this patch accomplish the above diagram? If so, why the
copy_to_user(), etc? Maybe I'm confusing this with my system, where the
"guest" is another physical system, separated by the PCI bus.

Ira
--

From: Michael S. Tsirkin
Date: Wednesday, August 12, 2009 - 10:55 pm

Not exactly. vhost passes packets to a physical device,

Guest memory is not pinned. Memory access needs to go through

Yes, that's different.
--

From: Michael S. Tsirkin
Date: Wednesday, August 12, 2009 - 10:21 am

I prefer keeping it simple. Much of abstraction in virtio is due to the
fact that it needs to work on top of different hardware emulations:
lguest,kvm, possibly others in the future.  vhost is always working on

I don't think so. For example, there's a callback field that gets
invoked in guest when buffers are consumed.  It could be overloaded to
mean "buffers are available" in host but you never handle both

As I pointed out earlier, most code in virtio net is asymmetrical: guest
provides buffers, host consumes them.  Possibly, one could use virtio
rings in a symmetrical way, but support of existing guest virtio net
means there's almost no shared code.

-- 
MST
--

From: Arnd Bergmann
Date: Wednesday, August 12, 2009 - 10:59 am

Well, that was my point: virtio can already work on a number of abstractions,

The trick is to swap the virtqueues instead. virtio-net is actually
mostly symmetric in just the same way that the physical wires on a
twisted pair ethernet are symmetric (I like how that analogy fits).

virtio_net kicks the transmit virtqueue when it has data and
it kicks the receive queue when it has empty buffers to fill,
and it has callbacks when the two are done. You can do the
same in both the guest and the host, but then the guests input
virtqueue is the hosts output virtqueue and vice versa.

Once a virtqueue got kicked from both sides, the vhost_virtqueue
implementation between the two only needs to do a copy_from_user
or copy_to_user (possibly from a thread if it is in atomic context)
and then call the two callback functions. This is basically the
same thing you do already, except that you use slightly different
names for the components.

	Arnd <><
--

From: Anthony Liguori
Date: Wednesday, August 12, 2009 - 12:27 pm

It's already been done between two guests.  See 
http://article.gmane.org/gmane.linux.kernel.virtualization/5423

Regards,

Anthony Liguori
--

From: Michael S. Tsirkin
Date: Wednesday, August 12, 2009 - 11:31 pm

Yes, this works by copying data (see PATCH 5/5).  Another possibility is
page flipping.  Either will kill performance.

-- 
MST
--

From: Michael S. Tsirkin
Date: Wednesday, August 12, 2009 - 11:06 pm

You need to really squint hard for it to look symmetric.

For example, for RX, virtio allocates an skb, puts a descriptor on a
ring and waits for host to fill it in. Host system can not do the same:
guest does not have access to host memory.

You can do a copy in transport to hide this fact, but it will kill
performance.

-- 
MST
--

From: Arnd Bergmann
Date: Thursday, August 13, 2009 - 6:38 am

Yes, that is what I was suggesting all along. The actual copy operation
has to be done by the host transport, which is obviously different from
the guest transport that just calls the host using vring_kick().

Right now, the number of copy operations in your code is the same.
You are doing the copy a little bit later in skb_copy_datagram_iovec(),
which is indeed a very nice hack. Changing to a virtqueue based method
would imply that the host needs to add each skb_frag_t to its outbound
virtqueue, which then gets copied into the guests inbound virtqueue.

Unfortunately, this also implies that you could no longer simply use the
packet socket interface as you do currently, as I realized only now.
This obviously has a significant impact on your user space interface.

	Arnd <><
--

From: Arnd Bergmann
Date: Thursday, August 13, 2009 - 6:48 am

Also, if we do the copy in the transport, it definitely means that we
can't get to zero-copy RX/TX from guest space any more. The current
vhost_net driver doesn't do that yet, but could be extended in the
same way that I'm hoping to do it for macvtap.

	Arnd <><
--

From: Michael S. Tsirkin
Date: Thursday, August 13, 2009 - 7:41 am

The best way to do this IMO would be to add zero copy support to raw
sockets, vhost will then get it basically for free.

-- 
MST
--

From: Arnd Bergmann
Date: Thursday, August 13, 2009 - 7:53 am

Yes, that would be nice. I wonder if that could lead to security
problems on TX though. I guess It will only bring significant performance
improvements if we leave the data writable in the user space or guest
during the operation. If the user finds the right timing, it could
modify the frame headers after they have been checked using netfilter,
or while the frames are being consumed in the kernel (e.g. an NFS
server running in a guest).

	Ardn <><
--

From: Avi Kivity
Date: Thursday, August 13, 2009 - 8:37 am

IIRC when the kernel consumes data it linearizes the skb.  We just need 
to make sure all the zerocopy data is in the nonlinear part, and the 
kernel will copy if/when it needs to access packet data.

-- 
error compiling committee.c: too many arguments to function

--

From: Rusty Russell
Date: Thursday, August 20, 2009 - 12:25 am

For this reason, we always linearize parts of packets we're filtering.
ie. copy.

Cheers,
Rusty.
--

From: Michael S. Tsirkin
Date: Thursday, August 13, 2009 - 7:39 am

And, it will remove our ability to implement zero copy
down the road (when raw sockets support it).

-- 
MST
--

From: Arnd Bergmann
Date: Thursday, August 13, 2009 - 7:58 am

Well, I don't see this part as much of a problem, because the code
already exists in virtio_net. If we really wanted to go down that road,
just using virtio_net would solve the problem of frame handling
entirely, but create new problems elsewhere, as we have mentioned.

	Arnd <><
--

From: Michael S. Tsirkin
Date: Thursday, August 13, 2009 - 8:03 am

From: Anthony Liguori
Date: Wednesday, August 12, 2009 - 12:22 pm

Actually, vhost may not always be limited to real hardware.

We may on day use vhost as the basis of a driver domain.  There's quite 
a lot of interest in this for networking.

At any rate, I'd like to see performance results before we consider 
trying to reuse virtio code.

Regards,

Anthony Liguori
--

From: Michael S. Tsirkin
Date: Thursday, August 13, 2009 - 1:45 am

Yes, any ethernet device will do. What I mean is that vhost does not

--

From: Arnd Bergmann
Date: Thursday, August 13, 2009 - 6:45 am

Yes, I agree. I'd also like to do more work on the macvlan extensions
to see if it works out without involving a socket. Passing the socket
into the vhost_net device is a nice feature of the current implementation
that we'd have to give up for something else (e.g. making the vhost
a real network interface that you can hook up to a bridge) if it were
to use virtio.

Unless I can come up with a solution that is clearly superior, I'm
taking back my objections on that part for now.

	Arnd <><
--

From: Paul E. McKenney
Date: Wednesday, August 12, 2009 - 12:58 pm

Much better -- a couple of documentation nits below.


How about something like "Therefore the beginning of workqueue
execution acts as rcu_read_lock() and the end of workqueue execution
acts as rcu_read_lock()"?

It would also be good to add comments to the workqueue functions
themselves saying that they act as read-side critical sections for
your kind of RCU.

--

Previous thread: [PATCH 1/2] export cpu_tlbstate to modules by Michael S. Tsirkin on Monday, August 10, 2009 - 11:53 am. (5 messages)

Next thread: module loading permissions and request_module permission inconsistencies by Eric Paris on Monday, August 10, 2009 - 12:45 pm. (6 messages)