[PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Michael Chan]

Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
hardware GRO to use the new flag.

v5:
- Documentation changes requested by Alexander Duyck.
- bnx2x changes requested by Manish Chopra to enable LRO by default, and
disable GRO_HW if disable_tpa module parameter is set.

v4:
- more changes requested by Alexander Duyck:
- check GRO_HW/GRO dependency in drivers's ndo_fix_features().
- Reverse the order of RXCSUM and GRO_HW dependency check in
netdev_fix_features().
- No propagation in netdev_disable_gro_hw().

v3:
- Let driver's ndo_fix_features() disable NETIF_F_LRO when NETIF_F_GRO_HW
is set instead of doing it in common netdev_fix_features().

v2:
- NETIF_F_GRO_HW flag propagation between upper and lower devices not
required (see patch 1).
- NETIF_F_GRO_HW depends on NETIF_F_GRO and NETIF_F_RXCSUM.
- Add dev_disable_gro_hw() to disable GRO_HW for generic XDP.
- Use ndo_fix_features() on all 3 drivers to drop GRO_HW when it is not
supported

Michael Chan (5):
  net: Introduce NETIF_F_GRO_HW.
  net: Disable GRO_HW when generic XDP is installed on a device.
  bnxt_en: Use NETIF_F_GRO_HW.
  bnx2x: Use NETIF_F_GRO_HW.
  qede: Use NETIF_F_GRO_HW.

 Documentation/networking/netdev-features.txt     |  9 +++++++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 24 +++++++++----------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  8 ++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c        | 27 +++++++++++++++------
 drivers/net/ethernet/qlogic/qede/qede.h          |  2 ++
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c  |  3 +++
 drivers/net/ethernet/qlogic/qede/qede_filter.c   | 21 +++++++++++------
 drivers/net/ethernet/qlogic/qede/qede_main.c     | 17 +++++---------
 include/linux/netdev_features.h                  |  3 +++
 net/core/dev.c                                   | 30 ++++++++++++++++++++++++
 net/core/ethtool.c                               |  1 +
 11 files changed, 105 insertions(+), 40 deletions(-)

-- 
1.8.3.1

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

[Michael Chan]

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.  Previously, drivers were using NETIF_F_GRO to
control hardware GRO and so it cannot be independently turned on or
off without affecting GRO.

Hardware GRO (just like GRO) guarantees that packets can be re-segmented
by TSO/GSO to reconstruct the original packet stream.  Logically,
GRO_HW should depend on GRO since it a subset, but we will let
individual drivers enforce this dependency as they see fit.

Since NETIF_F_GRO is not propagated between upper and lower devices,
NETIF_F_GRO_HW should follow suit since it is a subset of GRO.  In other
words, a lower device can independent have GRO/GRO_HW enabled or disabled
and no feature propagation is required.  This will preserve the current
GRO behavior.  This can be changed later if we decide to propagate GRO/
GRO_HW/RXCSUM from upper to lower devices.

Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: everest-linux-l2@cavium.com
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 Documentation/networking/netdev-features.txt |  9 +++++++++
 include/linux/netdev_features.h              |  3 +++
 net/core/dev.c                               | 12 ++++++++++++
 net/core/ethtool.c                           |  1 +
 4 files changed, 25 insertions(+)

diff --git a/Documentation/networking/netdev-features.txt b/Documentation/networking/netdev-features.txt
index 7413eb0..c77f9d5 100644
--- a/Documentation/networking/netdev-features.txt
+++ b/Documentation/networking/netdev-features.txt
@@ -163,3 +163,12 @@ This requests that the NIC receive all possible frames, 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 original packet stream.
+Hardware GRO is dependent on RXCSUM since every packet successfully merged
+by hardware must also have the checksum verified by hardware.
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index b1b0ca7..db84c51 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -78,6 +78,8 @@ enum {
 	NETIF_F_HW_ESP_TX_CSUM_BIT,	/* ESP with TX checksum offload */
 	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
@@ -97,6 +99,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)
diff --git a/net/core/dev.c b/net/core/dev.c
index b0eee49..4b43f5d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7424,6 +7424,18 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
 		features &= ~dev->gso_partial_features;
 	}
 
+	if (!(features & NETIF_F_RXCSUM)) {
+		/* NETIF_F_GRO_HW implies doing RXCSUM since every packet
+		 * successfully merged by hardware must also have the
+		 * checksum verified by hardware.  If the user does not
+		 * want to enable RXCSUM, logically, we should disable GRO_HW.
+		 */
+		if (features & NETIF_F_GRO_HW) {
+			netdev_dbg(dev, "Dropping NETIF_F_GRO_HW since no RXCSUM feature.\n");
+			features &= ~NETIF_F_GRO_HW;
+		}
+	}
+
 	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

[PATCH net-next v5 2/5] net: Disable GRO_HW when generic XDP is installed on a device.

[Michael Chan]

Hardware should not aggregate any packets when generic XDP is installed.

Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: everest-linux-l2@cavium.com
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 net/core/dev.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 4b43f5d..c7db399 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1542,6 +1542,23 @@ void dev_disable_lro(struct net_device *dev)
 }
 EXPORT_SYMBOL(dev_disable_lro);
 
+/**
+ *	dev_disable_gro_hw - disable HW Generic Receive Offload on a device
+ *	@dev: device
+ *
+ *	Disable HW Generic Receive Offload (GRO_HW) on a net device.  Must be
+ *	called under RTNL.  This is needed if Generic XDP is installed on
+ *	the device.
+ */
+static void dev_disable_gro_hw(struct net_device *dev)
+{
+	dev->wanted_features &= ~NETIF_F_GRO_HW;
+	netdev_update_features(dev);
+
+	if (unlikely(dev->features & NETIF_F_GRO_HW))
+		netdev_WARN(dev, "failed to disable GRO_HW!\n");
+}
+
 static int call_netdevice_notifier(struct notifier_block *nb, unsigned long val,
 				   struct net_device *dev)
 {
@@ -4564,6 +4581,7 @@ static int generic_xdp_install(struct net_device *dev, struct netdev_bpf *xdp)
 		} else if (new && !old) {
 			static_key_slow_inc(&generic_xdp_needed);
 			dev_disable_lro(dev);
+			dev_disable_gro_hw(dev);
 		}
 		break;
 
-- 
1.8.3.1

[PATCH net-next v5 3/5] bnxt_en: Use NETIF_F_GRO_HW.

[Michael Chan]

Advertise NETIF_F_GRO_HW in hw_features if hardware GRO is supported.
In bnxt_fix_features(), disable GRO_HW and LRO if current hardware
configuration does not allow it.  GRO_HW depends on GRO.  GRO_HW is
also mutually exclusive with LRO.  XDP setup will now rely on
bnxt_fix_features() to turn off aggregation.  During chip init, turn on
or off hardware GRO based on NETIF_F_GRO_HW in features flag.

Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 1d865ae..9efbdc6 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -2755,7 +2755,7 @@ void bnxt_set_tpa_flags(struct bnxt *bp)
 		return;
 	if (bp->dev->features & NETIF_F_LRO)
 		bp->flags |= BNXT_FLAG_LRO;
-	if (bp->dev->features & NETIF_F_GRO)
+	else if (bp->dev->features & NETIF_F_GRO_HW)
 		bp->flags |= BNXT_FLAG_GRO;
 }
 
@@ -2843,10 +2843,10 @@ int bnxt_set_rx_skb_mode(struct bnxt *bp, bool page_mode)
 			min_t(u16, bp->max_mtu, BNXT_MAX_PAGE_MODE_MTU);
 		bp->flags &= ~BNXT_FLAG_AGG_RINGS;
 		bp->flags |= BNXT_FLAG_NO_AGG_RINGS | BNXT_FLAG_RX_PAGE_MODE;
