Re: [PATCH net-next 6/6] eth: mpnic: add GSO_PARTIAL for other tunnels

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message is incomplete about what the new GSO_PARTIAL
  tunnel offload covers by default.

--

eth: mpnic: add GSO_PARTIAL for other tunnels

This commit advertises GSO_PARTIAL for GRE, IPXIP4 and UDP tunnels. For
GSO_PARTIAL skbs the TX path uses L3 type OTHER, so the device does not
update the l3 headers. mpnic_features_check_encap_gso() now requires DF on
an outer IPv4 header and MANGLEID when the inner header is IPv4.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index c349e54a35470..8b17acffe7e5f 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -395,11 +398,27 @@ static bool mpnic_ipxip6_gso_ok(struct sk_buff *skb)
>  static netdev_features_t
>  mpnic_features_check_encap_gso(struct sk_buff *skb, netdev_features_t features)
>  {
> +	struct iphdr *iph;
> +
>  	if (skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP6 &&
>  	    mpnic_ipxip6_gso_ok(skb))
>  		return features;
>
> -	return features & ~NETIF_F_GSO_IPXIP6;
> +	features &= ~NETIF_F_GSO_IPXIP6;
> +
> +	iph = ip_hdr(skb);
> +	if (iph->version == 4 && !(iph->frag_off & htons(IP_DF)))
> +		return features & ~NETIF_F_GSO_MASK;
> +
> +	/* HW can segment inner udp4 with fixed IDs, but there is no UDP
> +	 * analog for MANGLEID, so defer to SW GSO.
> +	 */
> +	if (inner_ip_hdr(skb)->version == 4)
> +		features &= ~NETIF_F_GSO_UDP_L4;
> +	if (!(features & NETIF_F_TSO_MANGLEID))
> +		features &= ~NETIF_F_TSO;

[Severity: Low]

This isn't a bug, but could the commit message say more about what the new
GSO_PARTIAL offload covers by default?

The subject says "add GSO_PARTIAL for other tunnels", and the body says
these tunnels "can be supported with GSO_PARTIAL".

mpnic_netdev_alloc() doesn't add NETIF_F_TSO_MANGLEID to netdev->features.
The core only makes it available as a hw_feature:

net/core/dev.c:register_netdevice() {
    ...
	if (dev->hw_features & NETIF_F_TSO)
		dev->hw_features |= NETIF_F_TSO_MANGLEID;
    ...
}

With default settings, this check clears NETIF_F_TSO. Then the
skb_gso_ok(skb, features | partial_features) check in __skb_gso_segment()
fails for SKB_GSO_TCPV4. As a result, inner-IPv4 TCP over GRE, IPXIP4 and
UDP tunnels is fully segmented in software until the user turns on
tx-tcp-mangleid-segmentation with ethtool.

Inner-UDPv4 tunnel GSO is never offloaded, because NETIF_F_GSO_UDP_L4 is
always cleared when the inner header is IPv4. The code comment explains
this, but the commit message doesn't mention it.

The message does say "MANGLEID is required when the inner l3 is ipv4".
Could it also say that, by default, only inner-IPv6 traffic over these
tunnels gets the new offload, and that inner-UDPv4 always uses software
GSO? fbnic has the same MANGLEID gate, so the behavior itself looks
intentional.

> +
> +	return features;
>  }
>
>  netdev_features_t mpnic_features_check(struct sk_buff *skb,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-mpnic-lso-v1-0-e943ff1493e4%40gmail.com