[PATCH v1 0/4] USB & USBNET: loose SG check and support usbnet DMA SG

[PATCH v1 0/4] USB & USBNET: loose SG check and support usbnet DMA SG

[Ming Lei]

Hi,

This patchset allows drivers to pass sg buffers which size can't be divided
by max packet size of endpoint if the host controllers(such ax xHCI) support
this kind of sg buffers.

Previously we added check[1] on the situation and don't allow these sg buffers
passed to USB HCD, looks the check is too strict to make use of new feature of
new hardware(xHCI) for some applications(such as network stack) which can't
provide this kind of sg buffers to usbnet driver, so the patch looses the check
in case that the host controller supports it.

Patch 3/4 implements DMA SG on usbnet driver, and patch 4/4 uses it on ax88179_178a
USB3 NIC for supporting TSO, so both CPU utilization and tx throughput can be
improved with TSO and DMA SG in case of the USB NIC is attached to xHCI controller.

This patchset is against Eric Dumazet's patch(ax88179_178a: avoid copy of tx
tcp packets), so maybe it is better to merge via net-next tree.

[1], http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=10e232c597ac757e7f8600649f7e872e86de190f

V1:
	- introduce and apply usb_device_no_sg_limit() helper as suggested by Greg
	- simplify patch 4/4 against Eric Dumazet's patch(ax88179_178a: avoid copy of tx tcp packets)
	- don't pass usbnet header as sg buffer

 drivers/net/usb/ax88179_178a.c |   12 ++++++++++++
 drivers/net/usb/usbnet.c       |   38 ++++++++++++++++++++++++++++++++++++--
 drivers/usb/core/urb.c         |    3 ++-
 drivers/usb/host/xhci.c        |    4 ++++
 include/linux/usb.h            |   11 ++++++++++-
 include/linux/usb/usbnet.h     |    1 +
 6 files changed, 65 insertions(+), 4 deletions(-)


Thanks,
--
Ming Lei

[PATCH v1 1/4] USB: introduce usb_device_no_sg_limit() helper

[Ming Lei]

Some host controllers(such as xHCI) can support building
packet from discontinuous buffers, so introduce one flag
and helper for this kind of host controllers, then the
feature can help some applications(such as, usbnet) by
supporting arbitrary length of sg buffers.

Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/core/urb.c |    3 ++-
 include/linux/usb.h    |   11 ++++++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index e75115a..7a88ae8 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -414,7 +414,8 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 			urb->iso_frame_desc[n].status = -EXDEV;
 			urb->iso_frame_desc[n].actual_length = 0;
 		}
