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

[netdev-bot+sashiko@kernel.org]

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] mpnic_clean_rcq()
  (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c) leaves good Rx frames…

--

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

This commit adds packet, byte and drop counters to the Tx work, Tx
completion and Rx completion rings, and reports them through
ndo_get_stats64. Rx frames that the device flagged with an uncorrectable
error count as errors, and any other Rx failure counts as a drop.

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

[ ... ]

> @@ -591,16 +609,25 @@ static int mpnic_clean_rcq(struct mpnic_napi_vector *nv,
>  			break;
>  		case MPNIC_RCD_TYPE_META: {
>  			struct sk_buff *skb = NULL;
> +			u32 pkt_bytes = 0;
>  
>  			if (likely(!(rcd &
>  				     MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK) &&
> -				   !state->pkt.add_frag_failed))
> +				   !state->pkt.add_frag_failed)) {
> +				pkt_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
> +				bytes += pkt_bytes;
> +			} 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++;

[ ... ]

> @@ -619,6 +646,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 - dropped - 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);
> +

[Severity: Medium]
Should rx_packets and rx_bytes still count frames that the device
reported as good but the driver then dropped?

In the MPNIC_RCD_TYPE_META branch, two cases reach dropped++ while
MPNIC_RCD_META_UNCORRECTABLE_ERR_MASK is clear:

  - mpnic_add_rx_frag() set state->pkt.add_frag_failed
  - xdp_build_skb_from_buff() returned NULL

Subtracting dropped here takes those frames out of rcq.packets. Their
bytes are lost as well, because bytes += pkt_bytes only runs when an skb
was built. When xdp_build_skb_from_buff() fails, pkt_bytes has already
been computed and is then thrown away.

mpnic_get_stats64() and mpnic_aggregate_ring_rcq_counters() pass these
values directly into rx_packets and rx_bytes. The rtnl_link_stats64
kernel-doc in include/uapi/linux/if_link.h says:

 * @rx_packets: Number of good packets received by the interface.
 *   For hardware interfaces counts all good packets received from the device
 *   by the host, including packets which host had to drop at various stages
 *   of processing (even in the driver).
 ...
 * @rx_bytes: Number of good received bytes, corresponding to @rx_packets.

Leaving out frames the hardware flagged as uncorrectable matches this,
since they go to rx_errors. Good frames dropped for lack of resources go
to rx_dropped, but by this text they should also be counted in
rx_packets and rx_bytes.

Could this subtract only errors from packets, and add pkt_bytes in the
dropped case too?

The later commits in the series ("eth: mpnic: report per-queue stats",
"eth: mpnic: count Tx queue stops and wakes" and "eth: mpnic: count Rx
allocation failures") keep both the packets - dropped - errors
subtraction and the byte counting that only happens on success. So the
problem is still there at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-mpnic-counters3-v1-0-7dc7644cc500%40gmail.com