RE: [PATCH net-next 1/4] net: Introduce NETIF_F_GRO_HW
From: Yuval Mintz <hidden>
Date: 2017-12-04 22:16:04
quoted hunk ↗ jump to hunk
Introduce NETIF_F_GRO_HW feature flag for NICs that support hardware GRO. With this flag, we can now independently turn on or off hardware GRO when GRO is on. Hardware GRO guarantees that packets can be re-segmented by TSO/GSO to reconstruct the original packet stream. Cc: Ariel Elior <redacted> Cc: everest-linux-l2@cavium.com Signed-off-by: Michael Chan <michael.chan@broadcom.com> --- Documentation/networking/netdev-features.txt | 7 +++++++ include/linux/netdev_features.h | 5 ++++- net/core/dev.c | 13 +++++++++++++ net/core/ethtool.c | 1 + 4 files changed, 25 insertions(+), 1 deletion(-)diff --git a/Documentation/networking/netdev-features.txtb/Documentation/networking/netdev-features.txt index 7413eb0..d76d332 100644--- a/Documentation/networking/netdev-features.txt +++ b/Documentation/networking/netdev-features.txt@@ -163,3 +163,10 @@ This requests that the NIC receive all possibleframes, including errored frames (such as bad FCS, etc). This can be helpful when sniffing a link with bad packets on it. Some NICs may receive more packets if also put into normal PROMISC mode. + +* rx-gro-hw + +This requests that the NIC enables Hardware GRO (generic receive offload). +Hardware GRO is basically the exact reverse of TSO, and is generally +stricter than Hardware LRO. A packet stream merged by Hardware GRO must +be re-segmentable by GSO or TSO back to the exact packet stream.diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h index dc8b489..d18ef6f 100644 --- a/include/linux/netdev_features.h +++ b/include/linux/netdev_features.h@@ -77,6 +77,8 @@ enum { NETIF_F_HW_ESP_TX_CSUM_BIT, /* ESP with TX checksumoffload */ NETIF_F_RX_UDP_TUNNEL_PORT_BIT, /* Offload of RX port for UDP tunnels */ + NETIF_F_GRO_HW_BIT, /* Hardware Generic receive offload */ + /* * Add your fresh new feature above and remember to update * netdev_features_strings[] in net/core/ethtool.c and maybe@@ -96,6 +98,7 @@ enum { #define NETIF_F_FRAGLIST __NETIF_F(FRAGLIST) #define NETIF_F_FSO __NETIF_F(FSO) #define NETIF_F_GRO __NETIF_F(GRO) +#define NETIF_F_GRO_HW __NETIF_F(GRO_HW) #define NETIF_F_GSO __NETIF_F(GSO) #define NETIF_F_GSO_ROBUST __NETIF_F(GSO_ROBUST) #define NETIF_F_HIGHDMA __NETIF_F(HIGHDMA)@@ -193,7 +196,7 @@ enum { * If upper/master device has these features disabled, they must be disabled * on all lower/slave devices as well. */ -#define NETIF_F_UPPER_DISABLES NETIF_F_LRO +#define NETIF_F_UPPER_DISABLES (NETIF_F_LRO | NETIF_F_GRO_HW) /* changeable features with no special hardware requirements */ #define NETIF_F_SOFT_FEATURES (NETIF_F_GSO | NETIF_F_GRO)diff --git a/net/core/dev.c b/net/core/dev.c index 30b5fe3..09c2ad0 100644 --- a/net/core/dev.c +++ b/net/core/dev.c@@ -7392,6 +7392,19 @@ static netdev_features_tnetdev_fix_features(struct net_device *dev, features &= ~dev->gso_partial_features; } + if (features & NETIF_F_GRO_HW) { + /* Hardware GRO depends on GRO. */ + if (!(features & NETIF_F_GRO)) {
While at it, perhaps also make it dependent on NETIF_F_RXCSUM?
+ netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since
no GRO 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;Isn't this considered to be breaking an existing API? After this, while NETIF_F_GRO_HW is published an application trying to set NETIF_F_LRO and then query its state would discover it failed [while previously it could have succeeded, such as for bnx2] While I understand the need to make sure core doesn't enable two competing aggregation offloads, why make GRO_HW > LRO? I understand it's probably the better one, but until LRO gets deprecated isn't it safer to do this limitation the opposite way? I.e., make sure NETIF_F_GRO_HW can't be set as long as NETIF_F_LRO is set?
quoted hunk ↗ jump to hunk
+ } + } + return features; }diff --git a/net/core/ethtool.c b/net/core/ethtool.c index f8fcf45..50a7920 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c@@ -73,6 +73,7 @@ int ethtool_op_get_ts_info(struct net_device *dev,struct ethtool_ts_info *info) [NETIF_F_LLTX_BIT] = "tx-lockless", [NETIF_F_NETNS_LOCAL_BIT] = "netns-local", [NETIF_F_GRO_BIT] = "rx-gro", + [NETIF_F_GRO_HW_BIT] = "rx-gro-hw", [NETIF_F_LRO_BIT] = "rx-lro", [NETIF_F_TSO_BIT] = "tx-tcp-segmentation", -- 1.8.3.1