Re: [PATCH v5 2/4] net/intel: use common Tx queue structure

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

On Thu, Mar 27, 2025 at 09:34:35PM +0530, Shaiq Wani wrote:
> Merge in additional fields used by the idpf driver and then convert it
> over to using the common Tx queue structure
> 
> Signed-off-by: Shaiq Wani <shaiq.wani@intel.com>
> ---
>  drivers/net/intel/common/tx.h                 | 20 +++++++
>  drivers/net/intel/cpfl/cpfl_ethdev.c          |  3 +-
>  drivers/net/intel/cpfl/cpfl_ethdev.h          |  2 +-
>  drivers/net/intel/cpfl/cpfl_rxtx.c            | 26 ++++-----
>  drivers/net/intel/cpfl/cpfl_rxtx.h            |  3 +-
>  drivers/net/intel/cpfl/cpfl_rxtx_vec_common.h |  3 +-
>  drivers/net/intel/idpf/idpf_common_rxtx.c     | 36 ++++++------
>  drivers/net/intel/idpf/idpf_common_rxtx.h     | 58 +++----------------
>  .../net/intel/idpf/idpf_common_rxtx_avx2.c    | 12 ++--
>  .../net/intel/idpf/idpf_common_rxtx_avx512.c  | 21 +++----
>  drivers/net/intel/idpf/idpf_common_virtchnl.c |  2 +-
>  drivers/net/intel/idpf/idpf_common_virtchnl.h |  2 +-
>  drivers/net/intel/idpf/idpf_ethdev.c          |  3 +-
>  drivers/net/intel/idpf/idpf_rxtx.c            | 21 ++++---
>  drivers/net/intel/idpf/idpf_rxtx.h            |  1 +
>  drivers/net/intel/idpf/idpf_rxtx_vec_common.h |  5 +-
>  16 files changed, 101 insertions(+), 117 deletions(-)
> 
> diff --git a/drivers/net/intel/common/tx.h b/drivers/net/intel/common/tx.h
> index d9cf4474fc..af32f4deda 100644
> --- a/drivers/net/intel/common/tx.h
> +++ b/drivers/net/intel/common/tx.h
> @@ -35,6 +35,7 @@ struct ci_tx_queue {
>  		volatile struct i40e_tx_desc *i40e_tx_ring;
>  		volatile struct iavf_tx_desc *iavf_tx_ring;
>  		volatile struct ice_tx_desc *ice_tx_ring;
> +		volatile struct idpf_base_tx_desc *idpf_tx_ring;
>  		volatile union ixgbe_adv_tx_desc *ixgbe_tx_ring;
>  	};
>  	volatile uint8_t *qtx_tail;               /* register address of tail */
> @@ -98,6 +99,25 @@ struct ci_tx_queue {
>  			uint8_t wthresh;   /**< Write-back threshold reg. */
>  			uint8_t using_ipsec;  /**< indicates that IPsec TX feature is in use */
>  		};
> +		struct { /* idpf specific values */
> +			volatile union {
> +				struct idpf_flex_tx_sched_desc *desc_ring;
> +				struct idpf_splitq_tx_compl_desc *compl_ring;
> +			};
> +			const struct idpf_txq_ops *idpf_ops;
> +			struct ci_tx_queue *complq;
> +			void **txqs;   /*only valid for split queue mode*/
> +			bool q_started;   /* if tx queue has been started */
> +
> +			/* only valid for split queue mode */
> +			uint32_t tx_start_qid;
> +			uint16_t sw_nb_desc;
> +			uint16_t sw_tail;
> +#define IDPF_TX_CTYPE_NUM	8
> +			uint16_t ctype[IDPF_TX_CTYPE_NUM];
> +			uint8_t expected_gen_id;
> +

Though not a blocker for merge, I'd still like to get this idpf structure
down in size a bit - it causes a 50% increase in the size of the overall
queue structure, from 128 bytes to 192 bytes in size.

There are a couple of ways I think we could cut this down a little:
* bool values only use 1 bytes in size, so we are wasting 3 bytes after the
  q_started flag. Two thoughts here:
  1. is the queue started state not already tracked somewhere in the ethdev
     data or queue data?
  2. Move this field down to the end, beside the expected_gen_id flag, to
     free up 4 bytes
* If we change the ctype array from an array to a pointer, we can save 8
  bytes. However, looking at the code, I'm also wondering how this is used:
  - in scalar code we don't use the ctype array, but check instead for just
    types 2 and 4
  - in the avx512 code, we do use the ctypes array, incrementing values
    that we get. However, from what I see, we only ever look at value 2 of
    the array in that code. Am I missing something here? Do we really need
    the array at all?
* The ops structure only contains a single function pointer for the release
  structure, which either contains the default release function or the
  AVX512 release function. However, with this patchset, the AVX2 code now
  seems to be using the same descriptor format at the AVX512 code, so I
  suspect that the release function may be wrong in those cases. Therefore,
  I believe that the idpf_ops structure should be removed, and the existing
  txq->vector_tx flag used instead to choose the release function. That would
  save another 8 bytes, and correct the AVX2 path.

Regards,
/Bruce