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

[Daniel Zahka]

On Tue Oct 13, 2026 at 8:05 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] 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?

This is intentional. Summing the bytes of dropped parts of the payload
and using that to adjust the rx byte count is a large change that I
don't think is worth it, given that with our supported MTU size each skb
should have a max of 4 frags.