diff mbox series

[net] bonding: Correctly support GSO ESP offload

Message ID 20250123150909.387415-1-cratiu@nvidia.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net] bonding: Correctly support GSO ESP offload | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 2 maintainers not CCed: jv@jvosburgh.net andrew+netdev@lunn.ch
netdev/build_clang success Errors and warnings before: 2 this patch: 2
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch fail ERROR: Remove Gerrit Change-Id's before submitting upstream WARNING: line length of 86 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 13 this patch: 13
netdev/source_inline success Was 0 now: 0

Commit Message

Cosmin Ratiu Jan. 23, 2025, 3:09 p.m. UTC 
The referenced fix is incomplete. It correctly computes
bond_dev->gso_partial_features across slaves, but unfortunately
netdev_fix_features discards gso_partial_features from the feature set
if NETIF_F_GSO_PARTIAL isn't set in bond->features.

This is visible with ethtool -k bond0 | grep esp:
tx-esp-segmentation: off [requested on]
esp-hw-offload: on
esp-tx-csum-hw-offload: on

This patch reworks the bonding GSO offload support by:
- making aggregating gso_partial_features across slaves similar to the
  other feature sets (this part is a no-op).
- adding NETIF_F_GSO_PARTIAL to hw_enc_features filtered across slaves.
- adding NETIF_F_GSO_PARTIAL to features in bond_setup()

With all of these, 'ethtool -k bond0 | grep esp' now reports:
tx-esp-segmentation: on
esp-hw-offload: on
esp-tx-csum-hw-offload: on

Fixes: 4861333b4217 ("bonding: add ESP offload features when slaves support")
Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
Change-Id: Iebd2a9d903d3e056e7717e8ca2527a9adf21b2e1
---
 drivers/net/bonding/bond_main.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Hangbin Liu Jan. 24, 2025, 3:29 a.m. UTC | #1 
On Thu, Jan 23, 2025 at 05:09:09PM +0200, Cosmin Ratiu wrote:
> The referenced fix is incomplete. It correctly computes
> bond_dev->gso_partial_features across slaves, but unfortunately
> netdev_fix_features discards gso_partial_features from the feature set
> if NETIF_F_GSO_PARTIAL isn't set in bond->features.
> 
> This is visible with ethtool -k bond0 | grep esp:
> tx-esp-segmentation: off [requested on]
> esp-hw-offload: on
> esp-tx-csum-hw-offload: on
> 
> This patch reworks the bonding GSO offload support by:
> - making aggregating gso_partial_features across slaves similar to the
>   other feature sets (this part is a no-op).
> - adding NETIF_F_GSO_PARTIAL to hw_enc_features filtered across slaves.
> - adding NETIF_F_GSO_PARTIAL to features in bond_setup()
> 
> With all of these, 'ethtool -k bond0 | grep esp' now reports:
> tx-esp-segmentation: on
> esp-hw-offload: on
> esp-tx-csum-hw-offload: on
> 
> Fixes: 4861333b4217 ("bonding: add ESP offload features when slaves support")
> Signed-off-by: Cosmin Ratiu <cratiu@nvidia.com>
> Change-Id: Iebd2a9d903d3e056e7717e8ca2527a9adf21b2e1

What's Change-Id here? Others looks good to me.

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
Cosmin Ratiu Jan. 24, 2025, 8:12 a.m. UTC | #2 
On Fri, 2025-01-24 at 03:29 +0000, Hangbin Liu wrote:
> 
> What's Change-Id here? Others looks good to me.

Apologies, I forgot to remove it. It's from gerrit, the internal code
review tool we use.
I'll resend v2

> 
> Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

Thanks for the review!
diff mbox series

Patch

diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 7b78c2bada81..de105868c009 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1538,17 +1538,20 @@  static netdev_features_t bond_fix_features(struct net_device *dev,
 				 NETIF_F_HIGHDMA | NETIF_F_LRO)
 
 #define BOND_ENC_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
-				 NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE)
+				 NETIF_F_RXCSUM | NETIF_F_GSO_SOFTWARE | \
+				 NETIF_F_GSO_PARTIAL)
 
 #define BOND_MPLS_FEATURES	(NETIF_F_HW_CSUM | NETIF_F_SG | \
 				 NETIF_F_GSO_SOFTWARE)
 
+#define BOND_GSO_PARTIAL_FEATURES (NETIF_F_GSO_ESP)
+
 
 static void bond_compute_features(struct bonding *bond)
 {
+	netdev_features_t gso_partial_features = BOND_GSO_PARTIAL_FEATURES;
 	unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE |
 					IFF_XMIT_DST_RELEASE_PERM;
-	netdev_features_t gso_partial_features = NETIF_F_GSO_ESP;
 	netdev_features_t vlan_features = BOND_VLAN_FEATURES;
 	netdev_features_t enc_features  = BOND_ENC_FEATURES;
 #ifdef CONFIG_XFRM_OFFLOAD
@@ -1582,8 +1585,9 @@  static void bond_compute_features(struct bonding *bond)
 							  BOND_XFRM_FEATURES);
 #endif /* CONFIG_XFRM_OFFLOAD */
 
-		if (slave->dev->hw_enc_features & NETIF_F_GSO_PARTIAL)
-			gso_partial_features &= slave->dev->gso_partial_features;
+		gso_partial_features = netdev_increment_features(gso_partial_features,
+								 slave->dev->gso_partial_features,
+								 BOND_GSO_PARTIAL_FEATURES);
 
 		mpls_features = netdev_increment_features(mpls_features,
 							  slave->dev->mpls_features,
@@ -1598,10 +1602,7 @@  static void bond_compute_features(struct bonding *bond)
 	}
 	bond_dev->hard_header_len = max_hard_header_len;
 
-	if (gso_partial_features & NETIF_F_GSO_ESP)
-		bond_dev->gso_partial_features |= NETIF_F_GSO_ESP;
-	else
-		bond_dev->gso_partial_features &= ~NETIF_F_GSO_ESP;
+	bond_dev->gso_partial_features = gso_partial_features;
 
 done:
 	bond_dev->vlan_features = vlan_features;
@@ -6046,6 +6047,7 @@  void bond_setup(struct net_device *bond_dev)
 	bond_dev->hw_features |= NETIF_F_GSO_ENCAP_ALL;
 	bond_dev->features |= bond_dev->hw_features;
 	bond_dev->features |= NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX;
+	bond_dev->features |= NETIF_F_GSO_PARTIAL;
 #ifdef CONFIG_XFRM_OFFLOAD
 	bond_dev->hw_features |= BOND_XFRM_FEATURES;
 	/* Only enable XFRM features if this is an active-backup config */