The fixed commit adds NETIF_F_GSO_ESP bit for bonding gso_partial_features. However, if we don't set the dev NETIF_F_GSO_PARTIAL bit, the later netdev_change_features() -> netdev_fix_features() will remove the NETIF_F_GSO_ESP bit from the dev features. This causes ethtool to show that the bond does not support tx-esp-segmentation. For example
# ethtool -k bond0 | grep esp tx-esp-segmentation: off [requested on] esp-hw-offload: on esp-tx-csum-hw-offload: on
Add the NETIF_F_GSO_PARTIAL bit to bond dev features when set gso_partial_features to fix this issue.
Fixes: 4861333b4217 ("bonding: add ESP offload features when slaves support") Reported-by: Liang Li liali@redhat.com Signed-off-by: Hangbin Liu liuhangbin@gmail.com --- v2: remove NETIF_F_GSO_PARTIAL bit if not set gso_partial_features.
The issue is reported internally, so there is no Closes tag.
BTW, I saw some drivers set NETIF_F_GSO_PARTIAL on dev->features. Some other drivers set NETIF_F_GSO_PARTIAL on dev->hw_enc_features. I haven't see a doc about where we should set. So I just set it on dev->features. --- drivers/net/bonding/bond_main.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 7b78c2bada81..09d5a8433d86 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1598,10 +1598,13 @@ 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) + if (gso_partial_features & NETIF_F_GSO_ESP) { bond_dev->gso_partial_features |= NETIF_F_GSO_ESP; - else + bond_dev->features |= NETIF_F_GSO_PARTIAL; + } else { bond_dev->gso_partial_features &= ~NETIF_F_GSO_ESP; + bond_dev->features &= ~NETIF_F_GSO_PARTIAL; + }
done: bond_dev->vlan_features = vlan_features;
On Wed, 2025-01-22 at 13:52 +0000, Hangbin Liu wrote:
The fixed commit adds NETIF_F_GSO_ESP bit for bonding gso_partial_features. However, if we don't set the dev NETIF_F_GSO_PARTIAL bit, the later netdev_change_features() -> netdev_fix_features() will remove the NETIF_F_GSO_ESP bit from the dev features. This causes ethtool to show that the bond does not support tx-esp-segmentation. For example
# ethtool -k bond0 | grep esp tx-esp-segmentation: off [requested on] esp-hw-offload: on esp-tx-csum-hw-offload: on
Add the NETIF_F_GSO_PARTIAL bit to bond dev features when set gso_partial_features to fix this issue.
Fixes: 4861333b4217 ("bonding: add ESP offload features when slaves support") Reported-by: Liang Li liali@redhat.com Signed-off-by: Hangbin Liu liuhangbin@gmail.com
v2: remove NETIF_F_GSO_PARTIAL bit if not set gso_partial_features.
I don't think this is needed, to avoid having bond_compute_features modify bond_dev->features directly. And in general, I think NETIF_F_GSO_PARTIAL should be set in bond_setup once and left on.
NETIF_F_GSO_PARTIAL is used in __skb_gso_segment to invoke skb_gso_ok, which checks if skb->gso_type is in (features & gso_partial_features). If not, it locally disables NETIF_F_GSO_PARTIAL. Later, skb_segment does another check for skb_gso_ok and skips segmentation if NETIF_F_GSO_PARTIAL is locally disabled. So a packet with SKB_GSO_ESP sent on a device with only NETIF_F_GSO_PARTIAL but no NETIF_F_GSO_ESP with behave correctly: __skb_gso_segment will locally remove NETIF_F_GSO_PARTIAL and skb_segment will not do segmentation.
The issue is reported internally, so there is no Closes tag.
BTW, I saw some drivers set NETIF_F_GSO_PARTIAL on dev->features. Some other drivers set NETIF_F_GSO_PARTIAL on dev->hw_enc_features. I haven't see a doc about where we should set. So I just set it on dev-
features.
It seems NETIF_F_GSO_PARTIAL is needed on both features and hw_enc_features, otherwise traffic is not segmented and performance suffers. netif_skb_features returns the intersection of features & hw_enc_features, and that is used to drive skb_gso_segment. The same approach (features & hw_enc_features) is taken in a few .gso_segment callbacks.
drivers/net/bonding/bond_main.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 7b78c2bada81..09d5a8433d86 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -1598,10 +1598,13 @@ 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)
- if (gso_partial_features & NETIF_F_GSO_ESP) {
bond_dev->gso_partial_features |= NETIF_F_GSO_ESP;
- else
bond_dev->features |= NETIF_F_GSO_PARTIAL;- } else {
bond_dev->gso_partial_features &= ~NETIF_F_GSO_ESP;
bond_dev->features &= ~NETIF_F_GSO_PARTIAL;- }
done: bond_dev->vlan_features = vlan_features;
I've sent another patch to suggest these changes. I've tested it (with iperf3 traffic) and by playing with ethtool -K on the bond device. With simple iperf3 TCP traffic and no other tweaks, I get 2x the performance over the bond device with my patch compared to without.
I hope I didn't miss anything...
https://lore.kernel.org/netdev/20250123150909.387415-1-cratiu@nvidia.com/T/#...
Cosmin.
On Thu, 23 Jan 2025 15:24:37 +0000 Cosmin Ratiu wrote:
I've sent another patch to suggest these changes.
FTR this is not the normal way to proceed in code review, please try to share your feedback rather than taking over the submission (unless the original author explicitly asks you to).
On Fri, 2025-01-24 at 07:38 -0800, Jakub Kicinski wrote:
On Thu, 23 Jan 2025 15:24:37 +0000 Cosmin Ratiu wrote:
I've sent another patch to suggest these changes.
FTR this is not the normal way to proceed in code review, please try to share your feedback rather than taking over the submission (unless the original author explicitly asks you to).
Apologies, I didn't want to take over the submission, both me and Hangbin were simultaneously looking to fix this issue. I was about to send my fix when I saw his proposed one (this thread) and in the interest of expediting the solution, I decided to send mine in addition to commenting here.
Given that we were both fixing the same thing, would adding Signed-off- by with both of us be ok?
Cosmin.
On Fri, 24 Jan 2025 16:03:32 +0000 Cosmin Ratiu wrote:
On Fri, 2025-01-24 at 07:38 -0800, Jakub Kicinski wrote:
On Thu, 23 Jan 2025 15:24:37 +0000 Cosmin Ratiu wrote:
I've sent another patch to suggest these changes.
FTR this is not the normal way to proceed in code review, please try to share your feedback rather than taking over the submission (unless the original author explicitly asks you to).
Apologies, I didn't want to take over the submission, both me and Hangbin were simultaneously looking to fix this issue. I was about to send my fix when I saw his proposed one (this thread) and in the interest of expediting the solution, I decided to send mine in addition to commenting here.
Given that we were both fixing the same thing, would adding Signed-off- by with both of us be ok?
That's up to you, but keep in mind for the future that the discussion on how to proceed has to happen before you post your own version.
linux-kselftest-mirror@lists.linaro.org
-
Cosmin Ratiu -
Hangbin Liu -
Jakub Kicinski