| Message ID | 1374311809-4155-1-git-send-email-freddy@asix.com.tw |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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
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.
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
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.
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
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.
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
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
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
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
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
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
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
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,
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
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,
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
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.
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,
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)
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
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;