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

[Stephen Hemminger <stephen@networkplumber.org>]

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*