Re: [Bridge] [PATCH] macvlan: add tap device backend

["Michael S. Tsirkin" <mst@redhat.com>]

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:
> > On Thu, Aug 06, 2009 at 09:50:28PM +0000, Arnd Bergmann wrote:
> > > * 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.
> 
> > > +/* 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.

> > > +	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.
> 
> > > +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 <><