On Mon, 11 May 2026 18:35:47 +0800
Zaiyu Wang <[email protected]> wrote:
> Amber-lite NIC's TX rate limiting has large deviations for small
> packets. To address this, the following changes are made:
>
> 1. Set TDM_RL_ADJ (0x1820c) to 21B (includes 7B Ethernet preamble,
> 1B SFD, 1B EFD, and 12B IPG).
> 2) Remove the rate offset in the driver (e.g., 105 / 100, a rough
> compensation value from Linux kernel driver tests).
>
> After these changes, accuracy deviation for 64B packets is within
> ~5%, while large packets show lower deviation.
>
> Fixes: a309ab43acf3 ("net/txgbe: support Tx queue rate limiting for
> Amber-Lite")
> Cc: [email protected]
>
> Signed-off-by: Zaiyu Wang <[email protected]>
> ---
AI feedback on this one. Looks ok as is but worth looking at.
# DPDK Patch Review: net/txgbe: fix inaccuracy in Tx rate limiting
## Summary
This patch improves TX rate limiting accuracy for Amber-lite NIC by:
1. Setting TDM_RL_ADJ register to 21B (Ethernet framing overhead)
2. Removing the 105/100 compensation factor
## Review Findings
### **Errors**
None found.
### **Warnings**
**1. TDM_RL_ADJ write placement causes per-queue reconfiguration**
The `TXGBE_TDM_RL_ADJ` register write is inside the per-queue rate limit
configuration loop, causing the same global register to be written every time
any queue's rate limit is set. This register appears to be a global hardware
adjustment parameter (affects all queues), not a per-queue setting.
**Suggested fix:** Move the register write outside the queue configuration
context, ideally to device initialization or as a one-time setup when rate
limiting is first enabled:
```c
/* Set once during device init or first rate limit config */
static bool rl_adj_initialized = false;
if (!rl_adj_initialized) {
wr32(hw, TXGBE_TDM_RL_ADJ, 21);
rl_adj_initialized = true;
}
wr32(hw, TXGBE_TDM_RL_QUEUE_IDX, queue_idx);
```
Or better yet, move it to a device initialization function if this is a
hardware parameter that should be set once at device start.
**2. Magic number 21 lacks explanation**
The value 21 is documented in the commit message but not in the code. Future
maintainers will not see the breakdown (7B preamble + 1B SFD + 1B EFD + 12B
IPG).
**Suggested fix:** Add a comment or define:
```c
/* Ethernet framing overhead: 7B preamble + 1B SFD + 1B EFD + 12B IPG */
#define TXGBE_TDM_RL_ADJ_OVERHEAD 21
wr32(hw, TXGBE_TDM_RL_ADJ, TXGBE_TDM_RL_ADJ_OVERHEAD);
```
**3. Missing release notes update**
This is a fix that changes hardware behavior and improves accuracy. Users may
observe different TX rates after upgrading. The change should be documented in
release notes to inform users of the accuracy improvement.
**Suggested fix:** Add an entry to `doc/guides/rel_notes/release_26_11.rst` (or
current release):
```rst
* **net/txgbe: Improved TX rate limiting accuracy for Amber-lite NIC.**
Fixed TX rate limiting accuracy for 64-byte packets from ~20% deviation
to within ~5% by correcting hardware framing overhead configuration.
```
### **Info**
**1. Consider validating tx_rate is non-zero**
The code computes `factor_int = link_speed / tx_rate`. If `tx_rate` is somehow
zero (due to caller error or invalid input), this would cause division by zero.
The existing code may have validation elsewhere, but defensive programming
would check this.
```c
if (tx_rate == 0) {
PMD_DRV_LOG(ERR, "TX rate cannot be zero");
return -EINVAL;
}
```
**2. Variable scope: factor_fra could be declared at initialization**
The variable `factor_fra` is declared but not initialized, then assigned in
both branches of the conditional. C99 style would allow:
```c
u32 factor_fra = (frac * 16384) / 10000;
if (factor_fra >= 8192)
factor_fra = 0;
```
This is stylistic only; the existing code is correct.
