Patchwork [v3,4/4] USBNET: ax88179_178a: enable tso if usb host supports sg dma

login
register
mail settings
Submitter Ming Lei
Date Aug. 6, 2013, 12:52 a.m.
Message ID <1375750370-18194-5-git-send-email-ming.lei@canonical.com>
Download mbox | patch
Permalink /patch/264825/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

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

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

Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
Cc: Grant Grundler <grundler@google.com>
Cc: Oliver Neukum <oneukum@suse.de>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: Freddy Xin <freddy@asix.com.tw>
Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 drivers/net/usb/ax88179_178a.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
Eric Dumazet - Aug. 6, 2013, 12:22 p.m.
On Tue, 2013-08-06 at 08:52 +0800, Ming Lei wrote:
> This patch enables 'can_dma_sg' flag for ax88179_178a device
> if the attached host controller supports building packet from
> discontinuous buffers(DMA SG is possible), so TSO can be enabled
> and skb fragment buffers can be passed to usb stack via urb->sg
> directly.
> 
> With the patch, system CPU utilization decreased ~50% and throughput
> increased by ~10% when doing iperf client test on one ARM A15 dual
> core board.
> 

Nice ;)

>  	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
> @@ -1310,6 +1318,10 @@ static int ax88179_reset(struct usbnet *dev)
>  
>  	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  				 NETIF_F_RXCSUM;
> +	if (dev->can_dma_sg) {
> +		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
> +		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
> +	}
>  

My concern with setting TSO on reset() is the following :

Admin can disable TSO with

ethtool -K ethX tso off


Then, one hour later, or one month later, a reset happens, and this code
magically re-enables TSO

So, I really think this part should be removed from your 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
Ming Lei - Aug. 6, 2013, 3:07 p.m.
On Tue, Aug 6, 2013 at 8:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2013-08-06 at 08:52 +0800, Ming Lei wrote:
>> This patch enables 'can_dma_sg' flag for ax88179_178a device
>> if the attached host controller supports building packet from
>> discontinuous buffers(DMA SG is possible), so TSO can be enabled
>> and skb fragment buffers can be passed to usb stack via urb->sg
>> directly.
>>
>> With the patch, system CPU utilization decreased ~50% and throughput
>> increased by ~10% when doing iperf client test on one ARM A15 dual
>> core board.
>>
>
> Nice ;)
>
>>              AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
>> @@ -1310,6 +1318,10 @@ static int ax88179_reset(struct usbnet *dev)
>>
>>       dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>>                                NETIF_F_RXCSUM;
>> +     if (dev->can_dma_sg) {
>> +             dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
>> +             dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
>> +     }
>>
>
> My concern with setting TSO on reset() is the following :
>
> Admin can disable TSO with
>
> ethtool -K ethX tso off
>
>
> Then, one hour later, or one month later, a reset happens, and this code
> magically re-enables TSO

The reset only happens during open(), and TSO can't be re-enabled magically,
unless the interface is re-opened by Admin.

>
> So, I really think this part should be removed from your patch.

OK, will remove it.


Thanks,
--
Ming Lei
--
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 - Aug. 6, 2013, 5:09 p.m.
On Tue, Aug 6, 2013 at 5:22 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
...
>> @@ -1310,6 +1318,10 @@ static int ax88179_reset(struct usbnet *dev)
>>
>>       dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>>                                NETIF_F_RXCSUM;
>> +     if (dev->can_dma_sg) {
>> +             dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
>> +             dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
>> +     }
>>
>
> My concern with setting TSO on reset() is the following :
>
> Admin can disable TSO with
>
> ethtool -K ethX tso off
>
>
> Then, one hour later, or one month later, a reset happens, and this code
> magically re-enables TSO
>
> So, I really think this part should be removed from your patch.

Following that logic, shouldn't all the features/hw_features settings
be removed from reset code path?

hw_features shouldn't change since power up.

FWIW, I do agree with you.

