[PATCH] bgmac: Remove all offloading features, including GRO.

On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:

Currently:

[  4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52410
[ ID] Interval        Transfer    Bandwidth       Reads   Dist(bin=16.0K)
[  4] 0.00-10.02 sec  52.4 MBytes  43.8 Mbits/sec  641    75:181:12:1:1:0:0:371
[  5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52412
[  5] 0.00-10.02 sec  52.4 MBytes  43.8 Mbits/sec  629    51:194:13:1:0:1:0:369
[  4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52414
[  4] 0.00-10.02 sec  51.9 MBytes  43.4 Mbits/sec  695    126:203:1:0:0:0:2:363
[  5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52416
[  5] 0.00-10.01 sec  52.4 MBytes  43.9 Mbits/sec  626    57:186:10:0:0:0:0:373
[  4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52420
[  4] 0.00-10.02 sec  52.4 MBytes  43.8 Mbits/sec  605    36:179:16:1:0:1:0:372
[  5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52422

After disabling everything - including GRO:

[  4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52440
[ ID] Interval        Transfer    Bandwidth       Reads   Dist(bin=16.0K)
[  4] 0.00-10.01 sec  55.1 MBytes  46.2 Mbits/sec  672    180:82:0:0:1:0:0:409
[  5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52442
[  5] 0.00-10.01 sec  56.0 MBytes  46.9 Mbits/sec  636    117:96:0:0:1:0:0:422
[  4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52444
[  4] 0.00-10.01 sec  55.4 MBytes  46.4 Mbits/sec  675    172:92:0:0:1:0:0:410
[  5] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52446
[  5] 0.00-10.01 sec  56.0 MBytes  46.9 Mbits/sec  633    119:90:0:1:1:0:0:422
[  4] local 192.168.1.1 port 5001 connected with 192.168.1.100 port 52448
[  4] 0.00-10.01 sec  55.2 MBytes  46.3 Mbits/sec  688    157:123:0:0:2:0:0:406

Signed-off-by: Rosen Penev <redacted>
---
 drivers/net/ethernet/broadcom/bgmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index 48d672b204a4..c2db5673b073 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c

@@ -1534,7 +1534,7 @@ int bgmac_enet_probe(struct bgmac *bgmac)
 		goto err_dma_free;
 	}
 
-	net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+	net_dev->features &= NETIF_F_GRO;
 	net_dev->hw_features = net_dev->features;
 	net_dev->vlan_features = net_dev->features;
 

-- 
2.13.5

On September 15, 2017 3:22:18 PM PDT, Rosen Penev [off-list ref] wrote:

On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading
features give no measurable benefit to speed or latency. Furthermore,
disabling GRO actually improves iperf performance by a whoppimg 3mbps.

Do you have a way to generate gigabit tests and see what results you are getting? We probably are not going to see a 30% improvement just by extrapolation.

-- 
Florian

From: Rosen Penev <redacted>
Date: Fri, 15 Sep 2017 15:22:18 -0700

On a linksys E1200v1 (actually a crossflashed E1000v2), the offloading features give no measurable benefit to speed or latency. Furthermore, disabling GRO actually improves iperf performance by a whoppimg 3mbps. Results:

 ...

-	net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
+	net_dev->features &= NETIF_F_GRO;
 	net_dev->hw_features = net_dev->features;

This doesn't disable GRO.

you're absolutely correct. will send an updated version shortly.

On Fri, 2017-09-15 at 16:14 -0700, David Miller wrote:

From: Rosen Penev <redacted>
Date: Fri, 15 Sep 2017 15:22:18 -0700

quoted

On a linksys E1200v1 (actually a crossflashed E1000v2), the

offloading features give no measurable benefit to speed or latency.
Furthermore, disabling GRO actually improves iperf performance by a
whoppimg 3mbps. Results:
 ...

quoted

-     net_dev->features = NETIF_F_SG | NETIF_F_IP_CSUM |

NETIF_F_IPV6_CSUM;

quoted

+     net_dev->features &= NETIF_F_GRO;
      net_dev->hw_features = net_dev->features;

This doesn't disable GRO.

On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:

On September 15, 2017 3:22:18 PM PDT, Rosen Penev [off-list ref]
wrote:

quoted

On a linksys E1200v1 (actually a crossflashed E1000v2), the

offloading

quoted

features give no measurable benefit to speed or latency. Furthermore,
disabling GRO actually improves iperf performance by a whoppimg

3mbps.

Do you have a way to generate gigabit tests and see what results you
are getting? We probably are not going to see a 30% improvement just
by extrapolation.

+1

It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
driver to handle these features.

And of course GRO was not removed, meaning the bench results were non
conclusive.

Ok fair enough. Will only disable GRO in the driver.

Tests were done using "ethtool -K eth0 gro off" and on.

On Fri, 2017-09-15 at 17:03 -0700, Eric Dumazet wrote:

On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:

quoted

On September 15, 2017 3:22:18 PM PDT, Rosen Penev <rosenp@gmail.com

quoted

wrote:

quoted

On a linksys E1200v1 (actually a crossflashed E1000v2), the

offloading

quoted

features give no measurable benefit to speed or latency.
Furthermore,
disabling GRO actually improves iperf performance by a whoppimg

3mbps.

Do you have a way to generate gigabit tests and see what results
you
are getting? We probably are not going to see a 30% improvement
just
by extrapolation.

+1

It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
driver to handle these features.

And of course GRO was not removed, meaning the bench results were non
conclusive.

On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:

Ok fair enough. Will only disable GRO in the driver.

Well, do not even try.

NETIF_F_SOFT_FEATURES is set by core networking stack in
register_netdevice(), ( commit 212b573f5552c60265da721ff9ce32e3462a2cdd
)

Absolutely no driver disables GRO (excepts the ones playing with LRO)

netif_receive_skb seems to have the same effect as "ethtool -K eth0 gro
off".

On Fri, 2017-09-15 at 17:18 -0700, Eric Dumazet wrote:

On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:

quoted

Ok fair enough. Will only disable GRO in the driver.

Well, do not even try.

NETIF_F_SOFT_FEATURES is set by core networking stack in
register_netdevice(), ( commit
212b573f5552c60265da721ff9ce32e3462a2cdd
)

Absolutely no driver disables GRO (excepts the ones playing with LRO)

On 2017-09-16 03:18, Eric Dumazet wrote:

On Fri, 2017-09-15 at 17:10 -0700, rosenp@gmail.com wrote:

quoted

Ok fair enough. Will only disable GRO in the driver.

Well, do not even try.

NETIF_F_SOFT_FEATURES is set by core networking stack in
register_netdevice(), ( commit 212b573f5552c60265da721ff9ce32e3462a2cdd
)

Absolutely no driver disables GRO (excepts the ones playing with LRO)

I believe also iperf is definitely inconclusive test.
Except iperf there is lot of different workloads and configurations, 
that might have different results.

From: Eric Dumazet <redacted>
Date: Fri, 15 Sep 2017 17:03:30 -0700

On Fri, 2017-09-15 at 15:54 -0700, Florian Fainelli wrote:

quoted

On September 15, 2017 3:22:18 PM PDT, Rosen Penev [off-list ref]
wrote:

quoted

On a linksys E1200v1 (actually a crossflashed E1000v2), the

offloading

quoted

features give no measurable benefit to speed or latency. Furthermore,
disabling GRO actually improves iperf performance by a whoppimg

3mbps.

Do you have a way to generate gigabit tests and see what results you
are getting? We probably are not going to see a 30% improvement just
by extrapolation.

+1

It seems silly to remove NETIF_F_SG | NETIF_F_IP_CSUM |
NETIF_F_IPV6_CSUM from dev->features, yet leave the dead-code in the
driver to handle these features.

And of course GRO was not removed, meaning the bench results were non
conclusive.

My sentiments exactly, I think this is a completely unwise change.