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

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 25 Aug 2026 20:00:02 +0800
Junlong Wang <wang.junlong1@zte.com.cn> wrote:

> v12:
>   - Patch 3/4 no longer introduces the unused `uint64_t offloads` field
>     in `struct zxdh_virtnet_rx`, and patch 4/4's commit log drops the
>     stale "drop offloads" claim so the message matches the actual diff.
> 
> v11:
>   - Restructure queue types for cache locality and dead-code removal (patch 2/4),
>     optimize the packed-ring Rx path with a single-segment fast path,
>     MTU/scatter refactor, and xstats cleanup (patch 3/4), and rework the
>     packed-ring Tx flush with per-descriptor mbuf free, walk budget, and
>     masked prefetch index (patch 4/4).
> 
> 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_11.rst |  10 +
>  drivers/net/zxdh/zxdh_ethdev.c         |  85 ++++---
>  drivers/net/zxdh/zxdh_ethdev_ops.c     |  22 +-
>  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          | 134 ++++------
>  drivers/net/zxdh/zxdh_rxtx.c           | 323 +++++++++++++++----------
>  drivers/net/zxdh/zxdh_rxtx.h           |  14 +-
>  10 files changed, 326 insertions(+), 302 deletions(-)
> 

Still get some detailed AI issues..
You don't need to fix every detail and AI does tend to "bikeshed"

Review of [PATCH v12 0/4] net/zxdh: optimize queue/Rx/Tx paths

Resolved since v11
------------------

- Patch 4: the unused "uint64_t offloads" field left behind in
  struct zxdh_virtnet_rx is gone; zxdh_rxtx.h is now updated in the
  same patch that stops using the removed members.
- Patch 4: the descriptor walk in the Tx flush is bounded (budget
  initialised to vq_nentries, break on id >= size) and the prefetch
  index is masked with (size - 1).
- Patch 3: zxdh_dev_mtu_set() and zxdh_scattered_rx() now use the
  same predicate (ZXDH_MTU_TO_PKTLEN vs min_rx_buf_size minus
  headroom), so the two no longer diverge.
- Patch 1: Fixes: 7677f3871ef3 resolves to a real commit whose
  subject matches, and the tag uses a 12-character hash.


Warnings
--------

Patch 3/4 (net/zxdh: optimize Rx recv pkts performance)

1. drivers/net/zxdh/zxdh_ethdev.c, zxdh_set_rxtx_funcs()

   The ZXDH_NET_F_MRG_RXBUF runtime check is removed and replaced
   with a comment stating the feature "is always negotiated (set in
   both ZXDH_PMD_DEFAULT_GUEST_FEATURES and
   ZXDH_PMD_DEFAULT_HOST_FEATURES)".  That premise does not hold on
   every path.  In zxdh_get_pci_dev_config():

       hw->host_features = ZXDH_PMD_DEFAULT_HOST_FEATURES;
       if (hw->switchoffload)
               hw->host_features = zxdh_pci_get_features(hw);
       nego_features = guest_features & hw->host_features;

   When switchoffload is set, host_features comes from the device,
   so ZXDH_NET_F_MRG_RXBUF can be absent from the negotiated set.
   Both zxdh_recv_pkts_packed() and zxdh_init_mbuf() then read
   header->type_hdr.num_buffers for a device that never agreed to
   populate it.

   Note the old check was already ineffective in a different way:
   the caller in dev_start ignores the int32_t return, so the -1
   only left rx_pkt_burst/tx_pkt_burst unassigned.  Either keep the
   check and make dev_start actually fail on it, or narrow the
   comment to the non-switchoffload case.

2. doc/guides/rel_notes/release_26_11.rst

   The release note enumerates the individual counters removed
   ("full", "norefill", "multicast_packets", "broadcast_packets").
   That level of detail does not belong in the release notes and
   dates badly as the counter set keeps changing.  A single bullet
   along the lines of

     * Changed the set of per-queue xstats counters.

   covers it.  The "New Features" bullets for the fast Rx path and
   the packed-ring optimisation are fine as they are.

Patch 4/4 (net/zxdh: optimize Tx xmit pkts performance)

3. drivers/net/zxdh/zxdh_rxtx.c, zxdh_xmit_fast_flush()

   The chain-walk scaffolding is now dead code.  Both enqueue paths
   write the descriptor's own index into the id field
   (zxdh_xmit_enqueue_push: dp->id = id where id == vq_avail_idx;
   zxdh_xmit_enqueue_append: start_dp[idx].id = idx), and the commit
   message states the flush relies on desc[k].id == k being
   preserved.  Given that invariant, "id" always equals "used_idx",
   so "curr_id != id" is false on the first pass and the do/while
   body executes exactly once per outer iteration.

   The result is a loop that simultaneously assumes id == index and
   retains the machinery for the case where it is not, which makes
   the bound reasoning harder to follow than it needs to be.
   Suggest dropping id, curr_id and the inner do/while and walking a
   single descriptor per outer iteration, keeping the budget counter
   and the id >= size guard as the corruption backstop:

       while (budget-- > 0 && desc_is_used(&desc[used_idx], vq)) {
               rte_prefetch0(&desc[(used_idx +
                       NEXT_CACHELINE_OFF_16B) & (size - 1)]);
               if (unlikely(desc[used_idx].id >= size))
                       break;
               dxp = &vq->vq_descx[used_idx];
               ...
       }


Info
----

Patch 3/4

4. drivers/net/zxdh/zxdh_rxtx.c, zxdh_update_packet_stats()

   The patch removes stats.multicast and stats.broadcast from
   struct zxdh_virtnet_stats, but the QUEUE_XSTAT block still
   references them (and an undeclared "ea").  The block was already
   uncompilable before this series, so nothing regresses, but since
   the series is cleaning out unused counters this is a good moment
   to delete the whole #ifdef QUEUE_XSTAT body.  The size_bins[]
   entries left in zxdh_rxq_stat_strings[]/zxdh_txq_stat_strings[]
   are always zero for the same reason and are candidates for the
   same cleanup.

5. drivers/net/zxdh/zxdh_rxtx.c, refill path

   zxdh_queue_notify() is called whenever vq_free_cnt > 0, including
   when rte_pktmbuf_alloc_bulk() inside zxdh_refill_que_descs()
   failed and no descriptor was made available.  Harmless, but it
   costs an MMIO write on the mempool-exhausted path.  Having
   zxdh_refill_que_descs() return the number refilled and notifying
   only on a non-zero result would avoid it.

Patch 4/4

6. drivers/net/zxdh/zxdh_rxtx.c

   NEXT_CACHELINE_OFF_16B is the only macro in the file without the
   ZXDH_ prefix used everywhere else in the driver.  There are also
   two consecutive blank lines before it.

7. drivers/net/zxdh/zxdh_rxtx.c, zxdh_xmit_enqueue_append()

   The comment naming the expected readers of head cookies lists
   zxdh_queue_rxvq_flush(), which only ever walks Rx virtqueues and
   never sees a Tx head descriptor.  zxdh_queue_detach_unused() is
   the relevant one.

8. drivers/net/zxdh/zxdh_queue.h

   struct zxdh_vq_desc_extra's "ndescs" is renamed to "rsv" rather
   than removed.  Nothing reads it after this patch, and "next" is
   only written by zxdh_vring_desc_init_packed() and never read, so
   the struct could be reduced to just the cookie pointer if you
   want the whole array to shrink.