[PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[freddy]

From: Freddy Xin <freddy@asix.com.tw>

Disable TSO and SG network features in reset() and bind() functions,
and check the return value of skb_linearize() in tx_fixup() to prevent
TX throttling.

Signed-off-by: Freddy Xin <freddy@asix.com.tw>
---
 drivers/net/usb/ax88179_178a.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 1e3c302..eb71331 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1029,7 +1029,7 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->mii.supports_gmii = 1;
 
 	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-			      NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+			      NETIF_F_RXCSUM;
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
@@ -1173,7 +1173,9 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 	if (((skb->len + 8) % frame_size) == 0)
 		tx_hdr2 |= 0x80008000;	/* Enable padding */
 
-	skb_linearize(skb);
+	if (skb_linearize(skb))
+		return NULL;
+
 	headroom = skb_headroom(skb);
 	tailroom = skb_tailroom(skb);
 
@@ -1317,7 +1319,7 @@ static int ax88179_reset(struct usbnet *dev)
 			  1, 1, tmp);
 
 	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-			      NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+			      NETIF_F_RXCSUM;
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
-- 
1.8.1.2

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Sat, 2013-07-20 at 17:16 +0800, freddy@asix.com.tw wrote:
> From: Freddy Xin <freddy@asix.com.tw>
> 
> Disable TSO and SG network features in reset() and bind() functions,
> and check the return value of skb_linearize() in tx_fixup() to prevent
> TX throttling.
> 
> Signed-off-by: Freddy Xin <freddy@asix.com.tw>
> ---
>  drivers/net/usb/ax88179_178a.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 1e3c302..eb71331 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -1029,7 +1029,7 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>  	dev->mii.supports_gmii = 1;
>  
>  	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> -			      NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
> +			      NETIF_F_RXCSUM;
>  
>  	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  				 NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
> @@ -1173,7 +1173,9 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
>  	if (((skb->len + 8) % frame_size) == 0)
>  		tx_hdr2 |= 0x80008000;	/* Enable padding */
>  
> -	skb_linearize(skb);
> +	if (skb_linearize(skb))
> +		return NULL;
> +
>  	

I guess that if a driver does not advertise NETIF_F_SG, this
skb_linearize() call is not needed : All frames reaching your xmit
function should already be linear

Thanks

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Ben Hutchings]

On Mon, 2013-07-22 at 10:07 -0700, Eric Dumazet wrote:
> On Sat, 2013-07-20 at 17:16 +0800, freddy@asix.com.tw wrote:
> > From: Freddy Xin <freddy@asix.com.tw>
> > 
> > Disable TSO and SG network features in reset() and bind() functions,
> > and check the return value of skb_linearize() in tx_fixup() to prevent
> > TX throttling.
> > 
> > Signed-off-by: Freddy Xin <freddy@asix.com.tw>
> > ---
> >  drivers/net/usb/ax88179_178a.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> > index 1e3c302..eb71331 100644
> > --- a/drivers/net/usb/ax88179_178a.c
> > +++ b/drivers/net/usb/ax88179_178a.c
> > @@ -1029,7 +1029,7 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
> >  	dev->mii.supports_gmii = 1;
> >  
> >  	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> > -			      NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
> > +			      NETIF_F_RXCSUM;
> >  
> >  	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
> >  				 NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
> > @@ -1173,7 +1173,9 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
> >  	if (((skb->len + 8) % frame_size) == 0)
> >  		tx_hdr2 |= 0x80008000;	/* Enable padding */
> >  
> > -	skb_linearize(skb);
> > +	if (skb_linearize(skb))
> > +		return NULL;
> > +
> >  	
> 
> I guess that if a driver does not advertise NETIF_F_SG, this
> skb_linearize() call is not needed : All frames reaching your xmit
> function should already be linear

Look at the hw_features initialisation...

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Grant Grundler]

On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
...
> I guess that if a driver does not advertise NETIF_F_SG, this
> skb_linearize() call is not needed : All frames reaching your xmit
> function should already be linear

As Ben Hutchings pointed out, hw_features is still setting this...but
I'm not sure how that matters.

