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

[Cosmin Ratiu <cratiu@nvidia.com>]

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;