On Sun, Aug 09, 2009 at 08:42:24PM +0000, Arnd Bergmann wrote:

On Sunday 09 August 2009 08:02:16 Michael S. Tsirkin wrote:

quoted

On Thu, Aug 06, 2009 at 09:50:28PM +0000, Arnd Bergmann wrote:

quoted

* The same framework in macvlan can be used to add a third backend
into a future kernel based virtio-net implementation.

Could you split the patches up, to make this last easier?
patch 1 - export framework
patch 2 - code using it

Sure, will do.

quoted

quoted

+/* Get packet from user space buffer */
+static ssize_t macvtap_get_user(struct macvtap_dev *vtap,
+			       const struct iovec *iv, size_t count,
+			       int noblock)
+{
+	struct sk_buff *skb;
+	size_t len = count;
+
+	if (unlikely(len < ETH_HLEN))
+		return -EINVAL;
+
+	skb = alloc_skb(NET_IP_ALIGN + len, GFP_KERNEL);
+
+	if (!skb) {
+		vtap->m.dev->stats.rx_dropped++;
+		return -ENOMEM;
+	}
+
+	skb_reserve(skb, NET_IP_ALIGN);
+	skb_put(skb, count);
+
+	if (skb_copy_datagram_from_iovec(skb, 0, iv, 0, len)) {
+		vtap->m.dev->stats.rx_dropped++;
+		kfree_skb(skb);
+		return -EFAULT;
+	}
+
+	skb_set_network_header(skb, ETH_HLEN);
+	skb->dev = vtap->m.lowerdev;
+
+	macvlan_start_xmit(skb, vtap->m.dev);
+
+	return count;
+}

With tap, we discovered that not limiting the number of outstanding
skbs hurts UDP performance. And the solution was to limit
the number of outstanding packets - with hacks to work around
the fact that userspace .

Something seems to be missing in your last sentence here.

Most userspace does not seem to implement software flow control for UDP,
even though it probably should.

My driver OTOH is also missing any sort of flow control in both
RX and TX direction ;) For RX, there should probably just be
a limit of frames that get buffered in the ring.

For TX, I guess there should be a way to let the packet
scheduler handle this and give us a chance to block and
unblock at the right time. I haven't found out yet how to
do that.

Would it be enough to check the dev_queue_xmit() return
code for NETDEV_TX_BUSY?

How would I get notified when it gets free again?

You can do this by creating a socket. Look at how tun does
this now.

quoted

quoted

+	ret = skb_copy_datagram_iovec(skb, 0, iv, len);
+
+	vtap->m.dev->stats.rx_packets++;
+	vtap->m.dev->stats.rx_bytes += len;

where does atomicity guarantee for these counters come from?

AFAIK, we never do for any driver. They are statistics only and
need not be 100% correct, so the networking stack goes for
lower overhead and 99.9% correct.

quoted

quoted

+static ssize_t macvtap_aio_read(struct kiocb *iocb, const struct iovec *iv,
+			    unsigned long count, loff_t pos)
+{
+	struct file *file = iocb->ki_filp;
+	struct macvtap_dev *vtap = file->private_data;
+	DECLARE_WAITQUEUE(wait, current);
+	struct sk_buff *skb;
+	ssize_t len, ret = 0;
+
+	if (!vtap)
+		return -EBADFD;
+
+	len = iov_length(iv, count);
+	if (len < 0) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	add_wait_queue(&vtap->wait, &wait);
+	while (len) {
+		current->state = TASK_INTERRUPTIBLE;
+
+		/* Read frames from the queue */
+		if (!(skb=skb_dequeue(&vtap->readq))) {
+			if (file->f_flags & O_NONBLOCK) {
+				ret = -EAGAIN;
+				break;
+			}
+			if (signal_pending(current)) {
+				ret = -ERESTARTSYS;
+				break;
+			}
+			/* Nothing to read, let's sleep */
+			schedule();
+			continue;
+		}
+		ret = macvtap_put_user(vtap, skb, (struct iovec *) iv, len);

Don't cast away the constness. Instead, fix macvtap_put_user
to used skb_copy_datagram_const_iovec which does not modify the iovec.

Ah, good catch. I had copied that from the tun driver before you
fixed it there and failed to fix it the right way when I adapted
it for the new interface.

Thanks for the review,

	Arnd <><