Re: [PATCH v3 05/13] net/ice: use the common Rx queue structure

[Bruce Richardson <bruce.richardson@intel.com>]

On Mon, May 12, 2025 at 01:54:31PM +0100, Anatoly Burakov wrote:
> Make the ice driver use the new common Rx queue structure.
> 
> Because the ice driver supports both 16-byte and 32-byte descriptor
> formats (controlled by RTE_LIBRTE_ICE_16BYTE_RX_DESC define), the common
> queue structure has to take that into account, so the ring queue
> structure will have both, while the actual descriptor format is picked by
> ice at compile time using the above macro. Direct usage of Rx queue
> structure is now meant to be replaced with a macro access that takes
> descriptor size into account.

I'd have the same comment as on the previous patch. I think it would be
better to not have to use a macro at each reference, but have the struct
type aliased as is done now.

> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     v2:
>     - Make xtr_field_offs of type ptrdiff_t instead of off_t to fix 32-bit compile
>       issues
> 
>  drivers/net/intel/common/rx.h               |  23 +++
>  drivers/net/intel/ice/ice_dcf.c             |   6 +-
>  drivers/net/intel/ice/ice_dcf_ethdev.c      |  22 +--
>  drivers/net/intel/ice/ice_ethdev.c          |   2 +-
>  drivers/net/intel/ice/ice_ethdev.h          |   5 +-
>  drivers/net/intel/ice/ice_rxtx.c            | 158 ++++++++++----------
>  drivers/net/intel/ice/ice_rxtx.h            |  78 ++--------
>  drivers/net/intel/ice/ice_rxtx_common_avx.h |   6 +-
>  drivers/net/intel/ice/ice_rxtx_vec_avx2.c   |  14 +-
>  drivers/net/intel/ice/ice_rxtx_vec_avx512.c |  16 +-
>  drivers/net/intel/ice/ice_rxtx_vec_common.h |   6 +-
>  drivers/net/intel/ice/ice_rxtx_vec_sse.c    |  22 +--
>  12 files changed, 164 insertions(+), 194 deletions(-)
> 
> diff --git a/drivers/net/intel/common/rx.h b/drivers/net/intel/common/rx.h
> index db49db57d0..9a691971bc 100644
> --- a/drivers/net/intel/common/rx.h
> +++ b/drivers/net/intel/common/rx.h
> @@ -5,6 +5,7 @@
>  #ifndef _COMMON_INTEL_RX_H_
>  #define _COMMON_INTEL_RX_H_
>  
> +#include <stddef.h>
>  #include <stdint.h>
>  #include <unistd.h>
>  #include <rte_mbuf.h>
> @@ -12,6 +13,7 @@
>  
>  #define CI_RX_BURST 32
>  #define CI_RX_MAX_BURST 32
> +#define CI_RX_MAX_NSEG 2
>  
>  struct ci_rx_queue;
>  
> @@ -23,6 +25,8 @@ struct ci_rx_entry_sc {
>  	struct rte_mbuf *fbuf; /* First segment of the fragmented packet.*/
>  };
>  
> +typedef void (*ci_rx_release_mbufs_t)(struct ci_rx_queue *rxq);
> +
>  /**
>   * Structure associated with each RX queue.
>   */
> @@ -32,6 +36,8 @@ struct ci_rx_queue {
>  		volatile union ixgbe_adv_rx_desc *ixgbe_rx_ring;
>  		volatile union i40e_16byte_rx_desc *i40e_rx_16b_ring;
>  		volatile union i40e_32byte_rx_desc *i40e_rx_32b_ring;
> +		volatile union ice_16b_rx_flex_desc *ice_rx_16b_ring;
> +		volatile union ice_32b_rx_flex_desc *ice_rx_32b_ring;
>  	};
>  	volatile uint8_t *qrx_tail;   /**< register address of tail */
>  	struct ci_rx_entry *sw_ring; /**< address of RX software ring. */
> @@ -64,10 +70,16 @@ struct ci_rx_queue {
>  	bool drop_en;  /**< if 1, drop packets if no descriptors are available. */
>  	uint64_t mbuf_initializer; /**< value to init mbufs */
>  	uint64_t offloads; /**< Rx offloads with RTE_ETH_RX_OFFLOAD_* */
> +	uint32_t rxdid; /**< RX descriptor format ID. */
> +	uint32_t proto_xtr; /* protocol extraction type */
> +	uint64_t xtr_ol_flag; /* flexible descriptor metadata extraction offload flag */
> +	ptrdiff_t xtr_field_offs; /* Protocol extraction matedata offset*/
> +	uint64_t hw_time_update; /**< Last time HW timestamp was updated */

Just to confirm - these are not in the ice-specific section because they
are also used by iavf?

>  	/** need to alloc dummy mbuf, for wraparound when scanning hw ring */
>  	struct rte_mbuf fake_mbuf;
>  	union { /* the VSI this queue belongs to */
>  		struct i40e_vsi *i40e_vsi;
> +		struct ice_vsi *ice_vsi;
>  	};

<snip>