-		bp->dev->hw_features &= ~NETIF_F_LRO;
-		bp->dev->features &= ~NETIF_F_LRO;
 		bp->rx_dir = DMA_BIDIRECTIONAL;
 		bp->rx_skb_func = bnxt_rx_page_skb;
+		/* Disable LRO or GRO_HW */
+		netdev_update_features(bp->dev);
 	} else {
 		bp->dev->max_mtu = bp->max_mtu;
 		bp->flags &= ~BNXT_FLAG_RX_PAGE_MODE;
@@ -6788,6 +6788,15 @@ static netdev_features_t bnxt_fix_features(struct net_device *dev,
 	if ((features & NETIF_F_NTUPLE) && !bnxt_rfs_capable(bp))
 		features &= ~NETIF_F_NTUPLE;
 
+	if (bp->flags & BNXT_FLAG_NO_AGG_RINGS)
+		features &= ~(NETIF_F_LRO | NETIF_F_GRO_HW);
+
+	if (!(features & NETIF_F_GRO))
+		features &= ~NETIF_F_GRO_HW;
+
+	if (features & NETIF_F_GRO_HW)
+		features &= ~NETIF_F_LRO;
+
 	/* Both CTAG and STAG VLAN accelaration on the RX side have to be
 	 * turned on or off together.
 	 */
@@ -6821,9 +6830,9 @@ static int bnxt_set_features(struct net_device *dev, netdev_features_t features)
 	bool update_tpa = false;
 
 	flags &= ~BNXT_FLAG_ALL_CONFIG_FEATS;
-	if ((features & NETIF_F_GRO) && !BNXT_CHIP_TYPE_NITRO_A0(bp))
+	if (features & NETIF_F_GRO_HW)
 		flags |= BNXT_FLAG_GRO;
-	if (features & NETIF_F_LRO)
+	else if (features & NETIF_F_LRO)
 		flags |= BNXT_FLAG_LRO;
 
 	if (bp->flags & BNXT_FLAG_NO_AGG_RINGS)
@@ -7924,8 +7933,8 @@ static int bnxt_get_dflt_rings(struct bnxt *bp, int *max_rx, int *max_tx,
 		if (rc)
 			return rc;
 		bp->flags |= BNXT_FLAG_NO_AGG_RINGS;
-		bp->dev->hw_features &= ~NETIF_F_LRO;
-		bp->dev->features &= ~NETIF_F_LRO;
+		bp->dev->hw_features &= ~(NETIF_F_LRO | NETIF_F_GRO_HW);
+		bp->dev->features &= ~(NETIF_F_LRO | NETIF_F_GRO_HW);
 		bnxt_set_ring_params(bp);
 	}
 
@@ -8108,7 +8117,11 @@ static int bnxt_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	dev->vlan_features = dev->hw_features | NETIF_F_HIGHDMA;
 	dev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_CTAG_TX |
 			    NETIF_F_HW_VLAN_STAG_RX | NETIF_F_HW_VLAN_STAG_TX;
+	if (!BNXT_CHIP_TYPE_NITRO_A0(bp))
+		dev->hw_features |= NETIF_F_GRO_HW;
 	dev->features |= dev->hw_features | NETIF_F_HIGHDMA;
+	if (dev->features & NETIF_F_GRO_HW)
+		dev->features &= ~NETIF_F_LRO;
 	dev->priv_flags |= IFF_UNICAST_FLT;
 
 #ifdef CONFIG_BNXT_SRIOV
-- 
1.8.3.1

[PATCH net-next v5 4/5] bnx2x: Use NETIF_F_GRO_HW.

[Michael Chan]

Advertise NETIF_F_GRO_HW and turn on TPA_MODE_GRO when NETIF_F_GRO_HW
is set.  Disable NETIF_F_GRO_HW in bnx2x_fix_features() if the MTU
does not support TPA_MODE_GRO or GRO is not set.  bnx2x_change_mtu() also
needs to disable NETIF_F_GRO_HW if the MTU does not support it.

Original parameter disable_tpa will continue to disable LRO and GRO_HW.

Preserve the original behavior of enabling LRO by default.  User has
to run ethtool -K to explicitly enable GRO_HW.

Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: everest-linux-l2@cavium.com
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 24 ++++++++++++------------
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  8 +++++---
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
index 4c739d5..01b7f2f 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
@@ -2482,8 +2482,7 @@ static void bnx2x_bz_fp(struct bnx2x *bp, int index)
 	 */
 	if (bp->dev->features & NETIF_F_LRO)
 		fp->mode = TPA_MODE_LRO;
-	else if (bp->dev->features & NETIF_F_GRO &&
-		 bnx2x_mtu_allows_gro(bp->dev->mtu))
+	else if (bp->dev->features & NETIF_F_GRO_HW)
 		fp->mode = TPA_MODE_GRO;
 	else
 		fp->mode = TPA_MODE_DISABLED;
@@ -4874,6 +4873,9 @@ int bnx2x_change_mtu(struct net_device *dev, int new_mtu)
 	 */
 	dev->mtu = new_mtu;
 
+	if (!bnx2x_mtu_allows_gro(new_mtu))
+		dev->features &= ~NETIF_F_GRO_HW;
+
 	if (IS_PF(bp) && SHMEM2_HAS(bp, curr_cfg))
 		SHMEM2_WR(bp, curr_cfg, CURR_CFG_MET_OS);
 
@@ -4903,10 +4905,13 @@ netdev_features_t bnx2x_fix_features(struct net_device *dev,
 	}
 
 	/* TPA requires Rx CSUM offloading */
-	if (!(features & NETIF_F_RXCSUM)) {
+	if (!(features & NETIF_F_RXCSUM))
+		features &= ~NETIF_F_LRO;
+
+	if (!(features & NETIF_F_GRO) || !bnx2x_mtu_allows_gro(dev->mtu))
+		features &= ~NETIF_F_GRO_HW;
+	if (features & NETIF_F_GRO_HW)
 		features &= ~NETIF_F_LRO;
-		features &= ~NETIF_F_GRO;
-	}
 
 	return features;
 }
@@ -4933,13 +4938,8 @@ int bnx2x_set_features(struct net_device *dev, netdev_features_t features)
 		}
 	}
 
-	/* if GRO is changed while LRO is enabled, don't force a reload */
-	if ((changes & NETIF_F_GRO) && (features & NETIF_F_LRO))
-		changes &= ~NETIF_F_GRO;
-
-	/* if GRO is changed while HW TPA is off, don't force a reload */
-	if ((changes & NETIF_F_GRO) && bp->disable_tpa)
-		changes &= ~NETIF_F_GRO;
+	/* Don't care about GRO changes */
+	changes &= ~NETIF_F_GRO;
 
 	if (changes)
 		bnx2x_reload = true;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 91e2a75..4d06548 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12400,8 +12400,8 @@ static int bnx2x_init_bp(struct bnx2x *bp)
 
 	/* Set TPA flags */
 	if (bp->disable_tpa) {
-		bp->dev->hw_features &= ~NETIF_F_LRO;
-		bp->dev->features &= ~NETIF_F_LRO;
+		bp->dev->hw_features &= ~(NETIF_F_LRO | NETIF_F_GRO_HW);
+		bp->dev->features &= ~(NETIF_F_LRO | NETIF_F_GRO_HW);
 	}
 
 	if (CHIP_IS_E1(bp))
