diff mbox series

[v3,11/18] net/ixgbe: add checks for max SIMD bitwidth

Message ID 20200930130415.11211-12-ciara.power@intel.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series add max SIMD bitwidth to EAL |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Power, Ciara Sept. 30, 2020, 1:04 p.m. UTC 
  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.zhao1@intel.com>
Cc: Jeff Guo <jia.guo@intel.com>

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 drivers/net/ixgbe/ixgbe_rxtx.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Ananyev, Konstantin Oct. 8, 2020, 3:05 p.m. UTC | #1 
> 
> 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.zhao1@intel.com>
> Cc: Jeff Guo <jia.guo@intel.com>
> 
> Signed-off-by: Ciara Power <ciara.power@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()
> +				>= RTE_MAX_128_SIMD) {

As a nit - I think it is a bit safer to do all checks first before doing txq_vec_setup().
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
 

>  			PMD_INIT_LOG(DEBUG, "Vector tx enabled.");
>  			dev->tx_pkt_burst = ixgbe_xmit_pkts_vec;
>  		} else
> @@ -4743,7 +4745,8 @@ ixgbe_set_rx_function(struct rte_eth_dev *dev)
>  	 * conditions to be met and Rx Bulk Allocation should be allowed.
>  	 */
>  	if (ixgbe_rx_vec_dev_conf_condition_check(dev) ||
> -	    !adapter->rx_bulk_alloc_allowed) {
> +	    !adapter->rx_bulk_alloc_allowed ||
> +			rte_get_max_simd_bitwidth() < RTE_MAX_128_SIMD) {
>  		PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet Vector Rx "
>  				    "preconditions",
>  			     dev->data->port_id);
> --
> 2.17.1
Wang, Haiyue Oct. 10, 2020, 1:13 p.m. UTC | #2 
Hi Ciara,

> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Wednesday, September 30, 2020 21:04
> To: dev@dpdk.org
> Cc: Power, Ciara <ciara.power@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia
> <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@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.zhao1@intel.com>
> Cc: Jeff Guo <jia.guo@intel.com>
> 
> Signed-off-by: Ciara Power <ciara.power@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);
		} else {
			return ixgbe_tx_done_cleanup_simple(txq, free_cnt);
		}
	}

> +				>= RTE_MAX_128_SIMD) {
>  			PMD_INIT_LOG(DEBUG, "Vector tx enabled.");
>  			dev->tx_pkt_burst = ixgbe_xmit_pkts_vec;
>  		} else
> @@ -4743,7 +4745,8 @@ ixgbe_set_rx_function(struct rte_eth_dev *dev)
>  	 * conditions to be met and Rx Bulk Allocation should be allowed.
>  	 */
>  	if (ixgbe_rx_vec_dev_conf_condition_check(dev) ||
> -	    !adapter->rx_bulk_alloc_allowed) {
> +	    !adapter->rx_bulk_alloc_allowed ||
> +			rte_get_max_simd_bitwidth() < RTE_MAX_128_SIMD) {
>  		PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet Vector Rx "
>  				    "preconditions",
>  			     dev->data->port_id);
> --
> 2.17.1
Ananyev, Konstantin Oct. 11, 2020, 10:31 p.m. UTC | #3 
> > From: Power, Ciara <ciara.power@intel.com>
> > Sent: Wednesday, September 30, 2020 21:04
> > To: dev@dpdk.org
> > Cc: Power, Ciara <ciara.power@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia
> > <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@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.zhao1@intel.com>
> > Cc: Jeff Guo <jia.guo@intel.com>
> >
> > Signed-off-by: Ciara Power <ciara.power@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?

> 		} else {
> 			return ixgbe_tx_done_cleanup_simple(txq, free_cnt);
> 		}
> 	}
> 
> > +				>= RTE_MAX_128_SIMD) {
> >  			PMD_INIT_LOG(DEBUG, "Vector tx enabled.");
> >  			dev->tx_pkt_burst = ixgbe_xmit_pkts_vec;
> >  		} else
> > @@ -4743,7 +4745,8 @@ ixgbe_set_rx_function(struct rte_eth_dev *dev)
> >  	 * conditions to be met and Rx Bulk Allocation should be allowed.
> >  	 */
> >  	if (ixgbe_rx_vec_dev_conf_condition_check(dev) ||
> > -	    !adapter->rx_bulk_alloc_allowed) {
> > +	    !adapter->rx_bulk_alloc_allowed ||
> > +			rte_get_max_simd_bitwidth() < RTE_MAX_128_SIMD) {
> >  		PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet Vector Rx "
> >  				    "preconditions",
> >  			     dev->data->port_id);
> > --
> > 2.17.1
Wang, Haiyue Oct. 12, 2020, 1:29 a.m. UTC | #4 
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, October 12, 2020 06:31
> To: Wang, Haiyue <haiyue.wang@intel.com>; Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> Subject: RE: [PATCH v3 11/18] net/ixgbe: add checks for max SIMD bitwidth
> 
> 
> 
> > > From: Power, Ciara <ciara.power@intel.com>
> > > Sent: Wednesday, September 30, 2020 21:04
> > > To: dev@dpdk.org
> > > Cc: Power, Ciara <ciara.power@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia
> > > <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@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.zhao1@intel.com>
> > > Cc: Jeff Guo <jia.guo@intel.com>
> > >
> > > Signed-off-by: Ciara Power <ciara.power@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))


> 
> > 		} else {
> > 			return ixgbe_tx_done_cleanup_simple(txq, free_cnt);
> > 		}


