2013-07-20 09:50:13

by Freddy Xin

[permalink] [raw]
Subject: [PATCH 1/1] TX throttling bug-fixing patch of AX88179_178A

From: Freddy Xin <[email protected]>

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 <[email protected]>
---
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


2013-07-22 17:08:05

by Eric Dumazet

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

On Sat, 2013-07-20 at 17:16 +0800, [email protected] wrote:
> From: Freddy Xin <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2013-07-22 17:11:21

by Ben Hutchings

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

On Mon, 2013-07-22 at 10:07 -0700, Eric Dumazet wrote:
> On Sat, 2013-07-20 at 17:16 +0800, [email protected] wrote:
> > From: Freddy Xin <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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.

2013-07-22 18:29:20

by Grant Grundler

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

On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet <[email protected]> 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

2013-07-22 18:38:39

by Ben Hutchings

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

On Mon, 2013-07-22 at 11:29 -0700, Grant Grundler wrote:
> On Mon, Jul 22, 2013 at 10:07 AM, Eric Dumazet <[email protected]> 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.

2013-07-22 18:47:28

by Eric Dumazet

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

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 <[email protected]> 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.


2013-07-22 19:47:59

by Ben Hutchings

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

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 <[email protected]> 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.

2013-07-23 06:10:33

by Eric Dumazet

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

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 <[email protected]> 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 ?


2013-07-23 23:46:58

by David Miller

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

From: Eric Dumazet <[email protected]>
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.

2013-07-23 23:56:58

by Eric Dumazet

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

On Tue, 2013-07-23 at 16:46 -0700, David Miller wrote:
> From: Eric Dumazet <[email protected]>
> 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.

2013-07-24 00:05:16

by Eric Dumazet

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

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.

2013-07-24 00:16:02

by Eric Dumazet

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

From: Eric Dumazet <[email protected]>

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 <[email protected]>

Signed-off-by: Eric Dumazet <[email protected]>
---
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);

2013-07-24 00:17:17

by David Miller

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

From: Eric Dumazet <[email protected]>
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.

2013-07-24 02:29:16

by Grant Grundler

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

On Tue, Jul 23, 2013 at 4:46 PM, David Miller <[email protected]> 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

2013-07-24 02:32:11

by Grant Grundler

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

On Tue, Jul 23, 2013 at 7:29 PM, Grant Grundler <[email protected]> wrote:
> On Tue, Jul 23, 2013 at 4:46 PM, David Miller <[email protected]> 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

2013-07-25 02:28:27

by Ming Lei

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

On Tue, Jul 23, 2013 at 2:10 PM, Eric Dumazet <[email protected]> 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 <[email protected]> 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

2013-07-25 05:11:05

by Eric Dumazet

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

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 ?


2013-07-25 05:25:16

by Ming Lei

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

On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet <[email protected]> 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

2013-07-25 11:01:48

by Eric Dumazet

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

On Thu, 2013-07-25 at 13:25 +0800, Ming Lei wrote:
> On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet <[email protected]> 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.


2013-07-25 13:34:17

by Ben Hutchings

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

On Thu, 2013-07-25 at 13:25 +0800, Ming Lei wrote:
> On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet <[email protected]> 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.

2013-07-25 14:52:17

by Ming Lei

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

On Thu, Jul 25, 2013 at 7:01 PM, Eric Dumazet <[email protected]> wrote:
> On Thu, 2013-07-25 at 13:25 +0800, Ming Lei wrote:
>> On Thu, Jul 25, 2013 at 1:10 PM, Eric Dumazet <[email protected]> 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
<[email protected]> 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

2013-07-25 15:00:42

by Ben Hutchings

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

On Thu, 2013-07-25 at 22:52 +0800, Ming Lei wrote:
[...]
> On Thu, Jul 25, 2013 at 9:34 PM, Ben Hutchings
> <[email protected]> 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.

2013-07-25 15:11:35

by Eric Dumazet

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

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.


2013-07-26 20:49:05

by David Miller

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

From: Eric Dumazet <[email protected]>
Date: Tue, 23 Jul 2013 17:15:54 -0700

> From: Eric Dumazet <[email protected]>
>
> 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 <[email protected]>
>
> Signed-off-by: Eric Dumazet <[email protected]>

Applied, thanks Eric.