@@ -13273,7 +13273,7 @@ static int bnx2x_init_dev(struct bnx2x *bp, struct pci_dev *pdev,
 
 	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
-		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_GRO |
+		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_GRO | NETIF_F_GRO_HW |
 		NETIF_F_RXHASH | NETIF_F_HW_VLAN_CTAG_TX;
 	if (!chip_is_e1x) {
 		dev->hw_features |= NETIF_F_GSO_GRE | NETIF_F_GSO_GRE_CSUM |
@@ -13309,6 +13309,8 @@ static int bnx2x_init_dev(struct bnx2x *bp, struct pci_dev *pdev,
 
 	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_CTAG_RX;
 	dev->features |= NETIF_F_HIGHDMA;
+	if (dev->features & NETIF_F_LRO)
+		dev->features &= ~NETIF_F_GRO_HW;
 
 	/* Add Loopback capability to the device */
 	dev->hw_features |= NETIF_F_LOOPBACK;
-- 
1.8.3.1

[PATCH net-next v5 5/5] qede: Use NETIF_F_GRO_HW.

[Michael Chan]

Advertise NETIF_F_GRO_HW and set edev->gro_disable according to the
feature flag.  Add qede_fix_features() to drop NETIF_F_GRO_HW if
XDP is running or MTU does not support GRO_HW or GRO is not set.
qede_change_mtu() also checks and disables GRO_HW if MTU is not
supported.

Cc: Ariel Elior <Ariel.Elior@cavium.com>
Cc: everest-linux-l2@cavium.com
Acked-by: Manish Chopra <manish.chopra@cavium.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/qlogic/qede/qede.h         |  2 ++
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c |  3 +++
 drivers/net/ethernet/qlogic/qede/qede_filter.c  | 21 ++++++++++++++-------
 drivers/net/ethernet/qlogic/qede/qede_main.c    | 17 ++++++-----------
 4 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede.h b/drivers/net/ethernet/qlogic/qede/qede.h
index a3a70ad..8a33651 100644
--- a/drivers/net/ethernet/qlogic/qede/qede.h
+++ b/drivers/net/ethernet/qlogic/qede/qede.h
@@ -494,6 +494,8 @@ int qede_free_tx_pkt(struct qede_dev *edev,
 void qede_vlan_mark_nonconfigured(struct qede_dev *edev);
 int qede_configure_vlan_filters(struct qede_dev *edev);
 
+netdev_features_t qede_fix_features(struct net_device *dev,
+				    netdev_features_t features);
 int qede_set_features(struct net_device *dev, netdev_features_t features);
 void qede_set_rx_mode(struct net_device *ndev);
 void qede_config_rx_mode(struct net_device *ndev);
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index dae7412..4ca3847 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -940,6 +940,9 @@ int qede_change_mtu(struct net_device *ndev, int new_mtu)
 	DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN),
 		   "Configuring MTU size of %d\n", new_mtu);
 
+	if (new_mtu > PAGE_SIZE)
+		ndev->features &= ~NETIF_F_GRO_HW;
+
 	/* Set the mtu field and re-start the interface if needed */
 	args.u.mtu = new_mtu;
 	args.func = &qede_update_mtu;
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index c1a0708..77aa826 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -895,19 +895,26 @@ static void qede_set_features_reload(struct qede_dev *edev,
 	edev->ndev->features = args->u.features;
 }
 
+netdev_features_t qede_fix_features(struct net_device *dev,
+				    netdev_features_t features)
+{
+	struct qede_dev *edev = netdev_priv(dev);
+
+	if (edev->xdp_prog || edev->ndev->mtu > PAGE_SIZE ||
+	    !(features & NETIF_F_GRO))
+		features &= ~NETIF_F_GRO_HW;
+
+	return features;
+}
+
 int qede_set_features(struct net_device *dev, netdev_features_t features)
 {
 	struct qede_dev *edev = netdev_priv(dev);
 	netdev_features_t changes = features ^ dev->features;
 	bool need_reload = false;
 
-	/* No action needed if hardware GRO is disabled during driver load */
-	if (changes & NETIF_F_GRO) {
-		if (dev->features & NETIF_F_GRO)
-			need_reload = !edev->gro_disable;
-		else
-			need_reload = edev->gro_disable;
-	}
+	if (changes & NETIF_F_GRO_HW)
+		need_reload = true;
 
 	if (need_reload) {
 		struct qede_reload_args args;
diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index 57332b3..90d79ae 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -545,6 +545,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 #endif
 	.ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid,
+	.ndo_fix_features = qede_fix_features,
 	.ndo_set_features = qede_set_features,
 	.ndo_get_stats64 = qede_get_stats64,
 #ifdef CONFIG_QED_SRIOV
@@ -572,6 +573,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	.ndo_change_mtu = qede_change_mtu,
 	.ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid,
+	.ndo_fix_features = qede_fix_features,
 	.ndo_set_features = qede_set_features,
 	.ndo_get_stats64 = qede_get_stats64,
 	.ndo_udp_tunnel_add = qede_udp_tunnel_add,
@@ -589,6 +591,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	.ndo_change_mtu = qede_change_mtu,
 	.ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid,
+	.ndo_fix_features = qede_fix_features,
 	.ndo_set_features = qede_set_features,
 	.ndo_get_stats64 = qede_get_stats64,
 	.ndo_udp_tunnel_add = qede_udp_tunnel_add,
@@ -676,7 +679,7 @@ static void qede_init_ndev(struct qede_dev *edev)
 	ndev->priv_flags |= IFF_UNICAST_FLT;
 
 	/* user-changeble features */
-	hw_features = NETIF_F_GRO | NETIF_F_SG |
+	hw_features = NETIF_F_GRO | NETIF_F_GRO_HW | NETIF_F_SG |
 		      NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 		      NETIF_F_TSO | NETIF_F_TSO6;
 
@@ -1228,18 +1231,9 @@ static int qede_alloc_sge_mem(struct qede_dev *edev, struct qede_rx_queue *rxq)
 	dma_addr_t mapping;
 	int i;
 
-	/* Don't perform FW aggregations in case of XDP */
-	if (edev->xdp_prog)
-		edev->gro_disable = 1;
-
 	if (edev->gro_disable)
 		return 0;
 
-	if (edev->ndev->mtu > PAGE_SIZE) {
-		edev->gro_disable = 1;
-		return 0;
-	}
-
 	for (i = 0; i < ETH_TPA_MAX_AGGS_NUM; i++) {
 		struct qede_agg_info *tpa_info = &rxq->tpa_info[i];
 		struct sw_rx_data *replace_buf = &tpa_info->buffer;
@@ -1269,6 +1263,7 @@ static int qede_alloc_sge_mem(struct qede_dev *edev, struct qede_rx_queue *rxq)
 err:
 	qede_free_sge_mem(edev, rxq);
 	edev->gro_disable = 1;
+	edev->ndev->features &= ~NETIF_F_GRO_HW;
 	return -ENOMEM;
 }
 
@@ -1511,7 +1506,7 @@ static void qede_init_fp(struct qede_dev *edev)
 			 edev->ndev->name, queue_id);
 	}
 
-	edev->gro_disable = !(edev->ndev->features & NETIF_F_GRO);
+	edev->gro_disable = !(edev->ndev->features & NETIF_F_GRO_HW);
 }
 
 static int qede_set_real_num_queues(struct qede_dev *edev)
-- 
1.8.3.1

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

[Alexander Duyck]

On Sat, Dec 16, 2017 at 12:09 AM, Michael Chan
<michael.chan@broadcom.com> wrote:
> 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.  Previously, drivers were using NETIF_F_GRO to
> control hardware GRO and so it cannot be independently turned on or
> off without affecting GRO.
>
> Hardware GRO (just like GRO) guarantees that packets can be re-segmented
> by TSO/GSO to reconstruct the original packet stream.  Logically,
> GRO_HW should depend on GRO since it a subset, but we will let
> individual drivers enforce this dependency as they see fit.
>
> Since NETIF_F_GRO is not propagated between upper and lower devices,
> NETIF_F_GRO_HW should follow suit since it is a subset of GRO.  In other
> words, a lower device can independent have GRO/GRO_HW enabled or disabled
> and no feature propagation is required.  This will preserve the current
> GRO behavior.  This can be changed later if we decide to propagate GRO/
> GRO_HW/RXCSUM from upper to lower devices.
>
> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> Cc: everest-linux-l2@cavium.com
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>

The changes look good to me. Thanks for doing all this work.

Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>

> ---
>  Documentation/networking/netdev-features.txt |  9 +++++++++
>  include/linux/netdev_features.h              |  3 +++
>  net/core/dev.c                               | 12 ++++++++++++
>  net/core/ethtool.c                           |  1 +
>  4 files changed, 25 insertions(+)
>
> diff --git a/Documentation/networking/netdev-features.txt b/Documentation/networking/netdev-features.txt
> index 7413eb0..c77f9d5 100644
> --- a/Documentation/networking/netdev-features.txt
> +++ b/Documentation/networking/netdev-features.txt
> @@ -163,3 +163,12 @@ This requests that the NIC receive all possible frames, 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 original packet stream.
> +Hardware GRO is dependent on RXCSUM since every packet successfully merged
> +by hardware must also have the checksum verified by hardware.
> diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> index b1b0ca7..db84c51 100644
> --- a/include/linux/netdev_features.h
> +++ b/include/linux/netdev_features.h
> @@ -78,6 +78,8 @@ enum {
>         NETIF_F_HW_ESP_TX_CSUM_BIT,     /* ESP with TX checksum offload */
>         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
> @@ -97,6 +99,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)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b0eee49..4b43f5d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7424,6 +7424,18 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>                 features &= ~dev->gso_partial_features;
>         }
>
> +       if (!(features & NETIF_F_RXCSUM)) {
> +               /* NETIF_F_GRO_HW implies doing RXCSUM since every packet
> +                * successfully merged by hardware must also have the
> +                * checksum verified by hardware.  If the user does not
> +                * want to enable RXCSUM, logically, we should disable GRO_HW.
> +                */
> +               if (features & NETIF_F_GRO_HW) {
> +                       netdev_dbg(dev, "Dropping NETIF_F_GRO_HW since no RXCSUM feature.\n");
> +                       features &= ~NETIF_F_GRO_HW;
> +               }
> +       }
> +
>         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
>