ax88179_set_features() doesn't allow setting SG or TSO features.  But
I expect it would be "not too difficult" to add such that ethtool
could set those features after boot.  Or perhaps add a driver module
parameter to set these features.  I just guessing the skb_linearize()
could be removed until set_features support for SG and/or TSO is
added. Is that correct?

thanks,
grant

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Ben Hutchings]

On Mon, 2013-07-22 at 11:29 -0700, Grant Grundler wrote:
> On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> ...
> > I guess that if a driver does not advertise NETIF_F_SG, this
> > skb_linearize() call is not needed : All frames reaching your xmit
> > function should already be linear
> 
> As Ben Hutchings pointed out, hw_features is still setting this...but
> I'm not sure how that matters.
> 
> ax88179_set_features() doesn't allow setting SG or TSO features.  But
> I expect it would be "not too difficult" to add such that ethtool
> could set those features after boot.
[...]

It already can.  That's what putting feature flags in hw_features does.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Mon, 2013-07-22 at 19:38 +0100, Ben Hutchings wrote:
> On Mon, 2013-07-22 at 11:29 -0700, Grant Grundler wrote:
> > On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > ...
> > > I guess that if a driver does not advertise NETIF_F_SG, this
> > > skb_linearize() call is not needed : All frames reaching your xmit
> > > function should already be linear
> > 
> > As Ben Hutchings pointed out, hw_features is still setting this...but
> > I'm not sure how that matters.
> > 
> > ax88179_set_features() doesn't allow setting SG or TSO features.  But
> > I expect it would be "not too difficult" to add such that ethtool
> > could set those features after boot.
> [...]
> 
> It already can.  That's what putting feature flags in hw_features does.

My original concern, that inspired this patch, was to remove SG support,
as this driver does not have SG support at all.

Linearize a full TSO packet needs order-5 allocations, thats likely to
fail and lead to very slow TCP performance, because it will only rely on
retransmits.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Ben Hutchings]

On Mon, 2013-07-22 at 11:47 -0700, Eric Dumazet wrote:
> On Mon, 2013-07-22 at 19:38 +0100, Ben Hutchings wrote:
> > On Mon, 2013-07-22 at 11:29 -0700, Grant Grundler wrote:
> > > On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > ...
> > > > I guess that if a driver does not advertise NETIF_F_SG, this
> > > > skb_linearize() call is not needed : All frames reaching your xmit
> > > > function should already be linear
> > > 
> > > As Ben Hutchings pointed out, hw_features is still setting this...but
> > > I'm not sure how that matters.
> > > 
> > > ax88179_set_features() doesn't allow setting SG or TSO features.  But
> > > I expect it would be "not too difficult" to add such that ethtool
> > > could set those features after boot.
> > [...]
> > 
> > It already can.  That's what putting feature flags in hw_features does.
> 
> My original concern, that inspired this patch, was to remove SG support,
> as this driver does not have SG support at all.
> 
> Linearize a full TSO packet needs order-5 allocations, thats likely to
> fail and lead to very slow TCP performance, because it will only rely on
> retransmits.

The driver could set gso_max_size to reduce that problem.  But I rather
doubt that TSO followed by skb_linearize() significantly improves
throughput or CPU-efficiency.  (If the device has a 1G link but is
connected to the host through a USB 2.0 port, then USB is the bottleneck
and TSO could improve throughput a few percent.  But that's a silly
configuration.)

