|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-08

I'll review this DPDK patch for the NEON-optimized Rx/Tx vector paths in the ice driver.

git sha: (not provided in patch)
Author: Jay Wang <jay.wang2 at arm.com>
Subject: [PATCH v3] net/ice: add NEON-optimised Rx/Tx vector paths

This patch adds NEON-optimized Rx and Tx paths to the ice driver for
ARM64 platforms. The implementation appears to be based on the existing
x86 vector paths.

> 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
> +	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR != (1u << 2));
> +	RTE_BUILD_BUG_ON(RTE_MBUF_F_RX_FDIR_ID != (1u << 13));

The coding style guide states that defines should be uppercase and may
include underscores. However, is FDID_MIS_MAGIC intended to be used
outside this function? If not, should it be a const variable instead of
a macro?

> +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)

The function parameter alignment doesn't follow the coding style. The
second line should be indented to align with the opening parenthesis of
the first line.

> +{
> +	const uint64x2_t mbuf_init = {rxq->mbuf_initializer, 0};

[ ... ]

> +	/**
> +	 * map the checksum, rss and vlan fields to the checksum, rss
> +	 * and vlan flag.
> +	 */
> +	const uint8x16_t cksum_flags = {
> +		(RTE_MBUF_F_RX_OUTER_L4_CKSUM_GOOD >> 20 | RTE_MBUF_F_RX_L4_CKSUM_GOOD |
> +		 RTE_MBUF_F_RX_IP_CKSUM_GOOD) >> 1,

These lines exceed 80 characters. Does this violate the line length
requirements in the coding style guide?

[ ... ]

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

Per the coding style guide, should this be written as:
	if (rxq->fdir_enabled != 0) {

The guide states that explicit comparisons should be used rather than
implicit boolean conversions.

[ ... ]

> +#ifndef	RTE_NET_INTEL_USE_16BYTE_DESC

There appears to be a tab character after ifndef. Should this use a
single space instead to match the coding style?

> +
> +		/**
> +		 * 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) {

Should this comparison be made explicit by comparing the result of the
bitwise AND against 0?

[ ... ]

> +static uint16_t
> +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)

Should this be written as explicit comparison: if (nb_bufs == 0)?
Actually, this is already explicit, so it's 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 need explicit NULL comparison?
	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) {

Same question about explicit NULL comparison here.

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

Should split_flags[i] be compared explicitly against 0?

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

[ ... ]

> +static __rte_always_inline uint16_t
> +ice_xmit_fixed_burst_vec(void *tx_queue, struct rte_mbuf **tx_pkts,
> +	uint16_t nb_pkts)
> +{
> +	struct ci_tx_queue *txq = (struct ci_tx_queue *)tx_queue;
> +	volatile struct ci_tx_desc *txdp;
> +	struct ci_tx_entry_vec *txep;
> +	uint16_t n, nb_commit, tx_id;
> +	uint64_t flags = CI_TX_DESC_CMD_DEFAULT;
> +	uint64_t rs = CI_TX_DESC_CMD_RS | CI_TX_DESC_CMD_DEFAULT;
> +	int i;
> +
> +	/* cross rx_thresh boundary is not allowed */
> +	nb_pkts = RTE_MIN(nb_pkts, txq->tx_rs_thresh);
> +
> +	if (txq->nb_tx_free < txq->tx_free_thresh)
> +		ci_tx_free_bufs_vec(txq, ice_tx_desc_done, false);

Does the ci_tx_free_bufs_vec() function check for errors? If it returns
an error code, is it being ignored here?

> +
> +	nb_pkts = (uint16_t)RTE_MIN(txq->nb_tx_free, nb_pkts);
> +	nb_commit = nb_pkts;
> +	if (unlikely(nb_pkts == 0))
> +		return 0;

[ ... ]

> diff --git a/drivers/net/intel/ice/meson.build b/drivers/net/intel/ice/meson.build
> index 293577676f..a205304c89 100644
> --- a/drivers/net/intel/ice/meson.build
> +++ b/drivers/net/intel/ice/meson.build
> @@ -33,6 +33,8 @@ endif
>  if arch_subdir == 'x86'
>      sources_avx2 += files('ice_rxtx_vec_avx2.c')
>      sources_avx512 += files('ice_rxtx_vec_avx512.c')
> +elif arch_subdir == 'arm' and dpdk_conf.get('RTE_ARCH_64')
> +    sources += files('ice_rxtx_vec_neon.c')
>  endif