On Thu, Dec 7, 2017 at 2:08 PM, Michael Chan [off-list ref] wrote:

On Thu, Dec 7, 2017 at 1:35 PM, Alexander Duyck
[off-list ref] wrote:

quoted

On Thu, Dec 7, 2017 at 10:44 AM, Michael Chan [off-list ref] wrote:

quoted

On Thu, Dec 7, 2017 at 10:13 AM, Alexander Duyck
[off-list ref] wrote:

quoted

On Thu, Dec 7, 2017 at 12:03 AM, Michael Chan [off-list ref] wrote:

quoted

@@ -7405,6 +7405,23 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
                features &= ~dev->gso_partial_features;
        }

+       if (features & NETIF_F_GRO_HW) {
+               /* Hardware GRO depends on GRO and RXCSUM. */
+               if (!(features & NETIF_F_GRO)) {
+                       netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO feature.\n");
+                       features &= ~NETIF_F_GRO_HW;
+               }

I'm not sure I agree with this part. The hardware offload should not
be dependent on the software offload.

As I said before, GRO_HW is basically hardware accelerated GRO. To me,
it makes perfect sense that GRO_HW depends on GRO.

quoted

If you are concerned with the
XDP case and such you should probably just look at handling it more
like how TSO is handled with a group of flags that aggregate GRO, LRO,
and GRO_HW into a group of features that must be disabled to support
XDP.

quoted

+               if (!(features & NETIF_F_RXCSUM)) {
+                       netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no RXCSUM feature.\n");
+                       features &= ~NETIF_F_GRO_HW;
+               }
+               /* Hardware GRO and LRO are mutually exclusive. */
+               if (features & NETIF_F_LRO) {
+                       netdev_dbg(dev, "Dropping NETIF_F_LRO since GRO_HW is set.\n");
+                       features &= ~NETIF_F_LRO;
+               }

This is going to be problematic. What if you bond one interface that
has LRO and one that has GRO_HW? It seems like this is going to cause
the bond interface to lie and say LRO isn't there when it actually is,
or worse yet it will force features off when they shouldn't be.

This is just dropping incompatible features similar to how other
incompatible feature flags are dropped in netdev_fix_features().
Hardware cannot aggregate in both LRO and GRO_HW modes at the same
time.  It's one or the other.

I'm not saying this is on a single device. The problem is in the case
of bonding you have combined 2 devices. We don't want to have an upper
device saying LRO isn't there when it is. A bonding setup is supposed
to advertise LRO as being enabled if a lower device has it present.
You are now making this an either/or thing at the driver level
generically when that isn't the case due to things like bridging and
bonding. This is why I was arguing that it would be easier to just
make this a super-set of LRO with one form that is reversible and one
that isn't. As is you are introducing all sorts of regressions that
are going to be problematic in any sort of mixed environment.

Are you saying that if LRO is dropped on a upper device (by the new
code added in this patch), all lower devices will need to drop it too?

If that's what you are saying, it's already taken care of.  There is
netdev_sync_upper_features() and netdev_sync_lower_features() that
will automatically do that as part of __netdev_update_features().

quoted

quoted

LRO and GRO_HW are different.  We may never agree on this but they are
different.  If a bond needs to disable LRO, it will be propagated to
all slaves.  But each slave can have GRO enabled.  If GRO is enabled,
GRO_HW, being a subset of GRO, can be enabled.

I get that they are very different. At a driver level it makes sense
to say you get one or the other at a physical driver level. The point
I was trying to make is that the bond is only able to set one or the
other when you might have a mix of devices. On the bonding device you
should be able to have both LRO and GRO_HW enabled.

On the bond, you can have LRO enabled and it is propagated to lower
devices so that lower devices will enable LRO if it is supported.  If
LRO is disabled on the bond (e.g. due to bridging), It will be
propagated so all lower devices will disable LRO if it is enabled.

GRO/GRO_HW is different and it is not propagated from upper devices to
lower devices like LRO.  Lower devices can have GRO/GRO_HW
enabled/disabled regardless of upper device.  This is no different
from say RXCSUM.  Lower devices can have RXCSUM on/off regardless.

So on the bond device you should be able to have LRO, GRO, and GRO_HW
enabled all at the same time and that means devices below it can have
any combination of those enabled. If I recall enabling it on the lower
device shouldn't be allowed if an upper device has it disabled. If it
is enabled on the upper device it can be either enabled or disabled on
the lower device depending on the state the device prefers.

The GRO and GRO_HW should be propagated just like RXCSUM and
everything else. Basically if I turn it off on the bond all of the
lower devices should have it disabled. Only if it is enabled should
lower devices be allowed to have it either enabled or disabled. The
easiest way to think of it is on the bond the features are like a mask
of what can be enabled/disabled for the lower devices. If a feature is
not present in the bond it should not be present on any of the devices
below it. That is why I am saying you cannot say LRO and GRO_HW are
mutually exclusive for all devices. For physical devices I would say
yes it can be mutually exclusive, but for things like bond it cannot
be since you can have one device doing LRO and one device doing GRO_HW
getting added to a bond and both features should be advertised there.

My concern is if I have a bond with both GRO_HW and LRO enabled on
lower devices, and then disable something like TSO we are going to see
LRO disabled on the bond and propagated to all the lower devices. That
is a bad behavior and shouldn't happen. In addition I would want to be
able to disable GRO_HW from the bond should I encounter a buggy
implementation of the GRO hardware offload at some point in the
future. I don't like it being tied so closely with GRO since all it
takes is one hardware failure and then we are dealing with people
disabling GRO due to reports of it being buggy instead of it being
tied specifically to GRO_HW.

- Alex