On Tue, 13 Jan 2026 15:14:24 +0000 Bruce Richardson <[email protected]> wrote:
> The scalar Tx paths, with support for offloads and multiple mbufs > per packet, are almost identical across drivers ice, i40e, iavf and > the single-queue mode of idpf. Therefore, we can do some rework to > combine these code paths into a single function which is parameterized > by compile-time constants, allowing code saving to give us a single > path to optimize and maintain - apart from edge cases like IPSec > support in iavf. > > The ixgbe driver has a number of similarities too, which we take > advantage of where we can, but the overall descriptor format is > sufficiently different that its main scalar code path is kept > separate. > > Once merged, we can then optimize the drivers a bit to improve > performance, and also easily extend some drivers to use additional > paths for better performance, e.g. add the "simple scalar" path > to IDPF driver for better performance on platforms without AVX. > > V2: > - reworked the simple-scalar path as well as full scalar one > - added simple scalar path support to idpf driver > - small cleanups, e.g. issues flagged by checkpatch > > Bruce Richardson (36): > net/intel: create common Tx descriptor structure > net/intel: use common Tx ring structure > net/intel: create common post-Tx cleanup function > net/intel: consolidate definitions for Tx desc fields > net/intel: create separate header for Tx scalar fns > net/intel: add common fn to calculate needed descriptors > net/ice: refactor context descriptor handling > net/i40e: refactor context descriptor handling > net/idpf: refactor context descriptor handling > net/intel: consolidate checksum mask definition > net/intel: create common checksum Tx offload function > net/intel: create a common scalar Tx function > net/i40e: use common scalar Tx function > net/intel: add IPsec hooks to common Tx function > net/intel: support configurable VLAN tag insertion on Tx > net/iavf: use common scalar Tx function > net/i40e: document requirement for QinQ support > net/idpf: use common scalar Tx function > net/intel: avoid writing the final pkt descriptor twice > eal: add macro for marking assumed alignment > net/intel: write descriptors using non-volatile pointers > net/intel: remove unnecessary flag clearing > net/intel: mark mid-burst ring cleanup as unlikely > net/intel: add special handling for single desc packets > net/intel: use separate array for desc status tracking > net/ixgbe: use separate array for desc status tracking > net/intel: drop unused Tx queue used count > net/intel: remove index for tracking end of packet > net/intel: merge ring writes in simple Tx for ice and i40e > net/intel: consolidate ice and i40e buffer free function > net/intel: complete merging simple Tx paths > net/intel: use non-volatile stores in simple Tx function > net/intel: align scalar simple Tx path with vector logic > net/intel: use vector SW ring entry for simple path > net/intel: use vector mbuf cleanup from simple scalar path > net/idpf: enable simple Tx function > > doc/guides/nics/i40e.rst | 18 + > drivers/net/intel/common/tx.h | 116 ++- > drivers/net/intel/common/tx_scalar_fns.h | 595 ++++++++++++++ > drivers/net/intel/cpfl/cpfl_rxtx.c | 8 +- > drivers/net/intel/i40e/i40e_fdir.c | 34 +- > drivers/net/intel/i40e/i40e_rxtx.c | 670 +++------------- > drivers/net/intel/i40e/i40e_rxtx.h | 16 - > .../net/intel/i40e/i40e_rxtx_vec_altivec.c | 25 +- > drivers/net/intel/i40e/i40e_rxtx_vec_avx2.c | 36 +- > drivers/net/intel/i40e/i40e_rxtx_vec_avx512.c | 52 +- > drivers/net/intel/i40e/i40e_rxtx_vec_common.h | 6 +- > drivers/net/intel/i40e/i40e_rxtx_vec_neon.c | 25 +- > drivers/net/intel/iavf/iavf_rxtx.c | 642 ++++----------- > drivers/net/intel/iavf/iavf_rxtx.h | 30 +- > drivers/net/intel/iavf/iavf_rxtx_vec_avx2.c | 55 +- > drivers/net/intel/iavf/iavf_rxtx_vec_avx512.c | 104 +-- > drivers/net/intel/iavf/iavf_rxtx_vec_common.h | 36 +- > drivers/net/intel/ice/ice_dcf_ethdev.c | 10 +- > drivers/net/intel/ice/ice_rxtx.c | 737 ++++-------------- > drivers/net/intel/ice/ice_rxtx.h | 15 - > drivers/net/intel/ice/ice_rxtx_vec_avx2.c | 55 +- > drivers/net/intel/ice/ice_rxtx_vec_avx512.c | 53 +- > drivers/net/intel/ice/ice_rxtx_vec_common.h | 43 +- > drivers/net/intel/idpf/idpf_common_device.h | 2 + > drivers/net/intel/idpf/idpf_common_rxtx.c | 315 ++------ > drivers/net/intel/idpf/idpf_common_rxtx.h | 24 +- > .../net/intel/idpf/idpf_common_rxtx_avx2.c | 53 +- > .../net/intel/idpf/idpf_common_rxtx_avx512.c | 55 +- > drivers/net/intel/idpf/idpf_rxtx.c | 43 +- > drivers/net/intel/idpf/idpf_rxtx_vec_common.h | 6 +- > drivers/net/intel/ixgbe/ixgbe_rxtx.c | 103 ++- > .../net/intel/ixgbe/ixgbe_rxtx_vec_common.c | 3 +- > lib/eal/include/rte_common.h | 6 + > 33 files changed, 1565 insertions(+), 2426 deletions(-) > create mode 100644 drivers/net/intel/common/tx_scalar_fns.h > Series-Acked-by: Stephen Hemminger <[email protected]> Love to see common code and "fix it once" It was too large for the batch scripts, but here is AI review. It sees only minor stuff which you could fix (or skip). # DPDK Patch Review: Intel Tx Consolidation Series **Series:** [PATCH v2 01-36/36] Intel Tx driver consolidation **Author:** Bruce Richardson <[email protected]> **Review Date:** 2026-01-22 **Review Against:** AGENTS.md (DPDK Code Review Guidelines) --- ## Executive Summary This 36-patch series consolidates Tx descriptor handling across Intel network drivers (i40e, iavf, ice, idpf, ixgbe). Overall the patches are **well-structured** with proper code organization, but there are several **issues requiring attention** before merge. | Severity | Count | Summary | |----------|-------|---------| | **Error** | 1 | Source code line exceeds 100 characters | | **Warning** | 5 | Commit message style issues, double blank lines | | **Info** | 2 | Minor style observations | --- ## Detailed Findings ### ERRORS (Must Fix) #### 1. Source Code Line Length Violation **Patch 17/36:** `net/i40e: document requirement for QinQ support` **Location:** `drivers/net/intel/i40e/i40e_rxtx.c` **Issue:** Line exceeds 100-character limit (135 characters) ```c PMD_DRV_LOG(WARNING, "Double VLAN insertion may not work correctly without RTE_ETH_RX_OFFLOAD_VLAN_EXTEND set in Rx configuration."); ``` **Fix:** Split the log message across multiple lines or into multiple log calls: ```c PMD_DRV_LOG(WARNING, "Double VLAN insertion may not work correctly " "without RTE_ETH_RX_OFFLOAD_VLAN_EXTEND in Rx config."); ``` --- ### WARNINGS (Should Fix) #### 2. Commit Body Starts with Lowercase **Patches 08/36 and 09/36** Per DPDK convention, commit message body should start with a capital letter. | Patch | Current | Should Be | |-------|---------|-----------| | 08/36 `net/i40e: refactor context descriptor handling` | "move all context..." | "Move all context..." | | 09/36 `net/idpf: refactor context descriptor handling` | "move all context..." | "Move all context..." | --- #### 3. Double Blank Lines in Code The following patches add consecutive blank lines, which is a minor style violation: | Patch | Location | Line | |-------|----------|------| | 06/36 `net/intel: add common fn to calculate needed` | tx_scalar_fns.h | ~line 5093 | | 07/36 `net/ice: refactor context descriptor handling` | ice_rxtx.c | ~line 5379 | | 36/36 `net/idpf: enable simple Tx function` | idpf_common_rxtx.c | After `idpf_dp_singleq_xmit_pkts_simple()` | **Example from Patch 36:** ```c +uint16_t +idpf_dp_singleq_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts, + uint16_t nb_pkts) +{ + return ci_xmit_pkts_simple(tx_queue, tx_pkts, nb_pkts); +} + + +/* TX prep functions */ <-- Extra blank line above ``` --- #### 4. "TX" vs "Tx" in Comments Multiple patches use "TX" in comments rather than the DPDK-preferred "Tx": - Patch 04: `/* Common TX Descriptor QW1 Field Definitions */` - Patch 06: `/* Calculate the number of TX descriptors needed for each pkt */` - Patch 07: `/* TX context descriptor based double VLAN insert */` - Patch 12: `/* Setup TX Descriptor */` - And others... While not strictly an error (comments aren't checked as strictly as commit messages), consistency with "Tx" is preferred per `devtools/words-case.txt`. --- #### 5. EAL Macro Addition Without Release Notes **Patch 20/36:** `eal: add macro for marking assumed alignment` Adds `__rte_assume_aligned` macro to `lib/eal/include/rte_common.h`. Consider adding a release notes entry for this new public macro, even though it's primarily for internal optimization use. --- ### INFO (Consider) #### 6. Subject Line Truncation **Patch 06/36:** Subject line `net/intel: add common fn to calculate needed` appears truncated (missing "descriptors"). While technically within the 60-char limit, the full meaning is lost. Consider: - `net/intel: add fn to calc needed descriptors` (43 chars) - Or keep two-line format in cover letter --- #### 7. New Internal API Properly Tagged **Patch 36/36** correctly adds `__rte_internal` tag for `idpf_dp_singleq_xmit_pkts_simple()` in the header file (not .c file). ✓ --- ## Compliance Summary ### Commit Message Checks | Check | Status | |-------|--------| | Subject ≤60 chars | ✓ All pass (35-51 chars) | | Lowercase after prefix | ✓ All pass | | No trailing period | ✓ All pass | | Signed-off-by present | ✓ All 36 patches | | Body ≤75 chars | ✓ All pass | | Imperative mood | ✓ All pass | | Correct prefix (net/intel, net/ice, etc.) | ✓ All pass | | Body starts with capital | ✗ 2 failures (patches 8, 9) | ### Code Style Checks | Check | Status | |-------|--------| | Lines ≤100 chars | ✗ 1 failure (patch 17) | | No trailing whitespace | ✓ All pass | | SPDX headers on new files | ✓ Pass (patch 5 new file) | | `__rte_internal` in headers only | ✓ Pass | | No double blank lines | ✗ 3 failures | | Proper Tx/Rx capitalization | ⚠ Comments use "TX" | ### License Checks | Check | Status | |-------|--------| | New file has SPDX | ✓ `tx_scalar_fns.h` has BSD-3-Clause | | Copyright follows SPDX | ✓ Pass | | Blank line before code | ✓ Pass | --- ## Files Changed Summary - **22 files** modified in patch 1 alone - **New file created:** `drivers/net/intel/common/tx_scalar_fns.h` - **Drivers affected:** i40e, iavf, ice, idpf, cpfl, ixgbe - **Documentation updated:** `doc/guides/nics/i40e.rst` (patch 17) --- ## Recommendations 1. **Critical:** Fix the 135-char line in patch 17 before merge 2. **Important:** Capitalize "Move" in patches 8 and 9 commit messages 3. **Minor:** Remove extra blank lines in patches 6, 7, and 36 4. **Optional:** Consider release notes entry for new EAL macro 5. **Optional:** Standardize comment style to use "Tx" instead of "TX" --- ## Verdict **Conditional Accept** - Series is well-designed and the code consolidation is valuable. Fix the error (line length) and warnings (commit message capitalization, double blank lines) before merge.