The real solution would be for someone to add SG support to the usbnet
core.  Trying to support 1GbE with only linear skbs is not a great
idea... and it can only be a matter of time before there is USB ultra
speed (or whatever comes after 'super') with 10GbE devices...

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Mon, 2013-07-22 at 20:47 +0100, Ben Hutchings wrote:
> On Mon, 2013-07-22 at 11:47 -0700, Eric Dumazet wrote:
> > On Mon, 2013-07-22 at 19:38 +0100, Ben Hutchings wrote:
> > > On Mon, 2013-07-22 at 11:29 -0700, Grant Grundler wrote:
> > > > On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > > > ...
> > > > > I guess that if a driver does not advertise NETIF_F_SG, this
> > > > > skb_linearize() call is not needed : All frames reaching your xmit
> > > > > function should already be linear
> > > > 
> > > > As Ben Hutchings pointed out, hw_features is still setting this...but
> > > > I'm not sure how that matters.
> > > > 
> > > > ax88179_set_features() doesn't allow setting SG or TSO features.  But
> > > > I expect it would be "not too difficult" to add such that ethtool
> > > > could set those features after boot.
> > > [...]
> > > 
> > > It already can.  That's what putting feature flags in hw_features does.
> > 
> > My original concern, that inspired this patch, was to remove SG support,
> > as this driver does not have SG support at all.
> > 
> > Linearize a full TSO packet needs order-5 allocations, thats likely to
> > fail and lead to very slow TCP performance, because it will only rely on
> > retransmits.
> 
> The driver could set gso_max_size to reduce that problem.  But I rather
> doubt that TSO followed by skb_linearize() significantly improves
> throughput or CPU-efficiency.  (If the device has a 1G link but is
> connected to the host through a USB 2.0 port, then USB is the bottleneck
> and TSO could improve throughput a few percent.  But that's a silly
> configuration.)
> 
> The real solution would be for someone to add SG support to the usbnet
> core.  Trying to support 1GbE with only linear skbs is not a great
> idea... and it can only be a matter of time before there is USB ultra
> speed (or whatever comes after 'super') with 10GbE devices...
> 

This sounds a good idea.

Is anybody working on adding SG to usbnet ?

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 22 Jul 2013 23:10:27 -0700

> On Mon, 2013-07-22 at 20:47 +0100, Ben Hutchings wrote:
>> The real solution would be for someone to add SG support to the usbnet
>> core.  Trying to support 1GbE with only linear skbs is not a great
>> idea... and it can only be a matter of time before there is USB ultra
>> speed (or whatever comes after 'super') with 10GbE devices...
>> 
> 
> This sounds a good idea.
> 
> Is anybody working on adding SG to usbnet ?

This is a good long-term plan, but right now we have to do something
reasonable.

A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have
this problem.

Instead of the patch starting this thread, I'd like to see one that
hits all three drivers and removes all SG and TSO features bits from
both the ->features _and_ ->hw_features settings.

Could someone toss that together quickly?

Thanks.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Tue, 2013-07-23 at 16:46 -0700, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Mon, 22 Jul 2013 23:10:27 -0700
> 
> > On Mon, 2013-07-22 at 20:47 +0100, Ben Hutchings wrote:
> >> The real solution would be for someone to add SG support to the usbnet
> >> core.  Trying to support 1GbE with only linear skbs is not a great
> >> idea... and it can only be a matter of time before there is USB ultra
> >> speed (or whatever comes after 'super') with 10GbE devices...
> >> 
> > 
> > This sounds a good idea.
> > 
> > Is anybody working on adding SG to usbnet ?
> 
> This is a good long-term plan, but right now we have to do something
> reasonable.
> 
> A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have
> this problem.
> 
> Instead of the patch starting this thread, I'd like to see one that
> hits all three drivers and removes all SG and TSO features bits from
> both the ->features _and_ ->hw_features settings.
> 
> Could someone toss that together quickly?

Yes, I can prepare this patch.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Tue, 2013-07-23 at 16:56 -0700, Eric Dumazet wrote:

> > A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have
> > this problem.
> > 
> > Instead of the patch starting this thread, I'd like to see one that
> > hits all three drivers and removes all SG and TSO features bits from
> > both the ->features _and_ ->hw_features settings.
> > 
> > Could someone toss that together quickly?
> 
> Yes, I can prepare this patch.

Looks like only ax88179_178a & smsc75xx are impacted.

[PATCH] usbnet: do not pretend to support SG/TSO

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

usbnet doesn't support yet SG, so drivers should not advertise SG or TSO
capabilities, as they allow TCP stack to build large TSO packets that 
need to be linearized and might use order-5 pages.

This adds an extra copy overhead and possible allocation failures.

