bonding: fix vlan_features computing

From: Michal Kubecek <[email protected]>
Date: Fri, 2 May 2014 18:54:48 +0200 (CEST)

Jay and others can you take a look at this one?

I find that handling of NETIF_F_GEN_CSUM (which is basically just a synonym for NETIF_F_HW_CSUM) strange.

I thought the whole idea with bond_compute_features() is that you could start with "everything" enabled, and as you add devices the feature bits get chopped off based upon what each slave can do.

But the special NETIF_F_GEN_CSUM in bond_compute_features() seems to defeat that.

The VLAN device doesn't get _HW_CSUM (aka _GEN_CSUM)? In
ethtool, this is tx-checksum-ip-generic, I believe.

I have only one machine with a _IP_CSUM only device (a tg3) set
up at the moment, it's running a Fedora 20 3.13 kernel, and a vlan -> bond0 -> eth0 stack shows "generic" offload for the vlan and bond0, and "ipv4" offload for the eth0 itself.

Is that not what you're seeing? Did this used to work correctly
for you? Does ethtool -K permit you to enable checksum offload on the VLAN device?

For VLANs, it was originally the other way around; the features
started with nothing, and the various slaves' offloads were added to the vlan_features.

I've been looking through the feature code for the last hour,
and I see what's going on in netdev_increment_features, but not why the vlan device ends up with no offloads at all.

My recollection is that, currently, the bond advertises that it
is checksum offload capable (via _CSUM_ALL in BOND_VLAN_FEATURES), the VLAN device believes it (picks up the feature bit), and when the skb is sent down through the bond, when it reaches the actual bottom of the stack, then the skb would have checksum offload applied according to the features of the final transmit device.

So this patch may work for the case that the slaves are
homogenous, but if one is _HW_CSUM (aka _GEN_CSUM) and one is only _IP or _IPV6, there may still be a problem because the _HW_CSUM slave will get its feature bit at the VLAN level, but the other one will not, and I thought that the mixed offload slaves case previously worked correctly.

I'll also point out that the team driver does the same thing as
bonding with regard to the handling of vlan_features, so whatever ends up changing in bonding should probably be updated in team as well.

	-J

---
-Jay Vosburgh, [email protected]

In my case the problem is that while bond has HW_CSUM in vlan_features, it has (only) IP_CSUM and IPV6_CSUM in features. As vlan starts with features of its real_dev and masks them with real_dev's vlan_features, the result is no checksum offloading.

However, I realized now that while I checked that vlan_features computing hasn't changed since 3.0, I didn't do the same with features so I better retest with mainline kernel.

But even if bond's features are set to HW_CSUM, I still don't think the logic of bond_compute_features() is correct:

My understanding is that there are two types of features: additive (NETIF_F_ONE_FOR_ALL, support in one slave is sufficient) and subtractive (NETIF_F_ALL_FOR_ALL, all slaves must support). That is how netdev_increment_features works (except the additional checksumming cleanup). For this to work, we should start with 0 for additive features and 1 for subtractive ones.

The problem IMHO is that bond_compute_features() initializes the value with BOND_VLAN_FEATURES which is a subset of NETIF_F_ONE_FOR_ALL. Therefore these flags always stay on in vlan_features no matter what slaves have (except cleaned up checksumming). But if this was intended, we wouldn't need to cycle through slaves at all and could always set vlan_features to BOND_VLAN_FEATURES (minus non-generic checksumming).

I tried to go through this and I believe it is safe to advertise HW_CSUM even if some slaves support only IP_CSUM (and possibly IPV6_CSUM). Even if the vlan assumes the packet can be checksummed but it is sent away by a slave not supporting that, the can_checksum_protocol() test in dev_hard_start_xmit() would detect that and checksum would be calculated in software. For IP(v6) packets, can_checksum_protocol() should return true for all three devices and packet would be checksummed by the slave it is sent by.

Michal Kubecek

I checked now and at least since 3.12 bond has NETIF_F_HW_CSUM in its features even if its slaves have only NETIF_F_IP(V6)_CSUM. Therefore NETIF_F_HW_CSUM in vlan_features does no harm and vlan on top of the bond gets NETIF_F_HW_CSUM.

Unfortunately it means that while I still think the change I proposed is correct, it would break things unless computing features of the bond is fixed in similar way or vlan_dev_fix_features is modified to handle checksumming features more carefully.

Michal Kubecek

From: Michal Kubecek <[email protected]>
Date: Tue, 6 May 2014 15:03:15 +0200

I think we need to think more about what exact behavior is desired in mixed csum feature set cases. Probably whatever csum offload type is the most prevelant should be the one advertised by the master. It means we will do that software checksum fixup for the oddball slaves, but it seems the best we can do.

Also, like Jay, I agree that whatever we do in bonding needs to be done in team too and I'd like the bug fixed at the same time in both places.

Therefore I'm marking this patch as "changes requested", thanks.

I've been thinking about it some more and I believe what I proposed is
correct: features that are or-ed (ONE_FOR_ALL) should start with 0, those which are and-ed (ALL_FOR_ALL) with 1.

But once we do that, we have to fix another problem in vlan module:
features of a vlan device are mostly computed as bitwise AND of features and vlan_features of its underlying device. The problem is checksumming features don't really behave like independent flags as their main purpose is to be used in can_checksum_protocol() whose logic is that HW_CSUM (GEN_CSUM) means "can checksum everything" so that it kind of contains IP(V6)_CSUM functionality. Therefore intersection of HW_CSUM and IP(V6)_CSUM should result in the latter.

