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.richard...@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