RE: [PATCH net-next v5 4/5] bnx2x: Use NETIF_F_GRO_HW.

[Chopra, Manish]

> -----Original Message-----
> From: Michael Chan [mailto:michael.chan@broadcom.com]
> Sent: Saturday, December 16, 2017 1:40 PM
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org; andrew.gospodarek@broadcom.com; Elior, Ariel
> <Ariel.Elior@cavium.com>; Dept-Eng Everest Linux L2 <Dept-
> EngEverestLinuxL2@cavium.com>
> Subject: [PATCH net-next v5 4/5] bnx2x: Use NETIF_F_GRO_HW.
> 
> Advertise NETIF_F_GRO_HW and turn on TPA_MODE_GRO when
> NETIF_F_GRO_HW is set.  Disable NETIF_F_GRO_HW in bnx2x_fix_features() if
> the MTU does not support TPA_MODE_GRO or GRO is not set.
> bnx2x_change_mtu() also needs to disable NETIF_F_GRO_HW if the MTU does
> not support it.
> 
> Original parameter disable_tpa will continue to disable LRO and GRO_HW.
> 
> Preserve the original behavior of enabling LRO by default.  User has to run
> ethtool -K to explicitly enable GRO_HW.
> 
> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> Cc: everest-linux-l2@cavium.com
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 24 ++++++++++++--------
> ----  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  8 +++++---
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> index 4c739d5..01b7f2f 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
> @@ -2482,8 +2482,7 @@ static void bnx2x_bz_fp(struct bnx2x *bp, int index)
>  	 */
>  	if (bp->dev->features & NETIF_F_LRO)
>  		fp->mode = TPA_MODE_LRO;
> -	else if (bp->dev->features & NETIF_F_GRO &&
> -		 bnx2x_mtu_allows_gro(bp->dev->mtu))
> +	else if (bp->dev->features & NETIF_F_GRO_HW)
>  		fp->mode = TPA_MODE_GRO;
>  	else
>  		fp->mode = TPA_MODE_DISABLED;
> @@ -4874,6 +4873,9 @@ int bnx2x_change_mtu(struct net_device *dev, int
> new_mtu)
>  	 */
>  	dev->mtu = new_mtu;
> 
> +	if (!bnx2x_mtu_allows_gro(new_mtu))
> +		dev->features &= ~NETIF_F_GRO_HW;
> +
>  	if (IS_PF(bp) && SHMEM2_HAS(bp, curr_cfg))
>  		SHMEM2_WR(bp, curr_cfg, CURR_CFG_MET_OS);
> 
> @@ -4903,10 +4905,13 @@ netdev_features_t bnx2x_fix_features(struct
> net_device *dev,
>  	}
> 
>  	/* TPA requires Rx CSUM offloading */
> -	if (!(features & NETIF_F_RXCSUM)) {
> +	if (!(features & NETIF_F_RXCSUM))
> +		features &= ~NETIF_F_LRO;
> +
> +	if (!(features & NETIF_F_GRO) || !bnx2x_mtu_allows_gro(dev->mtu))
> +		features &= ~NETIF_F_GRO_HW;
> +	if (features & NETIF_F_GRO_HW)
>  		features &= ~NETIF_F_LRO;
> -		features &= ~NETIF_F_GRO;
> -	}
> 
>  	return features;
>  }
> @@ -4933,13 +4938,8 @@ int bnx2x_set_features(struct net_device *dev,
> netdev_features_t features)
>  		}
>  	}
> 
> -	/* if GRO is changed while LRO is enabled, don't force a reload */
> -	if ((changes & NETIF_F_GRO) && (features & NETIF_F_LRO))
> -		changes &= ~NETIF_F_GRO;
> -
> -	/* if GRO is changed while HW TPA is off, don't force a reload */
> -	if ((changes & NETIF_F_GRO) && bp->disable_tpa)
> -		changes &= ~NETIF_F_GRO;
> +	/* Don't care about GRO changes */
> +	changes &= ~NETIF_F_GRO;
> 
>  	if (changes)
>  		bnx2x_reload = true;
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> index 91e2a75..4d06548 100644
> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
> @@ -12400,8 +12400,8 @@ static int bnx2x_init_bp(struct bnx2x *bp)
> 
>  	/* Set TPA flags */
>  	if (bp->disable_tpa) {
> -		bp->dev->hw_features &= ~NETIF_F_LRO;
> -		bp->dev->features &= ~NETIF_F_LRO;
> +		bp->dev->hw_features &= ~(NETIF_F_LRO |
> NETIF_F_GRO_HW);
> +		bp->dev->features &= ~(NETIF_F_LRO | NETIF_F_GRO_HW);
>  	}
> 
>  	if (CHIP_IS_E1(bp))
> @@ -13273,7 +13273,7 @@ static int bnx2x_init_dev(struct bnx2x *bp, struct
> pci_dev *pdev,
> 
>  	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM |
>  		NETIF_F_TSO | NETIF_F_TSO_ECN | NETIF_F_TSO6 |
> -		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_GRO |
> +		NETIF_F_RXCSUM | NETIF_F_LRO | NETIF_F_GRO |
> NETIF_F_GRO_HW |
>  		NETIF_F_RXHASH | NETIF_F_HW_VLAN_CTAG_TX;
>  	if (!chip_is_e1x) {
>  		dev->hw_features |= NETIF_F_GSO_GRE |
> NETIF_F_GSO_GRE_CSUM | @@ -13309,6 +13309,8 @@ static int
> bnx2x_init_dev(struct bnx2x *bp, struct pci_dev *pdev,
> 
>  	dev->features |= dev->hw_features | NETIF_F_HW_VLAN_CTAG_RX;
>  	dev->features |= NETIF_F_HIGHDMA;
> +	if (dev->features & NETIF_F_LRO)
> +		dev->features &= ~NETIF_F_GRO_HW;
> 
>  	/* Add Loopback capability to the device */
>  	dev->hw_features |= NETIF_F_LOOPBACK;
> --
> 1.8.3.1

Thanks Michael.
Acked-by: Manish Chopra <manish.chopra@cavium.com>

RE: [PATCH net-next v5 5/5] qede: Use NETIF_F_GRO_HW.

[Chopra, Manish]

> -----Original Message-----
> From: Michael Chan [mailto:michael.chan@broadcom.com]
> Sent: Saturday, December 16, 2017 1:40 PM
> To: davem@davemloft.net
> Cc: netdev@vger.kernel.org; andrew.gospodarek@broadcom.com; Elior, Ariel
> <Ariel.Elior@cavium.com>; Dept-Eng Everest Linux L2 <Dept-
> EngEverestLinuxL2@cavium.com>
> Subject: [PATCH net-next v5 5/5] qede: Use NETIF_F_GRO_HW.
> 
> Advertise NETIF_F_GRO_HW and set edev->gro_disable according to the feature
> flag.  Add qede_fix_features() to drop NETIF_F_GRO_HW if XDP is running or
> MTU does not support GRO_HW or GRO is not set.
> qede_change_mtu() also checks and disables GRO_HW if MTU is not supported.
> 
> Cc: Ariel Elior <Ariel.Elior@cavium.com>
> Cc: everest-linux-l2@cavium.com
> Acked-by: Manish Chopra <manish.chopra@cavium.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/qlogic/qede/qede.h         |  2 ++
>  drivers/net/ethernet/qlogic/qede/qede_ethtool.c |  3 +++
> drivers/net/ethernet/qlogic/qede/qede_filter.c  | 21 ++++++++++++++-------
>  drivers/net/ethernet/qlogic/qede/qede_main.c    | 17 ++++++-----------
>  4 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede.h
> b/drivers/net/ethernet/qlogic/qede/qede.h
> index a3a70ad..8a33651 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede.h
> +++ b/drivers/net/ethernet/qlogic/qede/qede.h
> @@ -494,6 +494,8 @@ int qede_free_tx_pkt(struct qede_dev *edev,  void
> qede_vlan_mark_nonconfigured(struct qede_dev *edev);  int
> qede_configure_vlan_filters(struct qede_dev *edev);
> 
> +netdev_features_t qede_fix_features(struct net_device *dev,
> +				    netdev_features_t features);
>  int qede_set_features(struct net_device *dev, netdev_features_t features);
> void qede_set_rx_mode(struct net_device *ndev);  void
> qede_config_rx_mode(struct net_device *ndev); diff --git
> a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> index dae7412..4ca3847 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
> @@ -940,6 +940,9 @@ int qede_change_mtu(struct net_device *ndev, int
> new_mtu)
>  	DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN),
>  		   "Configuring MTU size of %d\n", new_mtu);
> 
> +	if (new_mtu > PAGE_SIZE)
> +		ndev->features &= ~NETIF_F_GRO_HW;
> +
>  	/* Set the mtu field and re-start the interface if needed */
>  	args.u.mtu = new_mtu;
>  	args.func = &qede_update_mtu;
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> index c1a0708..77aa826 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
> @@ -895,19 +895,26 @@ static void qede_set_features_reload(struct
> qede_dev *edev,
>  	edev->ndev->features = args->u.features;  }
> 
> +netdev_features_t qede_fix_features(struct net_device *dev,
> +				    netdev_features_t features)
> +{
> +	struct qede_dev *edev = netdev_priv(dev);
> +
> +	if (edev->xdp_prog || edev->ndev->mtu > PAGE_SIZE ||
> +	    !(features & NETIF_F_GRO))
> +		features &= ~NETIF_F_GRO_HW;
> +
> +	return features;
> +}
> +
>  int qede_set_features(struct net_device *dev, netdev_features_t features)  {
>  	struct qede_dev *edev = netdev_priv(dev);
>  	netdev_features_t changes = features ^ dev->features;
>  	bool need_reload = false;
> 
> -	/* No action needed if hardware GRO is disabled during driver load */
> -	if (changes & NETIF_F_GRO) {
> -		if (dev->features & NETIF_F_GRO)
> -			need_reload = !edev->gro_disable;
> -		else
> -			need_reload = edev->gro_disable;
> -	}
> +	if (changes & NETIF_F_GRO_HW)
> +		need_reload = true;
> 
>  	if (need_reload) {
>  		struct qede_reload_args args;
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c
> b/drivers/net/ethernet/qlogic/qede/qede_main.c
> index 57332b3..90d79ae 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> @@ -545,6 +545,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq
> *ifr, int cmd)  #endif
>  	.ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid,
>  	.ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid,
> +	.ndo_fix_features = qede_fix_features,
>  	.ndo_set_features = qede_set_features,
>  	.ndo_get_stats64 = qede_get_stats64,
>  #ifdef CONFIG_QED_SRIOV
> @@ -572,6 +573,7 @@ static int qede_ioctl(struct net_device *dev, struct ifreq
> *ifr, int cmd)
>  	.ndo_change_mtu = qede_change_mtu,
>  	.ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid,
>  	.ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid,
> +	.ndo_fix_features = qede_fix_features,
>  	.ndo_set_features = qede_set_features,
>  	.ndo_get_stats64 = qede_get_stats64,
>  	.ndo_udp_tunnel_add = qede_udp_tunnel_add, @@ -589,6 +591,7 @@
> static int qede_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
>  	.ndo_change_mtu = qede_change_mtu,
>  	.ndo_vlan_rx_add_vid = qede_vlan_rx_add_vid,
>  	.ndo_vlan_rx_kill_vid = qede_vlan_rx_kill_vid,
> +	.ndo_fix_features = qede_fix_features,
>  	.ndo_set_features = qede_set_features,
>  	.ndo_get_stats64 = qede_get_stats64,
>  	.ndo_udp_tunnel_add = qede_udp_tunnel_add, @@ -676,7 +679,7 @@
> static void qede_init_ndev(struct qede_dev *edev)
>  	ndev->priv_flags |= IFF_UNICAST_FLT;
> 
>  	/* user-changeble features */
> -	hw_features = NETIF_F_GRO | NETIF_F_SG |
> +	hw_features = NETIF_F_GRO | NETIF_F_GRO_HW | NETIF_F_SG |
>  		      NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  		      NETIF_F_TSO | NETIF_F_TSO6;
> 
> @@ -1228,18 +1231,9 @@ static int qede_alloc_sge_mem(struct qede_dev
> *edev, struct qede_rx_queue *rxq)
>  	dma_addr_t mapping;
>  	int i;
> 
> -	/* Don't perform FW aggregations in case of XDP */
> -	if (edev->xdp_prog)
> -		edev->gro_disable = 1;
> -
>  	if (edev->gro_disable)
>  		return 0;
> 
> -	if (edev->ndev->mtu > PAGE_SIZE) {
> -		edev->gro_disable = 1;
> -		return 0;
> -	}
> -
>  	for (i = 0; i < ETH_TPA_MAX_AGGS_NUM; i++) {
>  		struct qede_agg_info *tpa_info = &rxq->tpa_info[i];
>  		struct sw_rx_data *replace_buf = &tpa_info->buffer; @@ -
> 1269,6 +1263,7 @@ static int qede_alloc_sge_mem(struct qede_dev *edev,
> struct qede_rx_queue *rxq)
>  err:
>  	qede_free_sge_mem(edev, rxq);
>  	edev->gro_disable = 1;
> +	edev->ndev->features &= ~NETIF_F_GRO_HW;
>  	return -ENOMEM;
>  }
> 
> @@ -1511,7 +1506,7 @@ static void qede_init_fp(struct qede_dev *edev)
>  			 edev->ndev->name, queue_id);
>  	}
> 
> -	edev->gro_disable = !(edev->ndev->features & NETIF_F_GRO);
> +	edev->gro_disable = !(edev->ndev->features & NETIF_F_GRO_HW);
>  }
> 
>  static int qede_set_real_num_queues(struct qede_dev *edev)
> --
> 1.8.3.1

