| Message ID | 20250122135218.183578-1-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [PATCHv2,net] Bonding: Fix support for gso_partial_features | expand |
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/#u Cosmin.
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;
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(-)