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
