Re: [RFC PATCH 1/6] ethdev: extend and refactor DCB configuration

[Stephen Hemminger <stephen@networkplumber.org>]

On Sat, 30 Aug 2025 17:17:01 +0000
Vladimir Medvedkin <vladimir.medvedkin@intel.com> wrote:

> Currently there are two structutes defined for DCB configuration, one for
> RX and one for TX. They do have slight semantic difference, but in terms
> of their structure they are identical. Refactor DCB configuration API to
> use common structute for both TX and RX.
> 
> Additionally, current structure do not reflect everything that is
> required by the DCB specification, such as per Traffic Class bandwidth
> allocation and Traffic Selection Algorithm (TSA). Extend rte_eth_dcb_conf
> with additional DCB settings
> 
> Signed-off-by: Vladimir Medvedkin <vladimir.medvedkin@intel.com>
> ---

This is a good idea, but requires a longer term workflow because of the
API and ABI changes.  Suggest as a first step making a deprecation notice.

The AI review feedback also spotted some things. I would not blindly follow
the recommendations on how to handle field changes.


## DPDK DCB Patch Review (RFC PATCH 1-6/6)

**Series:** DCB configuration refactoring and extension  
**Author:** Vladimir Medvedkin <vladimir.medvedkin@intel.com>  
**Status:** RFC

---

### Overall Assessment

This is a significant API/ABI-breaking RFC series that modernizes the DCB (Data Center Bridging) configuration API. The changes are architecturally sound but require attention to several issues before leaving RFC status.

---

### Patch 1/6: `ethdev: extend and refactor DCB configuration`

**Errors:**
- None

**Warnings:**
1. **Missing release notes** – This patch makes significant API changes (unifying `rte_eth_dcb_rx_conf` and `rte_eth_dcb_tx_conf` into `rte_eth_dcb_conf`, adding `dcb_tsa[]` and `dcb_tc_bw[]` fields). Per AGENTS.md, API changes require release notes in `doc/guides/rel_notes/`.

2. **ABI break without documentation** – Removing the separate Rx/Tx DCB structures is an ABI break. Needs to follow ABI versioning guidelines.

3. **New API fields not marked experimental** – The new `dcb_tsa` and `dcb_tc_bw` arrays should arguably be marked experimental, or the change documented in release notes.

**Info:**
- Line 71 commit body: "structutes" → "structures" (typo)
- Line 74: "structute" → "structure" (typo)
- Good that testpmd is updated in the same patch

---

### Patch 2/6: `ethdev: remove nb_tcs from rte_eth_dcb_conf structure`

**Errors:**
- None

**Warnings:**
1. **Missing release notes** – Removing `enum rte_eth_nb_tcs` and the `nb_tcs` field is an API/ABI break requiring documentation.

2. **Missing Cc: stable@dpdk.org** – If this is considered a cleanup/fix for hardware abstraction issues, consider tagging for stable.

**Info:**
- Good rationale explaining why `nb_tcs` was a hardware-specific artifact from ixgbe
- The driver correctly infers TC count from `dcb_tc[]` via popcount

---

### Patch 3/6: `ethdev: decouple VMDq and DCB cofiguration`

**Errors:**
1. **Subject line typo** – "cofiguration" should be "configuration"

**Warnings:**
1. **Missing release notes** – Removes `rte_eth_vmdq_dcb_conf` and `rte_eth_vmdq_dcb_tx_conf` structures, changes `tx_adv_conf` from union to struct.

2. **ABI break** – Changing `tx_adv_conf` from union to struct changes memory layout.

**Info:**
- Architecturally sound – VMDq and DCB should be independently configurable

---

### Patch 4/6: `ethdev: extend VMDq/DCB configuration with queue mapping`

**Errors:**
- None

**Warnings:**
1. **Missing release notes** – Moving `q_map` into `rte_eth_conf` and making it a configuration input (not just output) is a semantic API change.

