> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.anan...@intel.com>
> Sent: Tuesday, October 13, 2020 00:25
> To: Wang, Haiyue <haiyue.w...@intel.com>; Power, Ciara
> <ciara.po...@intel.com>; dev@dpdk.org
> Cc: Zhao1, Wei <wei.zh...@intel.com>; Guo, Jia <jia....@intel.com>
> Subject: RE: [PATCH v3 11/18] net/ixgbe: add checks for max SIMD bitwidth
>
>
> > > -----Original Message-----
> > > From: Ananyev, Konstantin <konstantin.anan...@intel.com>
> > > Sent: Monday, October 12, 2020 17:09
> > > To: Wang, Haiyue <haiyue.w...@intel.com>; Power, Ciara
> > > <ciara.po...@intel.com>; dev@dpdk.org
> > > Cc: Zhao1, Wei <wei.zh...@intel.com>; Guo, Jia <jia....@intel.com>
> > > Subject: RE: [PATCH v3 11/18] net/ixgbe: add checks for max SIMD bitwidth
> > >
> > > > > > > From: Power, Ciara <ciara.po...@intel.com>
> > > > > > > Sent: Wednesday, September 30, 2020 21:04
> > > > > > > To: dev@dpdk.org
> > > > > > > Cc: Power, Ciara <ciara.po...@intel.com>; Zhao1, Wei
> > > > > > > <wei.zh...@intel.com>; Guo, Jia
> > > > > > > <jia....@intel.com>; Wang, Haiyue <haiyue.w...@intel.com>
> > > > > > > Subject: [PATCH v3 11/18] net/ixgbe: add checks for max SIMD
> > > > > > > bitwidth
> > > > > > >
> > > > > > > When choosing a vector path to take, an extra condition must be
> > > > > > > satisfied to ensure the max SIMD bitwidth allows for the CPU
> > > > > > > enabled
> > > > > > > path.
> > > > > > >
> > > > > > > Cc: Wei Zhao <wei.zh...@intel.com>
> > > > > > > Cc: Jeff Guo <jia....@intel.com>
> > > > > > >
> > > > > > > Signed-off-by: Ciara Power <ciara.po...@intel.com>
> > > > > > > ---
> > > > > > > drivers/net/ixgbe/ixgbe_rxtx.c | 7 +++++--
> > > > > > > 1 file changed, 5 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > > > > > > b/drivers/net/ixgbe/ixgbe_rxtx.c
> > > > > > > index 977ecf5137..eadc7183f2 100644
> > > > > > > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > > > > > > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > > > > > > @@ -2503,7 +2503,9 @@ ixgbe_set_tx_function(struct rte_eth_dev
> > > > > > > *dev, struct ixgbe_tx_queue
> > > *txq)
> > > > > > > dev->tx_pkt_prepare = NULL;
> > > > > > > if (txq->tx_rs_thresh <= RTE_IXGBE_TX_MAX_FREE_BUF_SZ &&
> > > > > > > (rte_eal_process_type() !=
> > > > > > > RTE_PROC_PRIMARY ||
> > > > > > > - ixgbe_txq_vec_setup(txq) == 0))
> > > > > > > {
> > > > > > > + ixgbe_txq_vec_setup(txq) == 0)
> > > > > > > &&
> > > > > > > + rte_get_max_simd_bitwidth()
> > > > > >
> > > > > > As Konstantin mentioned: " I think it is a bit safer to do all
> > > > > > checks first before
> > > > > > doing txq_vec_setup()."
> > > > > >
> > > > > > Fox x86 & arm platforms, the setup is always 0, since 'sw_ring_v'
> > > > > > is union with
> > > > > > 'sw_ring' which is initialize at 'ixgbe_dev_tx_queue_setup'.
> > > > > >
> > > > > > union {
> > > > > > struct ixgbe_tx_entry *sw_ring; /**< address of SW ring
> > > > > > for scalar PMD. */
> > > > > > struct ixgbe_tx_entry_v *sw_ring_v; /**< address of SW
> > > > > > ring for vector PMD */
> > > > > > };
> > > > > >
> > > > > > static inline int
> > > > > > ixgbe_txq_vec_setup_default(struct ixgbe_tx_queue *txq,
> > > > > > const struct ixgbe_txq_ops *txq_ops)
> > > > > > {
> > > > > > if (txq->sw_ring_v == NULL)
> > > > > > return -1;
> > > > > >
> > > > > > /* leave the first one for overflow */
> > > > > > txq->sw_ring_v = txq->sw_ring_v + 1;
> > > > > > txq->ops = txq_ops;
> > > > > >
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > So we need check the SIMD bitwidth firstly to avoid changing the
> > > > > > sw_ring* pointer address.
> > > > > >
> > > > > >
> > > > > > Also, looks like we need to add check on:
> > > > > >
> > > > > > int
> > > > > > ixgbe_dev_tx_done_cleanup(void *tx_queue, uint32_t free_cnt)
> > > > > > {
> > > > > > struct ixgbe_tx_queue *txq = (struct ixgbe_tx_queue *)tx_queue;
> > > > > > if (txq->offloads == 0 &&
> > > > > > #ifdef RTE_LIBRTE_SECURITY
> > > > > > !(txq->using_ipsec) &&
> > > > > > #endif
> > > > > > txq->tx_rs_thresh >=
> > > > > > RTE_PMD_IXGBE_TX_MAX_BURST) {
> > > > > > if (txq->tx_rs_thresh <= RTE_IXGBE_TX_MAX_FREE_BUF_SZ &&
> > > > > >
> > > > > > <------------------- Add the same check
> > > > > > (rte_eal_process_type() !=
> > > > > > RTE_PROC_PRIMARY ||
> > > > > > txq->sw_ring_v != NULL)) {
> > > > > > return ixgbe_tx_done_cleanup_vec(txq, free_cnt);
> > > > >
> > > > > Could you probably explain a bit more why it is needed?
> > > >
> > > > To align with the vector selection path:
> > > >
> > > > if (txq->tx_rs_thresh <= RTE_IXGBE_TX_MAX_FREE_BUF_SZ &&
> > > > (rte_eal_process_type() !=
> > > > RTE_PROC_PRIMARY ||
> > > > ixgbe_txq_vec_setup(txq) == 0))
> > >
> > >
> > > Ok, so to make sure that TX is running in vector mode?
> >
> > That's right, since no variable to save the vector mode selection,
> > then the check condition should be the same.
>
> What I am saying, that here instead of conditions we should check
> was vector mode already selected or not.
> Probably the easiest way to do it - check what tx function is setup.
>
Misunderstood, yes, this is more intuitive and clean.
> > > > > > > 2.17.1