Thanks Michael.
Acked-by: Manish Chopra <manish.chopra@cavium.com>

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[David Miller]

From: Michael Chan <michael.chan@broadcom.com>
Date: Sat, 16 Dec 2017 03:09:39 -0500

> Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
> hardware GRO to use the new flag.

Series applied, thanks for following through with this work.

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Marcelo Ricardo Leitner]

On Tue, Dec 19, 2017 at 10:50:24AM -0500, David Miller wrote:
> From: Michael Chan <michael.chan@broadcom.com>
> Date: Sat, 16 Dec 2017 03:09:39 -0500
> 
> > Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
> > hardware GRO to use the new flag.
> 
> Series applied, thanks for following through with this work.

Can we clarify on the meaning/expectations of dev_weight? The
documentation currently says:
The maximum number of packets that kernel can handle on a NAPI
interrupt, it's a Per-CPU variable.

I believe 'packets' here refers to packets on the wire.

For drivers doing LRO, we don't have visibility on how many
packets were aggregated so they count as 1, aggregated or not.

But for GRO_HW, drivers implementing it will get a bonus on its
dev_weight because instead of pulling 5 packets in a cycle to create 1
gro'ed skb, it will pull 1 big packet (which includes 5) and count it
as 1.

I understand that for all that matters, the hardware operations
involved on GRO_HW are really for only 1 packet, so it would make
sense to count it as 1. OTOH, this bump may cause additional pressure
in other places as in fact we are allowing more packets in in a given
cycle.

