tun: export underlying socket

PATCH Superseded Michael S. Tsirkin 3 participants 32 messages Nov 4, 2009 – May 10, 2010 1 message not in the archive

Subsystems (10)

Networking Network drivers Memory management x86 +6 more subsystems

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael S. Tsirkin

author maintainer

2009-11-04 15:55 UTC #1 lore raw

PATCH v8 1/3 tun: export underlying socket

Tun device looks similar to a packet socket in that both pass complete frames from/to userspace.

This patch fills in enough fields in the socket underlying tun driver to support sendmsg/recvmsg operations, and message flags MSG_TRUNC and MSG_DONTWAIT, and exports access to this socket to modules. Regular read/write behaviour is unchanged.

This way, code using raw sockets to inject packets into a physical device, can support injecting packets into host network stack almost without modification.

First user of this interface will be vhost virtualization accelerator.

Signed-off-by Michael S. Tsirkin <[email protected]>

Acked-by Herbert Xu <[email protected]>

Acked-by David S. Miller <[email protected]>

---

DaveM agreed that this will go in through Rusty's tree together with vhost patch, because vhost is the first user of the new interface.

 drivers/net/tun.c      |  101 +++++++++++++++++++++++++++++++++++++++---------
 include/linux/if_tun.h |   14 +++++++
 2 files changed, 96 insertions(+), 19 deletions(-)

drivers/net/tun.c +82 ~~-19~~

index 4fdfa2a..18f8876 100644
@@ -144,6 +144,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file)
 	err = 0;
 	tfile->tun = tun;
 	tun->tfile = tfile;
+	tun->socket.file = file;
 	dev_hold(tun->dev);
 	sock_hold(tun->socket.sk);
 	atomic_inc(&tfile->count);

@@ -158,6 +159,7 @@ static void __tun_detach(struct tun_struct *tun)
 	/* Detach from net device */
 	netif_tx_lock_bh(tun->dev);
 	tun->tfile = NULL;
+	tun->socket.file = NULL;
 	netif_tx_unlock_bh(tun->dev);
 
 	/* Drop read queue */

@@ -387,7 +389,8 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Notify and wake up reader process */
 	if (tun->flags & TUN_FASYNC)
 		kill_fasync(&tun->fasync, SIGIO, POLL_IN);
-	wake_up_interruptible(&tun->socket.wait);
+	wake_up_interruptible_poll(&tun->socket.wait, POLLIN |
+				   POLLRDNORM | POLLRDBAND);
 	return NETDEV_TX_OK;
 
 drop:

