Re: [PATCH v2 00/36] combine multiple Intel scalar Tx paths

Stephen Hemminger
2026-01-13T17:17:31Z

On Tue, 13 Jan 2026 15:14:24 +0000
Bruce Richardson <bruce.richardson@intel.com> 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 <stephen@networkplumber.org>

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 <bruce.richardson@intel.com>
**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 <bruce.richardson@intel.com> |
| 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*