Re: [PATCH net] bonding: fix vlan_features computing

Re: [PATCH net] bonding: fix vlan_features computing

[David Miller]

From: Michal Kubecek <mkubecek@suse.cz>
Date: Fri,  2 May 2014 18:54:48 +0200 (CEST)

> 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 may end up with features none
> of the slaves provided.
> 
> In particular, BOND_VLAN_FEATURES contains NETIF_F_HW_CSUM so
> that netdev_increment_features() clears NETIF_F_IP{,V6}_CSUM and
> if we only have slaves with NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
> (e.g. ixgbe based cards), the same will be in features of the
> bond and a vlan device on top of the bond would support no
> checksumming, resulting in significant performance degradation.
> 
> 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.
> 
> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>

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.

Re: [PATCH net] bonding: fix vlan_features computing

[Jay Vosburgh]

David Miller <davem@davemloft.net> wrote:

>From: Michal Kubecek <mkubecek@suse.cz>
>Date: Fri,  2 May 2014 18:54:48 +0200 (CEST)
>
>> 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 may end up with features none
>> of the slaves provided.
>> 
>> In particular, BOND_VLAN_FEATURES contains NETIF_F_HW_CSUM so
>> that netdev_increment_features() clears NETIF_F_IP{,V6}_CSUM and
>> if we only have slaves with NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM
>> (e.g. ixgbe based cards), the same will be in features of the
>> bond and a vlan device on top of the bond would support no
>> checksumming, resulting in significant performance degradation.

	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?

>> 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.
>> 
>> Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>
>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.

	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.

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

	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, jay.vosburgh@canonical.com

Re: [PATCH net] bonding: fix vlan_features computing

[Michal Kubecek]

On Tue, May 06, 2014 at 12:53:27AM -0700, Jay Vosburgh wrote:
> 
> 	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?

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:

> >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.
> 
> 	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.

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).

> 	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 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

Re: [PATCH net] bonding: fix vlan_features computing

[Michal Kubecek]

On Tue, May 06, 2014 at 12:53:27AM -0700, Jay Vosburgh wrote:
> 
> 	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.

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

Re: [PATCH net] bonding: fix vlan_features computing

[David Miller]

From: Michal Kubecek <mkubecek@suse.cz>
Date: Tue, 6 May 2014 15:03:15 +0200

> On Tue, May 06, 2014 at 12:53:27AM -0700, Jay Vosburgh wrote:
>> 
>> 	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.
> 
> 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.

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.

[PATCH net-next v2 0/3] net: device features handling fixes

[Michal Kubecek]

> 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.

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.

> 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.

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

[PATCH net-next v2 1/3] vlan: more careful checksum features handling

[Michal Kubecek]

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().

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 include/linux/netdevice.h | 14 ++++++++++++++
 net/8021q/vlan_dev.c      |  4 ++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2dea98c..f4ad247 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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)
 {
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 8f025af..4181fb7 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -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

[PATCH net-next v2 2/3] bonding: fix vlan_features computing

[Michal Kubecek]

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.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/bonding/bond_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7123205..9071139 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -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

[PATCH net-next v2 3/3] teaming: fix vlan_features computing

[Michal Kubecek]

__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.

Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
 drivers/net/team/team.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 767fe61..9a9ce8d 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -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

Re: [PATCH net-next v2 0/3] net: device features handling fixes

[David Miller]

From: Michal Kubecek <mkubecek@suse.cz>
Date: Tue, 20 May 2014 08:29:15 +0200 (CEST)

>> 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.
> 
> 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.

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