[PATCH v1 09/21] net/ixgbe: use common Tx queue structure

[Bruce Richardson]

On Mon, Dec 02, 2024 at 01:51:35PM +0000, Medvedkin, Vladimir wrote:
>    Hi Bruce,
> 
>    On 02/12/2024 11:24, Bruce Richardson wrote:
> 
> Merge in additional fields used by the ixgbe driver and then convert it
> over to using the common Tx queue structure.
> 
> Signed-off-by: Bruce Richardson [1]<bruce.richardson at intel.com>
> ---
>  drivers/net/_common_intel/tx.h                | 14 +++-
>  drivers/net/ixgbe/ixgbe_ethdev.c              |  4 +-
>  .../ixgbe/ixgbe_recycle_mbufs_vec_common.c    |  2 +-
>  drivers/net/ixgbe/ixgbe_rxtx.c                | 64 +++++++++----------
>  drivers/net/ixgbe/ixgbe_rxtx.h                | 56 ++--------------
>  drivers/net/ixgbe/ixgbe_rxtx_vec_common.h     | 26 ++++----
>  drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c       | 14 ++--
>  drivers/net/ixgbe/ixgbe_rxtx_vec_sse.c        | 14 ++--
>  8 files changed, 80 insertions(+), 114 deletions(-)
> 
> diff --git a/drivers/net/_common_intel/tx.h b/drivers/net/_common_intel/tx.h
> index c4a1a0c816..51ae3b051d 100644
> --- a/drivers/net/_common_intel/tx.h
> +++ b/drivers/net/_common_intel/tx.h
> @@ -34,9 +34,13 @@ 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 union ixgbe_adv_tx_desc *ixgbe_tx_ring;
>         };
>         volatile uint8_t *qtx_tail;               /* register address of tail */
> -       struct ci_tx_entry *sw_ring; /* virtual address of SW ring */
> +       union {
> +               struct ci_tx_entry *sw_ring; /* virtual address of SW ring */
> +               struct ci_tx_entry_vec *sw_ring_vec;
> +       };
>         rte_iova_t tx_ring_dma;        /* TX ring DMA address */
>         uint16_t nb_tx_desc;           /* number of TX descriptors */
>         uint16_t tx_tail; /* current value of tail register */
> @@ -87,6 +91,14 @@ struct ci_tx_queue {
>                         uint8_t tc;
>                         bool use_ctx;  /* with ctx info, each pkt needs two desc
> riptors */
>                 };
> +               struct { /* ixgbe specific values */
> +                       const struct ixgbe_txq_ops *ops;
> +                       struct ixgbe_advctx_info *ctx_cache;
> 
>    'struct ixgbe_advctx_info ctx_cache[IXGBE_CTX_NUM];' takes only 80
>    bytes of memory, so using a pointer saves 72 bytes. Since the final
>    version of the 'struct ci_tx_queue' without driver specific fields
>    takes 96 bytes, embedding 'ixgbe_advctx_info ctx_cache[2]' array will
>    take one more cache line, which is hot a huge deal in my opinion.
> 

Maybe not, though another way to look at it is that it is that those two
context entries are nearly as big as the rest of the struct!

>    Or consider another (possibly better) approach, where for non IXGBE
>    'struct ci_tx_queue' will remain the same size, but only for IXGBE an
>    extra 80 bytes will be alllocated:
> 
>    struct { /* ixgbe specific values */
> 
>                            const struct ixgbe_txq_ops *ops;
> 
>                            uint32_t ctx_curr;
> 
>                            uint8_t pthresh;   /**< Prefetch threshold
>    register. */
> 
>                            uint8_t hthresh;   /**< Host threshold
>    register. */
> 
>                            uint8_t wthresh;   /**< Write-back threshold
>    reg. */
> 
>                            uint8_t using_ipsec;  /**< indicates that IPsec
>    TX feature is in use */
>                            struct ixgbe_advctx_info ctx_cache[0];
> 
>                    };
> 
> +                       uint32_t ctx_curr;
> +#ifdef RTE_LIB_SECURITY
> +                       uint8_t using_ipsec;  /**< indicates that IPsec TX featu
> re is in use */
> +#endif
> +               };
>         };
>  };
> 

I prefer solutions where the extra 80 bytes are only allocated for the one
driver that needs them. I'll see if this alternative can work ok for us.

/Bruce