Re: [PATCH v3 04/13] net/i40e: use the common Rx queue structure

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

On 5/14/2025 6:52 PM, Bruce Richardson wrote:
> On Mon, May 12, 2025 at 01:54:30PM +0100, Anatoly Burakov wrote:
>> Make the i40e driver use the new common Rx queue structure.
>>
>> Because the i40e driver supports both 16-byte and 32-byte descriptor
>> formats (controlled by RTE_LIBRTE_I40E_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 i40e 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.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   drivers/net/intel/common/rx.h                 |  14 ++
>>   drivers/net/intel/i40e/i40e_ethdev.c          |   4 +-
>>   drivers/net/intel/i40e/i40e_ethdev.h          |   4 +-
>>   drivers/net/intel/i40e/i40e_fdir.c            |  16 +--
>>   .../i40e/i40e_recycle_mbufs_vec_common.c      |   6 +-
>>   drivers/net/intel/i40e/i40e_rxtx.c            | 126 +++++++++---------
>>   drivers/net/intel/i40e/i40e_rxtx.h            |  74 +++-------
>>   drivers/net/intel/i40e/i40e_rxtx_common_avx.h |   6 +-
>>   .../net/intel/i40e/i40e_rxtx_vec_altivec.c    |  22 +--
>>   drivers/net/intel/i40e/i40e_rxtx_vec_avx2.c   |  12 +-
>>   drivers/net/intel/i40e/i40e_rxtx_vec_avx512.c |  12 +-
>>   drivers/net/intel/i40e/i40e_rxtx_vec_common.h |   4 +-
>>   drivers/net/intel/i40e/i40e_rxtx_vec_neon.c   |  24 ++--
>>   drivers/net/intel/i40e/i40e_rxtx_vec_sse.c    |  24 ++--
>>   14 files changed, 160 insertions(+), 188 deletions(-)
>>
>> diff --git a/drivers/net/intel/common/rx.h b/drivers/net/intel/common/rx.h
>> index 524de39f9c..db49db57d0 100644
>> --- a/drivers/net/intel/common/rx.h
>> +++ b/drivers/net/intel/common/rx.h
>> @@ -30,6 +30,8 @@ struct ci_rx_queue {
>>   	struct rte_mempool  *mp; /**< mbuf pool to populate RX ring. */
>>   	union { /* RX ring virtual address */
>>   		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;
> 
> Rather than doing this, would it not be better to keep using the current
> scheme of just having an #ifdef aliasing i40e_rx_desc to either the 16byte
> or 32byte variants?
> 
> /Bruce

I had that in my first iteration, but I don't really like having 
driver-specific #ifdefs in a common header. Plus, it also introduces 
header ordering dependencies (need to include header with definitions 
before including this one), so I decided against it.

-- 
Thanks,
Anatoly