Re: [PATCH v2 10/10] net/mlx5: add rate table capacity query API

[Vincent Jardin <vjardin@free.fr>]

Hi Stephen,

Thank you for the review, see below,

Le 11/03/26 09:35, Stephen Hemminger a écrit :
> On Wed, 11 Mar 2026 00:26:53 +0100
> Vincent Jardin <vjardin@free.fr> wrote:
> 
> > +RTE_EXPORT_EXPERIMENTAL_SYMBOL(rte_pmd_mlx5_pp_rate_table_query, 26.07)
> > +int rte_pmd_mlx5_pp_rate_table_query(uint16_t port_id,
> > +				     struct rte_pmd_mlx5_pp_rate_table_info *info)
> > +{
> > +	struct rte_eth_dev *dev;
> > +	struct mlx5_priv *priv;
> > +	uint16_t used = 0;
> > +	uint16_t *seen;
> > +	unsigned int i;
> > +
> > +	if (info == NULL)
> > +		return -EINVAL;
> 
> Prefer NULL checks in ethdev layer
> 
> > +	if (!rte_eth_dev_is_valid_port(port_id))
> > +		return -ENODEV;
> 
> Ditto checks for port_id should be at ethdev

This function is a PMD-specific API declared in rte_pmd_mlx5.h, not an ethdev op.
  application -> rte_pmd_mlx5_pp_rate_table_query() -> mlx5 internals

Therefore, the function must validate its own inputs:
 - port_id validity (via rte_eth_dev_get_by_name() / rte_eth_dev_is_valid_port())
 - info != NULL

Adding a generic ethdev op (ex eth_rate_table_query_t) for a concept
only one driver supports would be over-engineering.  If other drivers later
expose a similar rate table concept, that would be the time to factor out a
generic ethdev API.

> > +	dev = &rte_eth_devices[port_id];
> > +	priv = dev->data->dev_private;
> > +	if (!priv->sh->cdev->config.hca_attr.qos.packet_pacing) {
> > +		rte_errno = ENOTSUP;
> > +		return -ENOTSUP;
> > +	}
> > +	info->total = priv->sh->cdev->config.hca_attr.qos
> > +			.packet_pacing_rate_table_size;
> 
> Since DPDK allows 100 character lines now, don't need line break

ok

> > +	if (priv->txqs == NULL || priv->txqs_n == 0) {
> > +		info->used = 0;
> > +		return 0;
> > +	}
> > +	seen = mlx5_malloc(MLX5_MEM_ZERO, priv->txqs_n * sizeof(*seen),
> > +			   0, SOCKET_ID_ANY);
> 
> Since this only has lifetime of this function, use calloc() instead
> since that avoids using huge page memory, and compiler and other checkers
> "know about" malloc functions and engage more checks.

Nope, I'll use
  mlx5_malloc(MLX5_MEM_SYS | MLX5_MEM_ZERO, ...
in order to remain consistent with mlx5's cases I could grep despite there are
still 3 other calloc() occurences that I did not analyze.

In any cases, it'll end up with calloc() (mlx5_malloc()).

Best regards,
  Vincent