Re: [PATCH v10 0/4] net/zxdh: optimize Rx/Tx path performance

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon,  3 Aug 2026 19:24:14 +0800
Junlong Wang <wang.junlong1@zte.com.cn> wrote:

> v10:
>   - Based on the issues raised in the AI review, Patch 2/4 Patch 3/4 Patch 4/4 have been modified.
> 
> v9:
>   - Remove add simple Tx xmit functions (zxdh_xmit_pkts_simple) in the last patch.
> 
> v8:
>   - Add checked the size of ZXDH_DL_NET_HDR_SIZE and RTE_PKTMBUF_HEADROOM in
>     zxdh_xmit_pkts_simple() before submitting. Add static_assert to reject builds with insufficient
>     default headroom at compile time.
> 
> v7:
>   - Add a new xmit prepare func for xmit_pkts_simple, which will checked the size of
>     ZXDH_DL_NET_HDR_SIZE and RTE_PKTMBUF_HEADROOM.
> 
> v6:
>   - Remove unnecessary error checking code in submit_to_backend_simple() and
>     pkt_padding(). Since as the max dl_net_hdr_len is always less than
>     RTE_PKTMBUF_HEADROOM, rte_pktmbuf_prepend() cannot fail in the
>     simple path (single-segment mbufs).
> v5:
>   - Reorganize patch series, placing interrupt fix as the first patch
>     and fix condition check to properly enable interrupts.
>   - Fix zxdh_recv_single_pkts() not compacting rcv_pkts[] on failure,
>     which could cause use-after-free and mbuf leak.
>   - Fix tx_bunch() and tx1() missing store barrier before setting AVAIL flag,
>     preventing data race on weakly-ordered architectures.
>   - Fix submit_to_backend_simple() writing descriptors for packets that
>     failed pkt_padding(), causing mbuf leak.
> v4:
>   - fix some AI review issues.
>   - fix queue enable intr bug.
> v3:
>   - remove unnecessary NULL check in zxdh_init_queue.
>   - Split Ring: Bit[31] is unused and reserved, zxdh_queue_notify(): removing the
>     zxdh_pci_with_feature(hw, ZXDH_F_RING_PACKED) check;
>   - remove unnecessary double-free in in zxdh_recv_single_pkts();
>   - used rte_pktmbuf_mtod();
>   - remove rxq_get_vq(q) macro, use q->vq and apply it consistently;
>   - Refactoring scatter and mtu check logic in zxdh_dev_mtu_set();
>   - set txdp->id = avail_idx + i in tx_bunch/tx1.
>   - add comment documenting zxdh_xmit_enqueue_append() now sets dxp->cookie = NULL for
>     the head slot and stores cookies per descriptor via dep[idx].cookie.
>   - add one-line comment noting tx_bunch() is the simple path handles single-segment.
>   - remove unnecessary Extra initialization and the uint32_t cast.
> v2:
>   - zxdh_rxtx.c, pkt_padding(): modifyed the return value of pkt_padding();
>   - zxdh_rxtx.c, zxdh_recv_single_pkts(): modifyed When zxdh_init_mbuf() fails
>     the loop does "continue" and free mbufs;
>   - zxdh_rxtx.c, refill_desc_unwrap(): Add rte_io_wmb() before writing flags
>     in the refill_que_descs();
>   - zxdh_queue.h, zxdh_queue_enable_intr(): Remove unnecessary function of zxdh_queue_enable_intr;
>   - zxdh_ethdev.c, zxdh_init_queue(): changed the hdr_mz NULL check logic;
>   - zxdh_rxtx.c, zxdh_xmit_pkts_simple()、zxdh_recv_single_pkts(): add stats.bytes count;
>   - zxdh_rxtx.c, zxdh_init_mbuf():remove  rte_pktmbuf_dump(stdout, rxm, 40);
>   - zxdh_ethdev.c, zxdh_dev_free_mbufs(): using rte_pktmbuf_free() to free mbufs;
>   - Splitting into separate patches, structure reorganization and sw_ring removal、
>     RX recv optimize、Tx xmit optimize、Tx;
> v1:
>   This patch optimizes the ZXDH PMD's receive and transmit path for better
>   performance through several improvements:
> - Add simple TX/RX burst functions (zxdh_xmit_pkts_simple and
>   zxdh_recv_single_pkts) for single-segment packet scenarios.
> - Remove RX software ring (sw_ring) to reduce memory allocation and
>   copy.
> - Optimize descriptor management with prefetching and simplified
>   cleanup.
> - Reorganize structure fields for better cache locality.
> 
>   These changes reduce CPU cycles and memory bandwidth consumption,
>   resulting in improved packet processing throughput.
> 
> Junlong Wang (4):
>   net/zxdh: fix queue enable intr issues
>   net/zxdh: optimize queue structure to improve performance
>   net/zxdh: optimize Rx recv pkts performance
>   net/zxdh: optimize Tx xmit pkts performance
> 
>  doc/guides/rel_notes/release_26_07.rst |  11 +
>  drivers/net/zxdh/zxdh_ethdev.c         |  77 +++---
>  drivers/net/zxdh/zxdh_ethdev_ops.c     |  23 +-
>  drivers/net/zxdh/zxdh_ethdev_ops.h     |   7 +
>  drivers/net/zxdh/zxdh_pci.c            |  21 --
>  drivers/net/zxdh/zxdh_pci.h            |   1 -
>  drivers/net/zxdh/zxdh_queue.c          |  11 +-
>  drivers/net/zxdh/zxdh_queue.h          | 132 ++++------
>  drivers/net/zxdh/zxdh_rxtx.c           | 317 +++++++++++++++----------
>  drivers/net/zxdh/zxdh_rxtx.h           |  14 +-
>  10 files changed, 316 insertions(+), 298 deletions(-)
> 

