diff mbox series

[v3,04/18] net/i40e: add checks for max SIMD bitwidth

Message ID	20200930130415.11211-5-ciara.power@intel.com (mailing list archive)
State	Superseded, archived
Delegated to:	David Marchand
Headers	IronPort-SDR: QKcjbzj6inz+tNv3qBTWA0yl/EmUbC8uyHGIPk+01ABQGeEf4fJOJ3szQqz0mjFVrHfhawuejs 35BlS/OuZrtQ== IronPort-SDR: qjqpBsVN4HsNpoNkMnpmRVUC/j07Riqd/zu8dqkYgEgM4k8fSMiF8jVRiimJOnPo7dA5KdMGW4 ZV6qyq0rIPXA== From: Ciara Power <ciara.power@intel.com> To: dev@dpdk.org Cc: Ciara Power <ciara.power@intel.com>, Beilei Xing <beilei.xing@intel.com>, Jeff Guo <jia.guo@intel.com> Date: Wed, 30 Sep 2020 14:04:00 +0100 Message-Id: <20200930130415.11211-5-ciara.power@intel.com> In-Reply-To: <20200930130415.11211-1-ciara.power@intel.com> References: <20200807155859.63888-1-ciara.power@intel.com> <20200930130415.11211-1-ciara.power@intel.com> Subject: [dpdk-dev] [PATCH v3 04/18] net/i40e: add checks for max SIMD bitwidth Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>
Series	add max SIMD bitwidth to EAL \| [v3,00/18] add max SIMD bitwidth to EAL [v3,01/18] eal: add max SIMD bitwidth [v3,02/18] eal: add default SIMD bitwidth values [v3,03/18] doc: add detail on using max SIMD bitwidth [v3,04/18] net/i40e: add checks for max SIMD bitwidth [v3,05/18] net/axgbe: add checks for max SIMD bitwidth [v3,06/18] net/bnxt: add checks for max SIMD bitwidth [v3,07/18] net/enic: add checks for max SIMD bitwidth [v3,08/18] net/fm10k: add checks for max SIMD bitwidth [v3,09/18] net/iavf: add checks for max SIMD bitwidth [v3,10/18] net/ice: add checks for max SIMD bitwidth [v3,11/18] net/ixgbe: add checks for max SIMD bitwidth [v3,12/18] net/mlx5: add checks for max SIMD bitwidth [v3,13/18] net/virtio: add checks for max SIMD bitwidth [v3,14/18] distributor: add checks for max SIMD bitwidth [v3,15/18] member: add checks for max SIMD bitwidth [v3,16/18] efd: add checks for max SIMD bitwidth [v3,17/18] net: add checks for max SIMD bitwidth [v3,18/18] lpm: choose vector path at runtime

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: Beilei Xing <beilei.xing@intel.com>
Cc: Jeff Guo <jia.guo@intel.com>

Signed-off-by: Ciara Power <ciara.power@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