> > > 2.17.1
Ananyev, Konstantin Oct. 12, 2020, 9:09 a.m. UTC | #5 
> > > > From: Power, Ciara <ciara.power@intel.com>
> > > > Sent: Wednesday, September 30, 2020 21:04
> > > > To: dev@dpdk.org
> > > > Cc: Power, Ciara <ciara.power@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia
> > > > <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@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.zhao1@intel.com>
> > > > Cc: Jeff Guo <jia.guo@intel.com>
> > > >
> > > > Signed-off-by: Ciara Power <ciara.power@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?
If so, then doesn't txq->sw_ring_v != NULL was intended to do so?
BTW, is it a valid check? Considering that sw_ring and sw_ring_v
is a union?

> 
> 
> >
> > > 		} else {
> > > 			return ixgbe_tx_done_cleanup_simple(txq, free_cnt);
> > > 		}
> 
> 
> > > > 2.17.1
Wang, Haiyue Oct. 12, 2020, 4:04 p.m. UTC | #6 
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, October 12, 2020 17:09
> To: Wang, Haiyue <haiyue.wang@intel.com>; Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> Subject: RE: [PATCH v3 11/18] net/ixgbe: add checks for max SIMD bitwidth
> 
> > > > > From: Power, Ciara <ciara.power@intel.com>
> > > > > Sent: Wednesday, September 30, 2020 21:04
> > > > > To: dev@dpdk.org
> > > > > Cc: Power, Ciara <ciara.power@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia
> > > > > <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@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.zhao1@intel.com>
> > > > > Cc: Jeff Guo <jia.guo@intel.com>
> > > > >
> > > > > Signed-off-by: Ciara Power <ciara.power@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.

> If so, then doesn't txq->sw_ring_v != NULL was intended to do so?
> BTW, is it a valid check? Considering that sw_ring and sw_ring_v
> is a union?

Yes, sw_ring_v should always be !NULL ;-)

> 
> >
> >
> > >
> > > > 		} else {
> > > > 			return ixgbe_tx_done_cleanup_simple(txq, free_cnt);
> > > > 		}
> >
> >
> > > > > 2.17.1
Ananyev, Konstantin Oct. 12, 2020, 4:24 p.m. UTC | #7 
> > -----Original Message-----
> > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > Sent: Monday, October 12, 2020 17:09
> > To: Wang, Haiyue <haiyue.wang@intel.com>; Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> > Cc: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> > Subject: RE: [PATCH v3 11/18] net/ixgbe: add checks for max SIMD bitwidth
> >
> > > > > > From: Power, Ciara <ciara.power@intel.com>
> > > > > > Sent: Wednesday, September 30, 2020 21:04
> > > > > > To: dev@dpdk.org
> > > > > > Cc: Power, Ciara <ciara.power@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia
> > > > > > <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@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.zhao1@intel.com>
> > > > > > Cc: Jeff Guo <jia.guo@intel.com>
> > > > > >
> > > > > > Signed-off-by: Ciara Power <ciara.power@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. 

> 
> > If so, then doesn't txq->sw_ring_v != NULL was intended to do so?
> > BTW, is it a valid check? Considering that sw_ring and sw_ring_v
> > is a union?
> 
> Yes, sw_ring_v should always be !NULL ;-)
> 
> >
> > >
> > >
> > > >
> > > > > 		} else {
> > > > > 			return ixgbe_tx_done_cleanup_simple(txq, free_cnt);
> > > > > 		}
> > >
> > >
> > > > > > 2.17.1
Wang, Haiyue Oct. 13, 2020, 1:12 a.m. UTC | #8 
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Tuesday, October 13, 2020 00:25
> To: Wang, Haiyue <haiyue.wang@intel.com>; Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> Cc: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> Subject: RE: [PATCH v3 11/18] net/ixgbe: add checks for max SIMD bitwidth
> 
> 
> > > -----Original Message-----
> > > From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > > Sent: Monday, October 12, 2020 17:09
> > > To: Wang, Haiyue <haiyue.wang@intel.com>; Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> > > Cc: Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia <jia.guo@intel.com>
> > > Subject: RE: [PATCH v3 11/18] net/ixgbe: add checks for max SIMD bitwidth
> > >
> > > > > > > From: Power, Ciara <ciara.power@intel.com>
> > > > > > > Sent: Wednesday, September 30, 2020 21:04
> > > > > > > To: dev@dpdk.org
> > > > > > > Cc: Power, Ciara <ciara.power@intel.com>; Zhao1, Wei <wei.zhao1@intel.com>; Guo, Jia
> > > > > > > <jia.guo@intel.com>; Wang, Haiyue <haiyue.wang@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.zhao1@intel.com>
> > > > > > > Cc: Jeff Guo <jia.guo@intel.com>
> > > > > > >
> > > > > > > Signed-off-by: Ciara Power <ciara.power@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
diff mbox series

Patch

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()
+				>= RTE_MAX_128_SIMD) {
 			PMD_INIT_LOG(DEBUG, "Vector tx enabled.");
 			dev->tx_pkt_burst = ixgbe_xmit_pkts_vec;
 		} else
@@ -4743,7 +4745,8 @@  ixgbe_set_rx_function(struct rte_eth_dev *dev)
 	 * conditions to be met and Rx Bulk Allocation should be allowed.
 	 */
 	if (ixgbe_rx_vec_dev_conf_condition_check(dev) ||
-	    !adapter->rx_bulk_alloc_allowed) {
+	    !adapter->rx_bulk_alloc_allowed ||
+			rte_get_max_simd_bitwidth() < RTE_MAX_128_SIMD) {
 		PMD_INIT_LOG(DEBUG, "Port[%d] doesn't meet Vector Rx "
 				    "preconditions",
 			     dev->data->port_id);