Detailed AI review still finds several issues.

Applied on top of c1a46b9 and built each commit separately with
-Dwerror=true. All four commits compile clean with no warnings, so
the series is bisect safe. The v9 issues with F_NOTIFICATION_DATA,
the missing uplink header in ZXDH_ETH_OVERHEAD, the data_off double
count, and the recv_single compaction are all fixed. A few things
still need work.

Patch 2/4: optimize queue structure

Warning: the inlined zxdh_queue_notify() drops the
ZXDH_F_RING_PACKED gate that zxdh_notify_queue() had on bit 31.
The old code only set it when packed was negotiated; the new one
sets it from cached_flags unconditionally. The commit log says the
F_NOTIFICATION_DATA gate was kept but says nothing about removing
the packed gate. Either restore it, or state in the log that packed
is now assumed.

Info: the new next_qidx member of struct zxdh_virtqueue has no
readers and no writers anywhere in the tree. Dead field in a patch
whose purpose is removing dead fields.

Info: rsv_8B is a uint32_t, so four bytes not eight.

Info: zxdh_vq_desc_extra::ndescs has no readers or writers left
after 4/4 removes the last two. It should be removed with the rest.

Patch 3/4: optimize Rx recv pkts

Error: zxdh_dev_mtu_set() and zxdh_scattered_rx() use different
predicates. mtu_set includes LRO:

	uint8_t need_scatter =
		(dev->data->dev_conf.rxmode.offloads &
		 (RTE_ETH_RX_OFFLOAD_TCP_LRO |
		  RTE_ETH_RX_OFFLOAD_SCATTER)) || ...

but zxdh_scattered_rx() has no LRO term, and set_rxtx_funcs() keeps
LRO in a separate dev->data->lro flag. So scattered_rx never
reflects LRO. With LRO on, scatter off, and an MTU that fits the
buffer, need_scatter is 1 while scattered_rx is 0 and every
rte_eth_dev_set_mtu() on a started port fails with "Stop port
first." Both sites need the same predicate.