Current code ignore skb_linearize() return code so crashes are even
possible.

Best is to not pretend SG/TSO is supported, and add this again when/if
usbnet really supports SG for devices who could get a performance gain.

Based on a prior patch from Freddy Xin <freddy@asix.com.tw>

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 drivers/net/usb/ax88179_178a.c |    9 ++++-----
 drivers/net/usb/smsc75xx.c     |   12 +++---------
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index 1e3c302..2bc87e3 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1029,10 +1029,10 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->mii.supports_gmii = 1;
 
 	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-			      NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+			      NETIF_F_RXCSUM;
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-				 NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+				 NETIF_F_RXCSUM;
 
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
@@ -1173,7 +1173,6 @@ ax88179_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 	if (((skb->len + 8) % frame_size) == 0)
 		tx_hdr2 |= 0x80008000;	/* Enable padding */
 
-	skb_linearize(skb);
 	headroom = skb_headroom(skb);
 	tailroom = skb_tailroom(skb);
 
@@ -1317,10 +1316,10 @@ static int ax88179_reset(struct usbnet *dev)
 			  1, 1, tmp);
 
 	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-			      NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+			      NETIF_F_RXCSUM;
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-				 NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_TSO;
+				 NETIF_F_RXCSUM;
 
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c
index 7540974..66ebbac 100644
--- a/drivers/net/usb/smsc75xx.c
+++ b/drivers/net/usb/smsc75xx.c
@@ -45,7 +45,6 @@
 #define EEPROM_MAC_OFFSET		(0x01)
 #define DEFAULT_TX_CSUM_ENABLE		(true)
 #define DEFAULT_RX_CSUM_ENABLE		(true)
-#define DEFAULT_TSO_ENABLE		(true)
 #define SMSC75XX_INTERNAL_PHY_ID	(1)
 #define SMSC75XX_TX_OVERHEAD		(8)
 #define MAX_RX_FIFO_SIZE		(20 * 1024)
@@ -1410,17 +1409,14 @@ static int smsc75xx_bind(struct usbnet *dev, struct usb_interface *intf)
 
 	INIT_WORK(&pdata->set_multicast, smsc75xx_deferred_multicast_write);
 
-	if (DEFAULT_TX_CSUM_ENABLE) {
+	if (DEFAULT_TX_CSUM_ENABLE)
 		dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
-		if (DEFAULT_TSO_ENABLE)
-			dev->net->features |= NETIF_F_SG |
-				NETIF_F_TSO | NETIF_F_TSO6;
-	}
+
 	if (DEFAULT_RX_CSUM_ENABLE)
 		dev->net->features |= NETIF_F_RXCSUM;
 
 	dev->net->hw_features = NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
