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

login
register
mail settings
Submitter Freddy Xin
Date July 20, 2013, 9:16 a.m.
Message ID <1374311809-4155-1-git-send-email-freddy@asix.com.tw>
Download mbox | patch
Permalink /patch/260429/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Freddy Xin - July 20, 2013, 9:16 a.m.
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(-)
Eric Dumazet - July 22, 2013, 5:07 p.m.
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


--
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
Ben Hutchings - July 22, 2013, 5:11 p.m.
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.
Grant Grundler - July 22, 2013, 6:29 p.m.
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
--
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
Ben Hutchings - July 22, 2013, 6:38 p.m.
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.
Eric Dumazet - July 22, 2013, 6:47 p.m.
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.



--
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
Ben Hutchings - July 22, 2013, 7:47 p.m.
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.
Eric Dumazet - July 23, 2013, 6:10 a.m.
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 ?



--
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
David Miller - July 23, 2013, 11:46 p.m.
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.
--
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
Eric Dumazet - July 23, 2013, 11:56 p.m.
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.


--
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
Eric Dumazet - July 24, 2013, 12:05 a.m.
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.


--
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
David Miller - July 24, 2013, 12:17 a.m.
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.
--
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
Grant Grundler - July 24, 2013, 2:29 a.m.
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
--
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
Grant Grundler - July 24, 2013, 2:32 a.m.
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
--
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
tom.leiming@gmail.com - July 25, 2013, 2:28 a.m.
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,
Eric Dumazet - July 25, 2013, 5:10 a.m.
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 ?



--
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
tom.leiming@gmail.com - July 25, 2013, 5:25 a.m.
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,
Eric Dumazet - July 25, 2013, 11:01 a.m.
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.



--
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
Ben Hutchings - July 25, 2013, 1:34 p.m.
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.
tom.leiming@gmail.com - July 25, 2013, 2:52 p.m.
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,
Ben Hutchings - July 25, 2013, 3 p.m.
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)
Eric Dumazet - July 25, 2013, 3:11 p.m.
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.



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

Patch

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;