Re: [PATCH net-next 1/6] eth: mpnic: Tx checksum offload

[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] mpnic_tx_offloads()
  (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c) increments…
- [Low] The new mpnic_features_check() calls skb_mac_header_len(skb) for
  every CHECKSUM_PARTIAL skb without checking that a MAC header has been…

--

eth: mpnic: Tx checksum offload

This commit adds generic Tx checksum offload (NETIF_F_HW_CSUM) to the
mpnic driver. It also adds an ndo_features_check callback that checks
the skb header lengths and checksum offset fit into the Tx descriptor
fields and the 256 byte header limit. CSO frames are counted and
reported through the netdev qstats API.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 93abdec44642f..edb087b12a1e2 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> +static void
> +mpnic_tx_offloads(struct mpnic_ring *ring, struct sk_buff *skb, __le64 *meta)
> +{
> +	struct mpnic_tx_hdrs hdrs = {};
> +
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		return;
> +
> +	mpnic_tx_fill_hdrs(skb, &hdrs);
> +
> +	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_CSUM_OFFSET,
> +					skb->csum_offset / 2) |
> +			     MPNIC_TWD_FLAG_REQ_CSO);
> +
> +	u64_stats_update_begin(&ring->stats.syncp);
> +	u64_stats_inc(&ring->stats.twq.csum_partial);
> +	u64_stats_update_end(&ring->stats.syncp);

[ ... ]

> @@ -230,6 +263,8 @@ static netdev_tx_t mpnic_xmit_frame_ring(struct sk_buff *skb,
>  	MPNIC_XMIT_CB(skb)->bytecount = skb->len;
>  	MPNIC_XMIT_CB(skb)->desc_count = 0;
>  
> +	mpnic_tx_offloads(ring, skb, meta);
> +
>  	if (mpnic_tx_map(ring, skb, meta))
>  		goto err_free;
>  

[Severity: Low]
By this point mpnic_tx_offloads() has already incremented
twq.csum_partial. mpnic_tx_map() can then fail if skb_headlen() or a
frag size is above FIELD_MAX(MPNIC_TWD_LEN), or on dma_mapping_error().
In that case the err_free path frees the skb and counts it as dropped:

err_free:
	dev_kfree_skb_any(skb);
	ring->tx_buf[tail] = NULL;
	ring->tail = tail;
err_drop:
	...
	u64_stats_inc(&ring->stats.twq.dropped);

Does the csum_partial increment stay in place for a frame that was
never sent?

mpnic_get_queue_stats_tx() reports this counter as needs_csum, and
mpnic_aggregate_ring_twq_counters() adds it into base->tx.csum_partial.
So dropped frames would also be counted as checksum offloaded frames.

Could the counter be bumped only after mpnic_tx_map() succeeds?

The same ordering is still there at the end of the series (in "eth:
mpnic: add GSO_PARTIAL for other tunnels"). There, mpnic_tx_lso() also
increments hw_gso and hw_gso_wire before mpnic_tx_map() runs.

> @@ -256,6 +291,30 @@ netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev)
>  	return mpnic_xmit_frame_ring(skb, mpn->tx[skb_get_queue_mapping(skb)]);
>  }
>  
> +netdev_features_t mpnic_features_check(struct sk_buff *skb,
> +				       struct net_device *dev,
> +				       netdev_features_t features)
> +{
> +	unsigned int l2len, i3len;
> +
> +	if (skb->ip_summed != CHECKSUM_PARTIAL)
> +		return features;
> +
> +	l2len = skb_mac_header_len(skb);

[Severity: Low]
Can this run before the MAC header has been set? skb_mac_header_len()
starts with:

	DEBUG_NET_WARN_ON_ONCE(!skb_mac_header_was_set(skb));

ndo_features_check is called via netif_skb_features(). The IP GSO slow
paths call that before the L2 header exists, and before
__dev_queue_xmit() calls skb_reset_mac_header():

ip_finish_output_gso()
  netif_skb_features()
    mpnic_features_check()
      skb_mac_header_len()

ip6_finish_output_gso_slowpath_drop() does the same on the IPv6 side.

At that point a locally generated TCP or UDP GSO skb is
CHECKSUM_PARTIAL, and its mac_header is still ~0 from allocation. TCP
builds software GSO skbs even without NETIF_F_TSO, because
sk_setup_caps() adds NETIF_F_GSO_SOFTWARE. So this path can be hit,
for example, when the PMTU drops after the MSS was chosen.

With CONFIG_DEBUG_NET the warning fires. Without it, l2len becomes
network_header - 0xffff as an unsigned int, which fails FIELD_FIT().
The CSUM and GSO features are then masked and segmentation falls back
to software checksums, so the traffic itself stays correct.

Would skb_network_offset() avoid depending on mac_header here? At
ndo_start_xmit time skb->data points at the L2 header, and the slow
path only needs a conservative answer.

fbnic_features_check() in drivers/net/ethernet/meta/fbnic has the same
pattern. This code is also unchanged at the end of the series.

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