Re: [PATCH v9 19/23] net/sxe2: add mbuf validation in Tx debug mode

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 26 Jun 2026 14:47:51 +0800
liujie5@linkdatatechnology.com wrote:

> From: Jie Liu <liujie5@linkdatatechnology.com>
> 
> Introduce the `sxe2_txrx_check_mbuf` helper function to validate outgoing
> mbufs when `RTE_ETHDEV_DEBUG_TX` is enabled. This helps developers catch
> malformed mbufs (e.g., invalid segment lengths, bad offload flags, or
> unaligned buffers) before passing them to the hardware rings, avoiding
> potential hardware hangs or silent packet drops.
> 
> The validation is fully wrapped inside `RTE_ETHDEV_DEBUG_TX` conditional
> compilation blocks to ensure zero performance overhead in standard
> production builds.
> 
> Signed-off-by: Jie Liu <liujie5@linkdatatechnology.com>
> ---

I don't think I was clear enough before, the verbose description is:


Current state (v9):

The new sxe2_txrx_check_mbuf() helper added in 19/23 is called only
from sxe2_tx_pkts_prepare(), which is the driver's tx_prepare
callback. It is not called from any tx_burst path (sxe2_tx_pkts, the
vec paths, the poll path).

The commit message claims the validation is "fully wrapped inside
RTE_ETHDEV_DEBUG_TX conditional compilation blocks to ensure zero
performance overhead in standard production builds." This is not what
the code does. There is no #ifdef RTE_ETHDEV_DEBUG_TX anywhere in the
new file or around the new call site. In fact the patch *removes* an
existing #ifdef RTE_ETHDEV_DEBUG_TX that previously gated
rte_validate_tx_offload() in the same loop, making that always-on too.

The new function is marked __rte_unused both in the header
declaration and in the .c definition, even though it has exactly one
caller. That attribute was likely a leftover from an earlier draft
where the function was only conditionally referenced.

The validation itself (595 lines) re-derives outer/inner L2/L3/L4
lengths from the packet bytes and compares them against what the mbuf
header declares - i.e., it's verifying that the application
correctly set mbuf->l2_len, l3_len, outer_l2_len, etc. before
submitting. Much of this overlaps what rte_validate_tx_offload() and
rte_net_intel_cksum_prepare() already check, both of which are called
in the same tx_prepare loop.

Desired state:

tx_burst should not contain validation checks for application errors.
The fast path runs once per packet at line rate, and any branch that
exists to catch a caller bug is paid on every well-behaved packet
forever. For driver assumptions that an application must satisfy,
the three sensible options are:

  1. RTE_ASSERT() - compiles out in release builds, surfaces the
     assumption to static analyzers, and fires for users running
     debug builds.

  2. unlikely() with drop-and-counter - if the check is cheap and
     the assumption can be violated in production. The bad packet
     is dropped, a queue stat is incremented, but the burst loop
     continues.

  3. Document the assumption in the API contract and don't check it
     at runtime.

tx_prepare is where validation belongs, and an unconditional check
there is acceptable - tx_prepare is opt-in, and an application that
calls it has already agreed to pay for inspection. The 595-line
function as written is mostly fine in that context. What's wrong is:

  (a) The commit message says one thing and the code does another.
      The author should either add the #ifdef RTE_ETHDEV_DEBUG_TX
      they claimed to add, or rewrite the commit message to describe
      what the patch actually does.

  (b) Driver-specific tx_prepare validation should cover sxe2-
      specific hardware constraints (descriptor count limits, buffer
      alignment, segment size limits) - not redo the generic offload-
      flag and length-consistency checks that rte_validate_tx_offload()
      and rte_net_intel_cksum_prepare() perform. As currently written,
      every packet going through tx_prepare gets its offload flags
      validated roughly twice.

  (c) __rte_unused on the function should be dropped.


Redo this with one of the above and resubmit