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

Vincent Jardin
2026-03-12T15:05:51Z

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