Hi Rayane, Thanks for working on this. Unfortunately the patch cannot be accepted in its current form. Please see the comments below.
> -----Original Message----- > From: Rayane Boussanni <[email protected]> > Sent: Friday, April 17, 2026 6:40 AM > To: [email protected] > Cc: Raslan Darawsheh <[email protected]>; Rayane Boussanni > <[email protected]> > Subject: [PATCH] net/mlx5: add hardware query for basic stats > > External email: Use caution opening links or attachments > > > When MLX5_PMD_SOFT_COUNTERS is not defined, the basic dev statistics > (rte_eth_stats) previously fell back to returning zeroed values for all > packet and byte metrics. > > This patch resolves the FIXMEs in mlx5_stats_get and mlx5_stats_reset by > falling back to the target's dev counters map. It utilizes > mlx5_os_read_dev_counters to retrieve the bulk arrays from the hardware > and strategically filters them against the xstats_ctrl index to summarize > the basic opackets, obytes, ipackets, ibytes, oerrors, and ierrors without > duplicating the context-switch overhead. > > Signed-off-by: Rayane Boussanni <[email protected]> > --- > drivers/net/mlx5/mlx5_stats.c | 57 +++++++++++++++++++++++++++++++++-- > 1 file changed, 55 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/mlx5/mlx5_stats.c b/drivers/net/mlx5/mlx5_stats.c > index 5cd3e303cc..198f231559 100644 > --- a/drivers/net/mlx5/mlx5_stats.c > +++ b/drivers/net/mlx5/mlx5_stats.c > @@ -280,7 +280,46 @@ mlx5_stats_get(struct rte_eth_dev *dev, struct > rte_eth_stats *stats, > tmp.imissed = stats_ctrl->imissed; > } > #ifndef MLX5_PMD_SOFT_COUNTERS > - /* FIXME: retrieve and add hardware counters. */ > + { > + uint64_t counters[MLX5_MAX_XSTATS]; 1. The new code is never compiled. drivers/net/mlx5/mlx5_defs.h defines MLX5_PMD_SOFT_COUNTERS to 1 whenever it is not already defined, so both new #ifndef blocks are always excluded. Neither -DMLX5_PMD_SOFT_COUNTERS=0 nor -UMLX5_PMD_SOFT_COUNTERS changes this, so the patch has no effect in any supported build. A supported way to select hardware counters is needed first. Note also that the non-soft-counter configuration already fails with -Werror, because of the unused "ntcp" variables in mlx5_tx_packet_multi_tso() and mlx5_tx_burst_tso() in mlx5_tx.h. That is not caused by your patch, but it shows this mode is not currently built or tested. > + struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl; > + bool bond_master = (priv->master && priv->pf_bond >= 0); > + > + ret = mlx5_os_read_dev_counters(dev, bond_master, > counters); > + if (ret) { > + DRV_LOG(WARNING, "port %u unable to read device > counters", > + dev->data->port_id); > + return ret; > + } > + for (i = 0; i != xstats_ctrl->mlx5_stats_n; ++i) { > + const char *name = xstats_ctrl->info[i].dpdk_name; > + uint64_t val = (counters[i] - > + xstats_ctrl->base[i]); 2. mlx5_stats_reset() overwrites the xstats baselines but does not reset xstats_ctrl->hw_stats[], which mlx5_xstats_get() uses for 32-bit wrap tracking of device counters. After a basic stats reset with unchanged hardware counters, rx_out_of_buffer is reported as 4294967296. Please either perform a complete, consistent reset (as in mlx5_xstats_reset()) or keep separate baselines for the basic statistics. > + > + if (!strcmp(name, "rx_unicast_packets") || > + !strcmp(name, "rx_multicast_packets") || > + !strcmp(name, "rx_broadcast_packets")) > + tmp.ipackets += val; > + else if (!strcmp(name, "rx_unicast_bytes") || > + !strcmp(name, "rx_multicast_bytes") || > + !strcmp(name, "rx_broadcast_bytes")) > + tmp.ibytes += val; > + else if (!strcmp(name, "tx_unicast_packets") || > + !strcmp(name, "tx_multicast_packets") || > + !strcmp(name, "tx_broadcast_packets")) > + tmp.opackets += val; > + else if (!strcmp(name, "tx_unicast_bytes") || > + !strcmp(name, "tx_multicast_bytes") || > + !strcmp(name, "tx_broadcast_bytes")) > + tmp.obytes += val; > + else if (!strcmp(name, "rx_wqe_errors") || > + !strcmp(name, "rx_phy_crc_errors") || > + !strcmp(name, > "rx_phy_in_range_len_errors") || > + !strcmp(name, "rx_phy_symbol_errors")) > + tmp.ierrors += val; > + else if (!strcmp(name, "tx_phy_errors")) > + tmp.oerrors += val; > + } > + } > #endif > *stats = tmp; > return 0; > @@ -319,7 +358,21 @@ mlx5_stats_reset(struct rte_eth_dev *dev) > mlx5_os_read_dev_stat(priv, "out_of_buffer", &stats_ctrl- > >imissed_base); > stats_ctrl->imissed = 0; > #ifndef MLX5_PMD_SOFT_COUNTERS > - /* FIXME: reset hardware counters. */ > + { > + uint64_t counters[MLX5_MAX_XSTATS]; > + struct mlx5_xstats_ctrl *xstats_ctrl = &priv->xstats_ctrl; > + bool bond_master = (priv->master && priv->pf_bond >= 0); > + int ret; > + > + ret = mlx5_os_read_dev_counters(dev, bond_master, > counters); > + if (ret) { > + DRV_LOG(WARNING, "port %u unable to read device > counters", > + dev->data->port_id); > + return ret; > + } 3. On Windows, mlx5_os_read_dev_counters() always returns -ENOTSUP. Once this path is enabled, rte_eth_stats_get() and rte_eth_stats_reset() would fail completely there, instead of still returning the statistics that are available today. An unsupported backend should be handled gracefully. > + for (i = 0; i != xstats_ctrl->mlx5_stats_n; ++i) > + xstats_ctrl->base[i] = counters[i]; > + } > #endif 4. This changes what the basic statistics mean. The vport counters also include traffic received or sent by the Linux kernel (bifurcated driver), while doc/guides/nics/mlx5.rst says the basic Rx/Tx statistics count only packets handled by the PMD. If this is intended, the documentation must be updated and the change in behaviour explained in the commit message. My AI tool checked this by compiling mlx5_stats.c in both the default configuration and with the hardware branch forced on, and by running the patched get/reset callbacks in a small harness with the device reads stubbed out. I did not run it on hardware. For a v2, I'd suggest agreeing first on how hardware basic statistics should be selected, then addressing the points above. Please include the documentation update and test results on Linux (including bonding) and Windows. > > return 0; > -- > 2.34.1
