Re: [PATCH net-next V2 3/5] net/mlx5: nodnic: Add TX/RX statistics

[netdev-bot+sashiko@kernel.org]

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

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

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] mlx5_nodnic_handle_send_cqe() reads wi->num_bytes for the
  tx_bytes update (tx.c:182) after mlx5_nodnic_complete_send_wqe() has…
- [Medium] The new 64-bit counters in struct mlx5_nodnic_sq_stats and
  struct mlx5_nodnic_rq_stats (en.h:47-56) are plain u64 with no…
- [Low] The new RX packet and byte counters in
  mlx5_nodnic_handle_recv_cqe() (rx.c:155-156) are incremented only…

--

net/mlx5: nodnic: Add TX/RX statistics

This commit adds TX and RX packet and byte counters to the nodnic driver,
plus a TX drop counter. The data path updates the counters, and
ndo_get_stats64 exposes them.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/nodnic/en.h b/drivers/net/ethernet/mellanox/mlx5/nodnic/en.h
> index 19858e9da064a..d811bfe242ac6 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/nodnic/en.h
> +++ b/drivers/net/ethernet/mellanox/mlx5/nodnic/en.h
> @@ -44,6 +44,17 @@ enum mlx5_nodnic_cqe_opcode {
>  	OPCODE_CQ_INVALID,
>  };
>  
> +struct mlx5_nodnic_sq_stats {
> +	u64 packets;
> +	u64 bytes;
> +	u64 dropped;
> +};
> +
> +struct mlx5_nodnic_rq_stats {
> +	u64 packets;
> +	u64 bytes;
> +};

[Severity: Medium]
Can these counters tear when they are read on a 32-bit kernel?

They are plain u64 fields. There is no u64_stats_sync and no
READ_ONCE()/WRITE_ONCE(). Three places write them with plain ++ and +=:

  mlx5_nodnic_handle_recv_cqe()  - rq_stats packets/bytes, NAPI
  mlx5_nodnic_handle_send_cqe()  - sq_stats packets/bytes, NAPI
  mlx5_nodnic_xmit()             - sq_stats dropped, ndo_start_xmit

mlx5_nodnic_get_stats64() reads them with plain loads from process context
through dev_get_stats(). It shares no lock with the writers.

MLX5_NODNIC in Kconfig does not depend on 64BIT, and
include/linux/mlx5/doorbell.h has a BITS_PER_LONG != 64 fallback, so
32-bit builds look supported. On 32-bit, a u64 add is two 32-bit stores.
A reader such as ip -s link or /proc/net/dev could then see rx_bytes or
tx_bytes jump by about 2^32. On 64-bit these are still unannotated data
races that KCSAN would report.

Should these use u64_stats_t, with u64_stats_update_begin()/end() in the
writers and u64_stats_fetch_begin()/retry() in mlx5_nodnic_get_stats64()?

dropped is written from xmit context, while packets and bytes are written
from NAPI. So the TX side would need separate syncp instances, or separate
handling for dropped, so that each syncp has only one writer.

[ ... ]

> diff --git a/drivers/net/ethernet/mellanox/mlx5/nodnic/rx.c b/drivers/net/ethernet/mellanox/mlx5/nodnic/rx.c
> index 7736a1f24ea5b..808740ec39102 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/nodnic/rx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/nodnic/rx.c
> @@ -152,6 +152,9 @@ void mlx5_nodnic_handle_recv_cqe(struct mlx5_nodnic_cq *cq,
>  	skb->protocol = eth_type_trans(skb, priv->netdev);
>  	nodnic_handle_rx_csum(priv->netdev, cqe, skb);
>  
> +	priv->rq_stats.packets++;
> +	priv->rq_stats.bytes += byte_count;
> +

[Severity: Low]
Should good frames that are dropped because napi_build_skb() failed also be
counted? Earlier in mlx5_nodnic_handle_recv_cqe():

	skb = napi_build_skb(page_address(page), PAGE_SIZE);
	if (unlikely(!skb))
		goto err_recycle;

This jumps past both counter updates. The rtnl_link_stats64 kernel-doc
says @rx_packets includes "packets which host had to drop at various
stages of processing (even in the driver)", and that @rx_bytes
corresponds to @rx_packets.

Could the counters be incremented before the allocation? The failure
could also be counted in rx_dropped.

>  	napi_gro_receive(&cq->napi, skb);
>  	rq->cc++;
>  
> diff --git a/drivers/net/ethernet/mellanox/mlx5/nodnic/tx.c b/drivers/net/ethernet/mellanox/mlx5/nodnic/tx.c
> index f2efc8d3f5b85..6958c8a51231d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/nodnic/tx.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/nodnic/tx.c

[ ... ]

> @@ -176,6 +177,9 @@ void mlx5_nodnic_handle_send_cqe(struct mlx5_nodnic_cq *cq,
>  	wi = &sq->wqe_info[pi];
>  
>  	mlx5_nodnic_complete_send_wqe(sq, wi, napi_budget);
> +
> +	priv->sq_stats.packets++;
> +	priv->sq_stats.bytes += wi->num_bytes;

[Severity: Medium]
Could wi->num_bytes already belong to a different packet when it is read
here?

mlx5_nodnic_complete_send_wqe() gives the slot back to the producer before
it returns:

	sq->cc++;

	if (netif_tx_queue_stopped(txq))
		netif_tx_wake_queue(txq);

mlx5_nodnic_xmit() stops the queue when the ring is one entry short of
full, but it still posts the current packet:

	if (unlikely(mlx5_nodnic_tx_is_sq_full(sq)))
		netif_tx_stop_queue(sq->txq);

So the ring can become completely full (pc - cc == num_entries). Once this
completion increments cc and wakes the queue, mlx5_nodnic_xmit() on
another CPU computes pi = pc & mask, which is the slot that was just
completed. That CPU holds only the tx queue lock, which NAPI does not take.
mlx5_nodnic_tx_fill_wqe_info() then does:

	sq->wqe_info[pi].num_bytes = skb->len;

If that write lands before the read above, tx_bytes gets the new packet's
length. The new packet is then counted again when it completes.

Could wi->num_bytes be saved in a local variable before calling
mlx5_nodnic_complete_send_wqe()? Another option is to update the stats
before cc is advanced and the queue is woken.

>  }
>  
>  void mlx5_nodnic_complete_send_wqe(struct mlx5_nodnic_sq *sq,

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008171821.660739-1-tariqt%40nvidia.com