Re: [PATCH net-next v2 1/4] eth: mpnic: count packets, bytes, and drops per ring

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In mpnic_clean_rcq()
  (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c), an Rx frame the driver…

--

eth: mpnic: count packets, bytes, and drops per ring

This commit adds packet, byte, drop and error counters to each Tx work,
Tx completion and Rx completion ring in the mpnic driver. The counters are
reported through ndo_get_stats64. An RCU-protected base snapshot holds the
totals folded in from rings that have been torn down.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 5495e9a9aa651..d5185e64f804c 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -594,13 +612,20 @@ static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
>  
>  			if (likely(!(rcd &
>  				     MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK) &&
> -				   !state->pkt.add_frag_failed))
> +				   !state->pkt.add_frag_failed)) {
> +				bytes += xdp_get_buff_len(&state->pkt.buff);
>  				skb = xdp_build_skb_from_buff(&state->pkt.buff);
> +			}
>  
> -			if (likely(skb))
> +			if (likely(skb)) {
>  				napi_gro_receive(&nv->napi, skb);
> -			else
> +			} else {
>  				mpnic_put_pkt_buff(&state->pkt, true);
> +				if (rcd & MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK)
> +					errors++;
> +				else
> +					dropped++;
> +			}
>  
>  			state->pkt.buff.data_hard_start = NULL;
>  			packets++;

[Severity: Medium]
Is rx_bytes undercounted for frames that are dropped because
state->pkt.add_frag_failed is set?

mpnic_add_rx_frag() sets add_frag_failed when xdp_buff_add_frag() fails.
That happens once nr_frags reaches MAX_SKB_FRAGS and the fragment cannot
be coalesced:

mpnic_add_rx_frag() {
    ...
	if (!xdp_buff_add_frag(buff, page_to_netmem(page), pg_off, len,
			       truesz)) {
		state->payld.pagecnt_bias++;
		state->pkt.add_frag_failed = true;
	}
}

For such a frame the guarded branch above is skipped, so nothing is added
to bytes. skb stays NULL and the uncorrectable error bit is clear, so
dropped++ runs. packets++ runs unconditionally.

The frame is then counted in rcq.packets through packets - errors, but it
adds nothing to rcq.bytes:

> @@ -619,6 +644,13 @@ static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
>  
>  	rcq->head = head;
>  
> +	u64_stats_update_begin(&rcq->stats.syncp);
> +	u64_stats_add(&rcq->stats.rcq.packets, packets - errors);
> +	u64_stats_add(&rcq->stats.rcq.bytes, bytes);
> +	u64_stats_add(&rcq->stats.rcq.dropped, dropped);
> +	u64_stats_add(&rcq->stats.rcq.errors, errors);
> +	u64_stats_update_end(&rcq->stats.syncp);

mpnic_get_stats64() and mpnic_stats_fold_rings() report these values as
rx_packets and rx_bytes. The rtnl_link_stats64 kernel-doc says rx_packets
includes packets dropped by the driver, and that rx_bytes corresponds to
rx_packets.

The v2 changelog says:

  - count Rx frames dropped by the driver in rx_packets and rx_bytes

That seems true only for the xdp_build_skb_from_buff() failure path.

The same counters are used by mpnic_get_queue_stats_rx(), which is added
later in this series in "eth: mpnic: report per-queue stats". The code
looks the same at the end of the series, apart from the new alloc_failed
counter.

Moving the bytes += line out of the guarded branch might not be enough by
itself. __xdp_buff_add_frag() returns before it adds the rejected
fragment to xdp_frags_size:

	} else if (unlikely(nr_frags == MAX_SKB_FRAGS)) {
		return false;
	} else {
	...
	sinfo->xdp_frags_size += size;

Would it work to track the lengths reported by the descriptors separately
here? If not, should the changelog be reworded?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261012-mpnic-counters3-v2-0-8afba81c2d5a%40gmail.com