net: drop NETIF_F_GSO from hw_features without NETIF_F_SG

PATCH In review Roger Luethi 2 participants 5 messages Mar 18, 2011 – Mar 20, 2011

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Roger Luethi

author

2011-03-18 19:50 UTC #0 lore raw

PATCH 1/1 net: drop NETIF_F_GSO from hw_features without NETIF_F_SG

register_netdevice() removes NETIF_F_GSO from dev->features and dev->wanted_features for hardware without NETIF_F_SG to "avoid warning from netdev_fix_features()".

However, as the flag remains set in dev->hw_features, users trying to enable GSO via ethtool will still end up with NETIF_F_GSO stuck in wanted_features, resulting in a warning every time they try to change any feature (until they use ethtool to "disable" GSO):

# ethtool -K eth2 gso on
via-rhine 0000:03:00.0: eth2: Dropping NETIF_F_GSO since no SG feature.
# ethtool -K eth2 rx on
via-rhine 0000:03:00.0: eth2: Dropping NETIF_F_GSO since no SG feature.
via-rhine 0000:03:00.0: eth2: Features changed: 0x00004380 -> 0x20004380
# ethtool -K eth2 rx off
via-rhine 0000:03:00.0: eth2: Dropping NETIF_F_GSO since no SG feature.
via-rhine 0000:03:00.0: eth2: Features changed: 0x20004380 -> 0x00004380
# ethtool -K eth2 gso off
#

With NETIF_F_GSO depending on NETIF_F_SG, it seems we should not advertise the former as a changeable option if the latter is missing.

---
 net/core/dev.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

net/core/dev.c +1 -0

index 0d39032..122d4ca 100644
@@ -5433,6 +5433,7 @@ int register_netdevice(struct net_device *dev)
 
 	/* Avoid warning from netdev_fix_features() for GSO without SG */
 	if (!(dev->wanted_features & NETIF_F_SG)) {
+		dev->hw_features &= ~NETIF_F_GSO;
 		dev->wanted_features &= ~NETIF_F_GSO;
 		dev->features &= ~NETIF_F_GSO;
 	}

Signature · 1 line

1.7.3.4

Michał Mirosław

2011-03-18 20:43 UTC #1 lore raw

2011/3/18 Roger Luethi <[email protected]>:

Quoted 8 lines

register_netdevice() removes NETIF_F_GSO from dev->features and dev->wanted_features for hardware without NETIF_F_SG to "avoid warning from netdev_fix_features()".
However, as the flag remains set in dev->hw_features, users trying to enable GSO via ethtool will still end up with NETIF_F_GSO stuck in wanted_features, resulting in a warning every time they try to change any feature (until they use ethtool to "disable" GSO):

That's expected, as it allows the user to debug why his request didn't work as expected. I think that those messages should get demoted to DEBUG if that's too noisy. The network core imposed conditions could be explained in Documentation/ or ethtool manpage instead.

Best Regards,
Michał Mirosław

Roger Luethi

author

2011-03-19 07:46 UTC #2 lore raw

Quoted 10 lines · Micha? Miros?aw

2011/3/18 Roger Luethi <[email protected]>:
Quoted 8 lines
register_netdevice() removes NETIF_F_GSO from dev->features and dev->wanted_features for hardware without NETIF_F_SG to "avoid warning from netdev_fix_features()".
However, as the flag remains set in dev->hw_features, users trying to enable GSO via ethtool will still end up with NETIF_F_GSO stuck in wanted_features, resulting in a warning every time they try to change any feature (until they use ethtool to "disable" GSO):

Quoted 4 lines

That's expected, as it allows the user to debug why his request didn't work as expected. I think that those messages should get demoted to DEBUG if that's too noisy. The network core imposed conditions could be explained in Documentation/ or ethtool manpage instead.

I agree that it is good to have a note explaining what happened in the kernel log, and the patch I posted does not do that; that is easy to fix.

However, I still think the current, unpatched behavior is confusing users. ethtool_get_features() will happily tell the user that GSO is available for a NIC even if it is not (because the NIC does not support SG). Then, if the user tries to enable GSO, the operation fails silently: ethtool won't get an error code, so it looks as if the operation succeeded. The user needs to look into the kernel log or actually verify the offload settings to realize that something went wrong.

Roger

Michał Mirosław

2011-03-19 16:18 UTC #3 lore raw

2011/3/19 Roger Luethi <[email protected]>:

Quoted 19 lines · Micha? Miros?aw

Quoted 10 lines
2011/3/18 Roger Luethi <[email protected]>:
Quoted 8 lines
register_netdevice() removes NETIF_F_GSO from dev->features and dev->wanted_features for hardware without NETIF_F_SG to "avoid warning from netdev_fix_features()".
However, as the flag remains set in dev->hw_features, users trying to enable GSO via ethtool will still end up with NETIF_F_GSO stuck in wanted_features, resulting in a warning every time they try to change any feature (until they use ethtool to "disable" GSO):
Quoted 4 lines
That's expected, as it allows the user to debug why his request didn't work as expected. I think that those messages should get demoted to DEBUG if that's too noisy. The network core imposed conditions could be explained in Documentation/ or ethtool manpage instead.
I agree that it is good to have a note explaining what happened in the kernel log, and the patch I posted does not do that; that is easy to fix.

Quoted 7 lines

However, I still think the current, unpatched behavior is confusing users. ethtool_get_features() will happily tell the user that GSO is available for a NIC even if it is not (because the NIC does not support SG). Then, if the user tries to enable GSO, the operation fails silently: ethtool won't get an error code, so it looks as if the operation succeeded. The user needs to look into the kernel log or actually verify the offload settings to realize that something went wrong.

Actually, the user will get the information that GSO wasn't enabled, though not while using ETHTOOL_SGSO.

OTOH, I think I can see what's your point - I got confused by your proposed patch. The correct way is to remove NETIF_F_GSO from hw_features when NETIF_F_SG is not present in hw_features (and not wanted_features as you're patch is doing). The idea here is that a driver might advertise SG in changeable features (that's hw_features) but not enable it by default for whatever reason.

Do you want to prepare a patch for this? Please remember about compatibility with per-offload ethtool ops if you do - this needs to be there until all drivers are converted to hw_features.

Best Regards,
Michał Mirosław

Roger Luethi

author

2011-03-20 18:56 UTC #4 lore raw

Quoted 11 lines · Micha? Miros?aw

Quoted 7 lines
However, I still think the current, unpatched behavior is confusing users. ethtool_get_features() will happily tell the user that GSO is available for a NIC even if it is not (because the NIC does not support SG). Then, if the user tries to enable GSO, the operation fails silently: ethtool won't get an error code, so it looks as if the operation succeeded. The user needs to look into the kernel log or actually verify the offload settings to realize that something went wrong.
Do you want to prepare a patch for this? Please remember about compatibility with per-offload ethtool ops if you do - this needs to be there until all drivers are converted to hw_features.

Something like this?

net/core/dev.c +3 -0

index 0b88eba..5861a59 100644
@@ -5442,6 +5442,9 @@ int register_netdevice(struct net_device *dev)
 	if (!(dev->wanted_features & NETIF_F_SG)) {
 		dev->wanted_features &= ~NETIF_F_GSO;
 		dev->features &= ~NETIF_F_GSO;
+		if (!(dev->hw_features & NETIF_F_SG) &&
+		    (!dev->ethtool_ops || !dev->ethtool_ops->set_sg))
+			dev->hw_features &= ~NETIF_F_GSO;
 	}
 
 	/* Enable GRO and NETIF_F_HIGHDMA for vlans by default,