Re: [PATCH v4 05/20] net/txgbe: fix inaccuracy in Tx rate limiting

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 11 May 2026 18:35:47 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> 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: stable@dpdk.org
> 
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---

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.