On Mon, Dec 02, 2024 at 02:09:35PM +0000, Bruce Richardson wrote:
> 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.
>
Trying out this solution, I hit the problem described in the commit log of
the previous patch to this one - it introduces a dependency on ixgbe
structures inside the common driver. By changing the type of the ctx field
from an array to a pointer, we remove the need to have the actual type
defined at compile time - as long as we never dereference the pointer. This
no-reference is why, for example, we have have the union of all the
different descriptor types in the structure without having to include the
headers that define them.
If we include ixgbe_advctx_info as an array rather than a struct - even as
a zero-length array - then we need to have the definition of the structure
present at that point in the code. This means we either need to:
- copy in the definitions of ixgbe_advctx_info and ixgbe_tx_offload into
our common header structure, or
- put a #include ixgbe_rxtx.h in our common tx.h header file. [And it can't
go at the start because that header itself includes tx.h to get the
definitions of the queue types, meaning that it can only be included
half-way down tx.h]
In any case, either approach introduces ixgbe-specific definitions into the
common header files. Therefore, I would prefer to keep things as in this
patchset, giving us a smaller structure, and a clean separation of
driver-specific and common structures.
/Bruce