An alternative approach would be to always set IP(V6)_CSUM once HW_CSUM (any of GEN_CSUM) is set but as for long time people were taught not to do that and we even have a warning for it, switching to the exact opposite could cause a lot of problems.

Agreed. Added a similar patch for teaming driver.

Michal Kubecek (3):
vlan: more careful checksum features handling
bonding: fix vlan_features computing
teaming: fix vlan_features computing

 drivers/net/bonding/bond_main.c |  1 +
 drivers/net/team/team.c         |  2 +-
 include/linux/netdevice.h       | 14 ++++++++++++++
 net/8021q/vlan_dev.c            |  4 ++--
 4 files changed, 18 insertions(+), 3 deletions(-)

1.8.1.4

When combining real_dev's features and vlan_features, simple bitwise AND is used. This doesn't work well for checksum offloading features as if one set has NETIF_F_HW_CSUM and the other NETIF_F_IP_CSUM and/or NETIF_F_IPV6_CSUM, we end up with no checksum offloading. However, from the logical point of view (how can_checksum_protocol() works), NETIF_F_HW_CSUM contains the functionality of NETIF_F_IP_CSUM and NETIF_F_IPV6_CSUM so that the result should be IP/IPV6.

Add helper function netdev_intersect_features() implementing this logic and use it in vlan_dev_fix_features().

---
 include/linux/netdevice.h | 14 ++++++++++++++
 net/8021q/vlan_dev.c      |  4 ++--
 2 files changed, 16 insertions(+), 2 deletions(-)

index 2dea98c..f4ad247 100644
@@ -3153,6 +3153,20 @@ const char *netdev_drivername(const struct net_device *dev);
 
 void linkwatch_run_queue(void);
 
+static inline netdev_features_t netdev_intersect_features(netdev_features_t f1,
+							  netdev_features_t f2)
+{
+	if (f1 & NETIF_F_GEN_CSUM)
+		f1 |= (NETIF_F_ALL_CSUM & ~NETIF_F_GEN_CSUM);
+	if (f2 & NETIF_F_GEN_CSUM)
+		f2 |= (NETIF_F_ALL_CSUM & ~NETIF_F_GEN_CSUM);
+	f1 &= f2;
+	if (f1 & NETIF_F_GEN_CSUM)
+		f1 &= ~(NETIF_F_ALL_CSUM & ~NETIF_F_GEN_CSUM);
+
+	return f1;
+}
+
 static inline netdev_features_t netdev_get_wanted_features(
 	struct net_device *dev)
 {

index 8f025af..4181fb7 100644
@@ -678,9 +678,9 @@ static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
 	struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
 	netdev_features_t old_features = features;
 
-	features &= real_dev->vlan_features;
+	features = netdev_intersect_features(features, real_dev->vlan_features);
 	features |= NETIF_F_RXCSUM;
-	features &= real_dev->features;
+	features = netdev_intersect_features(features, real_dev->features);
 
 	features |= old_features & NETIF_F_SOFT_FEATURES;
 	features |= NETIF_F_LLTX;

1.8.1.4

bond_compute_features() uses netdev_increment_features() to combine vlan_features of slaves into vlan_features of the bond. As netdev_increment_features() only adds most features and we start with BOND_VLAN_FEATURES, we can end up with features none of the slaves provided.

If there is at least one slave, initialize vlan_features only with the flags in NETIF_F_ALL_FOR_ALL. Right now there is none in BOND_VLAN_FEATURES but stating it explicitely will make the code more future proof.

---
 drivers/net/bonding/bond_main.c | 1 +
 1 file changed, 1 insertion(+)

index 7123205..9071139 100644
@@ -1038,6 +1038,7 @@ static void bond_compute_features(struct bonding *bond)
 
 	if (!bond_has_slaves(bond))
 		goto done;
+	vlan_features &= NETIF_F_ALL_FOR_ALL;
 
 	bond_for_each_slave(bond, slave, iter) {
 		vlan_features = netdev_increment_features(vlan_features,

1.8.1.4

__team_compute_features() uses netdev_increment_features() to combine vlan_features of slaves into vlan_features of the team. As netdev_increment_features() only adds most features and we start with TEAM_VLAN_FEATURES, we can end up with features none of the slaves provided.

Initialize vlan_features only with the flags which are both in TEAM_VLAN_FEATURES and NETIF_F_ALL_FOR_ALL. Right now there is no such feature so that we actually initialize vlan_features with zero but stating it explicitely will make the code more future proof.

---
 drivers/net/team/team.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 767fe61..9a9ce8d 100644
@@ -968,7 +968,7 @@ static void team_port_disable(struct team *team,
 static void __team_compute_features(struct team *team)
 {
 	struct team_port *port;
-	u32 vlan_features = TEAM_VLAN_FEATURES;
+	u32 vlan_features = TEAM_VLAN_FEATURES & NETIF_F_ALL_FOR_ALL;
 	unsigned short max_hard_header_len = ETH_HLEN;
 	unsigned int flags, dst_release_flag = IFF_XMIT_DST_RELEASE;
 

1.8.1.4

From: Michal Kubecek <[email protected]>
Date: Tue, 20 May 2014 08:29:15 +0200 (CEST)

Ok, looks good, series applied to net-next, thanks.