At least qede driver is counting 1 GRO_HW pkt as 1 budget.

Thanks,
Marcelo

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[David Miller]

From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Tue, 19 Dec 2017 17:04:27 -0200

> I understand that for all that matters, the hardware operations
> involved on GRO_HW are really for only 1 packet, so it would make
> sense to count it as 1. OTOH, this bump may cause additional pressure
> in other places as in fact we are allowing more packets in in a given
> cycle.

More data, but not more packets or (realistically) "work".

The stack is going to parse only one SKB, one set of networking
headers, do one route lookup, one socket demux, etc.

In that view, counting the HW GRO packet as only one frame is
appropriate.

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Michael Chan]

On Tue, Dec 19, 2017 at 11:04 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> Can we clarify on the meaning/expectations of dev_weight? The
> documentation currently says:
> The maximum number of packets that kernel can handle on a NAPI
> interrupt, it's a Per-CPU variable.
>
> I believe 'packets' here refers to packets on the wire.
>
> For drivers doing LRO, we don't have visibility on how many
> packets were aggregated so they count as 1, aggregated or not.
>
> But for GRO_HW, drivers implementing it will get a bonus on its
> dev_weight because instead of pulling 5 packets in a cycle to create 1
> gro'ed skb, it will pull 1 big packet (which includes 5) and count it
> as 1.
>

Right, as I replied to you earlier, it's very simple to make this
adjustment for GRO_HW packets in the driver.  I will make this change
for bnxt_en in my next net-next patchset and I will update the
dev_weight documentation as well.

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Marcelo Ricardo Leitner]

On Tue, Dec 19, 2017 at 11:25:29AM -0800, Michael Chan wrote:
> On Tue, Dec 19, 2017 at 11:04 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > Can we clarify on the meaning/expectations of dev_weight? The
> > documentation currently says:
> > The maximum number of packets that kernel can handle on a NAPI
> > interrupt, it's a Per-CPU variable.
> >
> > I believe 'packets' here refers to packets on the wire.
> >
> > For drivers doing LRO, we don't have visibility on how many
> > packets were aggregated so they count as 1, aggregated or not.
> >
> > But for GRO_HW, drivers implementing it will get a bonus on its
> > dev_weight because instead of pulling 5 packets in a cycle to create 1
> > gro'ed skb, it will pull 1 big packet (which includes 5) and count it
> > as 1.
> >
> 
> Right, as I replied to you earlier, it's very simple to make this
> adjustment for GRO_HW packets in the driver.  I will make this change
> for bnxt_en in my next net-next patchset and I will update the
> dev_weight documentation as well.
> 

Sounds like just the documentation would be enough. I agree with Dave
in the other reply. It makes sense to count it as 1, but getting that
more clear in the doc is welcomed.

Thanks,
Marcelo

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Sabrina Dubroca]

Hello,

Sorry for commenting late.

2017-12-16, 03:09:39 -0500, Michael Chan wrote:
> Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
> hardware GRO to use the new flag.
> 
> v5:
> - Documentation changes requested by Alexander Duyck.
> - bnx2x changes requested by Manish Chopra to enable LRO by default, and
> disable GRO_HW if disable_tpa module parameter is set.
> 
> v4:
> - more changes requested by Alexander Duyck:
> - check GRO_HW/GRO dependency in drivers's ndo_fix_features().
> - Reverse the order of RXCSUM and GRO_HW dependency check in
> netdev_fix_features().
> - No propagation in netdev_disable_gro_hw().

IIUC, with the patches that were applied, each driver can define
whether GRO_HW depends on GRO? From a user's perspective, this
inconsistent behavior is going to be quite confusing.

Worse than inconsistent behavior, it looks like a driver deciding that
GRO_HW doesn't depend on GRO is going to introduce a change of
behavior.  Previously, when GRO was disabled, there wouldn't be any
packet over MTU handed to the network stack.  Now, even if GRO is
disabled, GRO_HW might still be enabled, so we might get over-MTU
packets because of hardware GRO.

I don't think drivers should be allowed to say "GRO_HW doesn't depend
on GRO".

I think it's reasonable to be able to disable software GRO even if
hardware GRO is enabled. Thus, I would propose:
- keep the current GRO flag
- add a GRO_HW flag, depending on GRO, enforced by the core as in
  earlier versions of these patches
- add a GRO_SW flag, also depending on GRO


Thanks,

-- 
Sabrina

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Alexander Duyck]

On Fri, Dec 22, 2017 at 6:57 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> Hello,
>
> Sorry for commenting late.
>
> 2017-12-16, 03:09:39 -0500, Michael Chan wrote:
>> Introduce NETIF_F_GRO_HW feature flag and convert drivers that support
>> hardware GRO to use the new flag.
>>
>> v5:
>> - Documentation changes requested by Alexander Duyck.
>> - bnx2x changes requested by Manish Chopra to enable LRO by default, and
>> disable GRO_HW if disable_tpa module parameter is set.
>>
>> v4:
>> - more changes requested by Alexander Duyck:
>> - check GRO_HW/GRO dependency in drivers's ndo_fix_features().
>> - Reverse the order of RXCSUM and GRO_HW dependency check in
>> netdev_fix_features().
>> - No propagation in netdev_disable_gro_hw().
>
> IIUC, with the patches that were applied, each driver can define
> whether GRO_HW depends on GRO? From a user's perspective, this
> inconsistent behavior is going to be quite confusing.
>
> Worse than inconsistent behavior, it looks like a driver deciding that
> GRO_HW doesn't depend on GRO is going to introduce a change of
> behavior.  Previously, when GRO was disabled, there wouldn't be any
> packet over MTU handed to the network stack.  Now, even if GRO is
> disabled, GRO_HW might still be enabled, so we might get over-MTU
> packets because of hardware GRO.

This isn't actually true. LRO was still handling packets larger than
MTU over even when GRO was disabled.

> I don't think drivers should be allowed to say "GRO_HW doesn't depend
> on GRO".

