Re: [PATCH v2 08/10] ethdev: add getter for per-queue Tx rate limit

[Vincent Jardin <vjardin@free.fr>]

Hi,

Thanks for the review, see below.

Le 11/03/26 09:26, Stephen Hemminger a écrit :
> A couple of observations about this new API.
> 
> 1. The queue index validation is duplicated in both ethdev and mlx5
> driver. Since this is a new API, the ethdev layer should be the single place
> that validates queue_idx < nb_tx_queues and rejects NULL output
> pointers - the driver callbacks should be able to assume these
> preconditions are met. The same applies to the existing
> mlx5_set_queue_rate_limit() which re-checks bounds that
> rte_eth_set_queue_rate_limit() already validates. Remove the redundant
> checks from the driver.

You are right. In v3, I'll:

- rte_eth_get_queue_rate_limit() - validate queue_idx
  and NULL tx_rate pointer before calling the driver op. This is the
  single validation point.

- mlx5_get_queue_rate_limit() - no longer check queue_idx bounds or
  NULL -- it trusts the ethdev preconditions.

- mlx5_set_queue_rate_limit() - also set a redundant queue_idx bounds
  check that duplicated what rte_eth_set_queue_rate_limit() already
  does -- removed as well.

The only driver-level checks that remain are things the ethdev layer
cannot know about and whether the HW supports packet_pacing since
it seems a bit specific. Later on, it could be revisisted.

> 
> 2. The new rte_eth_get_queue_rate_limit() API uses uint32_t for the
> rate. Since this is a brand-new experimental API with no backward
> compatibility constraint, use uint64_t for the rate value to
> accommodate future devices with rates exceeding 4 Gbps. This applies to
> both the getter and the setter callback - adding a new getter that
> already needs a type change next year defeats the purpose of getting
> the API right now.

No, it would break API symmetry which I do not like.

The rate unit is Mbps, not bps. This is documented in both the setter and getter,
and matches how rte_eth_set_queue_rate_limit().

uint32_t in Mbps covers up to about 4.3 Tbps. Current top-end NICs are 800 Gbps
aggregate; per-queue rates are a fraction of that. We are more than
three orders of magnitude away from saturating uint32_t.

Changing the getter to uint64_t would break API symmetry -- the existing
setter is:
   int rte_eth_set_queue_rate_limit(uint16_t port_id, uint16_t queue_idx,
                                    uint32_t tx_rate);

The day per-queue rates approach the Tbps range, both setter and getter
would need to evolve together as part of a broader ethdev ABI revision.
Until then, let's keep it consistent.

> 
> Surprising to me, that AI review found nits, but totally missed
> several architectural issues.

+1, AI helps, but it is appreciated with the human reviews too ;) so we can have our flavours.

Best regards,
  Vincent