-	} else if (dev->speed != USB_SPEED_WIRELESS && urb->num_sgs) {
+	} else if (urb->num_sgs && !urb->dev->bus->no_sg_limit &&
+			dev->speed != USB_SPEED_WIRELESS) {
 		struct scatterlist *sg;
 		int i;
 
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 84f14e2..5d03074 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -337,6 +337,7 @@ struct usb_bus {
 					 * the ep queue on a short transfer
 					 * with the URB_SHORT_NOT_OK flag set.
 					 */
+	unsigned no_sg_limit:1;		/* no sg list limit */
 	unsigned sg_tablesize;		/* 0 or largest number of sg list entries */
 
 	int devnum_next;		/* Next open device number in
@@ -684,6 +685,14 @@ static inline bool usb_device_supports_ltm(struct usb_device *udev)
 	return udev->bos->ss_cap->bmAttributes & USB_LTM_SUPPORT;
 }
 
+static inline bool usb_device_no_sg_limit(struct usb_device *udev)
+{
+	if (udev && udev->bus && udev->bus->no_sg_limit)
+		return true;
+	else
+		return false;
+}
+
 
 /*-------------------------------------------------------------------------*/
 
@@ -1249,7 +1258,7 @@ typedef void (*usb_complete_t)(struct urb *);
  *	transfer_buffer.
  * @sg: scatter gather buffer list, the buffer size of each element in
  * 	the list (except the last) must be divisible by the endpoint's
- * 	max packet size
+ * 	max packet size if no_sg_limit isn't set in 'struct usb_bus'
  * @num_mapped_sgs: (internal) number of mapped sg entries
  * @num_sgs: number of entries in the sg list
  * @transfer_buffer_length: How big is transfer_buffer.  The transfer may
-- 
1.7.9.5

Re: [PATCH v1 1/4] USB: introduce usb_device_no_sg_limit() helper

[Alan Stern]

On Sat, 3 Aug 2013, Ming Lei wrote:

> Some host controllers(such as xHCI) can support building
> packet from discontinuous buffers, so introduce one flag
> and helper for this kind of host controllers, then the
> feature can help some applications(such as, usbnet) by
> supporting arbitrary length of sg buffers.

> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 84f14e2..5d03074 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -337,6 +337,7 @@ struct usb_bus {
>  					 * the ep queue on a short transfer
>  					 * with the URB_SHORT_NOT_OK flag set.
>  					 */
> +	unsigned no_sg_limit:1;		/* no sg list limit */

Why do you call this "no_sg_limit"?  It isn't a limit on the SG list; 
the list can be arbitrarily long, provided all the entries except the 
last are divisible by the maxpacket size.

You could call it "no_sg_constraint" if you want.  Or 
"allow_arbitrary_sg", to put a more positive spin on it.

Alan Stern

Re: [PATCH v1 1/4] USB: introduce usb_device_no_sg_limit() helper

[Ming Lei]

On Sat, Aug 3, 2013 at 11:53 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Sat, 3 Aug 2013, Ming Lei wrote:
>
>> Some host controllers(such as xHCI) can support building
>> packet from discontinuous buffers, so introduce one flag
>> and helper for this kind of host controllers, then the
>> feature can help some applications(such as, usbnet) by
>> supporting arbitrary length of sg buffers.
>
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index 84f14e2..5d03074 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -337,6 +337,7 @@ struct usb_bus {
>>                                        * the ep queue on a short transfer
>>                                        * with the URB_SHORT_NOT_OK flag set.
>>                                        */
>> +     unsigned no_sg_limit:1;         /* no sg list limit */
>
> Why do you call this "no_sg_limit"?  It isn't a limit on the SG list;
> the list can be arbitrarily long, provided all the entries except the
> last are divisible by the maxpacket size.
>
> You could call it "no_sg_constraint" if you want.  Or
> "allow_arbitrary_sg", to put a more positive spin on it.

OK, I prefer no_sg_constraint, and will do it in v2.

Thanks,
--
Ming Lei

[PATCH v1 2/4] USB: XHCI: mark no_sg_limit

[Ming Lei]

This patch marks all xHCI controllers as no_sg_limit since
xHCI supports building packet from discontinuous buffers.

Cc: Alan Stern <stern@rowland.harvard.edu>
Acked-by: Sarah Sharp <sarah.a.sharp@linux.intel.com>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/usb/host/xhci.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 2c49f00..541b155 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4841,6 +4841,10 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 
 	/* Accept arbitrarily long scatter-gather lists */
 	hcd->self.sg_tablesize = ~0;
+
+	/* support to build packet from discontinuous buffers */
+	hcd->self.no_sg_limit = 1;
+
 	/* XHCI controllers don't stop the ep queue on short packets :| */
 	hcd->self.no_stop_on_short = 1;
 
-- 
1.7.9.5

[PATCH v1 3/4] USBNET: support DMA SG

[Ming Lei]

This patch introduces support of DMA SG if the USB host controller
which usbnet device is attached to is capable of building packet from
discontinuous buffers.

The patch supports passing the skb fragment buffers to usb stack directly
via urb->sg.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Cc: Grant Grundler <grundler@google.com>
Cc: Freddy Xin <freddy@asix.com.tw>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/usbnet.c   |   38 ++++++++++++++++++++++++++++++++++++--
 include/linux/usb/usbnet.h |    1 +
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index e4811d7..51753b3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1232,6 +1232,37 @@ EXPORT_SYMBOL_GPL(usbnet_tx_timeout);
 
 /*-------------------------------------------------------------------------*/
 
+static int build_dma_sg(const struct sk_buff *skb, struct urb *urb)
+{
+	unsigned num_sgs, total_len = 0;
+	int i, s = 0;
+
+	num_sgs = skb_shinfo(skb)->nr_frags + 1;
+	if (num_sgs == 1)
+		return 0;
+
+	urb->sg = kmalloc(num_sgs * sizeof(struct scatterlist), GFP_ATOMIC);
+	if (!urb->sg)
+		return -ENOMEM;
+
+	urb->num_sgs = num_sgs;
+	sg_init_table(urb->sg, urb->num_sgs);
+
+	sg_set_buf(&urb->sg[s++], skb->data, skb_headlen(skb));
+	total_len += skb_headlen(skb);
+
+	for (i = 0; i < skb_shinfo(skb)->nr_frags; i++) {
+		struct skb_frag_struct *f = &skb_shinfo(skb)->frags[i];
+
+		total_len += skb_frag_size(f);
+		sg_set_page(&urb->sg[i + s], f->page.p, f->size,
+				f->page_offset);
+	}
+	urb->transfer_buffer_length = total_len;
+
+	return 1;
+}
+
 netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 				     struct net_device *net)
 {
@@ -1258,7 +1289,6 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 			goto drop;
 		}
 	}
-	length = skb->len;
 
 	if (!(urb = usb_alloc_urb (0, GFP_ATOMIC))) {
 		netif_dbg(dev, tx_err, dev->net, "no urb\n");
@@ -1268,10 +1298,14 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
 	entry = (struct skb_data *) skb->cb;
 	entry->urb = urb;
 	entry->dev = dev;
-	entry->length = length;
 
 	usb_fill_bulk_urb (urb, dev->udev, dev->out,
 			skb->data, skb->len, tx_complete, skb);
+	if (dev->can_dma_sg) {
+		if (build_dma_sg(skb, urb) < 0)
+			goto drop;
+	}
+	entry->length = length = urb->transfer_buffer_length;
 
 	/* don't assume the hardware handles USB_ZERO_PACKET
 	 * NOTE:  strictly conforming cdc-ether devices should expect
diff --git a/include/linux/usb/usbnet.h b/include/linux/usb/usbnet.h
index 8fbc008..9cb2fe8 100644
--- a/include/linux/usb/usbnet.h
+++ b/include/linux/usb/usbnet.h
@@ -35,6 +35,7 @@ struct usbnet {
 	unsigned char		suspend_count;
 	unsigned char		pkt_cnt, pkt_err;
 	unsigned short		rx_qlen, tx_qlen;
+	unsigned		can_dma_sg:1;
 
 	/* i/o info: pipes etc */
 	unsigned		in, out;
-- 
1.7.9.5

Re: [PATCH v1 3/4] USBNET: support DMA SG

[Oliver Neukum]

On Sat, 2013-08-03 at 10:46 +0800, Ming Lei wrote:

> @@ -1268,10 +1298,14 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
>  	entry = (struct skb_data *) skb->cb;
>  	entry->urb = urb;
>  	entry->dev = dev;
> -	entry->length = length;
>  
>  	usb_fill_bulk_urb (urb, dev->udev, dev->out,
>  			skb->data, skb->len, tx_complete, skb);
> +	if (dev->can_dma_sg) {
> +		if (build_dma_sg(skb, urb) < 0)
> +			goto drop;

Where do you free urb->sg?

	Regards
		Oliver




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

Re: [PATCH v1 3/4] USBNET: support DMA SG

[David Miller]

From: Oliver Neukum <oneukum-l3A5Bk7waGM@public.gmane.org>
Date: Sat, 03 Aug 2013 07:56:53 +0200

> On Sat, 2013-08-03 at 10:46 +0800, Ming Lei wrote:
> 
>> @@ -1268,10 +1298,14 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
>>  	entry = (struct skb_data *) skb->cb;
>>  	entry->urb = urb;
>>  	entry->dev = dev;
>> -	entry->length = length;
>>  
>>  	usb_fill_bulk_urb (urb, dev->udev, dev->out,
>>  			skb->data, skb->len, tx_complete, skb);
>> +	if (dev->can_dma_sg) {
>> +		if (build_dma_sg(skb, urb) < 0)
>> +			goto drop;
> 
> Where do you free urb->sg?

Indeed, this appears to leak.

The devio.c code in the USB layer takes care to manage freeing of
the urb->sg, so this usbnet code will have to as well.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/4] USBNET: support DMA SG

[Ming Lei]

On Sun, Aug 4, 2013 at 3:07 AM, David Miller <davem@davemloft.net> wrote:
> From: Oliver Neukum <oneukum@suse.de>
> Date: Sat, 03 Aug 2013 07:56:53 +0200
>
>> On Sat, 2013-08-03 at 10:46 +0800, Ming Lei wrote:
>>
>>> @@ -1268,10 +1298,14 @@ netdev_tx_t usbnet_start_xmit (struct sk_buff *skb,
>>>      entry = (struct skb_data *) skb->cb;
>>>      entry->urb = urb;
>>>      entry->dev = dev;
>>> -    entry->length = length;
>>>
>>>      usb_fill_bulk_urb (urb, dev->udev, dev->out,
>>>                      skb->data, skb->len, tx_complete, skb);
>>> +    if (dev->can_dma_sg) {
>>> +            if (build_dma_sg(skb, urb) < 0)
>>> +                    goto drop;
>>
>> Where do you free urb->sg?
>
> Indeed, this appears to leak.
>
> The devio.c code in the USB layer takes care to manage freeing of
> the urb->sg, so this usbnet code will have to as well.

Hamm, kfree(urb->sg) is removed carelessly in v1, sorry for that.

Will fix it in -v2.

Thanks,
--
Ming Lei

[PATCH v1 4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma

[Ming Lei]

From: Ming Lei <tom.leiming@gmail.com>

This patch enables 'can_dma_sg' flag for ax88179_178a device
if the attached host controller supports building packet from
discontinuous buffers(DMA SG is possible), so TSO can be enabled
and skb fragment buffers can be passed to usb stack via urb->sg
directly.

With the patch, system CPU utilization decreased ~50% and throughput
increased by ~10% when doing iperf client test on one ARM A15 dual
core board.

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Cc: Grant Grundler <grundler@google.com>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Freddy Xin <freddy@asix.com.tw>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/ax88179_178a.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index fb0caa2..4f8162b 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1031,12 +1031,20 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->mii.phy_id = 0x03;
 	dev->mii.supports_gmii = 1;
 
+	if (usb_device_no_sg_limit(dev->udev))
+		dev->can_dma_sg = 1;
+
 	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 			      NETIF_F_RXCSUM;
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM;
 
+	if (dev->can_dma_sg) {
+		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
+		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
+	}
+
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
 	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
@@ -1310,6 +1318,10 @@ static int ax88179_reset(struct usbnet *dev)
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM;
+	if (dev->can_dma_sg) {
+		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
+		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
+	}
 
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
-- 
1.7.9.5

Re: [PATCH v1 0/4] USB & USBNET: loose SG check and support usbnet DMA SG

[Ming Lei]

On Sat, Aug 3, 2013 at 10:46 AM, Ming Lei <ming.lei@canonical.com> wrote:
> Hi,
>
> This patchset allows drivers to pass sg buffers which size can't be divided
> by max packet size of endpoint if the host controllers(such ax xHCI) support
> this kind of sg buffers.
>
> Previously we added check[1] on the situation and don't allow these sg buffers
> passed to USB HCD, looks the check is too strict to make use of new feature of
> new hardware(xHCI) for some applications(such as network stack) which can't
> provide this kind of sg buffers to usbnet driver, so the patch looses the check
> in case that the host controller supports it.
>
> Patch 3/4 implements DMA SG on usbnet driver, and patch 4/4 uses it on ax88179_178a
> USB3 NIC for supporting TSO, so both CPU utilization and tx throughput can be
> improved with TSO and DMA SG in case of the USB NIC is attached to xHCI controller.
>
> This patchset is against Eric Dumazet's patch(ax88179_178a: avoid copy of tx
> tcp packets), so maybe it is better to merge via net-next tree.

Looks it is a bit complicated, so it can't work via net-next.

- patch 1/4 depends on current usb-next tree
- patch 2/4 depends on patch 1/4
- patch 4/4 depends on both patch 1/4 and Eric's patch

Thanks,
--
Ming Lei