On Thu, 9 Jul 2026 18:46:32 +0800
Junlong Wang <[email protected]> wrote:
> 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
>
> drivers/net/zxdh/zxdh_ethdev.c | 76 ++++---
> drivers/net/zxdh/zxdh_ethdev_ops.c | 23 +-
> drivers/net/zxdh/zxdh_ethdev_ops.h | 4 +
> drivers/net/zxdh/zxdh_pci.c | 2 +-
> drivers/net/zxdh/zxdh_queue.c | 11 +-
> drivers/net/zxdh/zxdh_queue.h | 122 ++++++-----
> drivers/net/zxdh/zxdh_rxtx.c | 324 ++++++++++++++++++-----------
> drivers/net/zxdh/zxdh_rxtx.h | 27 +--
> 8 files changed, 329 insertions(+), 260 deletions(-)
>
There are some findings in AI review that need addressing.
Thee overly wordy AI review is :
Patch 2/4: net/zxdh: optimize queue structure to improve performance
Error: zxdh_queue_notify() drops the ZXDH_F_NOTIFICATION_DATA gate, not
just the packed check. The commit message says "remove unnecessary
feature check", but two checks were removed and they are not
equivalent. The old zxdh_notify_queue() had:
if (!zxdh_pci_with_feature(hw, ZXDH_F_NOTIFICATION_DATA)) {
rte_write16(vq->vq_queue_index, vq->notify_addr);
return;
}
That is a different doorbell format, not an optimization.
ZXDH_F_NOTIFICATION_DATA is negotiated, not mandatory:
zxdh_get_pci_dev_config() calls zxdh_pci_get_features(hw) when
hw->switchoffload is set, so host_features comes from the device.
zxdh_alloc_queues() explicitly handles the
"switchoffload && !(host_features & RING_PACKED)" case, so a
non-packed, non-notification-data device is a configuration this
driver still claims to support. On such a device the new inline
writes a 32-bit notify_data with bit 31 derived from cached_flags,
which is meaningless for a split ring.
Either keep the feature gate, or make both features mandatory and fail
probe when they are not negotiated -- and then remove the unreachable
split-ring queue init path.
Error: dead code left behind by the same change. After this patch
nothing calls ->notify_queue. zxdh_notify_queue() in zxdh_pci.c and
the .notify_queue member of struct zxdh_pci_ops are unreachable, yet
this patch edits zxdh_notify_queue() for the cached_flags rename.
Remove the function and the ops member instead of maintaining them.
Same pattern with zxdh_queue_kick_prepare_packed(): this patch changes
zxdh_mb(1) to rte_mb() inside it, and patch 3 then removes its only
caller. Drop the function in 3/4 or leave it alone in 2/4. Note that
zxdh_mb(1) was rte_atomic_thread_fence(rte_memory_order_seq_cst);
rte_mb() is a heavier full hardware fence on non-x86.
Error: Tx indirect descriptor init removed but the region is not.
Deleting the zxdh_vring_desc_init_indirect_packed() loop leaves that
function with zero callers, and tx_indir[]/tx_packed_indir[] in
struct zxdh_tx_region with zero users. ZXDH_MAX_TX_INDIRECT is 8, so
that is 128 bytes of hugepage memory per descriptor in the header
memzone, allocated and never used. In a performance series this
should be removed, not just left uninitialized.
Warning: dead struct members added. struct zxdh_vring and the
vq_split union arm have no user anywhere in drivers/net/zxdh. Same
for next_qidx and rsv_8B. If this is groundwork for split-ring
support, add it with the code that uses it.
Info: __rte_packed_begin on the vq_packed union arm. struct
zxdh_vring_packed is three pointers with no padding, so packing
changes nothing except telling the compiler the members may be
unaligned. It is a hot-path pointer load. Drop the attribute.
Info: rsv_8B is declared uint32_t. The name says 8 bytes.
Info: the patch removes the prefixed zxdh_desc_used() and keeps the
unprefixed desc_is_used(). Everything else in the file is zxdh_*.
Patch 3/4: net/zxdh: optimize Rx recv pkts performance
Error: ZXDH_ETH_OVERHEAD does not account for the uplink net header,
so scatter is not enabled when it is needed.
#define ZXDH_ETH_OVERHEAD \
(RTE_ETHER_HDR_LEN + RTE_ETHER_CRC_LEN + ZXDH_VLAN_TAG_LEN * 2)
The Rx descriptor is programmed as len = buf_len - RTE_PKTMBUF_HEADROOM
and the device writes its own header into that same buffer ahead of the
payload -- zxdh_init_mbuf() recovers the payload with
data_len = len - hdr_size. struct zxdh_net_hdr_ul is 4 + up to 56
bytes, so up to 60 bytes of the buffer are consumed before any packet
data. ZXDH_ETH_OVERHEAD accounts for none of it.
With the default 2176-byte data room (buf_size 2048) and MTU 2000,
2000 + 26 <= 2048, so zxdh_scattered_rx() returns 0 and
zxdh_recv_single_pkts is installed. The real requirement is
2000 + 14 + hdr_size, which exceeds 2048, so the device splits the
frame and zxdh_init_mbuf() drops it on num_buffers != 1. Silent packet
loss across a band of otherwise valid MTUs.
Error: zxdh_dev_mtu_set() rejects valid MTUs when scatter is already
enabled.
uint8_t need_scatter =
(uint32_t)ZXDH_MTU_TO_PKTLEN(new_mtu) > buf_size;
if (need_scatter != dev->data->scattered_rx)
return -EINVAL;
need_scatter is computed purely from size, but scattered_rx was set by
zxdh_scattered_rx(), which also returns 1 for RX_OFFLOAD_SCATTER and
RX_OFFLOAD_TCP_LRO regardless of size. A port configured with
RX_OFFLOAD_SCATTER and then set to MTU 1500 gets need_scatter == 0 and
scattered_rx == 1, and the call fails on a perfectly valid MTU. The
comparison has to mirror the offload checks, not just the size check.
Warning: three different Ethernet overhead definitions now coexist.
dev_info->max_mtu uses RTE_ETHER_HDR_LEN + RTE_VLAN_HLEN +
ZXDH_DL_NET_HDR_SIZE; ZXDH_ETH_OVERHEAD uses HDR_LEN + CRC_LEN +
2 * VLAN; and the actual Rx buffer requirement is
HDR_LEN + ZXDH_UL_NET_HDR_SIZE. Derive one per-device overhead helper
and use it in dev_infos_get, zxdh_scattered_rx() and
zxdh_dev_mtu_set().
Warning: zxdh_init_mbuf() drops the header-length validation the
mergeable path still has. zxdh_recv_pkts_packed() checks
"hdr_size > lens[i] || hdr_size < ZXDH_TYPE_HDR_SIZE" before using it.
zxdh_init_mbuf() checks only num_buffers != 1, then computes
data_len = len - hdr_size (uint16_t underflow on a bad pd_len) and sets
data_off = RTE_PKTMBUF_HEADROOM + hdr_size from an unvalidated
device-supplied value. The later data_len != pkt_len test usually
catches it, but only after zxdh_rx_update_mbuf() has run and data_off
is already set. Add the same bounds check.
Warning: no_free_tx_desc_err is added to zxdh_virtnet_stats and to the
txq xstats table but never incremented anywhere, so the xstat always
reads zero. The Tx path in 4/4 that would justify it just breaks
without counting.
Warning: the refill path notifies unconditionally.
refill:
if (vq->vq_free_cnt > 0) {
refill_que_descs(vq, dev);
zxdh_queue_notify(vq);
}
This replaces "if (unlikely(zxdh_queue_kick_prepare_packed(vq)))". The
doorbell write now happens on every burst call where the ring is not
full, including the num == 0 idle path and the case where
rte_pktmbuf_alloc_bulk() failed and nothing was refilled. That is an
MMIO write per empty poll, in a series about throughput.
Warning: new functions do not follow the DPDK function-definition
style. zxdh_scattered_rx, refill_desc_unwrap, refill_que_descs,
zxdh_init_mbuf and zxdh_recv_single_pkts all put the return type on the
same line as the name. Every pre-existing function in these files
splits them.
Info: refill_desc_unwrap and refill_que_descs are unprefixed.
Info: zxdh_scattered_rx() sets eth_dev->data->lro = 1 as a side effect
of a predicate and never clears it on the other paths. It also returns
int for a pure true/false result assigned to a 1-bit bitfield; bool
would be clearer.
Info: the trailing rte_io_wmb() in refill_que_descs() is redundant,
since zxdh_queue_store_flags_packed() already issues one before each
flag store.
Info: column alignment is broken for the "idle" entry in
zxdh_txq_stat_strings[].
Info: removing the full, norefill, multicast and broadcast xstats is a
user-visible change to per-queue xstats output with no release note.
Patch 4/4: net/zxdh: optimize Tx xmit pkts performance
Error: zxdh_recv_single_pkts() no longer compacts the output array;
use-after-free plus mbuf leak. This patch removes
"rcv_pkts[nb_rx] = rxm;" from the loop, leaving:
for (i = 0; i < num; i++) {
struct rte_mbuf *rxm = rcv_pkts[i];
...
if (unlikely(zxdh_init_mbuf(rxm, len, hw, &vq->rxq) < 0))
continue;
zxdh_update_packet_stats(&rxvq->stats, rxm);
nb_rx++;
}
rcv_pkts is the application's rx_pkts array. zxdh_init_mbuf() calls
rte_pktmbuf_free(rxm) on its failure paths, but the freed pointer is
left in rcv_pkts[i] and the good mbufs are never moved down. If packet
0 fails and 1..3 succeed, the function returns 3 and the application
reads rcv_pkts[0..2]: index 0 is a dangling pointer that will be freed
again, and the mbuf at index 3 is leaked -- neither returned to the
caller nor left in the ring. Patch 3/4 had this right; restore the
assignment.
Warning: zxdh_xmit_fast_flush() walks the completion chain unbounded.
id = desc[used_idx].id;
do {
...
used_idx += 1;
free_cnt += 1;
if (unlikely(used_idx == size)) {
used_idx = 0;
vq->used_wrap_counter ^= 1;
}
} while (curr_id != id);
id comes from device-written memory. The removed
zxdh_xmit_cleanup_inorder_packed() bounded the walk with num, and the
old zxdh_xmit_flush() advanced by the driver-owned dxp->ndescs. Now a
corrupt or unexpected id walks the whole ring, freeing cookies and
inflating vq_free_cnt past vq_nentries. Cap the inner loop at
vq_nentries.
Warning: the descriptor id semantics changed. The old path set
start_dp[idx].id = id (the head index) on every descriptor in a chain
and used dxp->ndescs to advance. zxdh_xmit_enqueue_append() now sets
start_dp[idx].id = idx per descriptor, and the flush loop terminates on
curr_id == id, which only works if the device reports the last
descriptor's buffer id. That is what the packed spec says, but the
flush loop also does "desc[used_idx].id = used_idx;" on every
descriptor -- a redundant write, since both enqueue paths already set
.id on everything they touch. Please state in the commit message which
behaviour the device implements, and drop the reset if it is not
load-bearing.
Warning: dead macros added. rxq_get_vq, txq_get_vq, N_PER_LOOP,
N_PER_LOOP_MASK and NEXT_CACHELINE_OFF_8B have zero users. Only
NEXT_CACHELINE_OFF_16B is referenced.
Warning: offloads fields added to struct zxdh_virtnet_rx and struct
zxdh_virtnet_tx (in 3/4 and 4/4) are never written or read. In
structures being reordered for cache locality, 8 unused bytes in each
is counterproductive.
Info: the "IMPORTANT:" comment in zxdh_xmit_enqueue_append() documents
a fragile invariant -- "any code path that attempts to read
vq_descx[head_id].cookie will see NULL and must handle this case
appropriately" -- without naming the paths. zxdh_queue_detach_unused()
and zxdh_queue_rxvq_flush() both skip NULL cookies, so it holds today,
but this reads like an invariant that wants an assertion.
Info: zxdh_queue_store_flags_packed() gains a volatile qualifier on its
parameter while the descriptor ring is non-volatile everywhere else it
is touched. Either qualify the ring consistently or leave it alone.
Info: "struct zxdh_vq_desc_extra *dep = &vq->vq_descx[0];" followed by
dep[idx] is just vq->vq_descx[idx] written indirectly.
