|SUCCESS| [v3] net/ice: add NEON-optimised Rx/Tx vector paths

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162909

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-07

This is my review of the NEON-optimized Rx/Tx vector paths patch for the ice driver.

commit [git sha needed]
Author: Jay Wang <jay.wang2 at arm.com>

[PATCH v3] net/ice: add NEON-optimised Rx/Tx vector paths

This patch adds the NEON-optimised Rx and Tx paths to the ice driver.

Tested on Ampere One platform with Intel E810-C NIC and 100G connection.
Tested with a single core and testpmd io forwarding mode. Observed
~30% performance boost in the above test compared to the default scalar
path.

Signed-off-by: Jay Wang <jay.wang2 at arm.com>

> diff --git a/drivers/net/intel/ice/ice_rxtx.c b/drivers/net/intel/ice/ice_rxtx.c
> index 31b74be9ba..b34231c212 100644
> --- a/drivers/net/intel/ice/ice_rxtx.c
> +++ b/drivers/net/intel/ice/ice_rxtx.c

[ ... ]

> +#elif defined(RTE_ARCH_ARM64)
> +	if (ad->ptp_ena || !ad->rx_bulk_alloc_allowed) {
> +		rx_simd_width = RTE_VECT_SIMD_DISABLED;
> +	} else {
> +		rx_simd_width = ice_get_max_simd_bitwidth();
> +		if (rx_simd_width >= RTE_VECT_SIMD_128)
> +			if (ice_rx_vec_dev_check(dev) == -1)
> +				rx_simd_width = RTE_VECT_SIMD_DISABLED;
> +	}

Does this code have double-if nesting? The coding style guide states
nested if statements should use braces. "Do not unnecessarily use braces
where a single statement will do."  However, when you have nested
conditions, braces improve readability and are required.

> diff --git a/drivers/net/intel/ice/ice_rxtx_vec_neon.c b/drivers/net/intel/ice/ice_rxtx_vec_neon.c
> new file mode 100644
> index 0000000000..8ec5942541
> --- /dev/null
> +++ b/drivers/net/intel/ice/ice_rxtx_vec_neon.c

> +static __rte_always_inline uint32x4_t
> +ice_flex_rxd_to_fdir_flags_vec(const uint32x4_t fdir_id0_3)
> +{
> +#define FDID_MIS_MAGIC 0xFFFFFFFFu

Does this code define a macro inside a function? The coding style guide
prefers defines at file scope. "Enumerations are preferred when defining
several related constants."  However if a macro must be used, it should
be at file scope, not within a function.

> +	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR != (1u << 2));
> +	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1u << 13));
> +
> +	const uint32x4_t pkt_fdir_bit = vdupq_n_u32((uint32_t)(RTE_MBUF_F_RX_FDIR |
> +		RTE_MBUF_F_RX_FDIR_ID));

Does this code have alignment issues? The continuation line should align
with the opening parenthesis on the previous line, or be indented by two
tabs if that would push it past 80 characters.

> +	const uint32x4_t fdir_mis_mask = vdupq_n_u32(FDID_MIS_MAGIC);
> +
> +	/* desc->flow_id field == 0xFFFFFFFF means fdir mismatch */
> +	uint32x4_t fdir_mask = vceqq_u32(fdir_id0_3, fdir_mis_mask);
> +
> +	/* XOR with 0xFFFFFFFF bit-reverses the mask */
> +	fdir_mask = veorq_u32(fdir_mask, fdir_mis_mask);
> +	const uint32x4_t fdir_flags = vandq_u32(fdir_mask, pkt_fdir_bit);
> +
> +	return fdir_flags;
> +}

[ ... ]

> +static __rte_always_inline void
> +ice_flex_rxd_to_olflags_v(struct ci_rx_queue *rxq, uint64x2_t descs[4],
> +	struct rte_mbuf **rx_pkts)

Does this code have incorrect indentation? Function parameters on a
continuation line should align with the opening parenthesis or be
indented by two tabs.

