[PATCHv2 net] Bonding: Fix support for gso_partial_features

[PATCHv2 net] Bonding: Fix support for gso_partial_features

[Hangbin Liu]

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;
-- 
2.39.5 (Apple Git-154)

Re: [PATCHv2 net] Bonding: Fix support for gso_partial_features

[Cosmin Ratiu]

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;

Re: [PATCHv2 net] Bonding: Fix support for gso_partial_features

[Cosmin Ratiu]

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.

Re: [PATCHv2 net] Bonding: Fix support for gso_partial_features

[Jakub Kicinski]

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).

Re: [PATCHv2 net] Bonding: Fix support for gso_partial_features

[Cosmin Ratiu]

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.

Re: [PATCHv2 net] Bonding: Fix support for gso_partial_features

[Jakub Kicinski]

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.