2. **Duplicate macro definition** – Line 1095-1096 redefines `RTE_ETH_DCB_NUM_TCS` and `RTE_ETH_MAX_VMDQ_POOL` which were already defined earlier (lines 1144-1145 in original). The patch moves them but the mbox shows both locations – verify no duplication in final code.

**Info:**
- Line 1107-1108: Comment says "Rx queues" twice – second should be "Tx queues":
  ```c
  /** Rx queues assigned to tc per Pool */  // line 1103
  ...
  /** Rx queues assigned to tc per Pool */  // line 1108 - should be Tx
  ```

---

### Patch 5/6: `ethdev: remove unused dcb_capability_en field`

**Errors:**
- None

**Warnings:**
1. **Missing release notes** – Removes `RTE_ETH_DCB_PG_SUPPORT`, `RTE_ETH_DCB_PFC_SUPPORT` macros and `dcb_capability_en` field.

2. **Subject line case** – "dcb_capability_en" contains underscore after colon. Per AGENTS.md line 78: "Underscores after the colon (indicates code in subject)". Consider: `ethdev: remove unused DCB capability field`

**Info:**
- Good cleanup – PFC is now handled via the standard API

---

### Patch 6/6: `ethdev: move mq_mode to [r,t]x_adv_conf`

**Errors:**
- None

**Warnings:**
1. **Missing release notes** – This is a major ABI break affecting virtually all DPDK applications that configure Rx/Tx modes.

2. **Type change from enum to uint32_t** – Changes `enum rte_eth_rx_mq_mode` and `enum rte_eth_tx_mq_mode` to `uint32_t`. While the rationale is sound (flags vs enum), this affects type safety.

3. **Deprecation of old enums** – The enums are converted to `#define` macros. Should document that applications using the enum types need updating.

**Info:**
- Wide-reaching change touching 19 files
- `RTE_ETH_MQ_TX_VMDQ_ONLY` appears to be removed – verify this is intentional

---

### Global Issues (All Patches)

**Errors:**
- None

**Warnings:**
1. **No deprecation notices** – For this magnitude of ABI change, the DPDK process typically requires deprecation notices in a prior release. Since this is RFC, that's acceptable, but should be addressed before final submission.

2. **No documentation updates** – The `doc/guides/` programmers guide sections on DCB configuration will need updating.

3. **Driver coverage** – Only the ice driver is updated. Other DCB-capable drivers (ixgbe, i40e, etc.) will need updates or the series is incomplete.

4. **Missing tests** – No updates to functional test suite for the new TSA/bandwidth configuration fields.

**Info:**
- Good that all patches have proper `Signed-off-by` tags
- Subject lines are within 60 character limit (except for typo in 3/6)
- Commit bodies are well-written and explain the rationale

---

### Summary Table

| Patch | Subject OK | S-o-b | Release Notes | Tests | Verdict |
|-------|-----------|-------|---------------|-------|---------|
| 1/6 | ✓ | ✓ | Missing | Partial | Needs Work |
| 2/6 | ✓ | ✓ | Missing | ✓ | Needs Work |
| 3/6 | Typo | ✓ | Missing | ✓ | Needs Work |
| 4/6 | ✓ | ✓ | Missing | ✓ | Needs Work |
| 5/6 | Warning | ✓ | Missing | ✓ | Needs Work |
| 6/6 | ✓ | ✓ | Missing | N/A | Needs Work |

---

### Recommendations

1. Fix typo in patch 3/6 subject: "cofiguration" → "configuration"
2. Add release notes documenting all API/ABI changes
3. Add deprecation notice for the removed enums and structures
4. Update DCB documentation in programmer's guide
5. Consider adding functional tests for new TSA/bandwidth fields
6. Address comment typo in patch 4/6 (Rx vs Tx queues)
7. Verify all DCB-capable drivers are updated or document which drivers support the new API
8. Consider experimental marking for new fields if uncertain about final API shape