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

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 5/14/2025 6:56 PM, Bruce Richardson wrote:
> 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?

Yes, it is for that reason. Any fields moved to common parts are present 
in multiple drivers. There will be some instances where driver specific 
fields do the same things slightly differently (prime example is 
timestamping support), this can be future work.

-- 
Thanks,
Anatoly