Warning: the same check also rejects MTU decreases. If scattered_rx
is already 1 and the new MTU fits in one buffer, need_scatter is 0
and the call is refused, even though the scattered Rx function
handles single segment packets fine. Only the 0 to 1 transition
needs rejecting.

Warning: set_rxtx_funcs() silently drops the ZXDH_NET_F_MRG_RXBUF
check that used to fail the port. zxdh_recv_pkts_packed() still
walks header->type_hdr.num_buffers to build chains, which is
mergeable rxbuf semantics. If dropping the requirement is
deliberate, say so in the log.

Warning: zxdh_queue_kick_prepare_packed() is removed and both Rx
paths now call zxdh_queue_notify() unconditionally on every burst
that refills. That ignores the device's RING_EVENT_FLAGS_DISABLE
suppression and adds an MMIO write per burst. That is a
pessimization in a patch titled "optimize" and it is not mentioned
in the log.

Info: the two Rx paths address the header differently.
recv_pkts_packed uses buf_addr + RTE_PKTMBUF_HEADROOM,
zxdh_init_mbuf uses rte_pktmbuf_mtod(). Both work today because
refill always allocates fresh mbufs, but pick one; mtod is the
idiom.

Info: zxdh_scattered_rx() returns bool but its comment says it
returns 1 or 0. And zxdh_set_rxtx_funcs() now has no failure path
and its return value is ignored by the only caller, so make it
void.

Info: this patch adds ZXDH_MTU_TO_PKTLEN() but zxdh_scattered_rx()
open codes mtu + ZXDH_ETH_OVERHEAD + ZXDH_UL_NET_HDR_SIZE instead
of using it.

Patch 4/4: optimize Tx xmit pkts

Error: the walk in zxdh_xmit_fast_flush() is still unbounded and
driven by device written memory:

	id = desc[used_idx].id;
	do {
		...
		used_idx += 1;
		if (unlikely(used_idx == size)) {
			used_idx = 0;
			vq->used_wrap_counter ^= 1;
		}
	} while (curr_id != id);

id comes out of the descriptor ring. If the device writes
id >= vq_nentries, curr_id never equals id and the loop spins
forever, freeing cookies around the whole ring and flipping
used_wrap_counter each lap. An in range but wrong id inflates
free_cnt past vq_nentries and corrupts ring accounting. Validate
id < size before entering the loop, or cap the iteration count at
vq_nentries. This was raised on v9 and the rewrite did not address
it.

Error: the prefetch indexes past the end of the descriptor array:

	rte_prefetch0(&desc[used_idx + NEXT_CACHELINE_OFF_16B]);

used_idx reaches size - 1, so this reads up to desc[size + 7].
vq_nentries is validated power of two in zxdh_queue.c, so mask it:

	rte_prefetch0(&desc[(used_idx + NEXT_CACHELINE_OFF_16B) &
			(size - 1)]);

Warning: point 2 of the commit log says "no reset of the id field
is performed in the flush loop", but the loop body starts with

	desc[used_idx].id = used_idx;

The store is also redundant, since both enqueue paths write .id on
every enqueue. It is dirtying a device shared cache line in the hot
path for nothing. Drop it and fix the log, or explain why it is
needed.

Warning: the release notes go in the wrong file. The tree is at
26.11.0-rc0; the hunk adds to doc/guides/rel_notes/release_26_07.rst
which is already released. It belongs in release_26_11.rst.

Warning: the xstats counters are removed in 3/4 but documented in
4/4. Code and docs should land in the same commit.

Info: the NEXT_CACHELINE_OFF_16B conditional is dead.
RTE_CACHE_LINE_SIZE / 16 already gives 8 and 4 for the 128 and 64
cases, so only the #else branch is needed.

Info: leftover comment in zxdh_xmit_pkts_packed(), "Positive value
indicates it need free vring descriptors", now sits above the
free_cnt test with nothing to refer to since need was removed. Also
a double space after > on that line.