VM Sockets for Linux upstreaming

In an effort to improve the out-of-the-box experience with Linux kernels for VMware users, VMware is working on readying the VM Sockets (VSOCK, formerly VMCI Sockets) (vsock) kernel module for inclusion in the Linux kernel. The purpose of this post is to acquire feedback on the vsock kernel module.

Unlike previous submissions, where the new socket family was entirely reliant on VMware's VMCI PCI device (and thus VMware's hypervisor), VM Sockets is now completely[1] separated out into two parts, each in its own module:

o Core socket code, which is transport-neutral and invokes transport callbacks to communicate with the hypervisor. This is vsock.ko.
o A VMCI transport, which communicates over VMCI with the VMware hypervisor. This is vmw_vsock_vmci_transport.ko, and it registers with the core module as a transport.

This should provide a path to introducing additional transports, for example virtio, with the ultimate goal being to make this new socket family hypervisor-neutral.

Hi David,

Quoted 15 lines · Andy King

In an effort to improve the out-of-the-box experience with Linux kernels for VMware users, VMware is working on readying the VM Sockets (VSOCK, formerly VMCI Sockets) (vsock) kernel module for inclusion in the Linux kernel. The purpose of this post is to acquire feedback on the vsock kernel module.

Unlike previous submissions, where the new socket family was entirely reliant on VMware's VMCI PCI device (and thus VMware's hypervisor), VM Sockets is now completely[1] separated out into two parts, each in its own module:

o Core socket code, which is transport-neutral and invokes transport callbacks to communicate with the hypervisor. This is vsock.ko.
o A VMCI transport, which communicates over VMCI with the VMware hypervisor. This is vmw_vsock_vmci_transport.ko, and it registers with the core module as a transport.

Quoted 3 lines

This should provide a path to introducing additional transports, for example virtio, with the ultimate goal being to make this new socket family hypervisor-neutral.

As Andy mentioned in another e-mail, we would like very much to get vsock in 3.9 release, so now that it is split into hypervisor neutral and transport parts is there any high level issues that we need to resolve before the code can be accepted?

In an effort to improve the out-of-the-box experience with Linux kernels for VMware users, VMware is working on readying the VM Sockets (VSOCK, formerly VMCI Sockets) (vsock) kernel module for inclusion in the Linux kernel. The purpose of this post is to acquire feedback on the vsock kernel module.

Unlike previous submissions, where the new socket family was entirely reliant on VMware's VMCI PCI device (and thus VMware's hypervisor), VM Sockets is now completely[1] separated out into two parts, each in its own module:

o Core socket code, which is transport-neutral and invokes transport callbacks to communicate with the hypervisor. This is vsock.ko.
o A VMCI transport, which communicates over VMCI with the VMware hypervisor. This is vmw_vsock_vmci_transport.ko, and it registers with the core module as a transport.

This should provide a path to introducing additional transports, for example virtio, with the ultimate goal being to make this new socket family hypervisor-neutral.

net/vmw_vsock/af_vsock.h new file +65 -0