I'll note that any "hiccup" in the USB side that causes the device to
get dropped and re-probed will cause the same symptom. There is
nothing the driver can do about it in this case. Perhaps add some udev
rules to preserve ethtool settings the same way I've seen udev rules
to record MAC address to enumerate devices (eth0, eth1, etc.)

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
Ming Lei - Aug. 7, 2013, 12:41 a.m.
On Wed, Aug 7, 2013 at 1:09 AM, Grant Grundler <grundler@google.com> wrote:
> On Tue, Aug 6, 2013 at 5:22 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> ...
>>> @@ -1310,6 +1318,10 @@ static int ax88179_reset(struct usbnet *dev)
>>>
>>>       dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>>>                                NETIF_F_RXCSUM;
>>> +     if (dev->can_dma_sg) {
>>> +             dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
>>> +             dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
>>> +     }
>>>
>>
>> My concern with setting TSO on reset() is the following :
>>
>> Admin can disable TSO with
>>
>> ethtool -K ethX tso off
>>
>>
>> Then, one hour later, or one month later, a reset happens, and this code
>> magically re-enables TSO
>>
>> So, I really think this part should be removed from your patch.
>
> Following that logic, shouldn't all the features/hw_features settings
> be removed from reset code path?

This patch won't touch other settings because that isn't related with
this patch.

>
> hw_features shouldn't change since power up.
> FWIW, I do agree with you.
>
> I'll note that any "hiccup" in the USB side that causes the device to
> get dropped and re-probed will cause the same symptom. There is

I am afraid that PCI network devices' setting still won't survive unbound&
re-probed, will they?

> nothing the driver can do about it in this case. Perhaps add some udev
> rules to preserve ethtool settings the same way I've seen udev rules
> to record MAC address to enumerate devices (eth0, eth1, etc.)

Some usbnet devices may have random MAC address assigned in every
probe().


Thanks,
--
Ming Lei
--
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 - Aug. 8, 2013, 5:25 p.m.
On Tue, Aug 6, 2013 at 5:41 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Wed, Aug 7, 2013 at 1:09 AM, Grant Grundler <grundler@google.com> wrote:
...
>> Following that logic, shouldn't all the features/hw_features settings
>> be removed from reset code path?
>
> This patch won't touch other settings because that isn't related with
> this patch.

Sorry - I didn't mean to imply your patch should do this. I was hoping
for a yes/no answer from you, Eric, or Dave.

...
>> I'll note that any "hiccup" in the USB side that causes the device to
>> get dropped and re-probed will cause the same symptom. There is
>
> I am afraid that PCI network devices' setting still won't survive unbound&
> re-probed, will they?

Correct - but PCI isn't as prone to "dropping off the bus" like USB
is. Master aborts on some PCI systems is a "Fatal Exception" and AFAIK
that's never been true for any USB device.

>> nothing the driver can do about it in this case. Perhaps add some udev
>> rules to preserve ethtool settings the same way I've seen udev rules
>> to record MAC address to enumerate devices (eth0, eth1, etc.)
>
> Some usbnet devices may have random MAC address assigned in every
> probe().

Ugh - ok. Then those scripts are broken for those devices. C'est la Vie.

My point is the mechanism (udev) exists to preserve any settings
ethtool can read/record the state of.

cheers,
grant

ps. thanks again for posting the USBNET sg dma series - I will likely
back port those to our chromeos-3.8 tree in the near future.
--
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
Ming Lei - Aug. 8, 2013, 11:48 p.m.
On Fri, Aug 9, 2013 at 1:25 AM, Grant Grundler <grundler@google.com> wrote:
> On Tue, Aug 6, 2013 at 5:41 PM, Ming Lei <ming.lei@canonical.com> wrote:
>> On Wed, Aug 7, 2013 at 1:09 AM, Grant Grundler <grundler@google.com> wrote:
> ...
>>> Following that logic, shouldn't all the features/hw_features settings
>>> be removed from reset code path?
>>
>> This patch won't touch other settings because that isn't related with
>> this patch.
>
> Sorry - I didn't mean to imply your patch should do this. I was hoping
> for a yes/no answer from you, Eric, or Dave.
>
> ...
>>> I'll note that any "hiccup" in the USB side that causes the device to
>>> get dropped and re-probed will cause the same symptom. There is
>>
>> I am afraid that PCI network devices' setting still won't survive unbound&
>> re-probed, will they?
>
> Correct - but PCI isn't as prone to "dropping off the bus" like USB

As far as I know, USB device still won't be disconnected easily, and
reset is possible, but we can make setting survive reset by implementing
.pre_reset() and .post_reset() callback. Or do you have other situation
of USB 'dropping off the bus'?

> is. Master aborts on some PCI systems is a "Fatal Exception" and AFAIK
> that's never been true for any USB device.

