On Sat, 30 Aug 2025 17:17:01 +0000 Vladimir Medvedkin <[email protected]> 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 <[email protected]> > --- 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 <[email protected]> **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: [email protected]** – 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
