[RFC usbnet.c] - Add packet aggregation capability

[RFC usbnet.c] - Add packet aggregation capability

[David Hollis]

[-- Attachment #1: Type: text/plain, Size: 1111 bytes --]

The ASIX AX88772 and AX88178 devices have the ability to transmit up 16K
worth of packets in a single bulk transfer.  The asix.c driver currently
doesn't support aggregating multiple packets into this buffer so that
packets can be transmitted in a single transfer.  On some systems -
notably ARM - high cpu utilization is experienced when transmitting
large numbers of packets.

The attached set of patches provided by ASIX add this capability to the
usbnet.c driver and add the implementation for the asix.c driver.  

These patches are not yet ready for merging but we are very interested
in any review and comments that can be provided at this time.

One item that I have is how to handle the portion in usbnet_probe() that
sets the hard_start_xmit handler with the change to netdevice_ops:

-	net->hard_start_xmit = usbnet_start_xmit;
+	if (info->tx_gather)
+		net->hard_start_xmit = usbnet_bundle_xmit;
+	else
+		net->hard_start_xmit = usbnet_start_xmit;


Would it be the most appropriate to merge the
usbnet_aggregate_skb_xmit() pieces into the existing usbnet_start_xmit
with appropriate conditionals?


[-- Attachment #2: usbnet.h.patch --]
[-- Type: text/x-patch, Size: 1126 bytes --]

--- ../../linux-2.6.29.4/include/linux/usb/usbnet.h	2009-05-19 07:52:34.000000000 +0800
+++ usbnet.h	2009-06-08 13:27:16.000000000 +0800
@@ -48,11 +48,15 @@
 	u32			xid;
 	u32			hard_mtu;	/* count any extra framing */
 	size_t			rx_urb_size;	/* size for rx urbs */
+	size_t			tx_threshold;
 	struct mii_if_info	mii;
 
 	/* various kinds of pending driver work */
 	struct sk_buff_head	rxq;
 	struct sk_buff_head	txq;
+	struct sk_buff_head	tx_waitq;
+	u32			tx_wait_bytes;
+	u32			tx_busy_pkt;
 	struct sk_buff_head	done;
 	struct urb		*interrupt;
 	struct tasklet_struct	bh;
@@ -114,6 +118,10 @@
 	struct sk_buff	*(*tx_fixup)(struct usbnet *dev,
 				struct sk_buff *skb, gfp_t flags);
 
+	/* aggregate tx packets */
+	struct sk_buff	*(*tx_gather)(struct usbnet *dev,
+				struct sk_buff_head *q, size_t *actual_len);
+
 	/* early initialization code, can sleep. This is for minidrivers
 	 * having 'subminidrivers' that need to do extra initialization
 	 * right after minidriver have initialized hardware. */
@@ -174,6 +182,7 @@
 	struct usbnet		*dev;
 	enum skb_state		state;
 	size_t			length;
+	size_t			pkt_cnt;
 };
 
 

[-- Attachment #3: usbnet.c.patch --]
[-- Type: text/x-patch, Size: 6363 bytes --]

--- ../../linux-2.6.29.4/drivers/net/usb/usbnet.c	2009-05-19 07:52:34.000000000 +0800
+++ usbnet.c	2009-06-10 10:18:18.000000000 +0800
@@ -45,6 +45,8 @@
 
 #define DRIVER_VERSION		"22-Aug-2005"
 
+static int usbnet_aggreate_skb_ximt (struct usbnet *dev,
+			struct sk_buff_head *q);
 
 /*-------------------------------------------------------------------------*/
 
@@ -551,6 +553,7 @@
 static int usbnet_stop (struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
+	struct sk_buff		*skb;
 	int			temp;
 	DECLARE_WAIT_QUEUE_HEAD_ONSTACK (unlink_wakeup);
 	DECLARE_WAITQUEUE (wait, current);
@@ -579,6 +582,11 @@
 	dev->wait = NULL;
 	remove_wait_queue (&unlink_wakeup, &wait);
 
+	while (!skb_queue_empty (&dev->tx_waitq)) {
+		skb = skb_dequeue(&dev->tx_waitq);
+		dev_kfree_skb (skb);
+	}
+
 	usb_kill_urb(dev->interrupt);
 
 	/* deferred work (task, timer, softirq) must also stop.
@@ -863,13 +871,20 @@
 	struct sk_buff		*skb = (struct sk_buff *) urb->context;
 	struct skb_data		*entry = (struct skb_data *) skb->cb;
 	struct usbnet		*dev = entry->dev;
+	struct driver_info	*info = dev->driver_info;
+	unsigned long		flags;
+
+	if (info->tx_gather) {
+		spin_lock_irqsave (&dev->tx_waitq.lock, flags);
+		dev->tx_busy_pkt -= entry->pkt_cnt;
+		spin_unlock (&dev->tx_waitq.lock);
+	}
 
 	if (urb->status == 0) {
-		dev->stats.tx_packets++;
 		dev->stats.tx_bytes += entry->length;
+		dev->stats.tx_packets += entry->pkt_cnt;
 	} else {
-		dev->stats.tx_errors++;
-
+		dev->stats.tx_errors += entry->pkt_cnt;
 		switch (urb->status) {
 		case -EPIPE:
 			usbnet_defer_kevent (dev, EVENT_TX_HALT);
@@ -920,6 +935,113 @@
 
 /*-------------------------------------------------------------------------*/
 
+static int usbnet_aggreate_skb_xmit (struct usbnet *dev, struct sk_buff_head *q)
+{
+
+	struct sk_buff		*skb = NULL;//, *skbnext = NULL;
+	int			pkt_cnt = 0;
+	struct skb_data		*entry;
+	int			retval = NET_XMIT_SUCCESS;
+	struct urb		*urb = NULL;
+	struct driver_info	*info = dev->driver_info;
+	size_t			actual_len = 0;
+
+	if (skb_queue_empty (q)) {
+		return retval;
+	}
+
+	spin_lock (&q->lock);
+
+	pkt_cnt = skb_queue_len (q);
+	skb = info->tx_gather (dev, q, &actual_len);
+
+	dev->tx_wait_bytes -= actual_len;
+	dev->tx_busy_pkt += pkt_cnt;
+
+	spin_unlock (&q->lock);	
+
+	if (!skb){
+		goto drop;
+	}
+
+	if (!(urb = usb_alloc_urb (0, GFP_ATOMIC))) {
+		if (netif_msg_tx_err (dev))
+			devdbg (dev, "no urb");	
+		goto drop;
+	}else{
+		entry = (struct skb_data *) skb->cb;
+		entry->length = actual_len;
+		entry->pkt_cnt = pkt_cnt;
+		entry->urb = urb;
+		entry->dev = dev;
+		entry->state = tx_start;
+
+		usb_fill_bulk_urb (urb, dev->udev, dev->out,
+				skb->data, skb->len, tx_complete, skb);
+	}
+
+	switch ((retval = usb_submit_urb (urb, GFP_ATOMIC))) {		
+	case -EPIPE:
+		netif_stop_queue (dev->net);
+		usbnet_defer_kevent (dev, EVENT_TX_HALT);
+		break;
+	default:
+		if (netif_msg_tx_err (dev))
+			devdbg (dev, "tx: submit urb err %d", retval);
+		break;
+	case 0:
+		dev->net->trans_start = jiffies;
+		__skb_queue_tail (&dev->txq, skb);
+	}
+
+	if (retval) {
+		if (netif_msg_tx_err (dev))
+			devdbg (dev, "drop, code %d", retval);
+drop:
+		spin_lock (&q->lock);
+		dev->tx_busy_pkt -= pkt_cnt;
+		spin_unlock (&q->lock);
+
+		retval = NET_XMIT_SUCCESS;
+		dev->stats.tx_dropped += pkt_cnt;
+		if (skb)
+			dev_kfree_skb_any (skb);
+		usb_free_urb (urb);
+	} else if (netif_msg_tx_queued (dev)) {
+		devdbg (dev, "> tx, len %d, type 0x%x",
+			skb->len, skb->protocol);
+	}
+
+	return retval;
+}
+
+static int usbnet_bundle_xmit (struct sk_buff *skb, struct net_device *net)
+{
+	struct usbnet		*dev = netdev_priv(net);
+	int			retval = NET_XMIT_SUCCESS;
+	unsigned long		flags;
+
+	spin_lock_irqsave (&dev->tx_waitq.lock, flags);
+	__skb_queue_tail (&dev->tx_waitq, skb);
+	dev->tx_wait_bytes += skb->len;
+	spin_unlock (&dev->tx_waitq.lock);
+
+	spin_lock (&dev->txq.lock);
+
+	if (skb_queue_empty (&dev->txq) ||
+	    (dev->tx_wait_bytes >= dev->tx_threshold) ||
+	    (dev->tx_waitq.qlen > 16)) {
+		retval = usbnet_aggreate_skb_xmit (dev, &dev->tx_waitq);
+	}
+
+	if ((dev->tx_busy_pkt + dev->tx_waitq.qlen) >= TX_QLEN (dev))
+		netif_stop_queue (net);
+
+	spin_unlock_irqrestore (&dev->txq.lock, flags);
+
+	return 	retval;
+}
+
 static int usbnet_start_xmit (struct sk_buff *skb, struct net_device *net)
 {
 	struct usbnet		*dev = netdev_priv(net);
@@ -953,6 +1075,7 @@
 	entry->dev = dev;
 	entry->state = tx_start;
 	entry->length = length;
+	entry->pkt_cnt = 1;
 
 	usb_fill_bulk_urb (urb, dev->udev, dev->out,
 			skb->data, skb->len, tx_complete, skb);
@@ -1014,6 +1137,8 @@
 	struct usbnet		*dev = (struct usbnet *) param;
 	struct sk_buff		*skb;
 	struct skb_data		*entry;
+	struct driver_info	*info = dev->driver_info;
+	unsigned long		flags;
 
 	while ((skb = skb_dequeue (&dev->done))) {
 		entry = (struct skb_data *) skb->cb;
@@ -1062,8 +1187,22 @@
 			if (dev->rxq.qlen < qlen)
 				tasklet_schedule (&dev->bh);
 		}
-		if (dev->txq.qlen < TX_QLEN (dev))
-			netif_wake_queue (dev->net);
+
+		if (info->tx_gather) {
+			spin_lock_irqsave (&dev->txq.lock, flags);
+			if (dev->txq.qlen == 0) {
+				usbnet_aggreate_skb_ximt(dev, &dev->tx_waitq);
+			}
+
+			if ((dev->tx_busy_pkt + dev->tx_waitq.qlen) <
+					 TX_QLEN (dev))
+				netif_wake_queue (dev->net);
+
+			spin_unlock_irqrestore(&dev->txq.lock, flags);
+		} else {
+			if (dev->txq.qlen < TX_QLEN (dev))
+				netif_wake_queue (dev->net);
+		}
 	}
 }
 
@@ -1156,6 +1295,7 @@
 	skb_queue_head_init (&dev->rxq);
 	skb_queue_head_init (&dev->txq);
 	skb_queue_head_init (&dev->done);
+	skb_queue_head_init (&dev->tx_waitq);
 	dev->bh.func = usbnet_bh;
 	dev->bh.data = (unsigned long) dev;
 	INIT_WORK (&dev->kevent, kevent);
@@ -1181,7 +1321,10 @@
 
 	net->change_mtu = usbnet_change_mtu;
 	net->get_stats = usbnet_get_stats;
-	net->hard_start_xmit = usbnet_start_xmit;
+	if (info->tx_gather)
+		net->hard_start_xmit = usbnet_bundle_xmit;
+	else
+		net->hard_start_xmit = usbnet_start_xmit;
 	net->open = usbnet_open;
 	net->stop = usbnet_stop;
 	net->watchdog_timeo = TX_TIMEOUT_JIFFIES;
@@ -1228,6 +1371,8 @@
 
 	if (!dev->rx_urb_size)
 		dev->rx_urb_size = dev->hard_mtu;
+	if (!dev->tx_threshold)
+		dev->tx_threshold = dev->rx_urb_size;
 	dev->maxpacket = usb_maxpacket (dev->udev, dev->out, 1);
 
 	SET_NETDEV_DEV(net, &udev->dev);

[-- Attachment #4: asix.c.patch --]
[-- Type: text/x-patch, Size: 2760 bytes --]

--- ../../../linux-2.6.29.4/drivers/net/usb/asix.c	2009-05-19 07:52:34.000000000 +0800
+++ asix.c	2009-06-10 10:15:17.000000000 +0800
@@ -354,6 +354,85 @@
 	return 1;
 }
 
+static struct sk_buff *
+ax88772_tx_gather (struct usbnet *dev, struct sk_buff_head *q,
+			size_t *actual_len)
+{
+	struct sk_buff		*skb = NULL, *skb_next = NULL;
+	int			pkt_cnt;
+	u16			pkt_size;
+	u32			skb_len = 0;
+
+	u32			packet_hdr;
+	u32			padbytes = 0xffff0000;
+	u8			padneeded = 0;
+	struct driver_info	*info = dev->driver_info;
+
+	pkt_cnt = skb_queue_len (q);
+
+	*actual_len = 0;
+	for (skb_next = q->next; skb_next != (struct sk_buff *) q;
+	     skb_next = skb_next->next){
+		*actual_len += skb_next->len;
+		pkt_size = (skb_next->len + 4 + 1) & 0xFFFE;
+		skb_len += pkt_size;
+	}
+
+	if ((skb_len % 512) == 0) {
+		skb_len += sizeof (padbytes);
+		padneeded = 1;
+	}
+
+	if (pkt_cnt > 1){
+
+		skb = dev_alloc_skb (skb_len); 
+
+		if (!skb){
+
+			while (pkt_cnt--) {
+				skb_next = __skb_dequeue(q);
+				dev_kfree_skb_any (skb_next);
+			}
+
+			devdbg (dev, "skb is NULL, len %d", skb_len);
+			return skb;
+		}
+
+		while (pkt_cnt--) {
+
+			skb_next = __skb_dequeue(q);
+
+			packet_hdr = ((skb_next->len ^ 0x0000ffff) << 16) +
+						skb_next->len;
+
+			le32_to_cpus(&packet_hdr);
+			memcpy (skb_put(skb, 4),  &packet_hdr, 4);
+			memcpy (skb_put(skb, skb_next->len),  skb_next->data,
+						skb_next->len);
+			if (skb->len & 1)
+				skb_put (skb, 1);
+			dev_kfree_skb_any (skb_next);
+		}
+
+		if (padneeded) {
+			memcpy (skb_put(skb, sizeof (padbytes)),
+					&padbytes, sizeof (padbytes));
+		}
+		
+	}else{ //only one packet to send
+		skb = __skb_dequeue(q);
+
+		skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
+		if (!skb) {
+			if (netif_msg_tx_err (dev))
+				devdbg (dev, "can't tx_fixup skb");
+			return skb;
+		}
+	}
+
+	return skb;
+}
+
 static struct sk_buff *asix_tx_fixup(struct usbnet *dev, struct sk_buff *skb,
 					gfp_t flags)
 {
@@ -1011,6 +1090,7 @@
 		/* hard_mtu  is still the default - the device does not support
 		   jumbo eth frames */
 		dev->rx_urb_size = 2048;
+		dev->tx_threshold = 16384;
 	}
 	return 0;
 
@@ -1281,6 +1361,7 @@
 		/* hard_mtu  is still the default - the device does not support
 		   jumbo eth frames */
 		dev->rx_urb_size = 2048;
+		dev->tx_threshold = 16384;
 	}
 	return 0;
 
@@ -1337,6 +1418,7 @@
 	.flags = FLAG_ETHER | FLAG_FRAMING_AX,
 	.rx_fixup = asix_rx_fixup,
 	.tx_fixup = asix_tx_fixup,
+	.tx_gather = ax88772_tx_gather,
 };
 
 static const struct driver_info ax88178_info = {
@@ -1348,6 +1430,7 @@
 	.flags = FLAG_ETHER | FLAG_FRAMING_AX,
 	.rx_fixup = asix_rx_fixup,
 	.tx_fixup = asix_tx_fixup,
+	.tx_gather = ax88772_tx_gather,
 };
 
 static const struct usb_device_id	products [] = {

Re: [RFC usbnet.c] - Add packet aggregation capability

[David Miller]

From: David Hollis <dhollis@davehollis.com>
Date: Wed, 10 Jun 2009 13:57:39 -0400

> One item that I have is how to handle the portion in usbnet_probe() that
> sets the hard_start_xmit handler with the change to netdevice_ops:
> 
> -	net->hard_start_xmit = usbnet_start_xmit;
> +	if (info->tx_gather)
> +		net->hard_start_xmit = usbnet_bundle_xmit;
> +	else
> +		net->hard_start_xmit = usbnet_start_xmit;
> 
> 
> Would it be the most appropriate to merge the
> usbnet_aggregate_skb_xmit() pieces into the existing usbnet_start_xmit
> with appropriate conditionals?

In the current tree you cannot even make this assignment.

Everything must go through a net_device_ops set of methods,
the pointer of which is const.

Re: [RFC usbnet.c] - Add packet aggregation capability

[David Hollis]

On Thu, 2009-06-11 at 05:38 -0700, David Miller wrote:
> From: David Hollis <dhollis@davehollis.com>
> Date: Wed, 10 Jun 2009 13:57:39 -0400
> 
> > One item that I have is how to handle the portion in usbnet_probe() that
> > sets the hard_start_xmit handler with the change to netdevice_ops:
> > 
> > -	net->hard_start_xmit = usbnet_start_xmit;
> > +	if (info->tx_gather)
> > +		net->hard_start_xmit = usbnet_bundle_xmit;
> > +	else
> > +		net->hard_start_xmit = usbnet_start_xmit;
> > 
> > 
> > Would it be the most appropriate to merge the
> > usbnet_aggregate_skb_xmit() pieces into the existing usbnet_start_xmit
> > with appropriate conditionals?
> 
> In the current tree you cannot even make this assignment.
> 
> Everything must go through a net_device_ops set of methods,
> the pointer of which is const.

So I'd need to have to separate net_device_ops and set
net->hard_start_xmit to the appropriate one based on info->tx_gather
being defined.

Not a problem.


> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html