I mean rmmod & modprobe still can reset setting of one PCI network
device after powering on the device, can't it?


Thanks,
--
Ming Lei
--
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 - Aug. 9, 2013, 12:18 a.m.
Ming,
We are splitting hairs now. :) I want to be clear I think your changes
are good and the rest of this conversation is just to learn something
new.

On Thu, Aug 8, 2013 at 4:48 PM, Ming Lei <ming.lei@canonical.com> wrote:
> On Fri, Aug 9, 2013 at 1:25 AM, Grant Grundler <grundler@google.com> wrote:
...
>>> I am afraid that PCI network devices' setting still won't survive unbound&
>>> re-probed, will they?
>>
>> Correct - but PCI isn't as prone to "dropping off the bus" like USB
>
> As far as I know, USB device still won't be disconnected easily, and
> reset is possible, but we can make setting survive reset by implementing
> .pre_reset() and .post_reset() callback. Or do you have other situation
> of USB 'dropping off the bus'?

So far only older USB core bugs like this one:
    https://codereview.chromium.org/4687002/show

I agree USB won't be disconnected easily.

>> is. Master aborts on some PCI systems is a "Fatal Exception" and AFAIK
>> that's never been true for any USB device.
>
> I mean rmmod & modprobe still can reset setting of one PCI network
> device after powering on the device, can't it?

Definitely. But this isn't something that will "randomly" happen and
will leave tracks all over the place of it happening. So I'm not
worried about trying to debug this scenario.

thanks,
grant

>
>
> Thanks,
> --
> Ming Lei
--
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
Ming Lei - Aug. 9, 2013, 1:44 a.m.
On Fri, Aug 9, 2013 at 8:18 AM, Grant Grundler <grundler@google.com> wrote:
> Ming,
> We are splitting hairs now. :) I want to be clear I think your changes
> are good and the rest of this conversation is just to learn something
> new.
>
> On Thu, Aug 8, 2013 at 4:48 PM, Ming Lei <ming.lei@canonical.com> wrote:
>> On Fri, Aug 9, 2013 at 1:25 AM, Grant Grundler <grundler@google.com> wrote:
> ...
>>>> I am afraid that PCI network devices' setting still won't survive unbound&
>>>> re-probed, will they?
>>>
>>> Correct - but PCI isn't as prone to "dropping off the bus" like USB
>>
>> As far as I know, USB device still won't be disconnected easily, and
>> reset is possible, but we can make setting survive reset by implementing
>> .pre_reset() and .post_reset() callback. Or do you have other situation
>> of USB 'dropping off the bus'?
>
> So far only older USB core bugs like this one:
>     https://codereview.chromium.org/4687002/show

This happens in configuration change situation, which is seldom
triggered, and also not "randomly" happen per your standpoint,
just like rmmod/modprobe , :-)


> I agree USB won't be disconnected easily.
>
>>> is. Master aborts on some PCI systems is a "Fatal Exception" and AFAIK
>>> that's never been true for any USB device.
>>
>> I mean rmmod & modprobe still can reset setting of one PCI network
>> device after powering on the device, can't it?
>
> Definitely. But this isn't something that will "randomly" happen and
> will leave tracks all over the place of it happening. So I'm not
> worried about trying to debug this scenario.

Thanks,
--
Ming Lei
--
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 fb0caa2..7a96326 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1031,12 +1031,20 @@  static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
 	dev->mii.phy_id = 0x03;
 	dev->mii.supports_gmii = 1;
 
+	if (usb_device_no_sg_constraint(dev->udev))
+		dev->can_dma_sg = 1;
+
 	dev->net->features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 			      NETIF_F_RXCSUM;
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM;
 
+	if (dev->can_dma_sg) {
+		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
+		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
+	}
+
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |
 	       AX_RXCOE_TCPV6 | AX_RXCOE_UDPV6;
@@ -1310,6 +1318,10 @@  static int ax88179_reset(struct usbnet *dev)
 
 	dev->net->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 				 NETIF_F_RXCSUM;
+	if (dev->can_dma_sg) {
+		dev->net->features |= NETIF_F_SG | NETIF_F_TSO;
+		dev->net->hw_features |= NETIF_F_SG | NETIF_F_TSO;
+	}
 
 	/* Enable checksum offload */
 	*tmp = AX_RXCOE_IP | AX_RXCOE_TCP | AX_RXCOE_UDP |