-		NETIF_F_SG | NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_RXCSUM;
+				NETIF_F_RXCSUM;
 
 	ret = smsc75xx_wait_ready(dev, 0);
 	if (ret < 0) {
@@ -2200,8 +2196,6 @@ static struct sk_buff *smsc75xx_tx_fixup(struct usbnet *dev,
 {
 	u32 tx_cmd_a, tx_cmd_b;
 
-	skb_linearize(skb);
-
 	if (skb_headroom(skb) < SMSC75XX_TX_OVERHEAD) {
 		struct sk_buff *skb2 =
 			skb_copy_expand(skb, SMSC75XX_TX_OVERHEAD, 0, flags);

Re: [PATCH] usbnet: do not pretend to support SG/TSO

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 23 Jul 2013 17:15:54 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> usbnet doesn't support yet SG, so drivers should not advertise SG or TSO
> capabilities, as they allow TCP stack to build large TSO packets that 
> need to be linearized and might use order-5 pages.
> 
> This adds an extra copy overhead and possible allocation failures.
> 
> Current code ignore skb_linearize() return code so crashes are even
> possible.
> 
> Best is to not pretend SG/TSO is supported, and add this again when/if
> usbnet really supports SG for devices who could get a performance gain.
> 
> Based on a prior patch from Freddy Xin <freddy@asix.com.tw>
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 23 Jul 2013 17:05:10 -0700

> On Tue, 2013-07-23 at 16:56 -0700, Eric Dumazet wrote:
> 
>> > A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have
>> > this problem.
>> > 
>> > Instead of the patch starting this thread, I'd like to see one that
>> > hits all three drivers and removes all SG and TSO features bits from
>> > both the ->features _and_ ->hw_features settings.
>> > 
>> > Could someone toss that together quickly?
>> 
>> Yes, I can prepare this patch.
> 
> Looks like only ax88179_178a & smsc75xx are impacted.

Indeed, smsc95xx doesn't set SG or TSO.

Sorry about that.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Grant Grundler]

On Tue, Jul 23, 2013 at 4:46 PM, David Miller <davem@davemloft.net> wrote:
...
> A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have
> this problem.
>
> Instead of the patch starting this thread, I'd like to see one that
> hits all three drivers and removes all SG and TSO features bits from
> both the ->features _and_ ->hw_features settings.

Since you are asking to remove TSO, do you also want skb_linearize()
calls in ax88179_178a.c and smsc75xx.c removed as well?

Not part of the original patch - but based on this thread, Eric seems
to think calling skb_linearize isn't necessary or helpful either.

cheers,
grant

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Grant Grundler]

On Tue, Jul 23, 2013 at 7:29 PM, Grant Grundler <grundler@google.com> wrote:
> On Tue, Jul 23, 2013 at 4:46 PM, David Miller <davem@davemloft.net> wrote:
> ...
>> A quick scan shows that smsc75xx, smsc95xx, and ax88179_178a all have
>> this problem.
>>
>> Instead of the patch starting this thread, I'd like to see one that
>> hits all three drivers and removes all SG and TSO features bits from
>> both the ->features _and_ ->hw_features settings.
>
> Since you are asking to remove TSO, do you also want skb_linearize()
> calls in ax88179_178a.c and smsc75xx.c removed as well?

Nevermind...Eric already removed skb_linearize calls in his patch.

cheers,
grant

>
> Not part of the original patch - but based on this thread, Eric seems
> to think calling skb_linearize isn't necessary or helpful either.
>
> cheers,
> grant

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Ming Lei]

On Tue, Jul 23, 2013 at 2:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2013-07-22 at 20:47 +0100, Ben Hutchings wrote:
>> On Mon, 2013-07-22 at 11:47 -0700, Eric Dumazet wrote:
>> > On Mon, 2013-07-22 at 19:38 +0100, Ben Hutchings wrote:
>> > > On Mon, 2013-07-22 at 11:29 -0700, Grant Grundler wrote:
>> > > > On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > > > ...
>> > > > > I guess that if a driver does not advertise NETIF_F_SG, this
>> > > > > skb_linearize() call is not needed : All frames reaching your xmit
>> > > > > function should already be linear
>> > > >
>> > > > As Ben Hutchings pointed out, hw_features is still setting this...but
>> > > > I'm not sure how that matters.
>> > > >
>> > > > ax88179_set_features() doesn't allow setting SG or TSO features.  But
>> > > > I expect it would be "not too difficult" to add such that ethtool
>> > > > could set those features after boot.
>> > > [...]
>> > >
>> > > It already can.  That's what putting feature flags in hw_features does.
>> >
>> > My original concern, that inspired this patch, was to remove SG support,
>> > as this driver does not have SG support at all.
>> >
>> > Linearize a full TSO packet needs order-5 allocations, thats likely to
>> > fail and lead to very slow TCP performance, because it will only rely on
>> > retransmits.
>>
>> The driver could set gso_max_size to reduce that problem.  But I rather
>> doubt that TSO followed by skb_linearize() significantly improves
>> throughput or CPU-efficiency.  (If the device has a 1G link but is
>> connected to the host through a USB 2.0 port, then USB is the bottleneck
>> and TSO could improve throughput a few percent.  But that's a silly
>> configuration.)
>>
>> The real solution would be for someone to add SG support to the usbnet
>> core.  Trying to support 1GbE with only linear skbs is not a great
>> idea... and it can only be a matter of time before there is USB ultra
>> speed (or whatever comes after 'super') with 10GbE devices...
>>
>
> This sounds a good idea.
>
> Is anybody working on adding SG to usbnet ?

It depends if size of sg buffer(except for last one) in the sg list can be
divided by usb endpoint's max packet size(512 or 1024), at least there
is the constraint:

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

I am wondering if network stack can meet that.  If not, it might be a
bit difficult
because lots of USB host controller don't support that, and driver may have
to support SG and non-SG at the same time for working well on all HCs.

Thanks,
-- 
Ming Lei

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Thu, 2013-07-25 at 10:28 +0800, Ming Lei wrote:

> 
> It depends if size of sg buffer(except for last one) in the sg list can be
> divided by usb endpoint's max packet size(512 or 1024), at least there
> is the constraint:
> 
> http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=10e232c597ac757e7f8600649f7e872e86de190f
> 
> I am wondering if network stack can meet that.  If not, it might be a
> bit difficult
> because lots of USB host controller don't support that, and driver may have
> to support SG and non-SG at the same time for working well on all HCs.

I do not see the problem.

If one skb has 2 fragments of 32KB, couldn't they be split into 64 1K
segments by the device driver ?

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Ming Lei]