@@ -743,7 +746,7 @@ static __inline__ ssize_t tun_put_user(struct tun_struct *tun,
 	len = min_t(int, skb->len, len);
 
 	skb_copy_datagram_const_iovec(skb, 0, iv, total, len);
-	total += len;
+	total += skb->len;
 
 	tun->dev->stats.tx_packets++;
 	tun->dev->stats.tx_bytes += len;

@@ -751,34 +754,23 @@ static __inline__ ssize_t tun_put_user(struct tun_struct *tun,
 	return total;
 }
 
-static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
-			    unsigned long count, loff_t pos)
+static ssize_t tun_do_read(struct tun_struct *tun,
+			   struct kiocb *iocb, const struct iovec *iv,
+			   ssize_t len, int noblock)
 {
-	struct file *file = iocb->ki_filp;
-	struct tun_file *tfile = file->private_data;
-	struct tun_struct *tun = __tun_get(tfile);
 	DECLARE_WAITQUEUE(wait, current);
 	struct sk_buff *skb;
-	ssize_t len, ret = 0;
-
-	if (!tun)
-		return -EBADFD;
+	ssize_t ret = 0;
 
 	DBG(KERN_INFO "%s: tun_chr_read\n", tun->dev->name);
 
-	len = iov_length(iv, count);
-	if (len < 0) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	add_wait_queue(&tun->socket.wait, &wait);
 	while (len) {
 		current->state = TASK_INTERRUPTIBLE;
 
 		/* Read frames from the queue */
 		if (!(skb=skb_dequeue(&tun->socket.sk->sk_receive_queue))) {
-			if (file->f_flags & O_NONBLOCK) {
+			if (noblock) {
 				ret = -EAGAIN;
 				break;
 			}

@@ -805,6 +797,27 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
 	current->state = TASK_RUNNING;
 	remove_wait_queue(&tun->socket.wait, &wait);
 
+	return ret;
+}
+
+static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
+			    unsigned long count, loff_t pos)
+{
+	struct file *file = iocb->ki_filp;
+	struct tun_file *tfile = file->private_data;
+	struct tun_struct *tun = __tun_get(tfile);
+	ssize_t len, ret;
+
+	if (!tun)
+		return -EBADFD;
+	len = iov_length(iv, count);
+	if (len < 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = tun_do_read(tun, iocb, iv, len, file->f_flags & O_NONBLOCK);
+	ret = min_t(ssize_t, ret, len);
 out:
 	tun_put(tun);
 	return ret;

@@ -847,7 +860,8 @@ static void tun_sock_write_space(struct sock *sk)
 		return;
 
 	if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
-		wake_up_interruptible_sync(sk->sk_sleep);
+		wake_up_interruptible_sync_poll(sk->sk_sleep, POLLOUT |
+						POLLWRNORM | POLLWRBAND);
 
 	tun = container_of(sk, struct tun_sock, sk)->tun;
 	kill_fasync(&tun->fasync, SIGIO, POLL_OUT);

@@ -858,6 +872,37 @@ static void tun_sock_destruct(struct sock *sk)
 	free_netdev(container_of(sk, struct tun_sock, sk)->tun->dev);
 }
 
+static int tun_sendmsg(struct kiocb *iocb, struct socket *sock,
+		       struct msghdr *m, size_t total_len)
+{
+	struct tun_struct *tun = container_of(sock, struct tun_struct, socket);
+	return tun_get_user(tun, m->msg_iov, total_len,
+			    m->msg_flags & MSG_DONTWAIT);
+}
+
+static int tun_recvmsg(struct kiocb *iocb, struct socket *sock,
+		       struct msghdr *m, size_t total_len,
+		       int flags)
+{
+	struct tun_struct *tun = container_of(sock, struct tun_struct, socket);
+	int ret;
+	if (flags & ~(MSG_DONTWAIT|MSG_TRUNC))
+		return -EINVAL;
+	ret = tun_do_read(tun, iocb, m->msg_iov, total_len,
+			  flags & MSG_DONTWAIT);
+	if (ret > total_len) {
+		m->msg_flags |= MSG_TRUNC;
+		ret = flags & MSG_TRUNC ? ret : total_len;
+	}
+	return ret;
+}
+
+/* Ops structure to mimic raw sockets with tun */
+static const struct proto_ops tun_socket_ops = {
+	.sendmsg = tun_sendmsg,
+	.recvmsg = tun_recvmsg,
+};
+
 static struct proto tun_proto = {
 	.name		= "tun",
 	.owner		= THIS_MODULE,

@@ -986,6 +1031,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 			goto err_free_dev;
 
 		init_waitqueue_head(&tun->socket.wait);
+		tun->socket.ops = &tun_socket_ops;
 		sock_init_data(&tun->socket, sk);
 		sk->sk_write_space = tun_sock_write_space;
 		sk->sk_sndbuf = INT_MAX;

@@ -1489,6 +1535,23 @@ static void tun_cleanup(void)
 	rtnl_link_unregister(&tun_link_ops);
 }
 
+/* Get an underlying socket object from tun file.  Returns error unless file is
+ * attached to a device.  The returned object works like a packet socket, it
+ * can be used for sock_sendmsg/sock_recvmsg.  The caller is responsible for
+ * holding a reference to the file for as long as the socket is in use. */
+struct socket *tun_get_socket(struct file *file)
+{
+	struct tun_struct *tun;
+	if (file->f_op != &tun_fops)
+		return ERR_PTR(-EINVAL);
+	tun = tun_get(file);
+	if (!tun)
+		return ERR_PTR(-EBADFD);
+	tun_put(tun);
+	return &tun->socket;
+}
+EXPORT_SYMBOL_GPL(tun_get_socket);
+
 module_init(tun_init);
 module_exit(tun_cleanup);
 MODULE_DESCRIPTION(DRV_DESCRIPTION);

include/linux/if_tun.h +14 -0

index 3f5fd52..404abe0 100644
@@ -86,4 +86,18 @@ struct tun_filter {
 	__u8   addr[0][ETH_ALEN];
 };
 
+#ifdef __KERNEL__
+#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
+struct socket *tun_get_socket(struct file *);
+#else
+#include <linux/err.h>
+#include <linux/errno.h>
+struct file;
+struct socket;
+static inline struct socket *tun_get_socket(struct file *f)
+{
+	return ERR_PTR(-EINVAL);
+}
+#endif /* CONFIG_TUN */
+#endif /* __KERNEL__ */
 #endif /* __IF_TUN_H */

Signature · 7 lines

1.6.5.2.143.g8cc62

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michael S. Tsirkin

author maintainer

2009-11-04 15:55 UTC #2 lore raw

PATCH v8 1/3 tun: export underlying socket

In reply to a message not in the archive

Tun device looks similar to a packet socket in that both pass complete frames from/to userspace.

This way, code using raw sockets to inject packets into a physical device, can support injecting packets into host network stack almost without modification.

First user of this interface will be vhost virtualization accelerator.

Signed-off-by Michael S. Tsirkin <[email protected]>

Acked-by Herbert Xu <[email protected]>

Acked-by David S. Miller <[email protected]>

---

DaveM agreed that this will go in through Rusty's tree together with vhost patch, because vhost is the first user of the new interface.

 drivers/net/tun.c      |  101 +++++++++++++++++++++++++++++++++++++++---------
 include/linux/if_tun.h |   14 +++++++
 2 files changed, 96 insertions(+), 19 deletions(-)

drivers/net/tun.c +82 ~~-19~~

index 4fdfa2a..18f8876 100644
@@ -144,6 +144,7 @@ static int tun_attach(struct tun_struct *tun, struct file *file)
 	err = 0;
 	tfile->tun = tun;
 	tun->tfile = tfile;
+	tun->socket.file = file;
 	dev_hold(tun->dev);
 	sock_hold(tun->socket.sk);
 	atomic_inc(&tfile->count);

@@ -158,6 +159,7 @@ static void __tun_detach(struct tun_struct *tun)
 	/* Detach from net device */
 	netif_tx_lock_bh(tun->dev);
 	tun->tfile = NULL;
+	tun->socket.file = NULL;
 	netif_tx_unlock_bh(tun->dev);
 
 	/* Drop read queue */

@@ -387,7 +389,8 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Notify and wake up reader process */
 	if (tun->flags & TUN_FASYNC)
 		kill_fasync(&tun->fasync, SIGIO, POLL_IN);
-	wake_up_interruptible(&tun->socket.wait);
+	wake_up_interruptible_poll(&tun->socket.wait, POLLIN |
+				   POLLRDNORM | POLLRDBAND);
 	return NETDEV_TX_OK;
 
 drop:

@@ -743,7 +746,7 @@ static __inline__ ssize_t tun_put_user(struct tun_struct *tun,
 	len = min_t(int, skb->len, len);
 
 	skb_copy_datagram_const_iovec(skb, 0, iv, total, len);
-	total += len;
+	total += skb->len;
 
 	tun->dev->stats.tx_packets++;
 	tun->dev->stats.tx_bytes += len;

@@ -751,34 +754,23 @@ static __inline__ ssize_t tun_put_user(struct tun_struct *tun,
 	return total;
 }
 
-static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
-			    unsigned long count, loff_t pos)
+static ssize_t tun_do_read(struct tun_struct *tun,
+			   struct kiocb *iocb, const struct iovec *iv,
+			   ssize_t len, int noblock)
 {
-	struct file *file = iocb->ki_filp;
-	struct tun_file *tfile = file->private_data;
-	struct tun_struct *tun = __tun_get(tfile);
 	DECLARE_WAITQUEUE(wait, current);
 	struct sk_buff *skb;
-	ssize_t len, ret = 0;
-
-	if (!tun)
-		return -EBADFD;
+	ssize_t ret = 0;
 
 	DBG(KERN_INFO "%s: tun_chr_read\n", tun->dev->name);
 
-	len = iov_length(iv, count);
-	if (len < 0) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	add_wait_queue(&tun->socket.wait, &wait);
 	while (len) {
 		current->state = TASK_INTERRUPTIBLE;
 
 		/* Read frames from the queue */
 		if (!(skb=skb_dequeue(&tun->socket.sk->sk_receive_queue))) {
-			if (file->f_flags & O_NONBLOCK) {
+			if (noblock) {
 				ret = -EAGAIN;
 				break;
 			}

@@ -805,6 +797,27 @@ static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
 	current->state = TASK_RUNNING;
 	remove_wait_queue(&tun->socket.wait, &wait);
 
+	return ret;
+}
+
+static ssize_t tun_chr_aio_read(struct kiocb *iocb, const struct iovec *iv,
+			    unsigned long count, loff_t pos)
+{
+	struct file *file = iocb->ki_filp;
+	struct tun_file *tfile = file->private_data;
+	struct tun_struct *tun = __tun_get(tfile);
+	ssize_t len, ret;
+
+	if (!tun)
+		return -EBADFD;
+	len = iov_length(iv, count);
+	if (len < 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = tun_do_read(tun, iocb, iv, len, file->f_flags & O_NONBLOCK);
+	ret = min_t(ssize_t, ret, len);
 out:
 	tun_put(tun);
 	return ret;

@@ -847,7 +860,8 @@ static void tun_sock_write_space(struct sock *sk)
 		return;
 
 	if (sk->sk_sleep && waitqueue_active(sk->sk_sleep))
-		wake_up_interruptible_sync(sk->sk_sleep);
+		wake_up_interruptible_sync_poll(sk->sk_sleep, POLLOUT |
+						POLLWRNORM | POLLWRBAND);
 
 	tun = container_of(sk, struct tun_sock, sk)->tun;
 	kill_fasync(&tun->fasync, SIGIO, POLL_OUT);

@@ -858,6 +872,37 @@ static void tun_sock_destruct(struct sock *sk)
 	free_netdev(container_of(sk, struct tun_sock, sk)->tun->dev);
 }
 
+static int tun_sendmsg(struct kiocb *iocb, struct socket *sock,
+		       struct msghdr *m, size_t total_len)
+{
+	struct tun_struct *tun = container_of(sock, struct tun_struct, socket);
+	return tun_get_user(tun, m->msg_iov, total_len,
+			    m->msg_flags & MSG_DONTWAIT);
+}
+
+static int tun_recvmsg(struct kiocb *iocb, struct socket *sock,
+		       struct msghdr *m, size_t total_len,
+		       int flags)
+{
+	struct tun_struct *tun = container_of(sock, struct tun_struct, socket);
+	int ret;
+	if (flags & ~(MSG_DONTWAIT|MSG_TRUNC))
+		return -EINVAL;
+	ret = tun_do_read(tun, iocb, m->msg_iov, total_len,
+			  flags & MSG_DONTWAIT);
+	if (ret > total_len) {
+		m->msg_flags |= MSG_TRUNC;
+		ret = flags & MSG_TRUNC ? ret : total_len;
+	}
+	return ret;
+}
+
+/* Ops structure to mimic raw sockets with tun */
+static const struct proto_ops tun_socket_ops = {
+	.sendmsg = tun_sendmsg,
+	.recvmsg = tun_recvmsg,
+};
+
 static struct proto tun_proto = {
 	.name		= "tun",
 	.owner		= THIS_MODULE,

@@ -986,6 +1031,7 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
 			goto err_free_dev;
 
 		init_waitqueue_head(&tun->socket.wait);
+		tun->socket.ops = &tun_socket_ops;
 		sock_init_data(&tun->socket, sk);
 		sk->sk_write_space = tun_sock_write_space;
 		sk->sk_sndbuf = INT_MAX;

@@ -1489,6 +1535,23 @@ static void tun_cleanup(void)
 	rtnl_link_unregister(&tun_link_ops);
 }
 
+/* Get an underlying socket object from tun file.  Returns error unless file is
+ * attached to a device.  The returned object works like a packet socket, it
+ * can be used for sock_sendmsg/sock_recvmsg.  The caller is responsible for
+ * holding a reference to the file for as long as the socket is in use. */
+struct socket *tun_get_socket(struct file *file)
+{
+	struct tun_struct *tun;
+	if (file->f_op != &tun_fops)
+		return ERR_PTR(-EINVAL);
+	tun = tun_get(file);
+	if (!tun)
+		return ERR_PTR(-EBADFD);
+	tun_put(tun);
+	return &tun->socket;
+}
+EXPORT_SYMBOL_GPL(tun_get_socket);
+
 module_init(tun_init);
 module_exit(tun_cleanup);
 MODULE_DESCRIPTION(DRV_DESCRIPTION);

include/linux/if_tun.h +14 -0

index 3f5fd52..404abe0 100644
@@ -86,4 +86,18 @@ struct tun_filter {
 	__u8   addr[0][ETH_ALEN];
 };
 
+#ifdef __KERNEL__
+#if defined(CONFIG_TUN) || defined(CONFIG_TUN_MODULE)
+struct socket *tun_get_socket(struct file *);
+#else
+#include <linux/err.h>
+#include <linux/errno.h>
+struct file;
+struct socket;
+static inline struct socket *tun_get_socket(struct file *f)
+{
+	return ERR_PTR(-EINVAL);
+}
+#endif /* CONFIG_TUN */
+#endif /* __KERNEL__ */
 #endif /* __IF_TUN_H */

Signature · 1 line

1.6.5.2.143.g8cc62

Michael S. Tsirkin

author maintainer

2009-11-04 15:55 UTC #3 lore raw

PATCH v8 2/3 mm: export use_mm/unuse_mm to modules

In reply to a message not in the archive

vhost net module wants to do copy to/from user from a kernel thread, which needs use_mm. Export it to modules.

Acked-by Andrea Arcangeli <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 mm/mmu_context.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

mm/mmu_context.c +3 -0

index ded9081..0777654 100644
@@ -5,6 +5,7 @@
 
 #include <linux/mm.h>
 #include <linux/mmu_context.h>
+#include <linux/module.h>
 #include <linux/sched.h>
 
 #include <asm/mmu_context.h>

@@ -37,6 +38,7 @@ void use_mm(struct mm_struct *mm)
 	if (active_mm != mm)
 		mmdrop(active_mm);
 }
+EXPORT_SYMBOL_GPL(use_mm);
 
 /*
  * unuse_mm

@@ -56,3 +58,4 @@ void unuse_mm(struct mm_struct *mm)
 	enter_lazy_tlb(mm, tsk);
 	task_unlock(tsk);
 }
+EXPORT_SYMBOL_GPL(unuse_mm);

Signature · 7 lines

1.6.5.2.143.g8cc62

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michael S. Tsirkin

author maintainer

2009-11-04 15:55 UTC #4 lore raw

PATCH v8 2/3 mm: export use_mm/unuse_mm to modules

In reply to a message not in the archive

vhost net module wants to do copy to/from user from a kernel thread, which needs use_mm. Export it to modules.

Acked-by Andrea Arcangeli <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---
 mm/mmu_context.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

mm/mmu_context.c +3 -0

index ded9081..0777654 100644
@@ -5,6 +5,7 @@
 
 #include <linux/mm.h>
 #include <linux/mmu_context.h>
+#include <linux/module.h>
 #include <linux/sched.h>
 
 #include <asm/mmu_context.h>

@@ -37,6 +38,7 @@ void use_mm(struct mm_struct *mm)
 	if (active_mm != mm)
 		mmdrop(active_mm);
 }
+EXPORT_SYMBOL_GPL(use_mm);
 
 /*
  * unuse_mm

@@ -56,3 +58,4 @@ void unuse_mm(struct mm_struct *mm)
 	enter_lazy_tlb(mm, tsk);
 	task_unlock(tsk);
 }
+EXPORT_SYMBOL_GPL(unuse_mm);

Signature · 1 line

1.6.5.2.143.g8cc62

Michael S. Tsirkin

author maintainer

2009-11-04 15:57 UTC #5 lore raw

PATCH v8 3/3 vhost_net: a kernel-level virtio server

In reply to a message not in the archive

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, bug work-arounds in userspace)
- write logging is supported (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 backends 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 tap device. Backend is also configured by userspace, including vlan/mac etc.

Status: This works for me, and I haven't see any crashes. Compared to userspace, people reported improved latency (as I save up to 4 system calls per packet), as well as better bandwidth and CPU utilization.

Features that I plan to look at in the future:
- mergeable buffers
- zero copy
- scalability tuning: figure out the best threading model to use

Note on RCU usage (this is also documented in vhost.h, near private_pointer which is the value protected by this variant of RCU):
what is happening is that the rcu_dereference() is being used in a workqueue item. The role of rcu_read_lock() is taken on by the start of execution of the workqueue item, of rcu_read_unlock() by the end of execution of the workqueue item, and of synchronize_rcu() by flush_workqueue()/flush_work().

Acked-by Arnd Bergmann <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Paul, I'm Cc you so that you can verify that nothing significant changed in vhost/net.c since you approved it the previous time. Could you please Ack use of rcu_dereference/rcu_assign_pointer there? Eric Dumazet proposed that I open-code these, he suggested that this is an abuse of these macros and not enough like RCU. What is your opinion?

 MAINTAINERS                |    9 +
 arch/x86/kvm/Kconfig       |    1 +
 drivers/Makefile           |    1 +
 drivers/vhost/Kconfig      |   11 +
 drivers/vhost/Makefile     |    2 +
 drivers/vhost/net.c        |  633 +++++++++++++++++++++++++++++
 drivers/vhost/vhost.c      |  970 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost.h      |  158 +++++++
 include/linux/Kbuild       |    1 +
 include/linux/miscdevice.h |    1 +
 include/linux/vhost.h      |  126 ++++++
 11 files changed, 1913 insertions(+), 0 deletions(-)
 create mode 100644 drivers/vhost/Kconfig
 create mode 100644 drivers/vhost/Makefile
 create mode 100644 drivers/vhost/net.c
 create mode 100644 drivers/vhost/vhost.c
 create mode 100644 drivers/vhost/vhost.h
 create mode 100644 include/linux/vhost.h

MAINTAINERS +9 -0

index 8824115..980a69b 100644
@@ -5619,6 +5619,15 @@ S:	Maintained
 F:	Documentation/filesystems/vfat.txt
 F:	fs/fat/
 
+VIRTIO HOST (VHOST)
+M:	"Michael S. Tsirkin" <[email protected]>
+L:	[email protected]
+L:	[email protected]
+L:	[email protected]
+S:	Maintained
+F:	drivers/vhost/
+F:	include/linux/vhost.h
+
 VIA RHINE NETWORK DRIVER
 M:	Roger Luethi <[email protected]>
 S:	Maintained

arch/x86/kvm/Kconfig +1 -0

index b84e571..94f44d9 100644
@@ -64,6 +64,7 @@ config KVM_AMD
 
 # OK, it's a little counter-intuitive to do this, but it puts it neatly under
 # the virtualization menu.
+source drivers/vhost/Kconfig
 source drivers/lguest/Kconfig
 source drivers/virtio/Kconfig

drivers/Makefile +1 -0

index 6ee53c7..81e3659 100644
@@ -106,6 +106,7 @@ obj-$(CONFIG_HID)		+= hid/
 obj-$(CONFIG_PPC_PS3)		+= ps3/
 obj-$(CONFIG_OF)		+= of/
 obj-$(CONFIG_SSB)		+= ssb/
+obj-$(CONFIG_VHOST_NET)		+= vhost/
 obj-$(CONFIG_VIRTIO)		+= virtio/
 obj-$(CONFIG_VLYNQ)		+= vlynq/
 obj-$(CONFIG_STAGING)		+= staging/

drivers/vhost/Kconfig new file +11 -0

new file mode 100644
index 0000000..9f409f4
@@ -0,0 +1,11 @@
+config VHOST_NET
+	tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
+	depends on NET && EVENTFD && EXPERIMENTAL
+	---help---
+	  This kernel module can be loaded in host kernel to accelerate
+	  guest networking with virtio_net. Not to be confused with virtio_net
+	  module itself which needs to be loaded in guest kernel.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called vhost_net.
+

drivers/vhost/Makefile new file +2 -0

new file mode 100644
index 0000000..72dd020
@@ -0,0 +1,2 @@
+obj-$(CONFIG_VHOST_NET) += vhost_net.o
+vhost_net-y := vhost.o net.o

drivers/vhost/net.c new file +633 -0

633 lines not on this page · raw message

drivers/vhost/vhost.c new file +970 -0

970 lines not on this page · raw message

drivers/vhost/vhost.h new file +158 -0

158 lines not on this page · raw message

include/linux/Kbuild +1 -0

index 3f384d4..b6335c6 100644
@@ -361,6 +361,7 @@ unifdef-y += uio.h
 unifdef-y += unistd.h
 unifdef-y += usbdevice_fs.h
 unifdef-y += utsname.h
+unifdef-y += vhost.h
 unifdef-y += videodev2.h
 unifdef-y += videodev.h
 unifdef-y += virtio_config.h

include/linux/miscdevice.h +1 -0

index adaf3c1..8b5f7cc 100644
@@ -30,6 +30,7 @@
 #define HPET_MINOR		228
 #define FUSE_MINOR		229
 #define KVM_MINOR		232
+#define VHOST_NET_MINOR		233
 #define MISC_DYNAMIC_MINOR	255
 
 struct device;

include/linux/vhost.h new file +126 -0

126 lines not on this page · raw message

Signature · 7 lines

1.6.5.2.143.g8cc62

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Rusty Russell

2009-11-06 04:59 UTC #6 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

Quoted 2 lines · Michael S. Tsirkin

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

Hi Michael,

Now everyone else has finally kicked all the tires and it seems to pass,
I've done a fairly complete review. Generally, it's really nice; just one bug and a few minor suggestions for polishing.

drivers/vhost/net.c line 80

Quoted 5 lines

drivers/vhost/net.c new file +5 -0

+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+	if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+		return;

likely? Really?

drivers/vhost/net.c line 129

Quoted 3 lines

drivers/vhost/net.c new file +3 -0

+	for (;;) {
+		head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
+					 NULL, NULL);

Danger! You need an arg to vhost_get_vq_desc to tell it the max desc size you can handle. Otherwise, it's only limited by ring size, and a malicious guest can overflow you here, and below:

Quoted 2 lines

drivers/vhost/net.c new file +2 -0

+		/* Skip header. TODO: support TSO. */
+		s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);

...

drivers/vhost/net.c line 121

Quoted 4 lines

drivers/vhost/net.c new file +4 -0

+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);

I prefer a name like "vhost_disable_notify()".

drivers/vhost/net.c line 228

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		/* OK, now we need to know about added descriptors. */
+		if (head == vq->num && vhost_notify(vq))
+			/* They could have slipped one in as we were doing that:
+			 * check again. */
+			continue;
+		/* Nothing new?  Wait for eventfd to tell us they refilled. */
+		if (head == vq->num)
+			break;
+		/* We don't need to be notified again. */
+		vhost_no_notify(vq);

Similarly, vhost_enable_notify. This one is particularly misleading since it doesn't actually notify anything!

In particular, this code would be neater as:

	if (head == vq->num) {
		if (vhost_enable_notify(vq)) {
			/* Try again, they could have slipped one in. */
			continue;
		}
		/* Nothing more to do. */
		break;
	}
	vhost_disable_notify(vq);

Now, AFAICT vhost_notify()/enable_notify() would be better rewritten to return true only when there's more pending. Saves a loop around here most of the time. Also, the vhost_no_notify/vhost_disable_notify() can be moved out of the loop entirely. (It could be under an if (unlikely(enabled)), not sure if it's worth it).

drivers/vhost/net.c line 261

Quoted 2 lines

drivers/vhost/net.c new file +2 -0

+		len = err;
+		err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);

That unsigned char * arg to memcpy_toiovec is annoying. A patch might be nice, separate from this effort.

drivers/vhost/net.c line 322

Quoted 9 lines

drivers/vhost/net.c new file +9 -0

+static int vhost_net_open(struct inode *inode, struct file *f)
+{
+	struct vhost_net *n = kzalloc(sizeof *n, GFP_KERNEL);
+	int r;
+	if (!n)
+		return -ENOMEM;
+	f->private_data = n;
+	n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
+	n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;

I have a personal dislike of calloc for structures. In userspace, it's because valgrind can't spot uninitialized fields. These days a similar argument applies in the kernel, because we have KMEMCHECK now. If someone adds a field to the struct and forgets to initialize it, we can spot it.

drivers/vhost/net.c line 348

Quoted 3 lines

drivers/vhost/net.c new file +3 -0

+static void vhost_net_enable_vq(struct vhost_net *n, int index)
+{
+	struct socket *sock = n->vqs[index].private_data;

OK, I can't help but this that presenting the vqs as an array doesn't buy us very much. Esp. if you change vhost_dev_init to take a NULL-terminated varargs. I think readability would improve. It means passing a vq around rather than an index.

Not completely sure it'll be a win tho.

Quoted 3 lines

drivers/vhost/net.c new file +3 -0

+static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
+{
+	struct socket *sock, *oldsock = NULL;

...

Quoted 8 lines

drivers/vhost/net.c new file +8 -0

+	sock = get_socket(fd);
+	if (IS_ERR(sock)) {
+		r = PTR_ERR(sock);
+		goto done;
+	}
+
+	/* start polling new socket */
+	oldsock = vq->private_data;

...

drivers/vhost/net.c line 503

Quoted 5 lines

drivers/vhost/net.c new file +5 -0

+done:
+	mutex_unlock(&n->dev.mutex);
+	if (oldsock) {
+		vhost_net_flush_vq(n, index);
+		fput(oldsock->file);

I dislike this style; I prefer multiple different goto points, one for when oldsock is set, and one for when it's not.

That way, gcc warns us about uninitialized variables if we get it wrong.

drivers/vhost/net.c line 512

Quoted 5 lines

drivers/vhost/net.c new file +5 -0

+static long vhost_net_reset_owner(struct vhost_net *n)
+{
+	struct socket *tx_sock = NULL;
+	struct socket *rx_sock = NULL;
+	long r;

This should be called "err", since that's what it is.

drivers/vhost/net.c line 535

Quoted 7 lines

drivers/vhost/net.c new file +7 -0

+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+		sizeof(struct virtio_net_hdr) : 0;
+	int i;
+	mutex_lock(&n->dev.mutex);
+	n->dev.acked_features = features;

Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

drivers/vhost/net.c line 571

Quoted 11 lines

drivers/vhost/net.c new file +11 -0

+	case VHOST_GET_FEATURES:
+		features = VHOST_FEATURES;
+		return put_user(features, featurep);
+	case VHOST_ACK_FEATURES:
+		r = get_user(features, featurep);
+		/* No features for now */
+		if (r < 0)
+			return r;
+		if (features & ~VHOST_FEATURES)
+			return -EOPNOTSUPP;
+		vhost_net_set_features(n, features);

OK, from the userspace POV it's "get features" then "ack features". But I think "VHOST_SET_FEATURES" is more consistent, despite this usage.

drivers/vhost/vhost.c line 264

Quoted 2 lines

drivers/vhost/vhost.c new file +2 -0
+	switch (ioctl) {
+	case VHOST_SET_VRING_NUM:

I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here, but not sure if it'd be simpler to use?

(Not the fd-setting ioctls of course)

Quoted 9 lines

drivers/vhost/vhost.c new file +9 -0

+	case VHOST_SET_VRING_LOG:
+		r = copy_from_user(&a, argp, sizeof a);
+		if (r < 0)
+			break;
+		if (a.padding) {
+			r = -EOPNOTSUPP;
+			break;
+		}
+		if (a.user_addr == VHOST_VRING_LOG_DISABLE) {

drivers/vhost/vhost.c line 362

Quoted 10 lines

drivers/vhost/vhost.c new file +10 -0

+			vq->log_used = false;
+			break;
+		}
+		if (a.user_addr & (sizeof *vq->used->ring - 1)) {
+			r = -EINVAL;
+			break;
+		}
+		vq->log_used = true;
+		vq->log_addr = a.user_addr;
+		break;

For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.

drivers/vhost/vhost.c line 525

Quoted 6 lines

drivers/vhost/vhost.c new file +6 -0

+/* TODO: This is really inefficient.  We need something like get_user()
+ * (instruction directly accesses the data, with an exception table entry
+ * returning -EFAULT). See Documentation/x86/exception-tables.txt.
+ */
+static int set_bit_to_user(int nr, void __user *addr)
+{

I guess we won't be dealing with many contiguous pages, otherwise we could get a cheap speedup making this set_bits_to_user(int nr, int num_bits...).

drivers/vhost/vhost.c line 635

Quoted 10 lines

drivers/vhost/vhost.c new file +10 -0

+/* Each buffer in the virtqueues is actually a chain of descriptors.  This
+ * function returns the next descriptor in the chain,
+ * or -1 if we're at the end. */
+static unsigned next_desc(struct vring_desc *desc)
+{
+	unsigned int next;
+
+	/* If this descriptor says it doesn't chain, we're done. */
+	if (!(desc->flags & VRING_DESC_F_NEXT))
+		return -1;

Hmm, prefer s/-1/-1U/ in comment, here, and below. Clarifies a bit.

Quoted 2 lines

+/* After we've used one of their buffers, we tell them about it. We'll then
+ * want to send them an interrupt, using vq->call. */

This comment has too much cut & paste:
... want to notify the guest, using the eventfd */

drivers/vhost/vhost.c line 904

Quoted 3 lines

drivers/vhost/vhost.c new file +3 -0

+/* This actually sends the interrupt for this virtqueue */
+void vhost_trigger_irq(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{

Rename vhost_notify_eventfd() or something, and fix comments?

drivers/vhost/vhost.h line 19

Quoted 2 lines

drivers/vhost/vhost.h new file +2 -0
+enum {
+	VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,

+2? Believable, but is it correct?

drivers/vhost/vhost.h line 24

Quoted 3 lines

drivers/vhost/vhost.h new file +3 -0

+/* Poll a file (eventfd or socket) */
+/* Note: there's nothing vhost specific about this structure. */
+struct vhost_poll {

This comment really helped while reading the code. Kudos!

Thanks!
Rusty.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michael S. Tsirkin

author maintainer

2009-11-08 11:35 UTC #7 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

Quoted 9 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin
What it is: vhost net is a character device that can be used to reduce the number of system calls involved in virtio networking.
Hi Michael,
Now everyone else has finally kicked all the tires and it seems to pass,
I've done a fairly complete review. Generally, it's really nice; just one bug and a few minor suggestions for polishing.

Thanks for the review! I'll add more polishing and repost. Answers to some questions below.

Quoted 7 lines

drivers/vhost/net.c line 80
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+	if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+		return;
likely? Really?

Hmm ... yes. tx poll stop is called on each packet (as long as we do not fill up 1/2 backend queue), the first call will stop polling the rest checks state and does nothing.

This is because we normally do not care when the message has left the queue in backend device: we tell backend to send it and forget. We only start polling when backend tx queue fills up.

Makes sense?

Quoted 7 lines

drivers/vhost/net.c line 129
Quoted 3 lines
drivers/vhost/net.c new file +3 -0
+	for (;;) {
+		head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
+					 NULL, NULL);
Danger! You need an arg to vhost_get_vq_desc to tell it the max desc size you can handle. Otherwise, it's only limited by ring size, and a malicious guest can overflow you here, and below:

In fact, I think this is not a bug. This happens to work correctly (even with malicious guests) because vhost_get_vq_desc is hard-coded to check VHOST_NET_MAX_SG, so in fact no overflow is possible. I agree that it's mich nicer to pass iovec size to vhost_get_vq_desc.

Quoted 10 lines

Quoted 3 lines
drivers/vhost/net.c new file +2 -0
+		/* Skip header. TODO: support TSO. */
+		s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
...
drivers/vhost/net.c line 121
Quoted 4 lines
drivers/vhost/net.c new file +4 -0
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
I prefer a name like "vhost_disable_notify()".

Good idea.

Quoted 5 lines

drivers/vhost/net.c new file +5 -0

+		/* OK, now we need to know about added descriptors. */
+		if (head == vq->num && vhost_notify(vq))
+			/* They could have slipped one in as we were doing that:
+			 * check again. */
+			continue;

Quoted 8 lines

drivers/vhost/net.c line 228
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+		/* Nothing new?  Wait for eventfd to tell us they refilled. */
+		if (head == vq->num)
+			break;
+		/* We don't need to be notified again. */
+		vhost_no_notify(vq);
Similarly, vhost_enable_notify. This one is particularly misleading since it doesn't actually notify anything!

Good idea.

Quoted 13 lines

In particular, this code would be neater as:

	if (head == vq->num) {
		if (vhost_enable_notify(vq)) {
			/* Try again, they could have slipped one in. */
			continue;
		}
		/* Nothing more to do. */
		break;
	}
	vhost_disable_notify(vq);

Quoted 3 lines

Now, AFAICT vhost_notify()/enable_notify() would be better rewritten to return true only when there's more pending. Saves a loop around here most of the time.

OKay, I'll look into this. It kind of annoys me that we would do get_user for the same value twice: once in vhost_enable_notify and once in vhost_get_vq_desc. OTOH, all the loop does is call vhost_get_vq_desc again.

Quoted 2 lines

Also, the vhost_no_notify/vhost_disable_notify() can be moved
out of the loop entirely.

I don't think it can, if we enabled notification and then see more descriptors in queue, we want to disable notification again. But it can be

Quoted 2 lines

(It could be under an if (unlikely(enabled)), not
sure if it's worth it).

if (unlikely(vhost_enable_notify(vq))) {
/* Try again, they have slipped one in. */ vhost_disable_notify(vq); continue;
}

Quoted 6 lines

drivers/vhost/net.c line 261
Quoted 3 lines
drivers/vhost/net.c new file +2 -0
+		len = err;
+		err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
That unsigned char * arg to memcpy_toiovec is annoying. A patch might be nice, separate from this effort.

Sounds good.

Quoted 11 lines

drivers/vhost/net.c line 322

Quoted 9 lines

drivers/vhost/net.c new file +9 -0

+static int vhost_net_open(struct inode *inode, struct file *f)
+{
+	struct vhost_net *n = kzalloc(sizeof *n, GFP_KERNEL);
+	int r;
+	if (!n)
+		return -ENOMEM;
+	f->private_data = n;
+	n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
+	n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;

I have a personal dislike of calloc for structures.

You mean zalloc?

Quoted 4 lines

In userspace, it's because valgrind can't spot uninitialized fields. These days a similar argument applies in the kernel, because we have KMEMCHECK now. If someone adds a field to the struct and forgets to initialize it, we can spot it.

OK.

Quoted 10 lines

drivers/vhost/net.c line 348
Quoted 3 lines
drivers/vhost/net.c new file +3 -0
+static void vhost_net_enable_vq(struct vhost_net *n, int index)
+{
+	struct socket *sock = n->vqs[index].private_data;
OK, I can't help but this that presenting the vqs as an array doesn't buy us very much. Esp. if you change vhost_dev_init to take a NULL-terminated varargs. I think readability would improve. It means passing a vq around rather than an index.
Not completely sure it'll be a win tho.

Hmm, varargs sounds a bit complex. But I agree readability for vhost_net_enable_vq and friends would benefit from passing a vq around rather than an index. I'll try it out and do it if it's a win.

Quoted 22 lines

Quoted 3 lines
drivers/vhost/net.c new file +3 -0
+static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
+{
+	struct socket *sock, *oldsock = NULL;
...
Quoted 8 lines
drivers/vhost/net.c new file +8 -0
+	sock = get_socket(fd);
+	if (IS_ERR(sock)) {
+		r = PTR_ERR(sock);
+		goto done;
+	}
+
+	/* start polling new socket */
+	oldsock = vq->private_data;
...
drivers/vhost/net.c line 503
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+done:
+	mutex_unlock(&n->dev.mutex);
+	if (oldsock) {
+		vhost_net_flush_vq(n, index);
+		fput(oldsock->file);
I dislike this style; I prefer multiple different goto points, one for when oldsock is set, and one for when it's not.

Quoted 1 line

That way, gcc warns us about uninitialized variables if we get it wrong.

OK.

Quoted 7 lines

drivers/vhost/net.c line 512
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+static long vhost_net_reset_owner(struct vhost_net *n)
+{
+	struct socket *tx_sock = NULL;
+	struct socket *rx_sock = NULL;
+	long r;
This should be called "err", since that's what it is.

OK.

Quoted 10 lines

drivers/vhost/net.c line 535
Quoted 7 lines
drivers/vhost/net.c new file +7 -0
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+		sizeof(struct virtio_net_hdr) : 0;
+	int i;
+	mutex_lock(&n->dev.mutex);
+	n->dev.acked_features = features;
Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

Not sure how do you mean. Userspace sets them, why does it want to get them exposed back?

Quoted 6 lines

drivers/vhost/net.c new file +6 -0

+	case VHOST_GET_FEATURES:
+		features = VHOST_FEATURES;
+		return put_user(features, featurep);
+	case VHOST_ACK_FEATURES:
+		r = get_user(features, featurep);
+		/* No features for now */

Quoted 8 lines

drivers/vhost/net.c line 571
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+		if (r < 0)
+			return r;
+		if (features & ~VHOST_FEATURES)
+			return -EOPNOTSUPP;
+		vhost_net_set_features(n, features);
OK, from the userspace POV it's "get features" then "ack features". But I think "VHOST_SET_FEATURES" is more consistent, despite this usage.

OK.

Quoted 6 lines

drivers/vhost/vhost.c line 264
Quoted 2 lines
drivers/vhost/vhost.c new file +2 -0
+	switch (ioctl) {
+	case VHOST_SET_VRING_NUM:
I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here,

Not by much though, right?

Quoted 1 line

but not sure if it'd be simpler to use?

The problem is with VHOST_SET_VRING_BASE as well. I want it to be separate because I want to make it possible to relocate e.g. used ring to another address while ring is running. This would be a good debugging tool (you look at kernel's used ring, check descriptor, then update guest's used ring) and also possibly an extra way to do migration. And it's nicer to have vring size separate as well, because it is initialized by host and never changed, right?

We could merge DESC, AVAIL, USED, and it will reduce the amount of code in userspace. With both base, size and fds separate, it seemed a bit more symmetrical to have desc/avail/used separate as well. What's your opinion?

Quoted 17 lines

(Not the fd-setting ioctls of course)

Quoted 15 lines

drivers/vhost/vhost.c new file +15 -0

+	case VHOST_SET_VRING_LOG:
+		r = copy_from_user(&a, argp, sizeof a);
+		if (r < 0)
+			break;
+		if (a.padding) {
+			r = -EOPNOTSUPP;
+			break;
+		}
+		if (a.user_addr == VHOST_VRING_LOG_DISABLE) {
+			vq->log_used = false;
+			break;
+		}
+		if (a.user_addr & (sizeof *vq->used->ring - 1)) {
+			r = -EINVAL;
+			break;

Quoted 8 lines

drivers/vhost/vhost.c line 362
Quoted 4 lines
drivers/vhost/vhost.c new file +4 -0
+		}
+		vq->log_used = true;
+		vq->log_addr = a.user_addr;
+		break;
For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.

Yes. It's not too late to split it out though: should I do it yet?

Quoted 9 lines

drivers/vhost/vhost.c line 525
Quoted 6 lines
drivers/vhost/vhost.c new file +6 -0
+/* TODO: This is really inefficient.  We need something like get_user()
+ * (instruction directly accesses the data, with an exception table entry
+ * returning -EFAULT). See Documentation/x86/exception-tables.txt.
+ */
+static int set_bit_to_user(int nr, void __user *addr)
+{
I guess we won't be dealing with many contiguous pages, otherwise we could get a cheap speedup making this set_bits_to_user(int nr, int num_bits...).

No idea. Let's keep it as simple as possible for now?

Quoted 4 lines

drivers/vhost/vhost.c new file +4 -0

+/* Each buffer in the virtqueues is actually a chain of descriptors.  This
+ * function returns the next descriptor in the chain,
+ * or -1 if we're at the end. */
+static unsigned next_desc(struct vring_desc *desc)

Quoted 8 lines

drivers/vhost/vhost.c line 635
Quoted 6 lines
drivers/vhost/vhost.c new file +6 -0
+{
+	unsigned int next;
+
+	/* If this descriptor says it doesn't chain, we're done. */
+	if (!(desc->flags & VRING_DESC_F_NEXT))
+		return -1;
Hmm, prefer s/-1/-1U/ in comment, here, and below. Clarifies a bit.

Good idea.

Quoted 4 lines

Quoted 2 lines
+/* After we've used one of their buffers, we tell them about it. We'll then
+ * want to send them an interrupt, using vq->call. */
This comment has too much cut & paste:

I tried to cut and paste as many comments as possible, this made it easy to audit the code by comparing it with lguest, and made them much more witty. But yes, this is definitely going overboard as we do not have vq->call at all :)

Quoted 7 lines

... want to notify the guest, using the eventfd */
drivers/vhost/vhost.c line 904
Quoted 3 lines
drivers/vhost/vhost.c new file +3 -0
+/* This actually sends the interrupt for this virtqueue */
+void vhost_trigger_irq(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{
Rename vhost_notify_eventfd() or something, and fix comments?

Sounds good. Since I'm renaming vhost_notify to vhost_enable_notify, this one can just become vhost_notify.

Quoted 4 lines

drivers/vhost/vhost.h line 19
Quoted 2 lines
drivers/vhost/vhost.h new file +2 -0
+enum {
+	VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
+2? Believable, but is it correct?

+ 1 is for skb head, + 1 is for virtio net header. I'll add a comment.

Quoted 8 lines

drivers/vhost/vhost.h line 24
Quoted 3 lines
drivers/vhost/vhost.h new file +3 -0
+/* Poll a file (eventfd or socket) */
+/* Note: there's nothing vhost specific about this structure. */
+struct vhost_poll {
This comment really helped while reading the code. Kudos!
Thanks!
Rusty.

Rusty Russell

2009-11-09 06:17 UTC #8 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

Quoted 9 lines · Michael S. Tsirkin

Quoted 7 lines · Rusty Russell
Review
Quoted 5 lines
(quoted diff) +5 -0
+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+	if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+		return;
likely? Really?

Quoted 7 lines

Hmm ... yes. tx poll stop is called on each packet (as long as we do not fill up 1/2 backend queue), the first call will stop polling the rest checks state and does nothing.
This is because we normally do not care when the message has left the queue in backend device: we tell backend to send it and forget. We only start polling when backend tx queue fills up.

OK, good.

Quoted 11 lines

Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +7 -0
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+		sizeof(struct virtio_net_hdr) : 0;
+	int i;
+	mutex_lock(&n->dev.mutex);
+	n->dev.acked_features = features;
Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

Quoted 2 lines

Not sure how do you mean. Userspace sets them, why does it want to get them exposed back?

There's something about the 'acked' which rubs me the wrong way. "enabled_features" is perhaps a better term than "acked_features"; "acked" seems more a user point-of-view, "enabled" seems more driver POV?

set_features matches your ioctl names, but it sounds like a fn name :(

It's marginal. And 'features' is shorter than both.

Quoted 11 lines

Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	switch (ioctl) {
+	case VHOST_SET_VRING_NUM:
I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here,
Not by much though, right?
Quoted 1 line
but not sure if it'd be simpler to use?

Quoted 7 lines

The problem is with VHOST_SET_VRING_BASE as well. I want it to be separate because I want to make it possible to relocate e.g. used ring to another address while ring is running. This would be a good debugging tool (you look at kernel's used ring, check descriptor, then update guest's used ring) and also possibly an extra way to do migration. And it's nicer to have vring size separate as well, because it is initialized by host and never changed, right?

Actually, this looks wrong to me:

+ case VHOST_SET_VRING_BASE:
...
+ vq->avail_idx = vq->last_avail_idx = s.num;

The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume. The only reason it's not in the ring itself is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.

I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.

Quoted 4 lines

We could merge DESC, AVAIL, USED, and it will reduce the amount of code in userspace. With both base, size and fds separate, it seemed a bit more symmetrical to have desc/avail/used separate as well. What's your opinion?

Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

Quoted 5 lines

Quoted 3 lines
For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.
Yes. It's not too late to split it out though: should I do it yet?

Only if you're feeling enthused. It's lightly reviewed now.

Cheers,
Rusty.

virtio: put last_used and last_avail index into ring itself.

Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.

Fortunately, we have room to expand: the ring is always a whole number of pages and there's hundreds of bytes of padding after the avail ring and the used ring, whatever the number of descriptors (which must be a power of 2).

We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   23 +++++++++++++++--------
 include/linux/virtio_ring.h  |   12 +++++++++++-
 2 files changed, 26 insertions(+), 9 deletions(-)

drivers/virtio/virtio_ring.c +15 -8

@@ -71,9 +71,6 @@ struct vring_virtqueue
 	/* Number we've added since last sync. */
 	unsigned int num_added;
 
-	/* Last used index we've seen. */
-	u16 last_used_idx;
-
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);

@@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
 
 static inline bool more_used(const struct vring_virtqueue *vq)
 {
-	return vq->last_used_idx != vq->vring.used->idx;
+	return vring_last_used(&vq->vring) != vq->vring.used->idx;
 }
 
 static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct vring_used_elem *u;
 	void *ret;
 	unsigned int i;

@@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
 		return NULL;
 	}
 
-	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
-	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+	u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
+	i = u->id;
+	*len = u->len;
+	/* Make sure we don't reload i after doing checks. */
+	rmb();
 
 	if (unlikely(i >= vq->vring.num)) {
 		BAD_RING(vq, "id %u out of range\n", i);

@@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
 	/* detach_buf clears data, so grab it now. */
 	ret = vq->data[i];
 	detach_buf(vq, i);
-	vq->last_used_idx++;
+	vring_last_used(&vq->vring)++;
+
 	END_USE(vq);
 	return ret;
 }

@@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
 	vq->vq.name = name;
 	vq->notify = notify;
 	vq->broken = false;
-	vq->last_used_idx = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG

@@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
 
+	/* We publish indices whether they offer it or not: if not, it's junk
+	 * space anyway.  But calling this acknowledges the feature. */
+	virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

@@ -443,6 +448,8 @@ void vring_transport_features(struct vir
 		switch (i) {
 		case VIRTIO_RING_F_INDIRECT_DESC:
 			break;
+		case VIRTIO_RING_F_PUBLISH_INDICES:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);

include/linux/virtio_ring.h +11 -1

@@ -29,6 +29,9 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES	29
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {

@@ -87,6 +90,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 last_used_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

@@ -95,6 +99,7 @@ struct vring {
  *	__u16 used_flags;
  *	__u16 used_idx;
  *	struct vring_used_elem used[num];
+ *	__u16 last_avail_idx;
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,

@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
 {
 	return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
 		 + align - 1) & ~(align - 1))
-		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;
 }
 
+/* We publish the last-seen used index at the end of the available ring, and
+ * vice-versa.  These are at the end for backwards compatibility. */
+#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Michael S. Tsirkin

2009-11-09 07:10 UTC #9 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

Quoted 11 lines · Rusty Russell

Quoted 11 lines
Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +7 -0
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+ size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+         sizeof(struct virtio_net_hdr) : 0;
+ int i;
+ mutex_lock(&n->dev.mutex);
+ n->dev.acked_features = features;
Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

Quoted 7 lines

Quoted 2 lines
Not sure how do you mean. Userspace sets them, why does it want to get them exposed back?
There's something about the 'acked' which rubs me the wrong way. "enabled_features" is perhaps a better term than "acked_features"; "acked" seems more a user point-of-view, "enabled" seems more driver POV?

Hmm. Are you happy with the ioctl name? If yes I think being consistent with that is important.

Quoted 3 lines

set_features matches your ioctl names, but it sounds like a fn name :(
It's marginal. And 'features' is shorter than both.

I started with this but I was always getting confused whether this includes all features or just acked features. I'll go with enabled_features.

Quoted 20 lines

Quoted 12 lines
Quoted 7 lines
Quoted 3 lines
+ switch (ioctl) {
+ case VHOST_SET_VRING_NUM:
I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here,
Not by much though, right?
Quoted 1 line
but not sure if it'd be simpler to use?
Quoted 7 lines
The problem is with VHOST_SET_VRING_BASE as well. I want it to be separate because I want to make it possible to relocate e.g. used ring to another address while ring is running. This would be a good debugging tool (you look at kernel's used ring, check descriptor, then update guest's used ring) and also possibly an extra way to do migration. And it's nicer to have vring size separate as well, because it is initialized by host and never changed, right?

Quoted 8 lines

Actually, this looks wrong to me:
+ case VHOST_SET_VRING_BASE:
...
+ vq->avail_idx = vq->last_avail_idx = s.num;
The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume.

Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a cached value for notify on empty, so what this does is clear the value. What exactly do you refer to when you say "this looks wrong"? This could trigger an extra notification if I ever called trigger_irq without get first. As I don't, it in fact has no effect.

Quoted 6 lines

The only reason it's not in the ring itself
is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.

Yes, I remember that patch. There seems to be little point though, at this stage.

Quoted 4 lines

I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.

avail_idx is there for notify on empty: I had this thought that it's better to leave the avail cache line alone when we are triggering interrupt to avoid bouncing it around if guest is updating it meanwhile on another CPU, and I think my testing showed that it helped performance, but could be a mistake. You don't believe this can help?

Quoted 7 lines

Quoted 5 lines
We could merge DESC, AVAIL, USED, and it will reduce the amount of code in userspace. With both base, size and fds separate, it seemed a bit more symmetrical to have desc/avail/used separate as well. What's your opinion?
Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

Will do.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.
Yes. It's not too late to split it out though: should I do it yet?
Only if you're feeling enthused. It's lightly reviewed now.

Not really :) I'll keep this in mind for the future. Thanks!

Quoted 3 lines

Cheers,
Rusty.

Rusty Russell

2009-11-10 01:08 UTC #10 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

Quoted 7 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell
There's something about the 'acked' which rubs me the wrong way. "enabled_features" is perhaps a better term than "acked_features"; "acked" seems more a user point-of-view, "enabled" seems more driver POV?
Hmm. Are you happy with the ioctl name? If yes I think being consistent with that is important.

I think in my original comments I noted that I preferred GET / SET, rather than GET/ACK.

Quoted 6 lines

Quoted 6 lines
Actually, this looks wrong to me:
(quoted diff) +2 -0
+       case VHOST_SET_VRING_BASE:
...
+               vq->avail_idx = vq->last_avail_idx = s.num;

Quoted 6 lines

Quoted 2 lines
The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume.
Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a cached value for notify on empty, so what this does is clear the value.

Ah, you actually refresh it every time anyway. Hmm, could you do my poor brain a favor and either just get_user it in vhost_trigger_irq(), or call it 'cached_avail_idx' or something?

Quoted 9 lines

Quoted 6 lines
The only reason it's not in the ring itself
is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.
Yes, I remember that patch. There seems to be little point though, at this stage.

Well, it avoids this ioctl, by exposing all the state. We may well need it later, to expand the ring in other ways.

Quoted 9 lines

Quoted 3 lines
I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.
avail_idx is there for notify on empty: I had this thought that it's better to leave the avail cache line alone when we are triggering interrupt to avoid bouncing it around if guest is updating it meanwhile on another CPU, and I think my testing showed that it helped performance, but could be a mistake. You don't believe this can help?

I believe it could help, but this is YA case where it would have been nice to have a dumb basic patch and this as a patch on top. But I am going to ask you to re-run that measurement, see if it stacks up (because it's an interesting lesson if it does..)

Thanks!
Rusty.

Rusty Russell

2009-11-10 01:08 UTC #11 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Michael S. Tsirkin

Quoted 7 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell
There's something about the 'acked' which rubs me the wrong way. "enabled_features" is perhaps a better term than "acked_features"; "acked" seems more a user point-of-view, "enabled" seems more driver POV?
Hmm. Are you happy with the ioctl name? If yes I think being consistent with that is important.

I think in my original comments I noted that I preferred GET / SET, rather than GET/ACK.

Quoted 6 lines

Quoted 6 lines
Actually, this looks wrong to me:
(quoted diff) +2 -0
+       case VHOST_SET_VRING_BASE:
...
+               vq->avail_idx = vq->last_avail_idx = s.num;

Quoted 6 lines

Quoted 2 lines
The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume.
Exactly. That's what VHOST_GET/SET_VRING_BASE does. avail_idx is just a cached value for notify on empty, so what this does is clear the value.

Ah, you actually refresh it every time anyway. Hmm, could you do my poor brain a favor and either just get_user it in vhost_trigger_irq(), or call it 'cached_avail_idx' or something?

Quoted 9 lines

Quoted 6 lines
The only reason it's not in the ring itself
is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.
Yes, I remember that patch. There seems to be little point though, at this stage.

Well, it avoids this ioctl, by exposing all the state. We may well need it later, to expand the ring in other ways.

Quoted 9 lines

Quoted 3 lines
I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.
avail_idx is there for notify on empty: I had this thought that it's better to leave the avail cache line alone when we are triggering interrupt to avoid bouncing it around if guest is updating it meanwhile on another CPU, and I think my testing showed that it helped performance, but could be a mistake. You don't believe this can help?

Thanks!
Rusty.

Michael S. Tsirkin

2009-11-09 07:10 UTC #12 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Rusty Russell

Quoted 11 lines · Rusty Russell

Quoted 11 lines
Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +7 -0
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+ size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+         sizeof(struct virtio_net_hdr) : 0;
+ int i;
+ mutex_lock(&n->dev.mutex);
+ n->dev.acked_features = features;
Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

Quoted 7 lines

Quoted 2 lines
Not sure how do you mean. Userspace sets them, why does it want to get them exposed back?
There's something about the 'acked' which rubs me the wrong way. "enabled_features" is perhaps a better term than "acked_features"; "acked" seems more a user point-of-view, "enabled" seems more driver POV?

Hmm. Are you happy with the ioctl name? If yes I think being consistent with that is important.

Quoted 3 lines

set_features matches your ioctl names, but it sounds like a fn name :(
It's marginal. And 'features' is shorter than both.

I started with this but I was always getting confused whether this includes all features or just acked features. I'll go with enabled_features.

Quoted 20 lines

Quoted 12 lines
Quoted 7 lines
Quoted 3 lines
+ switch (ioctl) {
+ case VHOST_SET_VRING_NUM:
I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here,
Not by much though, right?
Quoted 1 line
but not sure if it'd be simpler to use?
Quoted 7 lines
The problem is with VHOST_SET_VRING_BASE as well. I want it to be separate because I want to make it possible to relocate e.g. used ring to another address while ring is running. This would be a good debugging tool (you look at kernel's used ring, check descriptor, then update guest's used ring) and also possibly an extra way to do migration. And it's nicer to have vring size separate as well, because it is initialized by host and never changed, right?

Quoted 8 lines

Actually, this looks wrong to me:
+ case VHOST_SET_VRING_BASE:
...
+ vq->avail_idx = vq->last_avail_idx = s.num;
The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume.

Quoted 6 lines

The only reason it's not in the ring itself
is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.

Yes, I remember that patch. There seems to be little point though, at this stage.

Quoted 4 lines

I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.

Quoted 7 lines

Quoted 5 lines
We could merge DESC, AVAIL, USED, and it will reduce the amount of code in userspace. With both base, size and fds separate, it seemed a bit more symmetrical to have desc/avail/used separate as well. What's your opinion?
Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

Will do.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.
Yes. It's not too late to split it out though: should I do it yet?
Only if you're feeling enthused. It's lightly reviewed now.

Not really :) I'll keep this in mind for the future. Thanks!

Quoted 3 lines

Cheers,
Rusty.

Michael S. Tsirkin

author maintainer

2009-11-09 07:20 UTC #13 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Rusty Russell

Quoted 31 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin
Quoted 7 lines · Rusty Russell
Review
Quoted 5 lines
(quoted diff) +5 -0
+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+	if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+		return;
likely? Really?
Quoted 7 lines
Hmm ... yes. tx poll stop is called on each packet (as long as we do not fill up 1/2 backend queue), the first call will stop polling the rest checks state and does nothing.
This is because we normally do not care when the message has left the queue in backend device: we tell backend to send it and forget. We only start polling when backend tx queue fills up.
OK, good.
Quoted 11 lines
Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +7 -0
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+		sizeof(struct virtio_net_hdr) : 0;
+	int i;
+	mutex_lock(&n->dev.mutex);
+	n->dev.acked_features = features;
Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

Quoted 8 lines

Quoted 2 lines
Not sure how do you mean. Userspace sets them, why does it want to get them exposed back?
There's something about the 'acked' which rubs me the wrong way. "enabled_features" is perhaps a better term than "acked_features"; "acked" seems more a user point-of-view, "enabled" seems more driver POV?
set_features matches your ioctl names, but it sounds like a fn name :(

Hmm. Are you happy with the ioctl name? If yes I think being consistent with that is important.

Quoted 1 line

It's marginal. And 'features' is shorter than both.

I started with this but I was always getting confused whether this includes all features or just acked features. I'll go with enabled_features.

Quoted 25 lines

Quoted 11 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	switch (ioctl) {
+	case VHOST_SET_VRING_NUM:
I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here,
Not by much though, right?
Quoted 1 line
but not sure if it'd be simpler to use?
Quoted 7 lines
The problem is with VHOST_SET_VRING_BASE as well. I want it to be separate because I want to make it possible to relocate e.g. used ring to another address while ring is running. This would be a good debugging tool (you look at kernel's used ring, check descriptor, then update guest's used ring) and also possibly an extra way to do migration. And it's nicer to have vring size separate as well, because it is initialized by host and never changed, right?
Actually, this looks wrong to me:
(quoted diff) +2 -0
+	case VHOST_SET_VRING_BASE:
...
+		vq->avail_idx = vq->last_avail_idx = s.num;

Quoted 2 lines

The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume.

Quoted 6 lines

The only reason it's not in the ring itself
is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.

Yes, I remember that patch. There seems to be little point though, at this stage.

Quoted 4 lines

I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.

Quoted 6 lines

Quoted 4 lines
We could merge DESC, AVAIL, USED, and it will reduce the amount of code in userspace. With both base, size and fds separate, it seemed a bit more symmetrical to have desc/avail/used separate as well. What's your opinion?
Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

OK, I'll do this.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.
Yes. It's not too late to split it out though: should I do it yet?
Only if you're feeling enthused. It's lightly reviewed now.

Not really :) I'll keep this in mind for the future. Thanks!

Quoted 140 lines

Cheers,
Rusty.

virtio: put last_used and last_avail index into ring itself.

We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   23 +++++++++++++++--------
 include/linux/virtio_ring.h  |   12 +++++++++++-
 2 files changed, 26 insertions(+), 9 deletions(-)

drivers/virtio/virtio_ring.c +15 -8

@@ -71,9 +71,6 @@ struct vring_virtqueue
 	/* Number we've added since last sync. */
 	unsigned int num_added;
 
-	/* Last used index we've seen. */
-	u16 last_used_idx;
-
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);

@@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
 
 static inline bool more_used(const struct vring_virtqueue *vq)
 {
-	return vq->last_used_idx != vq->vring.used->idx;
+	return vring_last_used(&vq->vring) != vq->vring.used->idx;
 }
 
 static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct vring_used_elem *u;
 	void *ret;
 	unsigned int i;

@@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
 		return NULL;
 	}
 
-	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
-	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+	u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
+	i = u->id;
+	*len = u->len;
+	/* Make sure we don't reload i after doing checks. */
+	rmb();
 
 	if (unlikely(i >= vq->vring.num)) {
 		BAD_RING(vq, "id %u out of range\n", i);

@@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
 	/* detach_buf clears data, so grab it now. */
 	ret = vq->data[i];
 	detach_buf(vq, i);
-	vq->last_used_idx++;
+	vring_last_used(&vq->vring)++;
+
 	END_USE(vq);
 	return ret;
 }

@@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
 	vq->vq.name = name;
 	vq->notify = notify;
 	vq->broken = false;
-	vq->last_used_idx = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG

@@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
 
+	/* We publish indices whether they offer it or not: if not, it's junk
+	 * space anyway.  But calling this acknowledges the feature. */
+	virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

@@ -443,6 +448,8 @@ void vring_transport_features(struct vir
 		switch (i) {
 		case VIRTIO_RING_F_INDIRECT_DESC:
 			break;
+		case VIRTIO_RING_F_PUBLISH_INDICES:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);

include/linux/virtio_ring.h +6 -1

@@ -29,6 +29,9 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES	29
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {

@@ -87,6 +90,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 last_used_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

@@ -95,6 +99,7 @@ struct vring {
  *	__u16 used_flags;
  *	__u16 used_idx;
  *	struct vring_used_elem used[num];
+ *	__u16 last_avail_idx;
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,

@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
 {
 	return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
 		 + align - 1) & ~(align - 1))
-		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;

include/linux/virtio_ring.h static inline unsigned vring_size(unsign · line 129

Quoted 10 lines

include/linux/virtio_ring.h +5 -0

 }
 
+/* We publish the last-seen used index at the end of the available ring, and
+ * vice-versa.  These are at the end for backwards compatibility. */
+#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Michael S. Tsirkin

author maintainer

2009-11-09 07:20 UTC #14 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Rusty Russell

Quoted 31 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin
Quoted 7 lines · Rusty Russell
Review
Quoted 5 lines
(quoted diff) +5 -0
+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+	if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+		return;
likely? Really?
Quoted 7 lines
Hmm ... yes. tx poll stop is called on each packet (as long as we do not fill up 1/2 backend queue), the first call will stop polling the rest checks state and does nothing.
This is because we normally do not care when the message has left the queue in backend device: we tell backend to send it and forget. We only start polling when backend tx queue fills up.
OK, good.
Quoted 11 lines
Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +7 -0
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+		sizeof(struct virtio_net_hdr) : 0;
+	int i;
+	mutex_lock(&n->dev.mutex);
+	n->dev.acked_features = features;
Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

Quoted 8 lines

Quoted 2 lines
Not sure how do you mean. Userspace sets them, why does it want to get them exposed back?
There's something about the 'acked' which rubs me the wrong way. "enabled_features" is perhaps a better term than "acked_features"; "acked" seems more a user point-of-view, "enabled" seems more driver POV?
set_features matches your ioctl names, but it sounds like a fn name :(

Hmm. Are you happy with the ioctl name? If yes I think being consistent with that is important.

Quoted 1 line

It's marginal. And 'features' is shorter than both.

I started with this but I was always getting confused whether this includes all features or just acked features. I'll go with enabled_features.

Quoted 25 lines

Quoted 11 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	switch (ioctl) {
+	case VHOST_SET_VRING_NUM:
I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here,
Not by much though, right?
Quoted 1 line
but not sure if it'd be simpler to use?
Quoted 7 lines
The problem is with VHOST_SET_VRING_BASE as well. I want it to be separate because I want to make it possible to relocate e.g. used ring to another address while ring is running. This would be a good debugging tool (you look at kernel's used ring, check descriptor, then update guest's used ring) and also possibly an extra way to do migration. And it's nicer to have vring size separate as well, because it is initialized by host and never changed, right?
Actually, this looks wrong to me:
(quoted diff) +2 -0
+	case VHOST_SET_VRING_BASE:
...
+		vq->avail_idx = vq->last_avail_idx = s.num;

Quoted 2 lines

The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume.

Quoted 6 lines

The only reason it's not in the ring itself
is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.

Yes, I remember that patch. There seems to be little point though, at this stage.

Quoted 4 lines

I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.

Quoted 6 lines

Quoted 4 lines
We could merge DESC, AVAIL, USED, and it will reduce the amount of code in userspace. With both base, size and fds separate, it seemed a bit more symmetrical to have desc/avail/used separate as well. What's your opinion?
Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

OK, I'll do this.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.
Yes. It's not too late to split it out though: should I do it yet?
Only if you're feeling enthused. It's lightly reviewed now.

Not really :) I'll keep this in mind for the future. Thanks!

Quoted 150 lines

Cheers,
Rusty.

virtio: put last_used and last_avail index into ring itself.

We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   23 +++++++++++++++--------
 include/linux/virtio_ring.h  |   12 +++++++++++-
 2 files changed, 26 insertions(+), 9 deletions(-)

drivers/virtio/virtio_ring.c +15 -8

@@ -71,9 +71,6 @@ struct vring_virtqueue
 	/* Number we've added since last sync. */
 	unsigned int num_added;
 
-	/* Last used index we've seen. */
-	u16 last_used_idx;
-
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);

@@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
 
 static inline bool more_used(const struct vring_virtqueue *vq)
 {
-	return vq->last_used_idx != vq->vring.used->idx;
+	return vring_last_used(&vq->vring) != vq->vring.used->idx;
 }
 
 static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct vring_used_elem *u;
 	void *ret;
 	unsigned int i;

@@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
 		return NULL;
 	}
 
-	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
-	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+	u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
+	i = u->id;
+	*len = u->len;
+	/* Make sure we don't reload i after doing checks. */
+	rmb();
 
 	if (unlikely(i >= vq->vring.num)) {
 		BAD_RING(vq, "id %u out of range\n", i);

@@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
 	/* detach_buf clears data, so grab it now. */
 	ret = vq->data[i];
 	detach_buf(vq, i);
-	vq->last_used_idx++;
+	vring_last_used(&vq->vring)++;
+
 	END_USE(vq);
 	return ret;
 }

@@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
 	vq->vq.name = name;
 	vq->notify = notify;
 	vq->broken = false;
-	vq->last_used_idx = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG

@@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
 
+	/* We publish indices whether they offer it or not: if not, it's junk
+	 * space anyway.  But calling this acknowledges the feature. */
+	virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

@@ -443,6 +448,8 @@ void vring_transport_features(struct vir
 		switch (i) {
 		case VIRTIO_RING_F_INDIRECT_DESC:
 			break;
+		case VIRTIO_RING_F_PUBLISH_INDICES:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);

include/linux/virtio_ring.h +11 -1

@@ -29,6 +29,9 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES	29
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {

@@ -87,6 +90,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 last_used_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

@@ -95,6 +99,7 @@ struct vring {
  *	__u16 used_flags;
  *	__u16 used_idx;
  *	struct vring_used_elem used[num];
+ *	__u16 last_avail_idx;
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,

@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
 {
 	return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
 		 + align - 1) & ~(align - 1))
-		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;
 }
 
+/* We publish the last-seen used index at the end of the available ring, and
+ * vice-versa.  These are at the end for backwards compatibility. */
+#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Michael S. Tsirkin

author maintainer

2009-11-09 11:55 UTC #15 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Rusty Russell

Quoted 14 lines · Rusty Russell

Actually, this looks wrong to me:
(quoted diff) +2 -0
+	case VHOST_SET_VRING_BASE:
...
+		vq->avail_idx = vq->last_avail_idx = s.num;
The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume. The only reason it's not in the ring itself is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.

Quoted 3 lines

I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.

I remembered another reason for caching head in avail_idx. Basically, avail index could change between when I poll for descriptors and when I want to notify guest.

So we could have:
- poll descriptors until empty
- notify
detects not empty so does not notify

And the way to solve it would be to return flag from notify telling us to restart the polling loop.

But, this will be more code, on data path, than
what happens today where I simply keep state
from descriptor polling and use that to notify.

I also suspect that somehow this race in practice can not create deadlocks ... but I prefer to avoid it, these things are very tricky: if I see an empty ring, and stop processing descriptors, I want to trigger notify on empty.

So if we want to avoid keeping "empty" state, IMO the best way would be to pass a flag to vhost_signal that tells it that ring is empty. Makes sense?

Signature · 7 lines

MST

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michael S. Tsirkin

author maintainer

2009-11-09 11:55 UTC #16 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Rusty Russell

Quoted 14 lines · Rusty Russell

Actually, this looks wrong to me:
(quoted diff) +2 -0
+	case VHOST_SET_VRING_BASE:
...
+		vq->avail_idx = vq->last_avail_idx = s.num;
The last_avail_idx is part of the state of the driver. It needs to be saved and restored over susp/resume. The only reason it's not in the ring itself is because I figured the other side doesn't need to see it (which is true, but missed debugging opportunities as well as man-in-the-middle issues like this one). I had a patch which put this field at the end of the ring, I might resurrect it to avoid this problem. This is backwards compatible with all implementations. See patch at end.

Quoted 3 lines

I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.

I remembered another reason for caching head in avail_idx. Basically, avail index could change between when I poll for descriptors and when I want to notify guest.

So we could have:
- poll descriptors until empty
- notify
detects not empty so does not notify

And the way to solve it would be to return flag from notify telling us to restart the polling loop.

But, this will be more code, on data path, than
what happens today where I simply keep state
from descriptor polling and use that to notify.

So if we want to avoid keeping "empty" state, IMO the best way would be to pass a flag to vhost_signal that tells it that ring is empty. Makes sense?

Signature · 1 line

MST

Michael S. Tsirkin

author maintainer

2010-05-04 18:22 UTC #17 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

In reply to Rusty Russell

Quoted 13 lines

virtio: put last_used and last_avail index into ring itself.
Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.
Fortunately, we have room to expand: the ring is always a whole number of pages and there's hundreds of bytes of padding after the avail ring and the used ring, whatever the number of descriptors (which must be a power of 2).

Quoted 4 lines

We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.
Signed-off-by Rusty Russell <[email protected]>

I've been looking at this patch some more (more on why later), and I wonder: would it be better to add some alignment to the last used index address, so that if we later add more stuff at the tail, it all fits in a single cache line?

We use a new feature bit anyway, so layout change should not be a problem.

Since I raised the question of caches: for used ring, the ring is not aligned to 64 bit, so on CPUs with 64 bit or larger cache lines, used entries will often cross cache line boundaries. Am I right and might it
have been better to align ring entries to cache line boundaries?

What do you think?

Quoted 120 lines

---
 drivers/virtio/virtio_ring.c |   23 +++++++++++++++--------
 include/linux/virtio_ring.h  |   12 +++++++++++-
 2 files changed, 26 insertions(+), 9 deletions(-)

drivers/virtio/virtio_ring.c +15 -8

@@ -71,9 +71,6 @@ struct vring_virtqueue
 	/* Number we've added since last sync. */
 	unsigned int num_added;
 
-	/* Last used index we've seen. */
-	u16 last_used_idx;
-
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);

@@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
 
 static inline bool more_used(const struct vring_virtqueue *vq)
 {
-	return vq->last_used_idx != vq->vring.used->idx;
+	return vring_last_used(&vq->vring) != vq->vring.used->idx;
 }
 
 static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct vring_used_elem *u;
 	void *ret;
 	unsigned int i;

@@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
 		return NULL;
 	}
 
-	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
-	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+	u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
+	i = u->id;
+	*len = u->len;
+	/* Make sure we don't reload i after doing checks. */
+	rmb();
 
 	if (unlikely(i >= vq->vring.num)) {
 		BAD_RING(vq, "id %u out of range\n", i);

@@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
 	/* detach_buf clears data, so grab it now. */
 	ret = vq->data[i];
 	detach_buf(vq, i);
-	vq->last_used_idx++;
+	vring_last_used(&vq->vring)++;
+
 	END_USE(vq);
 	return ret;
 }

@@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
 	vq->vq.name = name;
 	vq->notify = notify;
 	vq->broken = false;
-	vq->last_used_idx = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG

@@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
 
+	/* We publish indices whether they offer it or not: if not, it's junk
+	 * space anyway.  But calling this acknowledges the feature. */
+	virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

@@ -443,6 +448,8 @@ void vring_transport_features(struct vir
 		switch (i) {
 		case VIRTIO_RING_F_INDIRECT_DESC:
 			break;
+		case VIRTIO_RING_F_PUBLISH_INDICES:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);

include/linux/virtio_ring.h +6 -1

@@ -29,6 +29,9 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES	29
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {

@@ -87,6 +90,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 last_used_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

@@ -95,6 +99,7 @@ struct vring {
  *	__u16 used_flags;
  *	__u16 used_idx;
  *	struct vring_used_elem used[num];
+ *	__u16 last_avail_idx;
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,

@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
 {
 	return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
 		 + align - 1) & ~(align - 1))
-		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;

include/linux/virtio_ring.h static inline unsigned vring_size(unsign · line 129

Quoted 10 lines

include/linux/virtio_ring.h +5 -0

 }
 
+/* We publish the last-seen used index at the end of the available ring, and
+ * vice-versa.  These are at the end for backwards compatibility. */
+#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Rusty Russell

2010-05-06 00:52 UTC #18 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

Quoted 16 lines · Michael S. Tsirkin

Quoted 16 lines
virtio: put last_used and last_avail index into ring itself.
Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.
Fortunately, we have room to expand: the ring is always a whole number of pages and there's hundreds of bytes of padding after the avail ring and the used ring, whatever the number of descriptors (which must be a power of 2).
We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.

Quoted 7 lines

Quoted 1 line
Signed-off-by Rusty Russell <[email protected]>
I've been looking at this patch some more (more on why later), and I wonder: would it be better to add some alignment to the last used index address, so that if we later add more stuff at the tail, it all fits in a single cache line?

In theory, but not in practice. We don't have many rings, so the difference between 1 and 2 cache lines is not very much.

Quoted 10 lines

We use a new feature bit anyway, so layout change should not be a problem.
Since I raised the question of caches: for used ring, the ring is not aligned to 64 bit, so on CPUs with 64 bit or larger cache lines, used entries will often cross cache line boundaries. Am I right and might it
have been better to align ring entries to cache line boundaries?
What do you think?

I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue.

Cheers,
Rusty.

Michael S. Tsirkin

author maintainer

2010-05-06 06:27 UTC #19 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

Quoted 19 lines · Rusty Russell

Quoted 18 lines · Michael S. Tsirkin
Quoted 17 lines
virtio: put last_used and last_avail index into ring itself.
Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.
Fortunately, we have room to expand: the ring is always a whole number of pages and there's hundreds of bytes of padding after the avail ring and the used ring, whatever the number of descriptors (which must be a power of 2).
We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.
Signed-off-by Rusty Russell <[email protected]>

Quoted 8 lines

Quoted 5 lines
I've been looking at this patch some more (more on why later), and I wonder: would it be better to add some alignment to the last used index address, so that if we later add more stuff at the tail, it all fits in a single cache line?
In theory, but not in practice. We don't have many rings, so the difference between 1 and 2 cache lines is not very much.

Fair enough.

Quoted 9 lines

Quoted 9 lines
We use a new feature bit anyway, so layout change should not be a problem.
Since I raised the question of caches: for used ring, the ring is not aligned to 64 bit, so on CPUs with 64 bit or larger cache lines, used entries will often cross cache line boundaries. Am I right and might it
have been better to align ring entries to cache line boundaries?

Quoted 7 lines

Quoted 1 line
What do you think?
I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue.
Cheers,
Rusty.

You mean with 64 bit descriptors we will be bouncing a cache line between host and guest, anyway?

Signature · 7 lines

MST

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Rusty Russell

2010-05-07 03:05 UTC #20 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

Quoted 9 lines · Michael S. Tsirkin

Quoted 5 lines · Rusty Russell
Quoted 1 line · Michael S. Tsirkin
What do you think?
I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue.
You mean with 64 bit descriptors we will be bouncing a cache line between host and guest, anyway?

I'm confused by this entire thread.

Descriptors are 16 bytes. They are at the start, so presumably aligned to cache boundaries.

Available ring follows that at 2 bytes per entry, so it's also packed nicely into cachelines.

Then there's padding to page boundary. That puts us on a cacheline again for the used ring; also 2 bytes per entry.

I don't see how any change in layout could be more cache friendly? Rusty.

Michael S. Tsirkin

author maintainer

2010-05-09 08:57 UTC #21 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

Quoted 13 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin
Quoted 5 lines · Rusty Russell
Quoted 1 line · Michael S. Tsirkin
What do you think?
I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue.
You mean with 64 bit descriptors we will be bouncing a cache line between host and guest, anyway?
I'm confused by this entire thread.

Quoted 9 lines

Descriptors are 16 bytes. They are at the start, so presumably aligned to cache boundaries.
Available ring follows that at 2 bytes per entry, so it's also packed nicely into cachelines.
Then there's padding to page boundary. That puts us on a cacheline again for the used ring; also 2 bytes per entry.

Hmm, is used ring really 2 bytes per entry?

/* u32 is used here for ids for padding reasons. */

struct vring_used_elem {
        /* Index of start of used descriptor chain. */
        __u32 id;
        /* Total length of the descriptor chain which was used (written to) */
        __u32 len;
};

struct vring_used {
        __u16 flags;
        __u16 idx;
        struct vring_used_elem ring[];
};

Quoted 2 lines

I don't see how any change in layout could be more cache friendly? Rusty.

I thought that used ring has 8 bytes per entry, and that struct vring_used is aligned at page boundary, this
would mean that ring element is at offset 4 bytes from page boundary. Thus with cacheline size 128 bytes, each 4th element crosses a cacheline boundary. If we had a 4 byte padding after idx, each used element would always be completely within a single cacheline.

What am I missing?

Signature · 7 lines

MST

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Rusty Russell

2010-05-10 03:11 UTC #22 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell
Then there's padding to page boundary. That puts us on a cacheline again for the used ring; also 2 bytes per entry.
Hmm, is used ring really 2 bytes per entry?

Err, no, I am an idiot.

Quoted 8 lines

/* u32 is used here for ids for padding reasons. */

struct vring_used_elem {
        /* Index of start of used descriptor chain. */
        __u32 id;
        /* Total length of the descriptor chain which was used (written to) */
        __u32 len;
};

Quoted 5 lines

struct vring_used {
        __u16 flags;
        __u16 idx;
        struct vring_used_elem ring[];
};

OK, now I get it. Sorry, I was focussed on the avail ring.

Quoted 6 lines

I thought that used ring has 8 bytes per entry, and that struct vring_used is aligned at page boundary, this
would mean that ring element is at offset 4 bytes from page boundary. Thus with cacheline size 128 bytes, each 4th element crosses a cacheline boundary. If we had a 4 byte padding after idx, each used element would always be completely within a single cacheline.

I think the numbers are: every 16th entry hits two cachelines. So currently the first 15 entries are "free" (assuming we hit the idx cacheline anyway), then 1 in 16 cost 2 cachelines. That makes the aligned version win when N > 240.

But, we access the array linearly. So the extra cacheline cost is in fact amortized. I doubt it could be measured, but maybe vring_get_buf() should prefetch? While you're there, we could use an & rather than a mod on the calculation, which may actually be measurable :)

Cheers,
Rusty.

Rusty Russell

2010-05-10 03:11 UTC #23 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

In reply to Michael S. Tsirkin

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines · Rusty Russell
Then there's padding to page boundary. That puts us on a cacheline again for the used ring; also 2 bytes per entry.
Hmm, is used ring really 2 bytes per entry?

Err, no, I am an idiot.

Quoted 8 lines

/* u32 is used here for ids for padding reasons. */

struct vring_used_elem {
        /* Index of start of used descriptor chain. */
        __u32 id;
        /* Total length of the descriptor chain which was used (written to) */
        __u32 len;
};

Quoted 5 lines

struct vring_used {
        __u16 flags;
        __u16 idx;
        struct vring_used_elem ring[];
};

OK, now I get it. Sorry, I was focussed on the avail ring.

Quoted 6 lines

I thought that used ring has 8 bytes per entry, and that struct vring_used is aligned at page boundary, this
would mean that ring element is at offset 4 bytes from page boundary. Thus with cacheline size 128 bytes, each 4th element crosses a cacheline boundary. If we had a 4 byte padding after idx, each used element would always be completely within a single cacheline.

Cheers,
Rusty.

Michael S. Tsirkin

author maintainer

2010-05-09 08:57 UTC #24 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

In reply to Rusty Russell

Quoted 13 lines · Rusty Russell

Quoted 9 lines · Michael S. Tsirkin
Quoted 5 lines · Rusty Russell
Quoted 1 line · Michael S. Tsirkin
What do you think?
I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue.
You mean with 64 bit descriptors we will be bouncing a cache line between host and guest, anyway?
I'm confused by this entire thread.

Quoted 9 lines

Descriptors are 16 bytes. They are at the start, so presumably aligned to cache boundaries.
Available ring follows that at 2 bytes per entry, so it's also packed nicely into cachelines.
Then there's padding to page boundary. That puts us on a cacheline again for the used ring; also 2 bytes per entry.

Hmm, is used ring really 2 bytes per entry?

/* u32 is used here for ids for padding reasons. */

struct vring_used_elem {
        /* Index of start of used descriptor chain. */
        __u32 id;
        /* Total length of the descriptor chain which was used (written to) */
        __u32 len;
};

struct vring_used {
        __u16 flags;
        __u16 idx;
        struct vring_used_elem ring[];
};

Quoted 2 lines

I don't see how any change in layout could be more cache friendly? Rusty.

What am I missing?

Signature · 1 line

MST

Rusty Russell

2010-05-07 03:05 UTC #25 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

Quoted 5 lines · Rusty Russell
Quoted 1 line · Michael S. Tsirkin
What do you think?
I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue.
You mean with 64 bit descriptors we will be bouncing a cache line between host and guest, anyway?

I'm confused by this entire thread.

Descriptors are 16 bytes. They are at the start, so presumably aligned to cache boundaries.

Available ring follows that at 2 bytes per entry, so it's also packed nicely into cachelines.

Then there's padding to page boundary. That puts us on a cacheline again for the used ring; also 2 bytes per entry.

I don't see how any change in layout could be more cache friendly? Rusty.

Michael S. Tsirkin

author maintainer

2010-05-06 06:27 UTC #26 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

In reply to Rusty Russell

Quoted 19 lines · Rusty Russell

Quoted 18 lines · Michael S. Tsirkin
Quoted 17 lines
virtio: put last_used and last_avail index into ring itself.
Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.
Fortunately, we have room to expand: the ring is always a whole number of pages and there's hundreds of bytes of padding after the avail ring and the used ring, whatever the number of descriptors (which must be a power of 2).
We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.
Signed-off-by Rusty Russell <[email protected]>

Quoted 8 lines

Quoted 5 lines
I've been looking at this patch some more (more on why later), and I wonder: would it be better to add some alignment to the last used index address, so that if we later add more stuff at the tail, it all fits in a single cache line?
In theory, but not in practice. We don't have many rings, so the difference between 1 and 2 cache lines is not very much.

Fair enough.

Quoted 9 lines

Quoted 9 lines
We use a new feature bit anyway, so layout change should not be a problem.
Since I raised the question of caches: for used ring, the ring is not aligned to 64 bit, so on CPUs with 64 bit or larger cache lines, used entries will often cross cache line boundaries. Am I right and might it
have been better to align ring entries to cache line boundaries?

Quoted 7 lines

Quoted 1 line
What do you think?
I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue.
Cheers,
Rusty.

You mean with 64 bit descriptors we will be bouncing a cache line between host and guest, anyway?

Signature · 1 line

MST

Rusty Russell

2010-05-06 00:52 UTC #27 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

In reply to Michael S. Tsirkin

Quoted 16 lines · Michael S. Tsirkin

Quoted 16 lines
virtio: put last_used and last_avail index into ring itself.
Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.
Fortunately, we have room to expand: the ring is always a whole number of pages and there's hundreds of bytes of padding after the avail ring and the used ring, whatever the number of descriptors (which must be a power of 2).
We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.

Quoted 7 lines

Quoted 1 line
Signed-off-by Rusty Russell <[email protected]>
I've been looking at this patch some more (more on why later), and I wonder: would it be better to add some alignment to the last used index address, so that if we later add more stuff at the tail, it all fits in a single cache line?

In theory, but not in practice. We don't have many rings, so the difference between 1 and 2 cache lines is not very much.

Quoted 10 lines

We use a new feature bit anyway, so layout change should not be a problem.
Since I raised the question of caches: for used ring, the ring is not aligned to 64 bit, so on CPUs with 64 bit or larger cache lines, used entries will often cross cache line boundaries. Am I right and might it
have been better to align ring entries to cache line boundaries?
What do you think?

I think everyone is settled on 128 byte cache lines for the forseeable future, so it's not really an issue.

Cheers,
Rusty.

Michael S. Tsirkin

author maintainer

2010-05-04 18:22 UTC #28 lore raw

Re: virtio: put last_used and last_avail index into ring itself.

In reply to Rusty Russell

Quoted 13 lines

virtio: put last_used and last_avail index into ring itself.
Generally, the other end of the virtio ring doesn't need to see where you're up to in consuming the ring. However, to completely understand what's going on from the outside, this information must be exposed. For example, if you want to save and restore a virtio_ring, but you're not the consumer because the kernel is using it directly.
Fortunately, we have room to expand: the ring is always a whole number of pages and there's hundreds of bytes of padding after the avail ring and the used ring, whatever the number of descriptors (which must be a power of 2).

Quoted 4 lines

We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.
Signed-off-by Rusty Russell <[email protected]>

We use a new feature bit anyway, so layout change should not be a problem.

What do you think?

Quoted 130 lines

---
 drivers/virtio/virtio_ring.c |   23 +++++++++++++++--------
 include/linux/virtio_ring.h  |   12 +++++++++++-
 2 files changed, 26 insertions(+), 9 deletions(-)

drivers/virtio/virtio_ring.c +15 -8

@@ -71,9 +71,6 @@ struct vring_virtqueue
 	/* Number we've added since last sync. */
 	unsigned int num_added;
 
-	/* Last used index we've seen. */
-	u16 last_used_idx;
-
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);

@@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
 
 static inline bool more_used(const struct vring_virtqueue *vq)
 {
-	return vq->last_used_idx != vq->vring.used->idx;
+	return vring_last_used(&vq->vring) != vq->vring.used->idx;
 }
 
 static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct vring_used_elem *u;
 	void *ret;
 	unsigned int i;

@@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
 		return NULL;
 	}
 
-	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
-	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+	u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
+	i = u->id;
+	*len = u->len;
+	/* Make sure we don't reload i after doing checks. */
+	rmb();
 
 	if (unlikely(i >= vq->vring.num)) {
 		BAD_RING(vq, "id %u out of range\n", i);

@@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
 	/* detach_buf clears data, so grab it now. */
 	ret = vq->data[i];
 	detach_buf(vq, i);
-	vq->last_used_idx++;
+	vring_last_used(&vq->vring)++;
+
 	END_USE(vq);
 	return ret;
 }

@@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
 	vq->vq.name = name;
 	vq->notify = notify;
 	vq->broken = false;
-	vq->last_used_idx = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG

@@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
 
+	/* We publish indices whether they offer it or not: if not, it's junk
+	 * space anyway.  But calling this acknowledges the feature. */
+	virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

@@ -443,6 +448,8 @@ void vring_transport_features(struct vir
 		switch (i) {
 		case VIRTIO_RING_F_INDIRECT_DESC:
 			break;
+		case VIRTIO_RING_F_PUBLISH_INDICES:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);

include/linux/virtio_ring.h +11 -1

@@ -29,6 +29,9 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES	29
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {

@@ -87,6 +90,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 last_used_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

@@ -95,6 +99,7 @@ struct vring {
  *	__u16 used_flags;
  *	__u16 used_idx;
  *	struct vring_used_elem used[num];
+ *	__u16 last_avail_idx;
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,

@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
 {
 	return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
 		 + align - 1) & ~(align - 1))
-		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;
 }
 
+/* We publish the last-seen used index at the end of the available ring, and
+ * vice-versa.  These are at the end for backwards compatibility. */
+#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Rusty Russell

2009-11-09 06:17 UTC #29 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Michael S. Tsirkin

Quoted 9 lines · Michael S. Tsirkin

Quoted 7 lines · Rusty Russell
Review
Quoted 5 lines
(quoted diff) +5 -0
+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+	if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+		return;
likely? Really?

Quoted 7 lines

Hmm ... yes. tx poll stop is called on each packet (as long as we do not fill up 1/2 backend queue), the first call will stop polling the rest checks state and does nothing.
This is because we normally do not care when the message has left the queue in backend device: we tell backend to send it and forget. We only start polling when backend tx queue fills up.

OK, good.

Quoted 11 lines

Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +7 -0
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+		sizeof(struct virtio_net_hdr) : 0;
+	int i;
+	mutex_lock(&n->dev.mutex);
+	n->dev.acked_features = features;
Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

Quoted 2 lines

Not sure how do you mean. Userspace sets them, why does it want to get them exposed back?

set_features matches your ioctl names, but it sounds like a fn name :(

It's marginal. And 'features' is shorter than both.

Quoted 11 lines

Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+	switch (ioctl) {
+	case VHOST_SET_VRING_NUM:
I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here,
Not by much though, right?
Quoted 1 line
but not sure if it'd be simpler to use?

Quoted 7 lines

The problem is with VHOST_SET_VRING_BASE as well. I want it to be separate because I want to make it possible to relocate e.g. used ring to another address while ring is running. This would be a good debugging tool (you look at kernel's used ring, check descriptor, then update guest's used ring) and also possibly an extra way to do migration. And it's nicer to have vring size separate as well, because it is initialized by host and never changed, right?

Actually, this looks wrong to me:

+ case VHOST_SET_VRING_BASE:
...
+ vq->avail_idx = vq->last_avail_idx = s.num;

I would drop avail_idx altogether: get_user is basically free, and simplifies a lot. As most state is in the ring, all you need is an ioctl to save/restore the last_avail_idx.

Quoted 4 lines

We could merge DESC, AVAIL, USED, and it will reduce the amount of code in userspace. With both base, size and fds separate, it seemed a bit more symmetrical to have desc/avail/used separate as well. What's your opinion?

Well, DESC, AVAIL, and USED could easily be turned into SET/GET_LAYOUT.

Quoted 5 lines

Quoted 3 lines
For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.
Yes. It's not too late to split it out though: should I do it yet?

Only if you're feeling enthused. It's lightly reviewed now.

Cheers,
Rusty.

virtio: put last_used and last_avail index into ring itself.

We add a feature bit so the guest can tell the host that it's writing out the current value there, if it wants to use that.

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/virtio/virtio_ring.c |   23 +++++++++++++++--------
 include/linux/virtio_ring.h  |   12 +++++++++++-
 2 files changed, 26 insertions(+), 9 deletions(-)

drivers/virtio/virtio_ring.c +15 -8

@@ -71,9 +71,6 @@ struct vring_virtqueue
 	/* Number we've added since last sync. */
 	unsigned int num_added;
 
-	/* Last used index we've seen. */
-	u16 last_used_idx;
-
 	/* How to notify other side. FIXME: commonalize hcalls! */
 	void (*notify)(struct virtqueue *vq);

@@ -278,12 +275,13 @@ static void detach_buf(struct vring_virt
 
 static inline bool more_used(const struct vring_virtqueue *vq)
 {
-	return vq->last_used_idx != vq->vring.used->idx;
+	return vring_last_used(&vq->vring) != vq->vring.used->idx;
 }
 
 static void *vring_get_buf(struct virtqueue *_vq, unsigned int *len)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
+	struct vring_used_elem *u;
 	void *ret;
 	unsigned int i;

@@ -300,8 +298,11 @@ static void *vring_get_buf(struct virtqu
 		return NULL;
 	}
 
-	i = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].id;
-	*len = vq->vring.used->ring[vq->last_used_idx%vq->vring.num].len;
+	u = &vq->vring.used->ring[vring_last_used(&vq->vring) % vq->vring.num];
+	i = u->id;
+	*len = u->len;
+	/* Make sure we don't reload i after doing checks. */
+	rmb();
 
 	if (unlikely(i >= vq->vring.num)) {
 		BAD_RING(vq, "id %u out of range\n", i);

@@ -315,7 +316,8 @@ static void *vring_get_buf(struct virtqu
 	/* detach_buf clears data, so grab it now. */
 	ret = vq->data[i];
 	detach_buf(vq, i);
-	vq->last_used_idx++;
+	vring_last_used(&vq->vring)++;
+
 	END_USE(vq);
 	return ret;
 }

@@ -402,7 +404,6 @@ struct virtqueue *vring_new_virtqueue(un
 	vq->vq.name = name;
 	vq->notify = notify;
 	vq->broken = false;
-	vq->last_used_idx = 0;
 	vq->num_added = 0;
 	list_add_tail(&vq->vq.list, &vdev->vqs);
 #ifdef DEBUG

@@ -413,6 +414,10 @@ struct virtqueue *vring_new_virtqueue(un
 
 	vq->indirect = virtio_has_feature(vdev, VIRTIO_RING_F_INDIRECT_DESC);
 
+	/* We publish indices whether they offer it or not: if not, it's junk
+	 * space anyway.  But calling this acknowledges the feature. */
+	virtio_has_feature(vdev, VIRTIO_RING_F_PUBLISH_INDICES);
+
 	/* No callback?  Tell other side not to bother us. */
 	if (!callback)
 		vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

@@ -443,6 +448,8 @@ void vring_transport_features(struct vir
 		switch (i) {
 		case VIRTIO_RING_F_INDIRECT_DESC:
 			break;
+		case VIRTIO_RING_F_PUBLISH_INDICES:
+			break;
 		default:
 			/* We don't understand this bit. */
 			clear_bit(i, vdev->features);

include/linux/virtio_ring.h +11 -1

@@ -29,6 +29,9 @@
 /* We support indirect buffer descriptors */
 #define VIRTIO_RING_F_INDIRECT_DESC	28
 
+/* We publish our last-seen used index at the end of the avail ring. */
+#define VIRTIO_RING_F_PUBLISH_INDICES	29
+
 /* Virtio ring descriptors: 16 bytes.  These can chain together via "next". */
 struct vring_desc
 {

@@ -87,6 +90,7 @@ struct vring {
  *	__u16 avail_flags;
  *	__u16 avail_idx;
  *	__u16 available[num];
+ *	__u16 last_used_idx;
  *
  *	// Padding to the next align boundary.
  *	char pad[];

@@ -95,6 +99,7 @@ struct vring {
  *	__u16 used_flags;
  *	__u16 used_idx;
  *	struct vring_used_elem used[num];
+ *	__u16 last_avail_idx;
  * };
  */
 static inline void vring_init(struct vring *vr, unsigned int num, void *p,

@@ -111,9 +116,14 @@ static inline unsigned vring_size(unsign
 {
 	return ((sizeof(struct vring_desc) * num + sizeof(__u16) * (2 + num)
 		 + align - 1) & ~(align - 1))
-		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num;
+		+ sizeof(__u16) * 2 + sizeof(struct vring_used_elem) * num + 2;
 }
 
+/* We publish the last-seen used index at the end of the available ring, and
+ * vice-versa.  These are at the end for backwards compatibility. */
+#define vring_last_used(vr) ((vr)->avail->ring[(vr)->num])
+#define vring_last_avail(vr) (*(__u16 *)&(vr)->used->ring[(vr)->num])
+
 #ifdef __KERNEL__
 #include <linux/irqreturn.h>
 struct virtio_device;

Michael S. Tsirkin

author maintainer

2009-11-08 11:35 UTC #30 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Rusty Russell

Quoted 9 lines · Rusty Russell

Quoted 2 lines · Michael S. Tsirkin
What it is: vhost net is a character device that can be used to reduce the number of system calls involved in virtio networking.
Hi Michael,
Now everyone else has finally kicked all the tires and it seems to pass,
I've done a fairly complete review. Generally, it's really nice; just one bug and a few minor suggestions for polishing.

Thanks for the review! I'll add more polishing and repost. Answers to some questions below.

Quoted 7 lines

drivers/vhost/net.c line 80
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+	if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+		return;
likely? Really?

Hmm ... yes. tx poll stop is called on each packet (as long as we do not fill up 1/2 backend queue), the first call will stop polling the rest checks state and does nothing.

This is because we normally do not care when the message has left the queue in backend device: we tell backend to send it and forget. We only start polling when backend tx queue fills up.

Makes sense?

Quoted 7 lines

drivers/vhost/net.c line 129
Quoted 3 lines
drivers/vhost/net.c new file +3 -0
+	for (;;) {
+		head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
+					 NULL, NULL);
Danger! You need an arg to vhost_get_vq_desc to tell it the max desc size you can handle. Otherwise, it's only limited by ring size, and a malicious guest can overflow you here, and below:

Quoted 10 lines

Quoted 3 lines
drivers/vhost/net.c new file +2 -0
+		/* Skip header. TODO: support TSO. */
+		s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);
...
drivers/vhost/net.c line 121
Quoted 4 lines
drivers/vhost/net.c new file +4 -0
+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);
I prefer a name like "vhost_disable_notify()".

Good idea.

Quoted 5 lines

drivers/vhost/net.c new file +5 -0

+		/* OK, now we need to know about added descriptors. */
+		if (head == vq->num && vhost_notify(vq))
+			/* They could have slipped one in as we were doing that:
+			 * check again. */
+			continue;

Quoted 8 lines

drivers/vhost/net.c line 228
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+		/* Nothing new?  Wait for eventfd to tell us they refilled. */
+		if (head == vq->num)
+			break;
+		/* We don't need to be notified again. */
+		vhost_no_notify(vq);
Similarly, vhost_enable_notify. This one is particularly misleading since it doesn't actually notify anything!

Good idea.

Quoted 13 lines

In particular, this code would be neater as:

	if (head == vq->num) {
		if (vhost_enable_notify(vq)) {
			/* Try again, they could have slipped one in. */
			continue;
		}
		/* Nothing more to do. */
		break;
	}
	vhost_disable_notify(vq);

Quoted 3 lines

Now, AFAICT vhost_notify()/enable_notify() would be better rewritten to return true only when there's more pending. Saves a loop around here most of the time.

Quoted 2 lines

Also, the vhost_no_notify/vhost_disable_notify() can be moved
out of the loop entirely.

I don't think it can, if we enabled notification and then see more descriptors in queue, we want to disable notification again. But it can be

Quoted 2 lines

(It could be under an if (unlikely(enabled)), not
sure if it's worth it).

if (unlikely(vhost_enable_notify(vq))) {
/* Try again, they have slipped one in. */ vhost_disable_notify(vq); continue;
}

Quoted 6 lines

drivers/vhost/net.c line 261
Quoted 3 lines
drivers/vhost/net.c new file +2 -0
+		len = err;
+		err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);
That unsigned char * arg to memcpy_toiovec is annoying. A patch might be nice, separate from this effort.

Sounds good.

Quoted 11 lines

drivers/vhost/net.c line 322

Quoted 9 lines

drivers/vhost/net.c new file +9 -0

+static int vhost_net_open(struct inode *inode, struct file *f)
+{
+	struct vhost_net *n = kzalloc(sizeof *n, GFP_KERNEL);
+	int r;
+	if (!n)
+		return -ENOMEM;
+	f->private_data = n;
+	n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
+	n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;

I have a personal dislike of calloc for structures.

You mean zalloc?

Quoted 4 lines

In userspace, it's because valgrind can't spot uninitialized fields. These days a similar argument applies in the kernel, because we have KMEMCHECK now. If someone adds a field to the struct and forgets to initialize it, we can spot it.

OK.

Quoted 10 lines

drivers/vhost/net.c line 348
Quoted 3 lines
drivers/vhost/net.c new file +3 -0
+static void vhost_net_enable_vq(struct vhost_net *n, int index)
+{
+	struct socket *sock = n->vqs[index].private_data;
OK, I can't help but this that presenting the vqs as an array doesn't buy us very much. Esp. if you change vhost_dev_init to take a NULL-terminated varargs. I think readability would improve. It means passing a vq around rather than an index.
Not completely sure it'll be a win tho.

Hmm, varargs sounds a bit complex. But I agree readability for vhost_net_enable_vq and friends would benefit from passing a vq around rather than an index. I'll try it out and do it if it's a win.

Quoted 22 lines

Quoted 3 lines
drivers/vhost/net.c new file +3 -0
+static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
+{
+	struct socket *sock, *oldsock = NULL;
...
Quoted 8 lines
drivers/vhost/net.c new file +8 -0
+	sock = get_socket(fd);
+	if (IS_ERR(sock)) {
+		r = PTR_ERR(sock);
+		goto done;
+	}
+
+	/* start polling new socket */
+	oldsock = vq->private_data;
...
drivers/vhost/net.c line 503
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+done:
+	mutex_unlock(&n->dev.mutex);
+	if (oldsock) {
+		vhost_net_flush_vq(n, index);
+		fput(oldsock->file);
I dislike this style; I prefer multiple different goto points, one for when oldsock is set, and one for when it's not.

Quoted 1 line

That way, gcc warns us about uninitialized variables if we get it wrong.

OK.

Quoted 7 lines

drivers/vhost/net.c line 512
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+static long vhost_net_reset_owner(struct vhost_net *n)
+{
+	struct socket *tx_sock = NULL;
+	struct socket *rx_sock = NULL;
+	long r;
This should be called "err", since that's what it is.

OK.

Quoted 10 lines

drivers/vhost/net.c line 535
Quoted 7 lines
drivers/vhost/net.c new file +7 -0
+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+		sizeof(struct virtio_net_hdr) : 0;
+	int i;
+	mutex_lock(&n->dev.mutex);
+	n->dev.acked_features = features;
Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

Not sure how do you mean. Userspace sets them, why does it want to get them exposed back?

Quoted 6 lines

drivers/vhost/net.c new file +6 -0

+	case VHOST_GET_FEATURES:
+		features = VHOST_FEATURES;
+		return put_user(features, featurep);
+	case VHOST_ACK_FEATURES:
+		r = get_user(features, featurep);
+		/* No features for now */

Quoted 8 lines

drivers/vhost/net.c line 571
Quoted 5 lines
drivers/vhost/net.c new file +5 -0
+		if (r < 0)
+			return r;
+		if (features & ~VHOST_FEATURES)
+			return -EOPNOTSUPP;
+		vhost_net_set_features(n, features);
OK, from the userspace POV it's "get features" then "ack features". But I think "VHOST_SET_FEATURES" is more consistent, despite this usage.

OK.

Quoted 6 lines

drivers/vhost/vhost.c line 264
Quoted 2 lines
drivers/vhost/vhost.c new file +2 -0
+	switch (ioctl) {
+	case VHOST_SET_VRING_NUM:
I haven't looked at your userspace implementation, but does a generic VHOST_SET_VRING_STATE & VHOST_GET_VRING_STATE with a struct make more sense? It'd be simpler here,

Not by much though, right?

Quoted 1 line

but not sure if it'd be simpler to use?

Quoted 17 lines

(Not the fd-setting ioctls of course)

Quoted 15 lines

drivers/vhost/vhost.c new file +15 -0

+	case VHOST_SET_VRING_LOG:
+		r = copy_from_user(&a, argp, sizeof a);
+		if (r < 0)
+			break;
+		if (a.padding) {
+			r = -EOPNOTSUPP;
+			break;
+		}
+		if (a.user_addr == VHOST_VRING_LOG_DISABLE) {
+			vq->log_used = false;
+			break;
+		}
+		if (a.user_addr & (sizeof *vq->used->ring - 1)) {
+			r = -EINVAL;
+			break;

Quoted 8 lines

drivers/vhost/vhost.c line 362
Quoted 4 lines
drivers/vhost/vhost.c new file +4 -0
+		}
+		vq->log_used = true;
+		vq->log_addr = a.user_addr;
+		break;
For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.

Yes. It's not too late to split it out though: should I do it yet?

Quoted 9 lines

drivers/vhost/vhost.c line 525
Quoted 6 lines
drivers/vhost/vhost.c new file +6 -0
+/* TODO: This is really inefficient.  We need something like get_user()
+ * (instruction directly accesses the data, with an exception table entry
+ * returning -EFAULT). See Documentation/x86/exception-tables.txt.
+ */
+static int set_bit_to_user(int nr, void __user *addr)
+{
I guess we won't be dealing with many contiguous pages, otherwise we could get a cheap speedup making this set_bits_to_user(int nr, int num_bits...).

No idea. Let's keep it as simple as possible for now?

Quoted 4 lines

drivers/vhost/vhost.c new file +4 -0

+/* Each buffer in the virtqueues is actually a chain of descriptors.  This
+ * function returns the next descriptor in the chain,
+ * or -1 if we're at the end. */
+static unsigned next_desc(struct vring_desc *desc)

Quoted 8 lines

drivers/vhost/vhost.c line 635
Quoted 6 lines
drivers/vhost/vhost.c new file +6 -0
+{
+	unsigned int next;
+
+	/* If this descriptor says it doesn't chain, we're done. */
+	if (!(desc->flags & VRING_DESC_F_NEXT))
+		return -1;
Hmm, prefer s/-1/-1U/ in comment, here, and below. Clarifies a bit.

Good idea.

Quoted 4 lines

Quoted 2 lines
+/* After we've used one of their buffers, we tell them about it. We'll then
+ * want to send them an interrupt, using vq->call. */
This comment has too much cut & paste:

Quoted 7 lines

... want to notify the guest, using the eventfd */
drivers/vhost/vhost.c line 904
Quoted 3 lines
drivers/vhost/vhost.c new file +3 -0
+/* This actually sends the interrupt for this virtqueue */
+void vhost_trigger_irq(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{
Rename vhost_notify_eventfd() or something, and fix comments?

Sounds good. Since I'm renaming vhost_notify to vhost_enable_notify, this one can just become vhost_notify.

Quoted 4 lines

drivers/vhost/vhost.h line 19
Quoted 2 lines
drivers/vhost/vhost.h new file +2 -0
+enum {
+	VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,
+2? Believable, but is it correct?

+ 1 is for skb head, + 1 is for virtio net header. I'll add a comment.

Quoted 8 lines

drivers/vhost/vhost.h line 24
Quoted 3 lines
drivers/vhost/vhost.h new file +3 -0
+/* Poll a file (eventfd or socket) */
+/* Note: there's nothing vhost specific about this structure. */
+struct vhost_poll {
This comment really helped while reading the code. Kudos!
Thanks!
Rusty.

Rusty Russell

2009-11-06 04:59 UTC #31 lore raw

Re: [PATCHv8 3/3] vhost_net: a kernel-level virtio server

In reply to Michael S. Tsirkin

Quoted 2 lines · Michael S. Tsirkin

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

Hi Michael,

Now everyone else has finally kicked all the tires and it seems to pass,
I've done a fairly complete review. Generally, it's really nice; just one bug and a few minor suggestions for polishing.

drivers/vhost/net.c line 80

Quoted 5 lines

drivers/vhost/net.c new file +5 -0

+/* Caller must have TX VQ lock */
+static void tx_poll_stop(struct vhost_net *net)
+{
+	if (likely(net->tx_poll_state != VHOST_NET_POLL_STARTED))
+		return;

likely? Really?

drivers/vhost/net.c line 129

Quoted 3 lines

drivers/vhost/net.c new file +3 -0

+	for (;;) {
+		head = vhost_get_vq_desc(&net->dev, vq, vq->iov, &out, &in,
+					 NULL, NULL);

Danger! You need an arg to vhost_get_vq_desc to tell it the max desc size you can handle. Otherwise, it's only limited by ring size, and a malicious guest can overflow you here, and below:

Quoted 2 lines

drivers/vhost/net.c new file +2 -0

+		/* Skip header. TODO: support TSO. */
+		s = move_iovec_hdr(vq->iov, vq->hdr, hdr_size, out);

...

drivers/vhost/net.c line 121

Quoted 4 lines

drivers/vhost/net.c new file +4 -0

+
+	use_mm(net->dev.mm);
+	mutex_lock(&vq->mutex);
+	vhost_no_notify(vq);

I prefer a name like "vhost_disable_notify()".

drivers/vhost/net.c line 228

Quoted 10 lines

drivers/vhost/net.c new file +10 -0

+		/* OK, now we need to know about added descriptors. */
+		if (head == vq->num && vhost_notify(vq))
+			/* They could have slipped one in as we were doing that:
+			 * check again. */
+			continue;
+		/* Nothing new?  Wait for eventfd to tell us they refilled. */
+		if (head == vq->num)
+			break;
+		/* We don't need to be notified again. */
+		vhost_no_notify(vq);

Similarly, vhost_enable_notify. This one is particularly misleading since it doesn't actually notify anything!

In particular, this code would be neater as:

	if (head == vq->num) {
		if (vhost_enable_notify(vq)) {
			/* Try again, they could have slipped one in. */
			continue;
		}
		/* Nothing more to do. */
		break;
	}
	vhost_disable_notify(vq);

drivers/vhost/net.c line 261

Quoted 2 lines

drivers/vhost/net.c new file +2 -0

+		len = err;
+		err = memcpy_toiovec(vq->hdr, (unsigned char *)&hdr, hdr_size);

That unsigned char * arg to memcpy_toiovec is annoying. A patch might be nice, separate from this effort.

drivers/vhost/net.c line 322

Quoted 9 lines

drivers/vhost/net.c new file +9 -0

+static int vhost_net_open(struct inode *inode, struct file *f)
+{
+	struct vhost_net *n = kzalloc(sizeof *n, GFP_KERNEL);
+	int r;
+	if (!n)
+		return -ENOMEM;
+	f->private_data = n;
+	n->vqs[VHOST_NET_VQ_TX].handle_kick = handle_tx_kick;
+	n->vqs[VHOST_NET_VQ_RX].handle_kick = handle_rx_kick;

drivers/vhost/net.c line 348

Quoted 3 lines

drivers/vhost/net.c new file +3 -0

+static void vhost_net_enable_vq(struct vhost_net *n, int index)
+{
+	struct socket *sock = n->vqs[index].private_data;

Not completely sure it'll be a win tho.

Quoted 3 lines

drivers/vhost/net.c new file +3 -0

+static long vhost_net_set_backend(struct vhost_net *n, unsigned index, int fd)
+{
+	struct socket *sock, *oldsock = NULL;

...

Quoted 8 lines

drivers/vhost/net.c new file +8 -0

+	sock = get_socket(fd);
+	if (IS_ERR(sock)) {
+		r = PTR_ERR(sock);
+		goto done;
+	}
+
+	/* start polling new socket */
+	oldsock = vq->private_data;

...

drivers/vhost/net.c line 503

Quoted 5 lines

drivers/vhost/net.c new file +5 -0

+done:
+	mutex_unlock(&n->dev.mutex);
+	if (oldsock) {
+		vhost_net_flush_vq(n, index);
+		fput(oldsock->file);

I dislike this style; I prefer multiple different goto points, one for when oldsock is set, and one for when it's not.

That way, gcc warns us about uninitialized variables if we get it wrong.

drivers/vhost/net.c line 512

Quoted 5 lines

drivers/vhost/net.c new file +5 -0

+static long vhost_net_reset_owner(struct vhost_net *n)
+{
+	struct socket *tx_sock = NULL;
+	struct socket *rx_sock = NULL;
+	long r;

This should be called "err", since that's what it is.

drivers/vhost/net.c line 535

Quoted 7 lines

drivers/vhost/net.c new file +7 -0

+static void vhost_net_set_features(struct vhost_net *n, u64 features)
+{
+	size_t hdr_size = features & (1 << VHOST_NET_F_VIRTIO_NET_HDR) ?
+		sizeof(struct virtio_net_hdr) : 0;
+	int i;
+	mutex_lock(&n->dev.mutex);
+	n->dev.acked_features = features;

Why is this called "acked_features"? Not just "features"? I expected to see code which exposed these back to userspace, and didn't.

drivers/vhost/net.c line 571

Quoted 11 lines

drivers/vhost/net.c new file +11 -0

+	case VHOST_GET_FEATURES:
+		features = VHOST_FEATURES;
+		return put_user(features, featurep);
+	case VHOST_ACK_FEATURES:
+		r = get_user(features, featurep);
+		/* No features for now */
+		if (r < 0)
+			return r;
+		if (features & ~VHOST_FEATURES)
+			return -EOPNOTSUPP;
+		vhost_net_set_features(n, features);

OK, from the userspace POV it's "get features" then "ack features". But I think "VHOST_SET_FEATURES" is more consistent, despite this usage.

drivers/vhost/vhost.c line 264

Quoted 2 lines

drivers/vhost/vhost.c new file +2 -0
+	switch (ioctl) {
+	case VHOST_SET_VRING_NUM:

(Not the fd-setting ioctls of course)

Quoted 9 lines

drivers/vhost/vhost.c new file +9 -0

+	case VHOST_SET_VRING_LOG:
+		r = copy_from_user(&a, argp, sizeof a);
+		if (r < 0)
+			break;
+		if (a.padding) {
+			r = -EOPNOTSUPP;
+			break;
+		}
+		if (a.user_addr == VHOST_VRING_LOG_DISABLE) {

drivers/vhost/vhost.c line 362

Quoted 10 lines

drivers/vhost/vhost.c new file +10 -0

+			vq->log_used = false;
+			break;
+		}
+		if (a.user_addr & (sizeof *vq->used->ring - 1)) {
+			r = -EINVAL;
+			break;
+		}
+		vq->log_used = true;
+		vq->log_addr = a.user_addr;
+		break;

For future reference, this is *exactly* the kind of thing which would have been nice as a followup patch. Easy to separate, easy to review, not critical to the core.

drivers/vhost/vhost.c line 525

Quoted 6 lines

drivers/vhost/vhost.c new file +6 -0

+/* TODO: This is really inefficient.  We need something like get_user()
+ * (instruction directly accesses the data, with an exception table entry
+ * returning -EFAULT). See Documentation/x86/exception-tables.txt.
+ */
+static int set_bit_to_user(int nr, void __user *addr)
+{

I guess we won't be dealing with many contiguous pages, otherwise we could get a cheap speedup making this set_bits_to_user(int nr, int num_bits...).

drivers/vhost/vhost.c line 635

Quoted 10 lines

drivers/vhost/vhost.c new file +10 -0

+/* Each buffer in the virtqueues is actually a chain of descriptors.  This
+ * function returns the next descriptor in the chain,
+ * or -1 if we're at the end. */
+static unsigned next_desc(struct vring_desc *desc)
+{
+	unsigned int next;
+
+	/* If this descriptor says it doesn't chain, we're done. */
+	if (!(desc->flags & VRING_DESC_F_NEXT))
+		return -1;

Hmm, prefer s/-1/-1U/ in comment, here, and below. Clarifies a bit.

Quoted 2 lines

+/* After we've used one of their buffers, we tell them about it. We'll then
+ * want to send them an interrupt, using vq->call. */

This comment has too much cut & paste:
... want to notify the guest, using the eventfd */

drivers/vhost/vhost.c line 904

Quoted 3 lines

drivers/vhost/vhost.c new file +3 -0

+/* This actually sends the interrupt for this virtqueue */
+void vhost_trigger_irq(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+{

Rename vhost_notify_eventfd() or something, and fix comments?

drivers/vhost/vhost.h line 19

Quoted 2 lines

drivers/vhost/vhost.h new file +2 -0
+enum {
+	VHOST_NET_MAX_SG = MAX_SKB_FRAGS + 2,

+2? Believable, but is it correct?

drivers/vhost/vhost.h line 24

Quoted 3 lines

drivers/vhost/vhost.h new file +3 -0

+/* Poll a file (eventfd or socket) */
+/* Note: there's nothing vhost specific about this structure. */
+struct vhost_poll {

This comment really helped while reading the code. Kudos!

Thanks!
Rusty.

Michael S. Tsirkin

author maintainer

2009-11-04 15:57 UTC #32 lore raw

PATCH v8 3/3 vhost_net: a kernel-level virtio server

In reply to a message not in the archive

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.

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.

Features that I plan to look at in the future:
- mergeable buffers
- zero copy
- scalability tuning: figure out the best threading model to use

Acked-by Arnd Bergmann <[email protected]>

Cc "Paul E. McKenney" <[email protected]>

Signed-off-by Michael S. Tsirkin <[email protected]>

---

 MAINTAINERS                |    9 +
 arch/x86/kvm/Kconfig       |    1 +
 drivers/Makefile           |    1 +
 drivers/vhost/Kconfig      |   11 +
 drivers/vhost/Makefile     |    2 +
 drivers/vhost/net.c        |  633 +++++++++++++++++++++++++++++
 drivers/vhost/vhost.c      |  970 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/vhost/vhost.h      |  158 +++++++
 include/linux/Kbuild       |    1 +
 include/linux/miscdevice.h |    1 +
 include/linux/vhost.h      |  126 ++++++
 11 files changed, 1913 insertions(+), 0 deletions(-)
 create mode 100644 drivers/vhost/Kconfig
 create mode 100644 drivers/vhost/Makefile
 create mode 100644 drivers/vhost/net.c
 create mode 100644 drivers/vhost/vhost.c
 create mode 100644 drivers/vhost/vhost.h
 create mode 100644 include/linux/vhost.h

MAINTAINERS +9 -0

index 8824115..980a69b 100644
@@ -5619,6 +5619,15 @@ S:	Maintained
 F:	Documentation/filesystems/vfat.txt
 F:	fs/fat/
 
+VIRTIO HOST (VHOST)
+M:	"Michael S. Tsirkin" <[email protected]>
+L:	[email protected]
+L:	[email protected]
+L:	[email protected]
+S:	Maintained
+F:	drivers/vhost/
+F:	include/linux/vhost.h
+
 VIA RHINE NETWORK DRIVER
 M:	Roger Luethi <[email protected]>
 S:	Maintained

arch/x86/kvm/Kconfig +1 -0

index b84e571..94f44d9 100644
@@ -64,6 +64,7 @@ config KVM_AMD
 
 # OK, it's a little counter-intuitive to do this, but it puts it neatly under
 # the virtualization menu.
+source drivers/vhost/Kconfig
 source drivers/lguest/Kconfig
 source drivers/virtio/Kconfig

drivers/Makefile +1 -0

index 6ee53c7..81e3659 100644
@@ -106,6 +106,7 @@ obj-$(CONFIG_HID)		+= hid/
 obj-$(CONFIG_PPC_PS3)		+= ps3/
 obj-$(CONFIG_OF)		+= of/
 obj-$(CONFIG_SSB)		+= ssb/
+obj-$(CONFIG_VHOST_NET)		+= vhost/
 obj-$(CONFIG_VIRTIO)		+= virtio/
 obj-$(CONFIG_VLYNQ)		+= vlynq/
 obj-$(CONFIG_STAGING)		+= staging/

drivers/vhost/Kconfig new file +11 -0

new file mode 100644
index 0000000..9f409f4
@@ -0,0 +1,11 @@
+config VHOST_NET
+	tristate "Host kernel accelerator for virtio net (EXPERIMENTAL)"
+	depends on NET && EVENTFD && EXPERIMENTAL
+	---help---
+	  This kernel module can be loaded in host kernel to accelerate
+	  guest networking with virtio_net. Not to be confused with virtio_net
+	  module itself which needs to be loaded in guest kernel.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called vhost_net.
+

drivers/vhost/Makefile new file +2 -0

new file mode 100644
index 0000000..72dd020
@@ -0,0 +1,2 @@
+obj-$(CONFIG_VHOST_NET) += vhost_net.o
+vhost_net-y := vhost.o net.o

drivers/vhost/net.c new file +633 -0

633 lines not on this page · raw message

drivers/vhost/vhost.c new file +970 -0

970 lines not on this page · raw message

drivers/vhost/vhost.h new file +158 -0

158 lines not on this page · raw message

include/linux/Kbuild +1 -0

index 3f384d4..b6335c6 100644
@@ -361,6 +361,7 @@ unifdef-y += uio.h
 unifdef-y += unistd.h
 unifdef-y += usbdevice_fs.h
 unifdef-y += utsname.h
+unifdef-y += vhost.h
 unifdef-y += videodev2.h
 unifdef-y += videodev.h
 unifdef-y += virtio_config.h

include/linux/miscdevice.h +1 -0

index adaf3c1..8b5f7cc 100644
@@ -30,6 +30,7 @@
 #define HPET_MINOR		228
 #define FUSE_MINOR		229
 #define KVM_MINOR		232
+#define VHOST_NET_MINOR		233
 #define MISC_DYNAMIC_MINOR	255
 
 struct device;

include/linux/vhost.h new file +126 -0

126 lines not on this page · raw message

Signature · 1 line

1.6.5.2.143.g8cc62