[PATCH v2] net/intel: fix idpf Tx payload corruption in split queue
Bruce Richardson
bruce.richardson at intel.com
Fri Oct 16 19:33:22 CEST 2026
On Wed, Oct 07, 2026 at 04:10:42PM +0530, Shaiq Wani wrote:
> Observed on ACC while running NVMe traffic tests: under sustained
> high-throughput split-queue Tx, payload bytes on the wire did not
> match what the application submitted, while headers and checksums
> looked valid.
>
> The RS-completion path was freeing mbufs by walking sw_ring[] slots
> between first_id and the EOP sw_id. sw_ring[] slots get reclaimed
> by RE and reused by subsequent submits well before RS lands, so the
> walker was freeing mbufs still in flight for later packets, whose
> buffers then got recycled and overwritten mid-DMA.
>
> Fix by tracking completion ownership in a shadow ring indexed by a
> software-defined compl_tag stamped on all data descriptors of the
> packet, as required by the IDPF spec, and echoed back by HW on RS.
> The shadow ring is the sole owner of in-flight mbufs on the scalar
> split-queue path, so sw_ring[].mbuf is no longer written and queue
> stop/release drain only the shadow ring.
>
> Since RE reclaims descriptors before RS completes, nb_tx_free does
> not bound the number of packets awaiting RS. Size the shadow ring to
> nb_tx_desc and stop transmitting when the next compl_tag slot is
> still occupied, so a pending entry is never overwritten and pending
> completions cannot overflow the completion queue.
>
> Allocate the shadow ring at queue setup and fail setup on allocation
> error, and free it on all setup error and release paths. Remove the
> now unused ci_tx_entry.first_id field.
>
> Fixes: 96cc9b6ea60c ("net/idpf: fix multi-segment mbuf leak in split Tx path")
> Cc: stable at dpdk.org
>
> Signed-off-by: Shaiq Wani <shaiq.wani at intel.com>
> ---
> v2:
> Addressed AI review comments, which include:
> * Do not store mbufs in sw_ring[] on the scalar split-queue Tx path;
> tx_pending_pkts[] is the sole owner, fixing double-free on queue
> stop/release.
> * Stop Tx when the next compl_tag slot is still pending, so RS
> entries cannot be overwritten when RE runs ahead of RS.
> * Allocate the shadow ring at queue setup and propagate -ENOMEM.
> * Free the shadow ring on idpf/cpfl setup error paths and in
> cpfl_tx_queue_release().
> Also aligned with the IDPF spec:
> * Write the same compl_tag to all data descriptors of a packet.
> * Size the shadow ring to nb_tx_desc, so pending completions cannot
> overflow the 2 * nb_tx_desc completion queue.
> Other:
> * Remove the now unused ci_tx_entry.first_id field.
> * Drop redundant per-descriptor sw_ring[].mbuf = NULL stores.
>
> drivers/net/intel/common/tx.h | 8 +-
> drivers/net/intel/cpfl/cpfl_rxtx.c | 6 ++
> drivers/net/intel/idpf/idpf_common_rxtx.c | 105 ++++++++++++++++------
> drivers/net/intel/idpf/idpf_common_rxtx.h | 4 +
> drivers/net/intel/idpf/idpf_rxtx.c | 5 ++
> 5 files changed, 100 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/net/intel/common/tx.h b/drivers/net/intel/common/tx.h
> index 2a63cd4330..7c9841a21a 100644
> --- a/drivers/net/intel/common/tx.h
> +++ b/drivers/net/intel/common/tx.h
> @@ -166,7 +166,6 @@ struct ci_tx_queue;
> struct ci_tx_entry {
> struct rte_mbuf *mbuf; /* mbuf associated with TX desc, if any. */
> uint16_t next_id; /* Index of next descriptor in ring. */
> - uint16_t first_id; /* Split-queue: first sw_id of packet at EOP entry. */
> };
>
> /**
> @@ -262,6 +261,13 @@ struct ci_tx_queue {
> uint16_t sw_nb_desc;
> uint16_t sw_tail;
> uint16_t rs_compl_count;
> + /* Split-Tx completion tracking: shadow ring indexed by a
> + * rolling compl_tag decouples RS-completion mbuf lifetime
> + * from sw_ring[] slot reuse. Sized nb_tx_desc.
> + */
> + struct rte_mbuf **tx_pending_pkts;
> + uint16_t tx_pending_size;
> + uint16_t tx_next_compl_tag;
> };
> };
> };
This addition increases the size of the tx_queue struct, affecting all
drivers. I asked AI for some suggestions about this:
---
While the patch is functionally correct, it introduces unnecessary memory overhead, redundant fields, and dead code in the fast path:
A. Avoid the ci_tx_queue Size Increase
1. tx_pending_size is completely redundant: It is always assigned txq->nb_tx_desc at allocation and checked against txq->nb_tx_desc. Since nb_tx_desc is already a member of ci_tx_queue, tx_pending_size can be removed entirely.
2. Reuse existing pointers/padding:
* In ci_tx_queue, sw_ring is a union:
union {
struct ci_tx_entry *sw_ring;
struct ci_tx_entry_vec *sw_ring_vec;
struct rte_mbuf **tx_pending_pkts; /* Can be placed here! */
};
* tx_next_compl_tag (2 bytes) can fit inside the existing 6-byte hole/padding in the idpf struct (or reuse sw_tail, which was previously the software tail index).
* This eliminates the 16-byte growth, keeping ci_tx_queue at 144 bytes.
B. Eliminate Redundant Memory Allocation (Saving ~32 KB per Queue)
Currently, in split-queue scalar mode:
* Both sw_ring (sized 2 * nb_desc * sizeof(ci_tx_entry) = 32 KB for 1024 descriptors) and tx_pending_pkts (sized nb_desc * sizeof(struct rte_mbuf *) = 8 KB) are allocated.
* In scalar split-queue Tx, sw_ring[].mbuf is now completely unused.
* Better options:
- Option 1 (Reuse sw_ring): Since sw_ring is already allocated and each struct ci_tx_entry has an mbuf pointer, store the packet directly in sw_ring[tag].mbuf. This avoids allocating tx_pending_pkts altogether, saves an extra rte_zmalloc_socket call, and avoids adding new fields to ci_tx_queue.
- Option 2 (Allocate only tx_pending_pkts): In split-queue scalar mode, do not allocate sw_ring at all (or alias it), saving 32 KB per queue.
C. Clean Up Dead Code in the Fast-Path Loop
In idpf_dp_splitq_xmit_pkts, the loop still contains:
txd = &txr[tx_id];
txn = &sw_ring[txe->next_id];
...
sw_id = txe->next_id;
txe = txn;
...
txq->sw_tail = sw_id;
Because nothing is stored in or read from sw_ring in this path, these operations generate unnecessary cache-line accesses on every descriptor in the burst. Cleaning them up will improve Tx packet burst efficiency.
> diff --git a/drivers/net/intel/cpfl/cpfl_rxtx.c b/drivers/net/intel/cpfl/cpfl_rxtx.c
> index aba3a5e916..0dbe47d336 100644
> --- a/drivers/net/intel/cpfl/cpfl_rxtx.c
> +++ b/drivers/net/intel/cpfl/cpfl_rxtx.c
> @@ -329,6 +329,7 @@ cpfl_tx_queue_release(void *txq)
> rte_free(q->complq);
> }
>
> + idpf_qc_split_tx_pending_free(q);
> ci_txq_release_all_mbufs(q, q->use_vec_entry);
> rte_free(q->sw_ring);
> rte_free(q->rs_last_id);
> @@ -624,6 +625,9 @@ cpfl_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
> idpf_qc_single_tx_queue_reset(txq);
> } else {
> txq->desc_ring = mz->addr;
> + ret = idpf_qc_split_tx_pending_alloc(txq, socket_id);
> + if (ret != 0)
> + goto err_pending_alloc;
> idpf_qc_split_tx_descq_reset(txq);
>
> /* Setup tx completion queue if split model */
> @@ -642,6 +646,8 @@ cpfl_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
> return 0;
>
> err_complq_setup:
> + idpf_qc_split_tx_pending_free(txq);
> +err_pending_alloc:
> rte_free(txq->rs_last_id);
> err_rs_last_id_alloc:
> rte_free(txq->sw_ring);
> diff --git a/drivers/net/intel/idpf/idpf_common_rxtx.c b/drivers/net/intel/idpf/idpf_common_rxtx.c
> index 649b5d1f99..e54056b5c0 100644
> --- a/drivers/net/intel/idpf/idpf_common_rxtx.c
> +++ b/drivers/net/intel/idpf/idpf_common_rxtx.c
> @@ -233,6 +233,51 @@ idpf_qc_split_tx_descq_reset(struct ci_tx_queue *txq)
> txq->rs_compl_count = 0;
> txq->tx_next_dd = txq->tx_rs_thresh - 1;
> txq->tx_next_rs = txq->tx_rs_thresh - 1;
> +
> + if (txq->tx_pending_pkts != NULL) {
> + for (i = 0; i < txq->tx_pending_size; i++) {
> + if (txq->tx_pending_pkts[i] != NULL) {
> + rte_pktmbuf_free(txq->tx_pending_pkts[i]);
> + txq->tx_pending_pkts[i] = NULL;
> + }
> + }
> + }
> + txq->tx_next_compl_tag = 0;
> +}
> +
> +RTE_EXPORT_INTERNAL_SYMBOL(idpf_qc_split_tx_pending_alloc)
> +int
> +idpf_qc_split_tx_pending_alloc(struct ci_tx_queue *txq, unsigned int socket_id)
> +{
> + /* Bounding pending RS to nb_tx_desc keeps the 2 * nb_tx_desc complq from overflowing. */
> + txq->tx_pending_pkts = rte_zmalloc_socket("idpf_tx_pending",
> + sizeof(struct rte_mbuf *) * txq->nb_tx_desc,
> + RTE_CACHE_LINE_SIZE, socket_id);
> + if (txq->tx_pending_pkts == NULL) {
> + DRV_LOG(ERR, "Failed to alloc idpf tx_pending shadow ring");
> + return -ENOMEM;
> + }
> + txq->tx_pending_size = txq->nb_tx_desc;
> + txq->tx_next_compl_tag = 0;
> +
> + return 0;
> +}
> +
> +RTE_EXPORT_INTERNAL_SYMBOL(idpf_qc_split_tx_pending_free)
> +void
> +idpf_qc_split_tx_pending_free(struct ci_tx_queue *txq)
> +{
> + uint32_t i;
> +
> + if (txq->tx_pending_pkts == NULL)
> + return;
> +
> + for (i = 0; i < txq->tx_pending_size; i++) {
> + if (txq->tx_pending_pkts[i] != NULL)
> + rte_pktmbuf_free(txq->tx_pending_pkts[i]);
> + }
> + rte_free(txq->tx_pending_pkts);
> + txq->tx_pending_pkts = NULL;
> }
>
> RTE_EXPORT_INTERNAL_SYMBOL(idpf_qc_split_tx_complq_reset)
> @@ -387,6 +432,7 @@ idpf_qc_tx_queue_release(void *txq)
> rte_free(q->complq);
> }
>
> + idpf_qc_split_tx_pending_free(q);
> ci_txq_release_all_mbufs(q, false);
> rte_free(q->rs_last_id);
> rte_free(q->sw_ring);
> @@ -787,7 +833,6 @@ idpf_split_tx_free(struct idpf_complq *cq)
> volatile struct idpf_splitq_tx_compl_desc *compl_ring = cq->compl_ring;
> volatile struct idpf_splitq_tx_compl_desc *txd;
> uint16_t next = cq->tx_tail;
> - struct ci_tx_entry *txe;
> struct ci_tx_queue *txq;
> uint16_t gen, qid, q_head;
> uint16_t nb_desc_clean;
> @@ -824,26 +869,21 @@ idpf_split_tx_free(struct idpf_complq *cq)
> txq->nb_tx_free += nb_desc_clean;
> txq->last_desc_cleaned = q_head;
> break;
> - case IDPF_TXD_COMPLT_RS:
> - /* Walk from first segment to EOP, freeing each segment. */
> - txe = &txq->sw_ring[q_head];
> - if (txe->mbuf != NULL) {
> - uint16_t first = txe->first_id;
> - uint16_t idx = first;
> - uint16_t end = (q_head + 1 == txq->sw_nb_desc) ?
> - 0 : q_head + 1;
> -
> - do {
> - txe = &txq->sw_ring[idx];
> - if (txe->mbuf != NULL) {
> - rte_pktmbuf_free_seg(txe->mbuf);
> - txe->mbuf = NULL;
> - }
> - idx = (idx + 1 == txq->sw_nb_desc) ?
> - 0 : idx + 1;
> - } while (idx != end);
> + case IDPF_TXD_COMPLT_RS: {
> + /* Shadow ring indexed by the software-defined compl_tag is
> + * the sole source of truth for RS-time mbuf ownership; free
> + * the whole multi-seg chain via mbuf->next in one call.
> + */
> + uint16_t tag = q_head;
> +
> + if (unlikely(tag >= txq->tx_pending_size)) {
> + TX_LOG(ERR, "invalid completion tag %u.", tag);
> + } else if (txq->tx_pending_pkts[tag] != NULL) {
> + rte_pktmbuf_free(txq->tx_pending_pkts[tag]);
> + txq->tx_pending_pkts[tag] = NULL;
> }
> break;
> + }
> default:
> TX_LOG(ERR, "unknown completion type.");
> return;
> @@ -976,6 +1016,19 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> if (txq->nb_tx_free < nb_used)
> break;
>
> + /* RE reclaims descriptors before RS, so tag occupancy is the
> + * only bound on packets awaiting RS completion.
> + */
> + uint16_t tag = txq->tx_next_compl_tag;
> +
> + if (unlikely(txq->tx_pending_pkts[tag] != NULL)) {
> + nb_to_clean = 2 * txq->tx_rs_thresh;
> + while (nb_to_clean--)
> + idpf_split_tx_free(txq->complq);
> + if (txq->tx_pending_pkts[tag] != NULL)
> + break;
> + }
> +
> if (ol_flags & CI_TX_CKSUM_OFFLOAD_MASK)
> cmd_dtype = IDPF_TXD_FLEX_FLOW_CMD_CS_EN;
>
> @@ -991,8 +1044,6 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> tx_id = 0;
> }
>
> - uint16_t first_sw_id = sw_id;
> -
> do {
> uint16_t slen = tx_pkt->data_len;
> rte_iova_t buf_dma_addr = rte_mbuf_data_iova(tx_pkt);
> @@ -1004,13 +1055,12 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> unlikely(slen > CI_MAX_DATA_PER_TXD)) {
> txd = &txr[tx_id];
> txn = &sw_ring[txe->next_id];
> - txe->mbuf = NULL;
>
> txd->buf_addr = rte_cpu_to_le_64(buf_dma_addr);
> txd->qw1.cmd_dtype = cmd_dtype |
> IDPF_TX_DESC_DTYPE_FLEX_FLOW_SCHE;
> txd->qw1.rxr_bufsize = CI_MAX_DATA_PER_TXD;
> - txd->qw1.compl_tag = sw_id;
> + txd->qw1.compl_tag = tag;
>
> buf_dma_addr += CI_MAX_DATA_PER_TXD;
> slen -= CI_MAX_DATA_PER_TXD;
> @@ -1024,14 +1074,14 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
>
> txd = &txr[tx_id];
> txn = &sw_ring[txe->next_id];
> - txe->mbuf = tx_pkt;
>
> /* Setup TX descriptor */
> txd->buf_addr = rte_cpu_to_le_64(buf_dma_addr);
> txd->qw1.cmd_dtype = cmd_dtype |
> IDPF_TX_DESC_DTYPE_FLEX_FLOW_SCHE;
> txd->qw1.rxr_bufsize = slen;
> - txd->qw1.compl_tag = sw_id;
> + /* Spec: COMPLETION_TAG must be identical on all data descs of a packet. */
> + txd->qw1.compl_tag = tag;
> tx_id++;
> if (tx_id == txq->nb_tx_desc)
> tx_id = 0;
> @@ -1043,8 +1093,9 @@ idpf_dp_splitq_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts,
> /* fill the last descriptor with End of Packet (EOP) bit */
> txd->qw1.cmd_dtype |= IDPF_TXD_FLEX_FLOW_CMD_EOP;
>
> - /* Record first sw_id at EOP so completion can walk forward. */
> - sw_ring[txd->qw1.compl_tag].first_id = first_sw_id;
> + txq->tx_pending_pkts[tag] = tx_pkts[nb_tx];
> + if (++txq->tx_next_compl_tag == txq->tx_pending_size)
> + txq->tx_next_compl_tag = 0;
>
> txq->nb_tx_free = (uint16_t)(txq->nb_tx_free - nb_used);
> txq->rs_compl_count += nb_used;
> diff --git a/drivers/net/intel/idpf/idpf_common_rxtx.h b/drivers/net/intel/idpf/idpf_common_rxtx.h
> index b2d33287df..3365631d80 100644
> --- a/drivers/net/intel/idpf/idpf_common_rxtx.h
> +++ b/drivers/net/intel/idpf/idpf_common_rxtx.h
> @@ -216,6 +216,10 @@ void idpf_qc_single_rx_queue_reset(struct idpf_rx_queue *rxq);
> __rte_internal
> void idpf_qc_split_tx_descq_reset(struct ci_tx_queue *txq);
> __rte_internal
> +int idpf_qc_split_tx_pending_alloc(struct ci_tx_queue *txq, unsigned int socket_id);
> +__rte_internal
> +void idpf_qc_split_tx_pending_free(struct ci_tx_queue *txq);
> +__rte_internal
> void idpf_qc_split_tx_complq_reset(struct idpf_complq *cq);
> __rte_internal
> void idpf_splitq_rearm_common(struct idpf_rx_queue *rx_bufq);
> diff --git a/drivers/net/intel/idpf/idpf_rxtx.c b/drivers/net/intel/idpf/idpf_rxtx.c
> index 077a92a8a9..0d41cd87eb 100644
> --- a/drivers/net/intel/idpf/idpf_rxtx.c
> +++ b/drivers/net/intel/idpf/idpf_rxtx.c
> @@ -495,6 +495,9 @@ idpf_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
> idpf_qc_single_tx_queue_reset(txq);
> } else {
> txq->desc_ring = mz->addr;
> + ret = idpf_qc_split_tx_pending_alloc(txq, socket_id);
> + if (ret != 0)
> + goto err_pending_alloc;
> idpf_qc_split_tx_descq_reset(txq);
>
> /* Setup tx completion queue if split model */
> @@ -512,6 +515,8 @@ idpf_tx_queue_setup(struct rte_eth_dev *dev, uint16_t queue_idx,
> return 0;
>
> err_complq_setup:
> + idpf_qc_split_tx_pending_free(txq);
> +err_pending_alloc:
> rte_free(txq->rs_last_id);
> err_rs_last_id_alloc:
> rte_free(txq->sw_ring);
> --
> 2.34.1
>
More information about the dev
mailing list