On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-07-25 at 10:28 +0800, Ming Lei wrote:
>
>>
>> It depends if size of sg buffer(except for last one) in the sg list can be
>> divided by usb endpoint's max packet size(512 or 1024), at least there
>> is the constraint:
>>
>> http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=10e232c597ac757e7f8600649f7e872e86de190f
>>
>> I am wondering if network stack can meet that.  If not, it might be a
>> bit difficult
>> because lots of USB host controller don't support that, and driver may have
>> to support SG and non-SG at the same time for working well on all HCs.
>
> I do not see the problem.
>
> If one skb has 2 fragments of 32KB, couldn't they be split into 64 1K
> segments by the device driver ?

OK, if length of fragments of all SKBs from network stack can always guarantee
to be divided by 1024, that is fine,  seems I worry about too much, :-)

Thanks,
-- 
Ming Lei

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Thu, 2013-07-25 at 13:25 +0800, Ming Lei wrote:
> On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2013-07-25 at 10:28 +0800, Ming Lei wrote:
> >
> >>
> >> It depends if size of sg buffer(except for last one) in the sg list can be
> >> divided by usb endpoint's max packet size(512 or 1024), at least there
> >> is the constraint:
> >>
> >> http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=10e232c597ac757e7f8600649f7e872e86de190f
> >>
> >> I am wondering if network stack can meet that.  If not, it might be a
> >> bit difficult
> >> because lots of USB host controller don't support that, and driver may have
> >> to support SG and non-SG at the same time for working well on all HCs.
> >
> > I do not see the problem.
> >
> > If one skb has 2 fragments of 32KB, couldn't they be split into 64 1K
> > segments by the device driver ?
> 
> OK, if length of fragments of all SKBs from network stack can always guarantee
> to be divided by 1024, that is fine,  seems I worry about too much, :-)

Unfortunately, there is no such guarantee. TSO permits sendfile() zero
copy operation, so the frags can be of any size, any offset...

In this mode, the first element (skb->head) will typically contains the
headers, and there are way below 512 bytes.

So even with lowering netdev->gso_max_size under PAGE_SIZE, most of the
packets will need to be copied into a single segment.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Ming Lei]

On Thu, Jul 25, 2013 at 7:01 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2013-07-25 at 13:25 +0800, Ming Lei wrote:
>> On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Thu, 2013-07-25 at 10:28 +0800, Ming Lei wrote:
>> >
>> >>
>> >> It depends if size of sg buffer(except for last one) in the sg list can be
>> >> divided by usb endpoint's max packet size(512 or 1024), at least there
>> >> is the constraint:
>> >>
>> >> http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=10e232c597ac757e7f8600649f7e872e86de190f
>> >>
>> >> I am wondering if network stack can meet that.  If not, it might be a
>> >> bit difficult
>> >> because lots of USB host controller don't support that, and driver may have
>> >> to support SG and non-SG at the same time for working well on all HCs.
>> >
>> > I do not see the problem.
>> >
>> > If one skb has 2 fragments of 32KB, couldn't they be split into 64 1K
>> > segments by the device driver ?
>>
>> OK, if length of fragments of all SKBs from network stack can always guarantee
>> to be divided by 1024, that is fine,  seems I worry about too much, :-)
>
> Unfortunately, there is no such guarantee. TSO permits sendfile() zero
> copy operation, so the frags can be of any size, any offset...

