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 > > -- > 2.51.0 Series-Acked-by: Stephen Hemminger <[email protected]> Looks ok to me, asked Claude for second opinion. Its suggestion about long log message is overblown. Although, I would suggest being more succinct. # DPDK Patch Review: Intel Tx Consolidation Series (v2) **Series:** `[PATCH v2 01-36/36]` Intel Tx code consolidation **Author:** Bruce Richardson <[email protected]> **Patches Reviewed:** 36 **Review Date:** January 13, 2026 --- ## Executive Summary This is a substantial refactoring series that consolidates Tx (transmit) descriptor structures and functions across Intel network drivers (i40e, ice, iavf, idpf, ixgbe). The series is well-structured with clear commit messages and proper attribution. A few minor issues were identified. | Severity | Count | |----------|-------| | Error | 1 | | Warning | 4 | | Info | 2 | --- ## Errors (Must Fix) ### 1. Patch 17/36: Line exceeds 100 characters **File:** `drivers/net/intel/i40e/i40e_rxtx.c` **Subject:** `net/i40e: document requirement for QinQ support` ```c PMD_DRV_LOG(WARNING, "Double VLAN insertion may not work correctly without RTE_ETH_RX_OFFLOAD_VLAN_EXTEND set in Rx configuration."); ``` **Issue:** Line is 136 characters, exceeding the 100-character limit for source code. **Suggested fix:** Split the log message: ```c PMD_DRV_LOG(WARNING, "Double VLAN insertion may not work correctly " "without RTE_ETH_RX_OFFLOAD_VLAN_EXTEND set in Rx configuration."); ``` --- ## Warnings (Should Fix) ### 2. Patch 14/36: Implicit pointer comparison **File:** `drivers/net/intel/common/tx_scalar_fns.h` **Subject:** `net/intel: add IPsec hooks to common Tx function` ```c md = RTE_MBUF_DYNFIELD(mbuf, txq->ipsec_crypto_pkt_md_offset, struct iavf_ipsec_crypto_pkt_metadata *); if (!md) ``` **Issue:** Pointer comparison uses `!md` instead of explicit `md == NULL`. **Suggested fix:** ```c if (md == NULL) ``` ### 3. Patch 16/36: Implicit integer comparison **File:** `drivers/net/intel/iavf/iavf_rxtx.c` **Subject:** `net/iavf: use common scalar Tx function` ```c if (!iavf_calc_context_desc(mbuf, iavf_vlan_flag)) ``` **Issue:** `iavf_calc_context_desc()` returns `uint16_t`. Comparison should be explicit. **Suggested fix:** ```c if (iavf_calc_context_desc(mbuf, iavf_vlan_flag) == 0) ``` ### 4. Patches 19, 25, 26: Implicit integer comparison with rte_is_power_of_2() **Multiple files across patches** ```c if (!rte_is_power_of_2(tx_rs_thresh)) { ``` **Issue:** While `rte_is_power_of_2()` acts as a boolean predicate, it returns `int`. Strictly, the comparison should be explicit. **Suggested fix:** ```c if (rte_is_power_of_2(tx_rs_thresh) == 0) { ``` *Note: This is a borderline issue as the function is semantically boolean. May be acceptable.* ### 5. Patch 36/36: Double blank lines **File:** `drivers/net/intel/idpf/idpf_common_rxtx.c` **Subject:** `net/idpf: enable simple Tx function` ```c return ci_xmit_pkts_simple(tx_queue, tx_pkts, nb_pkts); } /* TX prep functions */ ``` **Issue:** Two consecutive blank lines after function definition. **Suggested fix:** Remove one blank line. --- ## Info (Consider) ### 6. Patch 20/36: New EAL macro without release notes **File:** `lib/eal/include/rte_common.h` **Subject:** `eal: add macro for marking assumed alignment` The patch adds `__rte_assume_aligned` macro to EAL common header. While this is an internal optimization helper, significant EAL additions typically warrant a release note entry. **Suggestion:** Consider adding a brief mention in release notes for the current release cycle. ### 7. Overall: No documentation for new internal APIs The series adds new internal functions (e.g., `idpf_dp_singleq_xmit_pkts_simple`) marked with `__rte_internal`. While internal APIs don't require Doxygen, brief inline comments explaining their purpose would aid maintainability. --- ## Compliance Summary ### Commit Message Checklist | Check | Status | |-------|--------| | Subject lines ≤60 characters | ✅ All pass (max: 51 chars) | | Lowercase after colon | ✅ All pass | | Correct component prefix | ✅ All pass (`net/intel:`, `net/i40e:`, `eal:`, etc.) | | Imperative mood | ✅ All pass | | No trailing period | ✅ All pass | | Signed-off-by present | ✅ All 36 patches | | Real name and valid email | ✅ Bruce Richardson <[email protected]> | | Body wrapped at 75 chars | ✅ All pass | ### Code Style Checklist | Check | Status | |-------|--------| | Lines ≤100 characters | ❌ 1 violation (Patch 17) | | No trailing whitespace | ✅ Pass | | `__rte_internal` alone on line | ✅ Correct usage | | Explicit pointer comparisons | ⚠️ 1 violation (Patch 14) | | Explicit integer comparisons | ⚠️ ~6 instances | | No double blank lines | ⚠️ 1 violation (Patch 36) | | No unnecessary void* casts | ✅ Pass | | No forbidden tokens | ✅ Pass | ### Structure Checklist | Check | Status | |-------|--------| | Each commit compiles independently | ✅ Appears correct | | Code and docs updated together | ✅ Patch 17 adds docs with code | | New internal APIs marked `__rte_internal` | ✅ Correct | | Release notes updated | ⚠️ Consider for EAL changes | --- ## Technical Assessment The series accomplishes significant code consolidation: 1. **Common Tx descriptor structure** (`struct ci_tx_desc`) unifies identical 16-byte descriptors across i40e, iavf, ice, and idpf drivers. 2. **Shared scalar Tx function** (`ci_xmit_pkts()`) reduces code duplication significantly. 3. **Simple Tx path** optimization enables scalar code to use the more efficient vector SW ring entry format. 4. **New EAL macro** (`__rte_assume_aligned`) provides portable way to mark pointer alignment assumptions for compiler optimization. The refactoring maintains backward compatibility and should not introduce functional regressions. --- ## Recommendation **Acceptable with minor revisions.** Address the Error and consider fixing the Warnings before merge. --- *Review generated according to DPDK AGENTS.md guidelines*
