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

[Daniel Zahka]

On Sat Oct 10, 2026 at 7:52 AM EDT, netdev-bot+sashiko wrote:
> 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.

hmmm, I suppose that is technically correct. I wonder how many drivers
correctly handle skb allocation failures this way. I'll send a v2.

pw-bot: cr