Re: [PATCH net-next v2 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] The new per-queue Tx counter `twq.csum_partial`, which reaches
  userspace as the qstats `tx-needs-csum` value, goes up too early.

--

eth: mpnic: Tx checksum offload

This adds generic Tx checksum offload (NETIF_F_HW_CSUM) to mpnic, including
for encapsulated skbs via hw_enc_features. It adds mpnic_features_check() to
make sure the skb header layout fits the Tx descriptor fields. CSO frames are
counted and reported through the netdev qstats needs_csum counter.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 93abdec44642f..65de6d999d548 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);

[Severity: Low]
Should this counter be bumped only after the frame is actually queued to
the hardware?

mpnic_tx_offloads() increments twq.csum_partial for every CHECKSUM_PARTIAL
skb before mpnic_xmit_frame_ring() calls mpnic_tx_map():

> @@ -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;

mpnic_tx_map() can fail in two ways. One is dma_map_single() or
skb_frag_dma_map() returning a mapping error. The other is the head or a
fragment being larger than FIELD_MAX(MPNIC_TWD_LEN). In either case the
error path frees the skb and counts it as dropped:

mpnic_xmit_frame_ring() {
    ...
err_free:
	dev_kfree_skb_any(skb);
	ring->tx_buf[tail] = NULL;
	ring->tail = tail;
err_drop:
	mpnic_tx_flush_doorbell(ring);

	u64_stats_update_begin(&ring->stats.syncp);
	u64_stats_inc(&ring->stats.twq.dropped);
	u64_stats_update_end(&ring->stats.syncp);
    ...
}

The csum_partial increment is never undone. mpnic_get_queue_stats_tx()
then reports it as tx->needs_csum, so the same packet shows up as both
dropped and checksummed by the device, even though it never reached the
hardware.

The same ordering is still there later in the series. mpnic_tx_offloads()
also bumps hw_gso_wire through mpnic_tx_lso() before mpnic_tx_map() runs.

One option is to have mpnic_tx_offloads() report which offload it used,
and update the stats only after mpnic_tx_map() succeeds. Note that fbnic
uses the same count-before-map pattern in fbnic_tx_offloads().

> +
> +	*meta |= cpu_to_le64(FIELD_PREP(MPNIC_TWD_L2_HLEN,
> +					skb_mac_header_len(skb) / 2) |
> +			     FIELD_PREP(MPNIC_TWD_L3_IHLEN, hdrs.i3len / 2));
> +}

[ ... ]

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