Why not, it doesn't. In my mind GRO_HW is closer to LRO than it is to
GRO. The only ugly bit as I see it is that these devices were exposing
the feature via the GRO flag in the first place. So for the sake of
legacy they might want to carry around the dependency.

> I think it's reasonable to be able to disable software GRO even if
> hardware GRO is enabled. Thus, I would propose:
> - keep the current GRO flag
> - add a GRO_HW flag, depending on GRO, enforced by the core as in
>   earlier versions of these patches
> - add a GRO_SW flag, also depending on GRO

This seems like a bunch of extra overhead for not much gain. Do we
really need to fork GRO into 3 bits? I would argue that GRO_HW really
should have been branded something like FORWARDABLE_LRO, but nobody
wanted to touch the name LRO since it apparently has some negative
stigma to it. If we had used a name like that we probably wouldn't be
going through all these extra hoops. The only real reason why this is
even being associated with GRO in the first place is that is how this
feature was hidden by the drivers so they got around having to deal
with the LRO being disabled for routing/forwarding issue. Those are
the parts that want to keep it associated with GRO since that is how
they exposed it in their devices originally.

- Alex

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Sabrina Dubroca]

2017-12-22, 10:14:32 -0800, Alexander Duyck wrote:
> On Fri, Dec 22, 2017 at 6:57 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> > IIUC, with the patches that were applied, each driver can define
> > whether GRO_HW depends on GRO? From a user's perspective, this
> > inconsistent behavior is going to be quite confusing.
> >
> > Worse than inconsistent behavior, it looks like a driver deciding that
> > GRO_HW doesn't depend on GRO is going to introduce a change of
> > behavior.  Previously, when GRO was disabled, there wouldn't be any
> > packet over MTU handed to the network stack.  Now, even if GRO is
> > disabled, GRO_HW might still be enabled, so we might get over-MTU
> > packets because of hardware GRO.
> 
> This isn't actually true. LRO was still handling packets larger than
> MTU over even when GRO was disabled.

Sure, LRO will also cause that, but we're speaking in the context of
GRO here, which means no LRO.


> > I don't think drivers should be allowed to say "GRO_HW doesn't depend
> > on GRO".
> 
> Why not, it doesn't. In my mind GRO_HW is closer to LRO than it is to
> GRO.

Why do you say that? It looks like GRO to me. These drivers are
calling tcp_gro_complete() for example.

> The only ugly bit as I see it is that these devices were exposing
> the feature via the GRO flag in the first place. So for the sake of
> legacy they might want to carry around the dependency.
> 
> > I think it's reasonable to be able to disable software GRO even if
> > hardware GRO is enabled. Thus, I would propose:
> > - keep the current GRO flag
> > - add a GRO_HW flag, depending on GRO, enforced by the core as in
> >   earlier versions of these patches
> > - add a GRO_SW flag, also depending on GRO
> 
> This seems like a bunch of extra overhead for not much gain. Do we
> really need to fork GRO into 3 bits? I would argue that GRO_HW really
> should have been branded something like FORWARDABLE_LRO, but nobody
> wanted to touch the name LRO since it apparently has some negative
> stigma to it. If we had used a name like that we probably wouldn't be
> going through all these extra hoops. The only real reason why this is
> even being associated with GRO in the first place is that is how this
> feature was hidden by the drivers so they got around having to deal
> with the LRO being disabled for routing/forwarding issue. Those are
> the parts that want to keep it associated with GRO since that is how
> they exposed it in their devices originally.

I think it wouldn't have hidden behind GRO if it wasn't GRO. Again,
why do you think it's not GRO?

-- 
Sabrina

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Alexander Duyck]

On Fri, Dec 29, 2017 at 4:43 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2017-12-22, 10:14:32 -0800, Alexander Duyck wrote:
>> On Fri, Dec 22, 2017 at 6:57 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
>> > IIUC, with the patches that were applied, each driver can define
>> > whether GRO_HW depends on GRO? From a user's perspective, this
>> > inconsistent behavior is going to be quite confusing.
>> >
>> > Worse than inconsistent behavior, it looks like a driver deciding that
>> > GRO_HW doesn't depend on GRO is going to introduce a change of
>> > behavior.  Previously, when GRO was disabled, there wouldn't be any
>> > packet over MTU handed to the network stack.  Now, even if GRO is
>> > disabled, GRO_HW might still be enabled, so we might get over-MTU
>> > packets because of hardware GRO.
>>
>> This isn't actually true. LRO was still handling packets larger than
>> MTU over even when GRO was disabled.
>
> Sure, LRO will also cause that, but we're speaking in the context of
> GRO here, which means no LRO.

We are talking about GRO_HW. Which is basically aggregation being
performed in hardware. The choice of name is unfortunate though since
it implies this is GRO when what is actually happening is just
GRO-like. Really the only difference between LRO and GRO_HW is that
the frames are better formed in the final result. It is a matter of
what is performed versus where it is performed.

>> > I don't think drivers should be allowed to say "GRO_HW doesn't depend
>> > on GRO".
>>
>> Why not, it doesn't. In my mind GRO_HW is closer to LRO than it is to
>> GRO.
>
> Why do you say that? It looks like GRO to me. These drivers are
> calling tcp_gro_complete() for example.

The final frame output in the case of frames that are aggregated will
be the same as GRO. However LRO could theoretically do the same thing
if the hardware had been implemented correctly in the first place.

>> The only ugly bit as I see it is that these devices were exposing
>> the feature via the GRO flag in the first place. So for the sake of
>> legacy they might want to carry around the dependency.
>>
>> > I think it's reasonable to be able to disable software GRO even if
>> > hardware GRO is enabled. Thus, I would propose:
>> > - keep the current GRO flag
>> > - add a GRO_HW flag, depending on GRO, enforced by the core as in
>> >   earlier versions of these patches
>> > - add a GRO_SW flag, also depending on GRO
>>
>> This seems like a bunch of extra overhead for not much gain. Do we
>> really need to fork GRO into 3 bits? I would argue that GRO_HW really
>> should have been branded something like FORWARDABLE_LRO, but nobody
>> wanted to touch the name LRO since it apparently has some negative
>> stigma to it. If we had used a name like that we probably wouldn't be
>> going through all these extra hoops. The only real reason why this is
>> even being associated with GRO in the first place is that is how this
>> feature was hidden by the drivers so they got around having to deal
>> with the LRO being disabled for routing/forwarding issue. Those are
>> the parts that want to keep it associated with GRO since that is how
>> they exposed it in their devices originally.
>
> I think it wouldn't have hidden behind GRO if it wasn't GRO. Again,
> why do you think it's not GRO?

The only real piece I see as missing the propagation bits for GRO_HW
to upper devices. The argument was that GRO doesn't have to do that,
but I think we are going to have to get there at some point so that
when we encounter the first piece of hardware that does this wrong we
have a switch ready to allow us to disable it for debugging. If we are
insistent on having a switch that binds together GRO and GRO_HW one
thought I had was to change the meaning of GRO_HW for virtual devices
to indicate that we allow GRO to happen on the lower devices
associated with this device. My thought was that GRO_HW allows GRO to
take place on the physical hardware below the netdev, so why couldn't
we say that GRO_HW also applies to upper devices and them indicating
they don't want GRO, GRO_HW, or LRO for lower devices. Thoughts? The
only reason it occurred to me is that there is currently no way to
propagate a disable of GRO so GRO_HW might be a way to do that for
virtual devices. It would make GRO_HW more about the location of the
GRO feature instead of the feature itself. Basically if you clear it
then nothing below that point should be doing any sort of aggregation.

Thanks.

- Alex

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Michael Chan]