+struct vsock_transport_recv_notify_data {
+	u64 data1; /* Transport-defined. */
+	u64 data2; /* Transport-defined. */
+	bool notify_on_block;
+};
+
+struct vsock_transport_send_notify_data {
+	u64 data1; /* Transport-defined. */
+	u64 data2; /* Transport-defined. */
+};
+
+struct vsock_transport {
+	/* Initialize/tear-down socket. */
+	int (*init)(struct vsock_sock *, struct vsock_sock *);
+	void (*destruct)(struct vsock_sock *);
+	void (*release)(struct vsock_sock *);
+
+	/* Connections. */
+	int (*connect)(struct vsock_sock *);
+
+	/* DGRAM. */
+	int (*dgram_bind)(struct vsock_sock *, struct sockaddr_vm *);
+	int (*dgram_dequeue)(struct kiocb *kiocb, struct vsock_sock *vsk,
+			     struct msghdr *msg, size_t len, int flags);
+	int (*dgram_enqueue)(struct vsock_sock *, struct sockaddr_vm *,
+			     struct iovec *, size_t len);
+	bool (*dgram_allow)(u32 cid, u32 port);
+
+	/* STREAM. */
+	/* TODO: stream_bind() */
+	ssize_t (*stream_dequeue)(struct vsock_sock *, struct iovec *,
+				  size_t len, int flags);
+	ssize_t (*stream_enqueue)(struct vsock_sock *, struct iovec *,
+				  size_t len);
+	s64 (*stream_has_data)(struct vsock_sock *);
+	s64 (*stream_has_space)(struct vsock_sock *);
+	u64 (*stream_rcvhiwat)(struct vsock_sock *);
+	bool (*stream_is_active)(struct vsock_sock *);
+	bool (*stream_allow)(u32 cid, u32 port);
+
+	/* Notification. */
+	int (*notify_poll_in)(struct vsock_sock *, size_t, bool *);
+	int (*notify_poll_out)(struct vsock_sock *, size_t, bool *);
+	int (*notify_recv_init)(struct vsock_sock *, size_t,
+		struct vsock_transport_recv_notify_data *);
+	int (*notify_recv_pre_block)(struct vsock_sock *, size_t,
+		struct vsock_transport_recv_notify_data *);
+	int (*notify_recv_pre_dequeue)(struct vsock_sock *, size_t,
+		struct vsock_transport_recv_notify_data *);
+	int (*notify_recv_post_dequeue)(struct vsock_sock *, size_t,
+		ssize_t, bool, struct vsock_transport_recv_notify_data *);
+	int (*notify_send_init)(struct vsock_sock *,
+		struct vsock_transport_send_notify_data *);
+	int (*notify_send_pre_block)(struct vsock_sock *,
+		struct vsock_transport_send_notify_data *);
+	int (*notify_send_pre_enqueue)(struct vsock_sock *,
+		struct vsock_transport_send_notify_data *);
+	int (*notify_send_post_enqueue)(struct vsock_sock *, ssize_t,
+		struct vsock_transport_send_notify_data *);
+
+	/* Shutdown. */
+	int (*shutdown)(struct vsock_sock *, int);
+
+	/* Buffer sizes. */
+	void (*set_buffer_size)(struct vsock_sock *, u64);

Quoted 1 line

+struct vsock_transport {

Whoa. This has grown *alot*. Care to explain this please? Patch creating a Documentation/virtual/vsock.txt would be cool.

stream_has_data + stream_has_space + stream_rcvhiwat look like they are needed for buffer management. Details please (especially for stream_rcvhiwat).

What is stream_is_active?

What is *_allow?

What are all those notify_* calls?

Why do you need vsock_transport_{send,recv}_notify_data structs? Can't this live in vsock_sock->trans?

stream_has_data: Returns amount of data available (in bytes) in the socket's receive buffer, or -1 if empty.

stream_has_space: Returns amount of space available (in bytes) in the socket's send buffer, or -1 if full.

Quoted 4 lines

+/* Option name for socket's service label. Use as the option name in
+ * setsockopt(3) or getsockopt(3) to set or get the service label for a socket.
+ * The service label is a C-style NUL-terminated string. Only available

include/uapi/linux/vm_sockets.h line 62

Quoted 6 lines

for
+ * hypervisor endpoints.
+ */

include/uapi/linux/vm_sockets.h new file +3 -0

+
+#define SO_VM_SOCKETS_SERVICE_LABEL 4
+

What is this and how does it work?

Hi Gerd,

Quoted 1 line

Quoted 1 line

+struct vsock_transport {

...

Quoted 2 lines

Whoa. This has grown *alot*. Care to explain this please? Patch creating a Documentation/virtual/vsock.txt would be cool.

Yes, it grew because of the notification stuff, which I'd forgotten about until I removed the vmci header from the core code. You are free to use empty functions for these if they don't make any sense for virtio.

The alternative is for us to move the entire body of vsock_stream_recv/send into the transport, where we can hide the notification stuff, but it seems like folks will just end up duplicating a lot of code then.

Quoted 3 lines

stream_has_data + stream_has_space + stream_rcvhiwat look like they are needed for buffer management. Details please (especially for stream_rcvhiwat).

stream_has_data: Returns amount of data available (in bytes) in the socket's receive buffer, or -1 if empty.

stream_has_space: Returns amount of space available (in bytes) in the socket's send buffer, or -1 if full.

stream_rcvhiwat: The upper bound on the socket's receive buffer. Which technically is the same as the value returned by get_buffer_size(), so perhaps we could substitute that here and drop this one.

Quoted 1 line

What is stream_is_active?

For the VMCI transport, it indicates if the underlying queuepair is still around (i.e., make sure we haven't torn it down while sleeping in a blocking send or receive). Perhaps it's not the best name?

Quoted 1 line

What is *_allow?

It's very basic filtering. We have specific addresses that we don't allow, and we look for them in the allow() functions. You can just return true if you like.

Quoted 1 line

What are all those notify_* calls?

They're to do with signaling and flow-control. Again, they might not make any sense at all, but it's hard to know without having written another transport :)

Quoted 2 lines

Why do you need vsock_transport_{send,recv}_notify_data structs? Can't this live in vsock_sock->trans?

Those have to be setup on a per-call basis (per-thread), so it's just easier to have them on the stack of the send/recv calls. If you think there's a better name, or a better way to allocate them, I'm all ears.

net/vmw_vsock/af_vsock.c line 1868

Quoted 3 lines

net/vmw_vsock/af_vsock.c new file +3 -0

+	if (protocol)
+		return -EPROTONOSUPPORT;
+

IMO protocol == PF_VSOCK should not get rejected here.

I've seen you have a notify_ops in the vmci bits. Do you have different notify ops depending on socket type or something? Does it make sense to move the notify ops ptr into "struct vsock_sock" maybe?

And can we make it optional please (i.e. allow the function pointers to be NULL)?

Which problem you are trying to tackle with the notifications?

Quoted 3 lines

For the VMCI transport, it indicates if the underlying queuepair is still around (i.e., make sure we haven't torn it down while sleeping in a blocking send or receive). Perhaps it's not the best name?

How you'd hit that? Peer closing the socket while sleeping? Other thread closing the socket wile sleeping? Both?

I think a state field in struct vsock_sock would be a better solution here.

Quoted 6 lines

Quoted 2 lines

What is *_allow?

It's very basic filtering. We have specific addresses that we don't allow, and we look for them in the allow() functions. You can just return true if you like.

Can we make those calls optional too please?

The notify_*_init could return a opaque pointer instead. But then you'll need a notify_*_free too. And you can't place it on the stack and thus have a allocation in the hot path, which I guess you are trying to avoid in the first place.

Quoted 3 lines

I've seen you have a notify_ops in the vmci bits. Do you have different notify ops depending on socket type or something? Does it make sense to move the notify ops ptr into "struct vsock_sock" maybe?

The notify stuff only applies to STREAMs. However, we have two different notify impls, one for legacy ESX and one for newer, and we figure out at runtime which protocol we're using with the hypervisor and set the callbacks appropriately.

The difference between the two is that the
newer one is much smarter and knows not to signal (the peer) quite so much, i.e., it has some basic but sensible flow-control, which improves performance quite a bit.

Again, that might not make any sense at all
for virtio. Do you need to signal when you enqueue to a ring? And is there coalescing?

Quoted 2 lines

And can we make it optional please (i.e. allow the function pointers to be NULL)?

They were originally allowed to be NULL, but I changed it in the last round of patches while moving them into the transport, since I disliked the NULL checks so much. I can put them back, but that's a bigger change, and I'm not sure we want to push large patches to Dave right now :)

Quoted 1 line

Which problem you are trying to tackle with the notifications?

It's to do with signaling the peer, or more appropriately, trying to avoid signaling the peer when possible. The naive impl. is to signal every time we enqueue or dequeue data (into our VMCI queuepairs). But signaling is slow, since it involves a world exit, so we prefer not to. Which means we need to keep track of rate of flow and figure out when we should and should not, and that's what all the notification stuff does. It's...ugly...

Quoted 8 lines

Quoted 3 lines

For the VMCI transport, it indicates if the underlying queuepair is still around (i.e., make sure we haven't torn it down while sleeping in a blocking send or receive). Perhaps it's not the best name?

How you'd hit that? Peer closing the socket while sleeping? Other thread closing the socket wile sleeping? Both?

I think a state field in struct vsock_sock would be a better solution here.

Hrm, lemme think about this one.

+ * Specifically, we initialize the vsock_bind_table array to a size of
+ * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through
+ * vsock_bind_table[VSOCK_HASH_SIZE - 1] are for bound sockets and
+ * vsock_bind_table[VSOCK_HASH_SIZE] is for unbound sockets. The hash function
+ * mods with VSOCK_HASH_SIZE - 1 to ensure this.
+ */

net/vmw_vsock/af_vsock.c new file +8 -0

+#define VSOCK_HASH_SIZE         251
+#define MAX_PORT_RETRIES        24
+
+#define VSOCK_HASH(addr)        ((addr)->svm_port % (VSOCK_HASH_SIZE - 1))
+#define vsock_bound_sockets(addr) (&vsock_bind_table[VSOCK_HASH(addr)])
+#define vsock_unbound_sockets     (&vsock_bind_table[VSOCK_HASH_SIZE])
+
+/* XXX This can probably be implemented in a better way. */

include/uapi/linux/vm_sockets.h new file +9 -0

+/* Use this as the destination CID in an address when referring to the
+ * hypervisor.  VMCI relies on it being 0, but this would be useful for other
+ * transports too.
+ */
+
+#define VMADDR_CID_HYPERVISOR 0
+
+/* This CID is specific to VMCI and can be considered reserved (even VMCI
+ * doesn't use it anymore, it's a legacy value from an older release).

net/vmw_vsock/af_vsock.c line 1868

Quoted 3 lines

net/vmw_vsock/af_vsock.c new file +3 -0

+	if (protocol)
+		return -EPROTONOSUPPORT;
+

IMO protocol == PF_VSOCK should not get rejected here.

net/vmw_vsock/af_vsock.c line 1632

Quoted 3 lines

net/vmw_vsock/af_vsock.c new file +3 -0

+		written = transport->stream_enqueue(
+				vsk, msg->msg_iov,
+				len - total_written);

Hmm, shouldn't we pass total_written to stream_enqueue here?

In case a blocking send(big-buffer) call gets splitted into multiple stream_enqueue calls the second (and further) stream_enqueue calls need to know at which msg offset they should continue sending the data, no?

Quoted 2 lines

+ * Specifically, we initialize the vsock_bind_table array to a size of
+ * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through

Why isn't it using the kernel's linux/hashtable.h?

Quoted 2 lines

+ * Specifically, we initialize the vsock_bind_table array to a size of
+ * VSOCK_HASH_SIZE + 1 so that vsock_bind_table[0] through

Why isn't it using the kernel's linux/hashtable.h?

Hi Gerd,

Quoted 6 lines

net/vmw_vsock/af_vsock.c line 1632

Quoted 3 lines

net/vmw_vsock/af_vsock.c new file +3 -0

+		written = transport->stream_enqueue(
+				vsk, msg->msg_iov,
+				len - total_written);

Hmm, shouldn't we pass total_written to stream_enqueue here?

Quoted 3 lines

In case a blocking send(big-buffer) call gets splitted into multiple stream_enqueue calls the second (and further) stream_enqueue calls need to know at which msg offset they should continue sending the data, no?

On the client side, the iov tracks it internally; see memcpy_fromiovec().