Ananyev, Konstantin Oct. 8, 2020, 3:21 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: Beilei Xing <beilei.xing@intel.com>
> Cc: Jeff Guo <jia.guo@intel.com>
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 60b33d20a1..9b535b52fa 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -3098,7 +3098,8 @@ static eth_rx_burst_t
>  i40e_get_latest_rx_vec(bool scatter)
>  {
>  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> +			rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
>  		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
>  				 i40e_recv_pkts_vec_avx2;

Hmm, but that means - if user will set --simd-bitwidth=128 we'll select scalar function, right?
Even though sse one is available.
Is that what we really want in that case?

>  #endif
> @@ -3115,7 +3116,8 @@ i40e_get_recommend_rx_vec(bool scatter)
>  	 * use of AVX2 version to later plaforms, not all those that could
>  	 * theoretically run it.
>  	 */
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
> +			rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
>  		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
>  				 i40e_recv_pkts_vec_avx2;
>  #endif
> @@ -3154,7 +3156,8 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
>  		}
>  	}
> 
> -	if (ad->rx_vec_allowed) {
> +	if (ad->rx_vec_allowed  && rte_get_max_simd_bitwidth()
> +			>= RTE_MAX_128_SIMD) {
>  		/* Vec Rx path */
>  		PMD_INIT_LOG(DEBUG, "Vector Rx path will be used on port=%d.",
>  				dev->data->port_id);
> @@ -3268,7 +3271,8 @@ static eth_tx_burst_t
>  i40e_get_latest_tx_vec(void)
>  {
>  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> +			rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
>  		return i40e_xmit_pkts_vec_avx2;
>  #endif
>  	return i40e_xmit_pkts_vec;
> @@ -3283,7 +3287,8 @@ i40e_get_recommend_tx_vec(void)
>  	 * use of AVX2 version to later plaforms, not all those that could
>  	 * theoretically run it.
>  	 */
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
> +			rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
>  		return i40e_xmit_pkts_vec_avx2;
>  #endif
>  	return i40e_xmit_pkts_vec;
> @@ -3311,7 +3316,9 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
>  	}
> 
>  	if (ad->tx_simple_allowed) {
> -		if (ad->tx_vec_allowed) {
> +		if (ad->tx_vec_allowed &&
> +				rte_get_max_simd_bitwidth()
> +				>= RTE_MAX_128_SIMD) {
>  			PMD_INIT_LOG(DEBUG, "Vector tx finally be used.");
>  			if (ad->use_latest_vec)
>  				dev->tx_pkt_burst =
> --
> 2.17.1

Power, Ciara Oct. 8, 2020, 4:05 p.m. UTC | #2

Hi Konstantin,

 
>-----Original Message-----
>From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
>Sent: Thursday 8 October 2020 16:22
>To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
>Cc: Power, Ciara <ciara.power@intel.com>; Xing, Beilei
><beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
>Subject: RE: [dpdk-dev] [PATCH v3 04/18] net/i40e: 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: Beilei Xing <beilei.xing@intel.com>
>> Cc: Jeff Guo <jia.guo@intel.com>
>>
>> Signed-off-by: Ciara Power <ciara.power@intel.com>
>> ---
>>  drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/i40e/i40e_rxtx.c
>> b/drivers/net/i40e/i40e_rxtx.c index 60b33d20a1..9b535b52fa 100644
>> --- a/drivers/net/i40e/i40e_rxtx.c
>> +++ b/drivers/net/i40e/i40e_rxtx.c
>> @@ -3098,7 +3098,8 @@ static eth_rx_burst_t
>> i40e_get_latest_rx_vec(bool scatter)  {  #if defined(RTE_ARCH_X86) &&
>> defined(CC_AVX2_SUPPORT)
>> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
>> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
>> +			rte_get_max_simd_bitwidth() >=
>RTE_MAX_256_SIMD)
>>  		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
>>  				 i40e_recv_pkts_vec_avx2;
>
>Hmm, but that means - if user will set --simd-bitwidth=128 we'll select
>scalar function, right?
>Even though sse one is available.
>Is that what we really want in that case?
>

If the max SIMD is 128, the second return in this function is used, which I believe is SSE:

	return scatter ? i40e_recv_scattered_pkts_vec :
			 i40e_recv_pkts_vec;

And that function is only called if the max SIMD is >=128, scalar is used otherwise.

Am I missing something else here?

Thanks,
Ciara 

>>  #endif
>> @@ -3115,7 +3116,8 @@ i40e_get_recommend_rx_vec(bool scatter)
>>  	 * use of AVX2 version to later plaforms, not all those that could
>>  	 * theoretically run it.
>>  	 */
>> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
>> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
>> +			rte_get_max_simd_bitwidth() >=
>RTE_MAX_256_SIMD)
>>  		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
>>  				 i40e_recv_pkts_vec_avx2;
>>  #endif
>> @@ -3154,7 +3156,8 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
>>  		}
>>  	}
>>
>> -	if (ad->rx_vec_allowed) {
>> +	if (ad->rx_vec_allowed  && rte_get_max_simd_bitwidth()
>> +			>= RTE_MAX_128_SIMD) {
>>  		/* Vec Rx path */
>>  		PMD_INIT_LOG(DEBUG, "Vector Rx path will be used on
>port=%d.",
>>  				dev->data->port_id);
>> @@ -3268,7 +3271,8 @@ static eth_tx_burst_t
>>  i40e_get_latest_tx_vec(void)
>>  {
>>  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
>> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
>> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
>> +			rte_get_max_simd_bitwidth() >=
>RTE_MAX_256_SIMD)
>>  		return i40e_xmit_pkts_vec_avx2;
>>  #endif
>>  	return i40e_xmit_pkts_vec;
>> @@ -3283,7 +3287,8 @@ i40e_get_recommend_tx_vec(void)
>>  	 * use of AVX2 version to later plaforms, not all those that could
>>  	 * theoretically run it.
>>  	 */
>> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
>> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
>> +			rte_get_max_simd_bitwidth() >=
>RTE_MAX_256_SIMD)
>>  		return i40e_xmit_pkts_vec_avx2;
>>  #endif
>>  	return i40e_xmit_pkts_vec;
>> @@ -3311,7 +3316,9 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
>>  	}
>>
>>  	if (ad->tx_simple_allowed) {
>> -		if (ad->tx_vec_allowed) {
>> +		if (ad->tx_vec_allowed &&
>> +				rte_get_max_simd_bitwidth()
>> +				>= RTE_MAX_128_SIMD) {
>>  			PMD_INIT_LOG(DEBUG, "Vector tx finally be used.");
>>  			if (ad->use_latest_vec)
>>  				dev->tx_pkt_burst =
>> --
>> 2.17.1

Ananyev, Konstantin Oct. 8, 2020, 4:14 p.m. UTC | #3

> Hi Konstantin,
> 
> 
> >-----Original Message-----
> >From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> >Sent: Thursday 8 October 2020 16:22
> >To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> >Cc: Power, Ciara <ciara.power@intel.com>; Xing, Beilei
> ><beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
> >Subject: RE: [dpdk-dev] [PATCH v3 04/18] net/i40e: 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: Beilei Xing <beilei.xing@intel.com>
> >> Cc: Jeff Guo <jia.guo@intel.com>
> >>
> >> Signed-off-by: Ciara Power <ciara.power@intel.com>
> >> ---
> >>  drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
> >>  1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/i40e/i40e_rxtx.c
> >> b/drivers/net/i40e/i40e_rxtx.c index 60b33d20a1..9b535b52fa 100644
> >> --- a/drivers/net/i40e/i40e_rxtx.c
> >> +++ b/drivers/net/i40e/i40e_rxtx.c
> >> @@ -3098,7 +3098,8 @@ static eth_rx_burst_t
> >> i40e_get_latest_rx_vec(bool scatter)  {  #if defined(RTE_ARCH_X86) &&
> >> defined(CC_AVX2_SUPPORT)
> >> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> >> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> >> +			rte_get_max_simd_bitwidth() >=
> >RTE_MAX_256_SIMD)
> >>  		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
> >>  				 i40e_recv_pkts_vec_avx2;
> >
> >Hmm, but that means - if user will set --simd-bitwidth=128 we'll select
> >scalar function, right?
> >Even though sse one is available.
> >Is that what we really want in that case?
> >
> 
> If the max SIMD is 128, the second return in this function is used, which I believe is SSE:
> 
> 	return scatter ? i40e_recv_scattered_pkts_vec :
> 			 i40e_recv_pkts_vec;
> 
> And that function is only called if the max SIMD is >=128, scalar is used otherwise.
> 
> Am I missing something else here?

Nope, you are right, that was me not reading code properly 😊
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 
> Thanks,
> Ciara
> 
> >>  #endif
> >> @@ -3115,7 +3116,8 @@ i40e_get_recommend_rx_vec(bool scatter)
> >>  	 * use of AVX2 version to later plaforms, not all those that could
> >>  	 * theoretically run it.
> >>  	 */
> >> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
> >> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
> >> +			rte_get_max_simd_bitwidth() >=
> >RTE_MAX_256_SIMD)
> >>  		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
> >>  				 i40e_recv_pkts_vec_avx2;
> >>  #endif
> >> @@ -3154,7 +3156,8 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
> >>  		}
> >>  	}
> >>
> >> -	if (ad->rx_vec_allowed) {
> >> +	if (ad->rx_vec_allowed  && rte_get_max_simd_bitwidth()
> >> +			>= RTE_MAX_128_SIMD) {
> >>  		/* Vec Rx path */
> >>  		PMD_INIT_LOG(DEBUG, "Vector Rx path will be used on
> >port=%d.",
> >>  				dev->data->port_id);
> >> @@ -3268,7 +3271,8 @@ static eth_tx_burst_t
> >>  i40e_get_latest_tx_vec(void)
> >>  {
> >>  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
> >> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> >> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> >> +			rte_get_max_simd_bitwidth() >=
> >RTE_MAX_256_SIMD)
> >>  		return i40e_xmit_pkts_vec_avx2;
> >>  #endif
> >>  	return i40e_xmit_pkts_vec;
> >> @@ -3283,7 +3287,8 @@ i40e_get_recommend_tx_vec(void)
> >>  	 * use of AVX2 version to later plaforms, not all those that could
> >>  	 * theoretically run it.
> >>  	 */
> >> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
> >> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
> >> +			rte_get_max_simd_bitwidth() >=
> >RTE_MAX_256_SIMD)
> >>  		return i40e_xmit_pkts_vec_avx2;
> >>  #endif
> >>  	return i40e_xmit_pkts_vec;
> >> @@ -3311,7 +3316,9 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
> >>  	}
> >>
> >>  	if (ad->tx_simple_allowed) {
> >> -		if (ad->tx_vec_allowed) {
> >> +		if (ad->tx_vec_allowed &&
> >> +				rte_get_max_simd_bitwidth()
> >> +				>= RTE_MAX_128_SIMD) {
> >>  			PMD_INIT_LOG(DEBUG, "Vector tx finally be used.");
> >>  			if (ad->use_latest_vec)
> >>  				dev->tx_pkt_burst =
> >> --
> >> 2.17.1

Guo, Jia Oct. 9, 2020, 3:02 a.m. UTC | #4

Hi, power

> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Wednesday, September 30, 2020 9:04 PM
> To: dev@dpdk.org
> Cc: Power, Ciara <ciara.power@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
> Subject: [PATCH v3 04/18] net/i40e: 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: Beilei Xing <beilei.xing@intel.com>
> Cc: Jeff Guo <jia.guo@intel.com>
> 
> Signed-off-by: Ciara Power <ciara.power@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index
> 60b33d20a1..9b535b52fa 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -3098,7 +3098,8 @@ static eth_rx_burst_t  i40e_get_latest_rx_vec(bool
> scatter)  {  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> +			rte_get_max_simd_bitwidth() >=

Nitpick: I think if consistent to keep alignment for open parenthesis in this patch set would be better. Do you think so?

> RTE_MAX_256_SIMD)
>  		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
>  				 i40e_recv_pkts_vec_avx2;
>  #endif
> @@ -3115,7 +3116,8 @@ i40e_get_recommend_rx_vec(bool scatter)
>  	 * use of AVX2 version to later plaforms, not all those that could
>  	 * theoretically run it.
>  	 */
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
> +			rte_get_max_simd_bitwidth() >=
> RTE_MAX_256_SIMD)
>  		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
>  				 i40e_recv_pkts_vec_avx2;
>  #endif
> @@ -3154,7 +3156,8 @@ i40e_set_rx_function(struct rte_eth_dev *dev)
>  		}
>  	}
> 
> -	if (ad->rx_vec_allowed) {
> +	if (ad->rx_vec_allowed  && rte_get_max_simd_bitwidth()
> +			>= RTE_MAX_128_SIMD) {
>  		/* Vec Rx path */
>  		PMD_INIT_LOG(DEBUG, "Vector Rx path will be used on
> port=%d.",
>  				dev->data->port_id);
> @@ -3268,7 +3271,8 @@ static eth_tx_burst_t
>  i40e_get_latest_tx_vec(void)
>  {
>  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> +			rte_get_max_simd_bitwidth() >=
> RTE_MAX_256_SIMD)
>  		return i40e_xmit_pkts_vec_avx2;
>  #endif
>  	return i40e_xmit_pkts_vec;
> @@ -3283,7 +3287,8 @@ i40e_get_recommend_tx_vec(void)
>  	 * use of AVX2 version to later plaforms, not all those that could
>  	 * theoretically run it.
>  	 */
> -	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
> +			rte_get_max_simd_bitwidth() >=
> RTE_MAX_256_SIMD)
>  		return i40e_xmit_pkts_vec_avx2;
>  #endif
>  	return i40e_xmit_pkts_vec;
> @@ -3311,7 +3316,9 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
>  	}
> 
>  	if (ad->tx_simple_allowed) {
> -		if (ad->tx_vec_allowed) {
> +		if (ad->tx_vec_allowed &&
> +				rte_get_max_simd_bitwidth()
> +				>= RTE_MAX_128_SIMD) {
>  			PMD_INIT_LOG(DEBUG, "Vector tx finally be used.");
>  			if (ad->use_latest_vec)
>  				dev->tx_pkt_burst =
> --
> 2.17.1

Power, Ciara Oct. 9, 2020, 2:02 p.m. UTC | #5

Hi Jeff,

>-----Original Message-----
>From: Guo, Jia <jia.guo@intel.com>
>Sent: Friday 9 October 2020 04:03
>To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
>Cc: Xing, Beilei <beilei.xing@intel.com>
>Subject: RE: [PATCH v3 04/18] net/i40e: add checks for max SIMD bitwidth
>
>Hi, power
>
>> -----Original Message-----
>> From: Power, Ciara <ciara.power@intel.com>
>> Sent: Wednesday, September 30, 2020 9:04 PM
>> To: dev@dpdk.org
>> Cc: Power, Ciara <ciara.power@intel.com>; Xing, Beilei
>> <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
>> Subject: [PATCH v3 04/18] net/i40e: 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: Beilei Xing <beilei.xing@intel.com>
>> Cc: Jeff Guo <jia.guo@intel.com>
>>
>> Signed-off-by: Ciara Power <ciara.power@intel.com>
>> ---
>>  drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
>>  1 file changed, 13 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/i40e/i40e_rxtx.c
>> b/drivers/net/i40e/i40e_rxtx.c index 60b33d20a1..9b535b52fa 100644
>> --- a/drivers/net/i40e/i40e_rxtx.c
>> +++ b/drivers/net/i40e/i40e_rxtx.c
>> @@ -3098,7 +3098,8 @@ static eth_rx_burst_t
>> i40e_get_latest_rx_vec(bool
>> scatter)  {  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) -if
>> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
>> +if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
>> +rte_get_max_simd_bitwidth() >=
>
>Nitpick: I think if consistent to keep alignment for open parenthesis in this
>patch set would be better. Do you think so?
>

This file doesn't seem to have any if statements indented as you suggest, 
Some do have a double indent for the continued line as I have done here though.

<snip>

Thanks,
Ciara

Guo, Jia Oct. 10, 2020, 2:07 a.m. UTC | #6

Hi, power

> -----Original Message-----
> From: Power, Ciara <ciara.power@intel.com>
> Sent: Friday, October 9, 2020 10:03 PM
> To: Guo, Jia <jia.guo@intel.com>; dev@dpdk.org
> Cc: Xing, Beilei <beilei.xing@intel.com>
> Subject: RE: [PATCH v3 04/18] net/i40e: add checks for max SIMD bitwidth
> 
> Hi Jeff,
> 
> >-----Original Message-----
> >From: Guo, Jia <jia.guo@intel.com>
> >Sent: Friday 9 October 2020 04:03
> >To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> >Cc: Xing, Beilei <beilei.xing@intel.com>
> >Subject: RE: [PATCH v3 04/18] net/i40e: add checks for max SIMD
> >bitwidth
> >
> >Hi, power
> >
> >> -----Original Message-----
> >> From: Power, Ciara <ciara.power@intel.com>
> >> Sent: Wednesday, September 30, 2020 9:04 PM
> >> To: dev@dpdk.org
> >> Cc: Power, Ciara <ciara.power@intel.com>; Xing, Beilei
> >> <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
> >> Subject: [PATCH v3 04/18] net/i40e: 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: Beilei Xing <beilei.xing@intel.com>
> >> Cc: Jeff Guo <jia.guo@intel.com>
> >>
> >> Signed-off-by: Ciara Power <ciara.power@intel.com>
> >> ---
> >>  drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
> >>  1 file changed, 13 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/net/i40e/i40e_rxtx.c
> >> b/drivers/net/i40e/i40e_rxtx.c index 60b33d20a1..9b535b52fa 100644
> >> --- a/drivers/net/i40e/i40e_rxtx.c
> >> +++ b/drivers/net/i40e/i40e_rxtx.c
> >> @@ -3098,7 +3098,8 @@ static eth_rx_burst_t
> >> i40e_get_latest_rx_vec(bool
> >> scatter)  {  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
> >> -if
> >> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> >> +if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> >> +rte_get_max_simd_bitwidth() >=
> >
> >Nitpick: I think if consistent to keep alignment for open parenthesis
> >in this patch set would be better. Do you think so?
> >
> 
> This file doesn't seem to have any if statements indented as you suggest,
> Some do have a double indent for the continued line as I have done here
> though.
> 

Sorry, maybe I didn't say clear, what I said is the "CHECK" as below when use checkpatch.pl to guaranty the patch's format.

CHECK: Alignment should match open parenthesis
#733: FILE: drivers/net/i40e/i40e_rxtx.c:3102:
+       if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
+                       rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)

CHECK: Alignment should match open parenthesis
#743: FILE: drivers/net/i40e/i40e_rxtx.c:3120:
+       if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
+                       rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)

CHECK: Alignment should match open parenthesis
#763: FILE: drivers/net/i40e/i40e_rxtx.c:3275:
+       if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
+                       rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)

CHECK: Alignment should match open parenthesis
#773: FILE: drivers/net/i40e/i40e_rxtx.c:3291:
+       if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
+                       rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)

CHECK: Alignment should match open parenthesis
#783: FILE: drivers/net/i40e/i40e_rxtx.c:3320:
+               if (ad->tx_vec_allowed &&
+                               rte_get_max_simd_bitwidth()

> <snip>
> 
> Thanks,
> Ciara
>

Bruce Richardson Oct. 12, 2020, 9:37 a.m. UTC | #7

On Sat, Oct 10, 2020 at 02:07:15AM +0000, Guo, Jia wrote:
> Hi, power
> 
> > -----Original Message-----
> > From: Power, Ciara <ciara.power@intel.com>
> > Sent: Friday, October 9, 2020 10:03 PM
> > To: Guo, Jia <jia.guo@intel.com>; dev@dpdk.org
> > Cc: Xing, Beilei <beilei.xing@intel.com>
> > Subject: RE: [PATCH v3 04/18] net/i40e: add checks for max SIMD bitwidth
> > 
> > Hi Jeff,
> > 
> > >-----Original Message-----
> > >From: Guo, Jia <jia.guo@intel.com>
> > >Sent: Friday 9 October 2020 04:03
> > >To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> > >Cc: Xing, Beilei <beilei.xing@intel.com>
> > >Subject: RE: [PATCH v3 04/18] net/i40e: add checks for max SIMD
> > >bitwidth
> > >
> > >Hi, power
> > >
> > >> -----Original Message-----
> > >> From: Power, Ciara <ciara.power@intel.com>
> > >> Sent: Wednesday, September 30, 2020 9:04 PM
> > >> To: dev@dpdk.org
> > >> Cc: Power, Ciara <ciara.power@intel.com>; Xing, Beilei
> > >> <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
> > >> Subject: [PATCH v3 04/18] net/i40e: 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: Beilei Xing <beilei.xing@intel.com>
> > >> Cc: Jeff Guo <jia.guo@intel.com>
> > >>
> > >> Signed-off-by: Ciara Power <ciara.power@intel.com>
> > >> ---
> > >>  drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
> > >>  1 file changed, 13 insertions(+), 6 deletions(-)
> > >>
> > >> diff --git a/drivers/net/i40e/i40e_rxtx.c
> > >> b/drivers/net/i40e/i40e_rxtx.c index 60b33d20a1..9b535b52fa 100644
> > >> --- a/drivers/net/i40e/i40e_rxtx.c
> > >> +++ b/drivers/net/i40e/i40e_rxtx.c
> > >> @@ -3098,7 +3098,8 @@ static eth_rx_burst_t
> > >> i40e_get_latest_rx_vec(bool
> > >> scatter)  {  #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
> > >> -if
> > >> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> > >> +if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> > >> +rte_get_max_simd_bitwidth() >=
> > >
> > >Nitpick: I think if consistent to keep alignment for open parenthesis
> > >in this patch set would be better. Do you think so?
> > >
> > 
> > This file doesn't seem to have any if statements indented as you suggest,
> > Some do have a double indent for the continued line as I have done here
> > though.
> > 
> 
> Sorry, maybe I didn't say clear, what I said is the "CHECK" as below when use checkpatch.pl to guaranty the patch's format.
> 
> CHECK: Alignment should match open parenthesis
> #733: FILE: drivers/net/i40e/i40e_rxtx.c:3102:
> +       if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> +                       rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
> 
Did you run checkpatch using the DPDK "checkpatches.sh" script? In that
script there are a list of things to ignore, one of which is
"PARENTHESIS_ALIGNMENT", so that should not be flagged here. It's also not
flagged in patchwork by the CI system.

/Bruce

Guo, Jia Oct. 13, 2020, 2:15 a.m. UTC | #8

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Monday, October 12, 2020 5:38 PM
> To: Guo, Jia <jia.guo@intel.com>
> Cc: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org; Xing, Beilei
> <beilei.xing@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 04/18] net/i40e: add checks for max SIMD
> bitwidth
> 
> On Sat, Oct 10, 2020 at 02:07:15AM +0000, Guo, Jia wrote:
> > Hi, power
> >
> > > -----Original Message-----
> > > From: Power, Ciara <ciara.power@intel.com>
> > > Sent: Friday, October 9, 2020 10:03 PM
> > > To: Guo, Jia <jia.guo@intel.com>; dev@dpdk.org
> > > Cc: Xing, Beilei <beilei.xing@intel.com>
> > > Subject: RE: [PATCH v3 04/18] net/i40e: add checks for max SIMD
> > > bitwidth
> > >
> > > Hi Jeff,
> > >
> > > >-----Original Message-----
> > > >From: Guo, Jia <jia.guo@intel.com>
> > > >Sent: Friday 9 October 2020 04:03
> > > >To: Power, Ciara <ciara.power@intel.com>; dev@dpdk.org
> > > >Cc: Xing, Beilei <beilei.xing@intel.com>
> > > >Subject: RE: [PATCH v3 04/18] net/i40e: add checks for max SIMD
> > > >bitwidth
> > > >
> > > >Hi, power
> > > >
> > > >> -----Original Message-----
> > > >> From: Power, Ciara <ciara.power@intel.com>
> > > >> Sent: Wednesday, September 30, 2020 9:04 PM
> > > >> To: dev@dpdk.org
> > > >> Cc: Power, Ciara <ciara.power@intel.com>; Xing, Beilei
> > > >> <beilei.xing@intel.com>; Guo, Jia <jia.guo@intel.com>
> > > >> Subject: [PATCH v3 04/18] net/i40e: 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: Beilei Xing <beilei.xing@intel.com>
> > > >> Cc: Jeff Guo <jia.guo@intel.com>
> > > >>
> > > >> Signed-off-by: Ciara Power <ciara.power@intel.com>
> > > >> ---
> > > >>  drivers/net/i40e/i40e_rxtx.c | 19 +++++++++++++------
> > > >>  1 file changed, 13 insertions(+), 6 deletions(-)
> > > >>
> > > >> diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > >> b/drivers/net/i40e/i40e_rxtx.c index 60b33d20a1..9b535b52fa
> > > >> 100644
> > > >> --- a/drivers/net/i40e/i40e_rxtx.c
> > > >> +++ b/drivers/net/i40e/i40e_rxtx.c
> > > >> @@ -3098,7 +3098,8 @@ static eth_rx_burst_t
> > > >> i40e_get_latest_rx_vec(bool
> > > >> scatter)  {  #if defined(RTE_ARCH_X86) &&
> > > >> defined(CC_AVX2_SUPPORT) -if
> > > >> (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
> > > >> +if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> > > >> +rte_get_max_simd_bitwidth() >=
> > > >
> > > >Nitpick: I think if consistent to keep alignment for open
> > > >parenthesis in this patch set would be better. Do you think so?
> > > >
> > >
> > > This file doesn't seem to have any if statements indented as you
> > > suggest, Some do have a double indent for the continued line as I
> > > have done here though.
> > >
> >
> > Sorry, maybe I didn't say clear, what I said is the "CHECK" as below when
> use checkpatch.pl to guaranty the patch's format.
> >
> > CHECK: Alignment should match open parenthesis
> > #733: FILE: drivers/net/i40e/i40e_rxtx.c:3102:
> > +       if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
> > +                       rte_get_max_simd_bitwidth() >=
> > + RTE_MAX_256_SIMD)
> >
> Did you run checkpatch using the DPDK "checkpatches.sh" script? In that
> script there are a list of things to ignore, one of which is
> "PARENTHESIS_ALIGNMENT", so that should not be flagged here. It's also
> not flagged in patchwork by the CI system.
> 

Ok, seems that parenthesis alignment had been explicit ignored even I would prefer to make the format to be more consistent. @ power, you could choose keep it or not if there is a coming new version, that is both fine base on the rule.  

> /Bruce

diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 60b33d20a1..9b535b52fa 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -3098,7 +3098,8 @@  static eth_rx_burst_t
 i40e_get_latest_rx_vec(bool scatter)
 {
 #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
+			rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
 		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
 				 i40e_recv_pkts_vec_avx2;
 #endif
@@ -3115,7 +3116,8 @@  i40e_get_recommend_rx_vec(bool scatter)
 	 * use of AVX2 version to later plaforms, not all those that could
 	 * theoretically run it.
 	 */
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
+			rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
 		return scatter ? i40e_recv_scattered_pkts_vec_avx2 :
 				 i40e_recv_pkts_vec_avx2;
 #endif
@@ -3154,7 +3156,8 @@  i40e_set_rx_function(struct rte_eth_dev *dev)
 		}
 	}
 
-	if (ad->rx_vec_allowed) {
+	if (ad->rx_vec_allowed  && rte_get_max_simd_bitwidth()
+			>= RTE_MAX_128_SIMD) {
 		/* Vec Rx path */
 		PMD_INIT_LOG(DEBUG, "Vector Rx path will be used on port=%d.",
 				dev->data->port_id);
@@ -3268,7 +3271,8 @@  static eth_tx_burst_t
 i40e_get_latest_tx_vec(void)
 {
 #if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT)
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2))
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2) &&
+			rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
 		return i40e_xmit_pkts_vec_avx2;
 #endif
 	return i40e_xmit_pkts_vec;
@@ -3283,7 +3287,8 @@  i40e_get_recommend_tx_vec(void)
 	 * use of AVX2 version to later plaforms, not all those that could
 	 * theoretically run it.
 	 */
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F))
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX512F) &&
+			rte_get_max_simd_bitwidth() >= RTE_MAX_256_SIMD)
 		return i40e_xmit_pkts_vec_avx2;
 #endif
 	return i40e_xmit_pkts_vec;
@@ -3311,7 +3316,9 @@  i40e_set_tx_function(struct rte_eth_dev *dev)
 	}
 
 	if (ad->tx_simple_allowed) {
-		if (ad->tx_vec_allowed) {
+		if (ad->tx_vec_allowed &&
+				rte_get_max_simd_bitwidth()
+				>= RTE_MAX_128_SIMD) {
 			PMD_INIT_LOG(DEBUG, "Vector tx finally be used.");
 			if (ad->use_latest_vec)
 				dev->tx_pkt_burst =