Re: [PATCH net] net/mlx5e: Keep HW timestamp stats monotonic across reconfiguration

[netdev-bot+sashiko]

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

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

- [Medium] mlx5e_stats_ts_get()
  (drivers/net/ethernet/mellanox/mlx5/core/en_stats.c:1199-1236)…

Pre-existing issues:
- [Medium] The mlx5 PTP CQ -> struct ethtool_ts_stats mapping retained
  (and re-indented) by this patch in mlx5e_stats_ts_get()…

--

net/mlx5e: Keep HW timestamp stats monotonic across reconfiguration

mlx5e_stats_ts_get() no longer picks between DMA and port timestamp
counters based on tx_ptp_opened, and instead always sums the DMA
timestamp counters from the persistent per-channel statistics, adding the
port timestamp counters once the PTP TX queues have been opened. The
state_lock is dropped since only persistent stats are read now.

> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> index e7e6db7f6bf1..cd94bb44f6ab 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_stats.c
> @@ -1199,50 +1199,39 @@ void mlx5e_stats_rmon_get(struct mlx5e_priv *priv,
>  void mlx5e_stats_ts_get(struct mlx5e_priv *priv,
>  			struct ethtool_ts_stats *ts_stats)
>  {
> -	int i, j;
> +	u16 nch = mlx5e_stats_nch_read(priv);
> +	int i, tc;
>  
> -	mutex_lock(&priv->state_lock);
> +	ts_stats->pkts = 0;
>  
> -	if (priv->tx_ptp_opened) {
> -		struct mlx5e_ptp *ptp = priv->channels.ptp;
> +	for (i = 0; i < nch; i++) {
> +		struct mlx5e_channel_stats *channel_stats =
> +			priv->channel_stats[i];
>  
> -		ts_stats->pkts = 0;
> +		for (tc = 0; tc < priv->max_opened_tc; tc++)
> +			ts_stats->pkts += channel_stats->sq[tc].timestamps;
> +	}

[Severity: Medium]
Does this loop miss the HTB/QoS send queue timestamp counters?

The DMA accumulation here only walks priv->channel_stats[i]->sq[tc], but
HTB offload SQs keep their statistics elsewhere, in
mlx5e_open_qos_sq():

drivers/net/ethernet/mellanox/mlx5/core/en/qos.c:mlx5e_open_qos_sq() {
	...
	err = mlx5e_open_txqsq(c, tisn, txq_ix, params, &param_sq, sq, 0, hw_id,
			       priv->htb_qos_sq_stats[node_qid]);
	...
}

Those SQs are opened without a ptpsq (sq->ptpsq is only assigned in
mlx5e_ptp_open_txqsq()), so every hardware timestamped skb completing on
an HTB class queue takes the DMA branch:

drivers/net/ethernet/mellanox/mlx5/core/en_tx.c:mlx5e_consume_skb() {
	...
		} else {
			skb_tstamp_tx(skb, &hwts);
			sq->stats->timestamps++;
		}
	...
}

incrementing priv->htb_qos_sq_stats[qid]->timestamps, which the new code
never reads. With HTB TX offload configured, are those packets then
permanently absent from ETHTOOL_A_TS_STAT_PKTS, even though the driver
still reports the same events privately as qos_txN_timestamps via
MLX5E_DECLARE_QOS_TX_STAT(struct mlx5e_sq_stats, timestamps)?

If so, should the new comment claiming DMA timestamp counters are
accumulated also mention this queue class, or should the loop fold in
priv->htb_qos_sq_stats[] as well?

> +
> +	/* Accumulate DMA and port timestamp counters so values stay monotonic
> +	 * across channel teardown and mode switches.
> +	 */
> +	if (priv->tx_ptp_opened) {
> +		/* Err and Lost stats are only relevant for port timestamping,
> +		 * as the DMA layer will always successfully timestamp packets.
> +		 */
>  		ts_stats->err = 0;
>  		ts_stats->lost = 0;
>  
> -		if (!ptp)
> -			goto out;
> -
> -		/* Aggregate stats across all TCs */
> -		for (i = 0; i < ptp->num_tc; i++) {
> +		for (tc = 0; tc < priv->max_opened_tc; tc++) {
>  			struct mlx5e_ptp_cq_stats *stats =
> -				ptp->ptpsq[i].cq_stats;
> +				&priv->ptp_stats.cq[tc];
>  
>  			ts_stats->pkts += stats->cqe;
>  			ts_stats->err += stats->abort + stats->err_cqe +
> -				stats->late_cqe;
> +					stats->late_cqe;
>  			ts_stats->lost += stats->lost_cqe;
>  		}

[Severity: Medium]
This is a pre-existing mapping that this patch only re-indents, so it is
not introduced here, but can the same timestamp request end up counted in
pkts, lost and err at once?

The kernel-doc for struct ethtool_ts_stats in include/linux/ethtool.h
describes err as:

 * @err: Number of arbitrary timestamp generation error events that the
 *	hardware encountered, exclusive of @lost statistics.

In the driver, a request whose port CQE has not arrived within the
undelivered timeout bumps lost_cqe while the skb stays in the metadata
map:

drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c:mlx5e_ptpsq_mark_ts_cqes_undelivered() {
	...
		ptpsq->cq_stats->lost_cqe++;
	...
}

If that CQE arrives later, mlx5e_ptp_handle_ts_cqe() takes the
!nodes[metadata_id].inuse path and bumps late_cqe (mapped to err), and
for a non-error CQE also cqe (mapped to pkts).

Similarly, an aborted timestamp never reaches skb_tstamp_tx():

drivers/net/ethernet/mellanox/mlx5/core/en/ptp.c:mlx5e_skb_cb_hwtstamp_tx() {
	...
		ptpsq->cq_stats->abort++;
		ptpsq->cq_stats->abort_abs_diff_ns += diff;
	...
		return;
}

yet cq_stats->cqe was already incremented for that CQE, so pkts counts a
packet whose timestamp was never delivered to the socket while err counts
it too.

Since this patch turns pkts into a permanent DMA plus port sum, would it
be worth documenting or correcting the overlap so userspace summing
pkts/lost/err does not exceed the number of timestamp requests?

>  	}
> -
> -out:
> -	mutex_unlock(&priv->state_lock);
>  }

[ ... ]

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