USB protocol doesn't care offset or buffer start address, but has requirement
on sg buffer size(except for last one) in sg list.

>
> In this mode, the first element (skb->head) will typically contains the
> headers, and there are way below 512 bytes.
>
> So even with lowering netdev->gso_max_size under PAGE_SIZE, most of the
> packets will need to be copied into a single segment.

Maybe need to try it with TSO enabled, in my test on ax88179_178a NIC after
applying your disabling TSO patch, tx throughput is less than 600Mbps, but rx
is close to 900Mbps.

On Thu, Jul 25, 2013 at 9:34 PM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
>
> Not that I have any experience with USB drivers, but perhaps
> usb_sg_init()?

USB SG library doesn't support submitting SG URB asynchronously, but that isn't
a big deal.  The problem is that many USB host controllers can't build
one single
packet from two buffers, what is why USB stack requires size of all
buffers(except
for last one) in sg list can be divided by max endpoint packet
size.(1024 for usbnet)

Thanks,
-- 
Ming Lei

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Ben Hutchings]

On Thu, 2013-07-25 at 22:52 +0800, Ming Lei wrote:
[...]
> On Thu, Jul 25, 2013 at 9:34 PM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
> >
> > Not that I have any experience with USB drivers, but perhaps
> > usb_sg_init()?
> 
> USB SG library doesn't support submitting SG URB asynchronously, but that isn't
> a big deal.  The problem is that many USB host controllers can't build
> one single
> packet from two buffers, what is why USB stack requires size of all
> buffers(except
> for last one) in sg list can be divided by max endpoint packet
> size.(1024 for usbnet)

Ugh.  Maybe the USB stack should allow drivers to find out about and
take advantage of a more flexible host controller.  Sounds like it could
be a big job, though.

Ben. (glad not to be using any USB net devices)

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Eric Dumazet]

On Thu, 2013-07-25 at 22:52 +0800, Ming Lei wrote:

> Maybe need to try it with TSO enabled, in my test on ax88179_178a NIC after
> applying your disabling TSO patch, tx throughput is less than 600Mbps, but rx
> is close to 900Mbps.

It looks like TCP stack could for this case allocate linear skbs
(GFP_KERNEL context), using order-3 pages, and not adding frags on them,
to avoid the skb_linearize() hazard (in GFP_ATOMIC)

In case of retransmits, skb are small (one MSS) so the skb_linearize()
should succeed most of the time.

Re: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

[Ben Hutchings]

On Thu, 2013-07-25 at 13:25 +0800, Ming Lei wrote:
> On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > On Thu, 2013-07-25 at 10:28 +0800, Ming Lei wrote:
> >
> >>
> >> It depends if size of sg buffer(except for last one) in the sg list can be
> >> divided by usb endpoint's max packet size(512 or 1024), at least there
> >> is the constraint:
> >>
> >> http://git.kernel.org/cgit/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next&id=10e232c597ac757e7f8600649f7e872e86de190f
> >>
> >> I am wondering if network stack can meet that.  If not, it might be a
> >> bit difficult
> >> because lots of USB host controller don't support that, and driver may have
> >> to support SG and non-SG at the same time for working well on all HCs.
> >
> > I do not see the problem.
> >
> > If one skb has 2 fragments of 32KB, couldn't they be split into 64 1K
> > segments by the device driver ?
> 
> OK, if length of fragments of all SKBs from network stack can always guarantee
> to be divided by 1024, that is fine,  seems I worry about too much, :-)

Not that I have any experience with USB drivers, but perhaps
usb_sg_init()?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.