Re: [PATCH v7 00/47] net/sxe2: re-align with reference and fix review findings

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 31 Aug 2026 10:41:03 +0800
liujie5@linkdatatechnology.com wrote:

> From: Jie Liu <liujie5@linkdatatechnology.com>
> 
> This series updates the SXE2 poll mode driver (drivers/net/sxe2) and
> its common library (drivers/common/sxe2), fixing issues found in the
> previous version and re-aligning the driver with the reference
> implementation.
> 
> Changes since v6:
> - drop the now-trivial sxe2_vsi_node_free() helper and inline
>   rte_free() at its call sites
> - unify the duplicated FNAV/ACL COUNT action arms in the flow parser
> - use a file-scope static const ops table for the vectorized Tx path
>   and fail sxe2_tx_queues_vec_prepare() on a NULL txq
> - split "unify vectorized Tx buffer handling" into three commits:
>   the buffer_ring union fix, the NEON Tx 4-wide descriptor fill, and
>   the NEON Rx ptype/memory-ordering fixes
> - use the primary VSI id directly for representors (the kernel
>   guarantees it is always valid)
> - drop the packed/aligned attributes from the TM command message
>   structs to match the historical kernel layout
> - fix VEC mode selection in the Tx/Rx mode set functions
> - clean up commit messages (headline case/length, missing Fixes tags)

Getting much better. There are two items that AI flags that need
a second look; either say "yes thats OK AI is wrong" or put out
a new version.

The two that are worth examining are:

 Patch 26: fnav_enable change. Ends up enabling FDIR on all
   Rx paths per queue

 Patch 31: fnav lane ordering.

AI also gets worried that AVX512 fast-free change will change performance.
But in the overall performance of any real code, that change
will hardly be visible.

I admit AI has dug far more into detail here than I have.

Full report from AI (Claude Opus)


The v6 items are fixed: 14/47 and 15/47 commit messages match the
code now, 26/47 no longer duplicates the fnav command structs, the
command struct patch (now 43/47) states the sxe2_tm_res 4 to 2 byte
change, and the old 30/45 is split into 29, 30 and 31.

I applied the series and read the tree.  I did not build or bisect it.


Patch 08/47: use base device name for representor
---------------------------------------------------

Warning: the commit message says representor devices are allocated
on the same NUMA node as the parent.  The patch sets numa_node, but
the allocation two lines later still uses rte_socket_id():

	eth_dev->data->numa_node = adapter->dev_info.dev_data->numa_node;
	eth_dev->data->dev_private = rte_zmalloc_socket(name,
		sizeof(struct sxe2_adapter),
		RTE_CACHE_LINE_SIZE,
		rte_socket_id());

rte_socket_id() is the socket of the calling thread, not the socket
of the device.  Use the same numa_node for the allocation.

Info: the literal "vf" is passed as a "%s" argument.  Put it in the
format string.


Patch 26/47: add ACL engine event statistics support
------------------------------------------------------

Warning: this patch also changes Rx queue init in sxe2_queues_init():

	if (adapter->flow_ctxt.fnav_inited)
		rxq->fnav_enable = true;

sxe2_flow_init() sets fnav_inited to 1 always, so this sets
fnav_enable on every Rx queue.  Before this patch nothing assigned
that field, so it was always false.  The fnav blocks in all four
vector Rx paths (sse, avx2, avx512, neon) were dead code and are now
live on every queue.

That is a data path change.  It is not ACL statistics and it is not
in the commit message.  Please move it to its own patch and explain
why every queue needs it.


Patch 31/47: fix NEON Rx ptype mapping and memory ordering
------------------------------------------------------------

Warning: the fnav flags still use the old lane order.

The patch builds desc_lo and desc_hi in natural order:

	uint32x4_t q1_01 = vzip2q_u32(d0, d1);
	uint32x4_t q1_23 = vzip2q_u32(d2, d3);

so desc_lo is {d0[2], d1[2], d2[2], d3[2]}.  The old code built
{d1[2], d0[2], d3[2], d2[2]}.  flags lane i is written to rx_pkts[i]
through rearm0..rearm3, so this patch fixes the VLAN, checksum and
RSS flags, which were swapped inside each packet pair.

sxe2_rx_desc_fnav_flags_neon() is not touched and still builds

	descs_tmp1 = vzip1q_u32(d1, d0);
	descs_tmp2 = vzip1q_u32(d3, d2);

which gives {d1[0], d0[0], d3[0], d2[0]}.  That vector is OR'ed into
flags in sxe2_rx_desc_offloads_para_fill_neon(), so
RTE_MBUF_F_RX_FDIR and RTE_MBUF_F_RX_FDIR_ID are still set on the
wrong mbuf of each pair.  The bug is older than this patch, but this
is the patch that fixes lane order, and 26/47 makes the path live.

Warning: the patch is much wider than the commit message.  Besides
the ptype lanes and the fences it also changes:

  - staterr, from a 32 bit zip to a 16 bit zip.  Lanes 0..3 are now
    the low half of the staterr word for pkt0..pkt3 and lanes 4..7
    the high half.
  - the umbcast shuffle mask, the eop shuffle mask, rxe_mask and
    eop_mask.  These follow from the new staterr layout and I read
    them as correct.
  - the DD count.  The old code was rte_popcount64() of the DD bits,
    which counts DD bits anywhere in the group of four.  It could
    return 2 when packets 0 and 2 are done and packet 1 is not, and
    hand up a descriptor the hardware has not written.  The new code
    counts the leading run with rte_ctz64().  That is a real fix and
    it deserves its own patch, or at least a line in the message.

Also, the ptype lane change is not a fix.  With the old staterr
layout, lanes 5/1/7/3 selected pkt0/1/2/3 correctly.  With the new
layout lanes 1/3/5/7 are correct.  Both are right for their own
layout.  The message describes a bug that was not there.


Patch 32/47: refine vectorized Tx/Rx mode setup
-------------------------------------------------

Info: "Tx mode flags:0x%016x" and the Rx one print a uint32_t with a
width of 16, so there are always eight leading zeros.  %08x fits.


Patch 35/47: restore link update call in status query
-------------------------------------------------------

Warning: the commit message says the sxe2_link_update() call is moved
back into sxe2_drv_mac_link_status_get().  The diff does not touch
that function.  It adds two (void)sxe2_link_update() calls in
sxe2_event_irq_common_handler() and keeps the call in
sxe2_link_update_init().

The effect is the same for the three call sites that exist today, so
the code is fine.  Please make the message describe what the patch
does.


Patch 17/47: move PCI register read macro to common
-----------------------------------------------------

Info, pre-existing: the two macros are now next to each other and the
byte order handling does not match.

	#define SXE2_PCI_REG_WRITE(reg, value) rte_write32((rte_cpu_to_le_32(value)), (reg))
	#define SXE2_PCI_REG_READ(reg)         rte_read32(reg)

Other PMDs wrap the read in rte_le_to_cpu_32().  Not a change for
this patch, but worth a follow up.


Patch 37/47: use primary VSI ID for representor VSI
-----------------------------------------------------

Info: I accept the kernel guarantee stated in the commit message.
One loose end: sxe2_vsi_id_str() in sxe2_dump.c maps
SXE2_INVALID_VSI_ID to "NA" and is used for this exact field.  If the
primary VSI id is always valid, that branch is dead and can go.