> +{
> +	const uint64x2_t mbuf_init = {rxq->mbuf_initializer, 0};
> +	uint64x2_t rearm0, rearm1, rearm2, rearm3;
> +
> +	uint32x4_t tmp_desc, flags, rss_vlan;

Does this blank line belong here? Variable declarations should be
grouped together without blank lines between different declaration
blocks within the same function.

> +	/**
> +	 * mask everything except checksum, RSS and VLAN flags
> +	 * bit fields defined in enum ice_rx_flex_desc_status_error_0_bits
> +	 * bit7:4 for checksum.
> +	 * bit12 for RSS indication.
> +	 * bit13 for VLAN indication.
> +	 */

Does this comment follow proper style? Multi-line comments should begin
with "/*" on its own line, and the DPDK style for network comments
typically uses this format. However, the current format appears to be
non-standard with "/**" which is typically reserved for Doxygen-style
documentation.

> +	const uint32x4_t desc_mask = {0x30f0, 0x30f0, 0x30f0, 0x30f0};
> +	const uint32x4_t cksum_mask = {
> +		RTE_MBUF_F_RX_IP_CKSUM_MASK | RTE_MBUF_F_RX_L4_CKSUM_MASK |
> +		RTE_MBUF_F_RX_OUTER_L4_CKSUM_MASK | RTE_MBUF_F_RX_OUTER_IP_CKSUM_BAD,
> +		RTE_MBUF_F_RX_IP_CKSUM_MASK | RTE_MBUF_F_RX_L4_CKSUM_MASK |
> +		RTE_MBUF_F_RX_OUTER_L4_CKSUM_MASK | RTE_MBUF_F_RX_OUTER_IP_CKSUM_BAD,
> +		RTE_MBUF_F_RX_IP_CKSUM_MASK | RTE_MBUF_F_RX_L4_CKSUM_MASK |
> +		RTE_MBUF_F_RX_OUTER_L4_CKSUM_MASK | RTE_MBUF_F_RX_OUTER_IP_CKSUM_BAD,
> +		RTE_MBUF_F_RX_IP_CKSUM_MASK | RTE_MBUF_F_RX_L4_CKSUM_MASK |
> +		RTE_MBUF_F_RX_OUTER_L4_CKSUM_MASK | RTE_MBUF_F_RX_OUTER_IP_CKSUM_BAD
> +	};

Does this code exceed 80 columns? These lines exceed the 80-character
limit specified in the coding style guide. Lines should be wrapped to
stay within this limit.

[ ... ]

> +	/**
> +	 * extract status_error0 field from 4 descriptors,
> +	 * and mask out everything else not in desc_mask
> +	 */
> +	flags = vzip2q_u32(vreinterpretq_u32_u64(descs[0]),
> +			vreinterpretq_u32_u64(descs[1]));
> +	tmp_desc = vzip2q_u32(vreinterpretq_u32_u64(descs[2]),
> +			vreinterpretq_u32_u64(descs[3]));
> +	tmp_desc = vreinterpretq_u32_u64(vzip1q_u64(vreinterpretq_u64_u32(flags),
> +			vreinterpretq_u64_u32(tmp_desc)));

[ ... ]

> +	/* check the additional fdir_flags if fdir is enabled */
> +	if (rxq->fdir_enabled) {

Does this code have an implicit boolean comparison? The coding style
guide requires explicit comparisons: "if (rxq->fdir_enabled != 0)" or
"if (rxq->fdir_enabled == true)" depending on the type.

[ ... ]

> +static __rte_always_inline void
> +ice_flex_rxd_to_ptype_v(uint64x2_t descs[4], struct rte_mbuf **rx_pkts,
> +	uint32_t *ptype_tbl)

Does this code have incorrect parameter indentation? The continuation
should align properly.

[ ... ]

> +static __rte_always_inline uint16_t
> +_ice_recv_raw_pkts_vec(struct ci_rx_queue *rxq, struct rte_mbuf **rx_pkts,
> +	uint16_t nb_pkts, uint8_t *split_packet)
> +{

[ ... ]

> +	uint16x8_t crc_adjust = {
> +		0, 0,			/* ignore pkt_type field */
> +		rxq->crc_len,	/* sub crc on pkt_len */
> +		0,				/* ignore high-16bits of pkt_len */
> +		rxq->crc_len,	/* sub crc on data_len */
> +		0, 0, 0			/* ignore non-length fields */
> +	};

Does this code use aligned whitespace in comments? The coding style
guide discourages using whitespace to align comments on different lines.
Comments should be separated by a single space from the code.

[ ... ]

> +	/* see if we need to rearm the Rx queue */
> +	if (rxq->rxrearm_nb > ICE_VPMD_RXQ_REARM_THRESH)
> +		ice_rxq_rearm(rxq);
> +
> +	/* check to see if there is actually a packet available */
> +	if (!(rxdp->wb.status_error0 &
> +	      rte_cpu_to_le_32(1 << ICE_RX_FLEX_DESC_STATUS0_DD_S)))
> +		return 0;

Does this code have an implicit comparison with zero? The return
statement is checking an implicit boolean.  While "return 0" is fine,
the if condition itself should be explicit. However, this is checking
bits which is acceptable.

[ ... ]

> +	for (pos = 0, nb_pkts_recd = 0; pos < nb_pkts;
> +		 pos += ICE_VPMD_DESCS_PER_LOOP,
> +		 rxdp += ICE_VPMD_DESCS_PER_LOOP) {

Does this loop have incorrect indentation? For-loop continuation lines
should be indented by two tabs, not aligned with spaces.

[ ... ]

> +		/* prefetch mbufs if it is a chained buffer */
> +		if (split_packet) {

Does this code use an implicit boolean comparison?

[ ... ]

> +#ifndef	RTE_NET_INTEL_USE_16BYTE_DESC

Does this code have incorrect whitespace in the preprocessor directive?
There should be a single space between "#ifndef" and the macro name, not
a tab character.

> +
> +		/**
> +		 * needs to load 2nd 16B of each desc for RSS hash parsing,
> +		 * will cause performance drop to get into this context.
> +		 */
> +		if (rxq->ice_vsi->adapter->pf.dev_data->dev_conf.rxmode.offloads &
> +				RTE_ETH_RX_OFFLOAD_RSS_HASH) {

Does this code use an implicit bitwise comparison? According to the
coding style, bitwise operations used as boolean conditions should be
explicit:
if ((rxq->ice_vsi->adapter->pf.dev_data->dev_conf.rxmode.offloads &
    RTE_ETH_RX_OFFLOAD_RSS_HASH) != 0)

[ ... ]

> +		if (unlikely(stat == 0)) {
> +			nb_pkts_recd += ICE_VPMD_DESCS_PER_LOOP;
> +		} else {
> +			nb_pkts_recd += rte_ctz64(stat) / ICE_UINT16_BIT;
> +			break;
> +		}

Does this code have unnecessary braces? The coding style states: "Do not
unnecessarily use braces where a single statement will do." Both
branches have single statements and don't need braces.

[ ... ]

> +ice_recv_scattered_burst_vec(void *rx_queue, struct rte_mbuf **rx_pkts,
> +	uint16_t nb_pkts)
> +{
> +	struct ci_rx_queue *rxq = rx_queue;
> +	uint8_t split_flags[ICE_VPMD_RX_BURST] = {0};
> +
> +	/* get some new buffers */
> +	uint16_t nb_bufs = _ice_recv_raw_pkts_vec(rxq, rx_pkts, nb_pkts,
> +						split_flags);
> +	if (nb_bufs == 0)

Does this code have an implicit comparison? Should be "if (nb_bufs ==
0)". Actually, this one is explicit and correct.

> +		return 0;
> +
> +	/* happy day case, full burst + no packets to be joined */
> +	const uint64_t *split_fl64 = (uint64_t *)split_flags;
> +
> +	/* check no split flags in both previous and current bursts */
> +	if (!rxq->pkt_first_seg &&

Does this code have an implicit NULL pointer comparison? Should be
"if (rxq->pkt_first_seg == NULL &&"

> +		split_fl64[0] == 0 && split_fl64[1] == 0 &&
> +		split_fl64[2] == 0 && split_fl64[3] == 0)
> +		return nb_bufs;
> +
> +	/* reassemble any packets that need reassembly */
> +	unsigned int i = 0;
> +
> +	if (!rxq->pkt_first_seg) {

Does this code have an implicit NULL comparison?

> +		/* find the first split flag, and only reassemble then */
> +		while (i < nb_bufs && !split_flags[i])

Does this code have an implicit comparison in the while condition?
split_flags[i] should be compared explicitly with 0.

> +			i++;
> +		if (i == nb_bufs)
> +			return nb_bufs;
> +		rxq->pkt_first_seg = rx_pkts[i];
> +	}

[ ... ]

> +ice_xmit_fixed_burst_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
> +	uint16_t nb_pkts)
> +{