On Fri, Dec 29, 2017 at 7:12 AM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Fri, Dec 29, 2017 at 4:43 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
>> 2017-12-22, 10:14:32 -0800, Alexander Duyck wrote:
>>> On Fri, Dec 22, 2017 at 6:57 AM, Sabrina Dubroca <sd@queasysnail.net> wrote:
>>> > IIUC, with the patches that were applied, each driver can define
>>> > whether GRO_HW depends on GRO? From a user's perspective, this
>>> > inconsistent behavior is going to be quite confusing.
>>> >
>>> > Worse than inconsistent behavior, it looks like a driver deciding that
>>> > GRO_HW doesn't depend on GRO is going to introduce a change of
>>> > behavior.  Previously, when GRO was disabled, there wouldn't be any
>>> > packet over MTU handed to the network stack.  Now, even if GRO is
>>> > disabled, GRO_HW might still be enabled, so we might get over-MTU
>>> > packets because of hardware GRO.
>>>
>>> This isn't actually true. LRO was still handling packets larger than
>>> MTU over even when GRO was disabled.
>>
>> Sure, LRO will also cause that, but we're speaking in the context of
>> GRO here, which means no LRO.
>
> We are talking about GRO_HW. Which is basically aggregation being
> performed in hardware. The choice of name is unfortunate though since
> it implies this is GRO when what is actually happening is just
> GRO-like. Really the only difference between LRO and GRO_HW is that
> the frames are better formed in the final result. It is a matter of
> what is performed versus where it is performed.

I think the name GRO_HW is perfectly fine.  It is GRO aggregation done
in hardware, and hardware providing extra information to the driver to
setup the SKB just like GRO.  I don't know what better name to call it
than GRO_HW.

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[David Miller]

From: Michael Chan <michael.chan@broadcom.com>
Date: Fri, 29 Dec 2017 21:20:02 -0800

> I think the name GRO_HW is perfectly fine.  It is GRO aggregation done
> in hardware, and hardware providing extra information to the driver to
> setup the SKB just like GRO.  I don't know what better name to call it
> than GRO_HW.

Agreed.

RE: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Shay Agroskin]

Hi,
To use NETIF_F_GRO_HW offload, we first need to double-check what are its exact well-defined requirements/expectations.

1) What is the defined relation between the (SW) GRO (that can be easily modified in kernel) and HW-GRO that
should be a well-defined HW capability ?

2) While we understand that implementing GRO in HW can result in performance gain, is HW-GRO's
singularity in its ability to construct packets that can be re-segmented? (as appose to LRO).
What else is expected from HW when turning it on?

3) For what I managed to gather from different mail-threads/presentations,
this is the list of conditions that makes an aggregated packet reversible:

a) All segments (aggregated packets) must be of the same size (except possibly the last one).
(This in order to know each segments size, when re-segmenting an aggregated packet)

b) IP ID must be incrementing unless DF is 
(To have  the whole 4th layer, in order to know if a packet belongs to a certain flow)

c) Also, all the other fields need to be the same to belong to the same flow and be aggregated.

Any thoughts? Do I miss anything here ?
Thanks,
Shay Agroskin

-----Original Message-----
From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On Behalf Of Michael Chan
Sent: Saturday, December 16, 2017 10:10 AM
To: davem@davemloft.net
Cc: netdev@vger.kernel.org; andrew.gospodarek@broadcom.com
Subject: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

Introduce NETIF_F_GRO_HW feature flag and convert drivers that support hardware GRO to use the new flag.

v5:
- Documentation changes requested by Alexander Duyck.
- bnx2x changes requested by Manish Chopra to enable LRO by default, and disable GRO_HW if disable_tpa module parameter is set.

v4:
- more changes requested by Alexander Duyck:
- check GRO_HW/GRO dependency in drivers's ndo_fix_features().
- Reverse the order of RXCSUM and GRO_HW dependency check in netdev_fix_features().
- No propagation in netdev_disable_gro_hw().

v3:
- Let driver's ndo_fix_features() disable NETIF_F_LRO when NETIF_F_GRO_HW is set instead of doing it in common netdev_fix_features().

v2:
- NETIF_F_GRO_HW flag propagation between upper and lower devices not required (see patch 1).
- NETIF_F_GRO_HW depends on NETIF_F_GRO and NETIF_F_RXCSUM.
- Add dev_disable_gro_hw() to disable GRO_HW for generic XDP.
- Use ndo_fix_features() on all 3 drivers to drop GRO_HW when it is not supported

Michael Chan (5):
  net: Introduce NETIF_F_GRO_HW.
  net: Disable GRO_HW when generic XDP is installed on a device.
  bnxt_en: Use NETIF_F_GRO_HW.
  bnx2x: Use NETIF_F_GRO_HW.
  qede: Use NETIF_F_GRO_HW.

 Documentation/networking/netdev-features.txt     |  9 +++++++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c  | 24 +++++++++----------  drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |  8 ++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c        | 27 +++++++++++++++------
 drivers/net/ethernet/qlogic/qede/qede.h          |  2 ++
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c  |  3 +++
 drivers/net/ethernet/qlogic/qede/qede_filter.c   | 21 +++++++++++------
 drivers/net/ethernet/qlogic/qede/qede_main.c     | 17 +++++---------
 include/linux/netdev_features.h                  |  3 +++
 net/core/dev.c                                   | 30 ++++++++++++++++++++++++
 net/core/ethtool.c                               |  1 +
 11 files changed, 105 insertions(+), 40 deletions(-)

--
1.8.3.1

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[David Miller]

From: Shay Agroskin <shayag@mellanox.com>
Date: Mon, 7 Jan 2019 14:00:33 +0000

> 2) While we understand that implementing GRO in HW can result in
> performance gain, is HW-GRO's singularity in its ability to
> construct packets that can be re-segmented? (as appose to LRO).
> What else is expected from HW when turning it on?

This is the basic requirement, that the original packet stream can
be accurately reconstituted from the HW GRO packet.

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Michael Chan]

On Mon, Jan 7, 2019 at 6:00 AM Shay Agroskin <shayag@mellanox.com> wrote:

> a) All segments (aggregated packets) must be of the same size (except possibly the last one).
> (This in order to know each segments size, when re-segmenting an aggregated packet)
>
> b) IP ID must be incrementing unless DF is
> (To have  the whole 4th layer, in order to know if a packet belongs to a certain flow)
>
> c) Also, all the other fields need to be the same to belong to the same flow and be aggregated.
>
> Any thoughts? Do I miss anything here ?

In addition, the GRO SKB needs to be setup properly.  For example, the
gso_type, gso_size, gso_segs, CHECKSUM_PARTIAL, etc, all have to be
setup.  Hardware needs to provide all the necessary information to set
this up.  In case this GRO SKB is re-routed to an output device, TSO
or GSO needs to be performed on the packet and the original packet
stream should be re-constituted exactly.

Re: [PATCH net-next v5 0/5] Introduce NETIF_F_GRO_HW

[Shay Agroskin]

Thank you for your replies. It answered my questions.

Shay

On 1/8/2019 12:58 PM, Michael Chan wrote:
> On Mon, Jan 7, 2019 at 6:00 AM Shay Agroskin <shayag@mellanox.com> wrote:
>
>> a) All segments (aggregated packets) must be of the same size (except possibly the last one).
>> (This in order to know each segments size, when re-segmenting an aggregated packet)
>>
>> b) IP ID must be incrementing unless DF is
>> (To have  the whole 4th layer, in order to know if a packet belongs to a certain flow)
>>
>> c) Also, all the other fields need to be the same to belong to the same flow and be aggregated.
>>
>> Any thoughts? Do I miss anything here ?
> In addition, the GRO SKB needs to be setup properly.  For example, the
> gso_type, gso_size, gso_segs, CHECKSUM_PARTIAL, etc, all have to be
> setup.  Hardware needs to provide all the necessary information to set
> this up.  In case this GRO SKB is re-routed to an output device, TSO
> or GSO needs to be performed on the packet and the original packet
> stream should be re-constituted exactly.