Re: [PATCH net-next v2 1/5] net: Introduce NETIF_F_GRO_HW.

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

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.

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.

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.