diff mbox series

[1/2] net/i40e: fix generic build on FreeBSD

Message ID 20210818163816.19143-1-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Qi Zhang
Headers
Series [1/2] net/i40e: fix generic build on FreeBSD |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Bruce Richardson Aug. 18, 2021, 4:38 p.m. UTC 
  The common header file for vectorization is included in multiple files,
and so must use macros for the current compilation unit, rather than the
compiler-capability flag set for the whole driver. With the current,
incorrect, macro, the AVX512 or AVX2 flags may be set when compiling up
SSE code, leading to compilation errors. Changing from "CC_AVX*_SUPPORT"
to the compiler-defined "__AVX*__" macros fixes this issue.

Bugzilla ID: 788
Fixes: 0604b1f2208f ("net/i40e: fix crash in AVX512")
Cc: wenzhuo.lu@intel.com
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Bruce Richardson Aug. 18, 2021, 4:42 p.m. UTC | #1 
On Wed, Aug 18, 2021 at 05:38:15PM +0100, Bruce Richardson wrote:
> The common header file for vectorization is included in multiple files,
> and so must use macros for the current compilation unit, rather than the
> compiler-capability flag set for the whole driver. With the current,
> incorrect, macro, the AVX512 or AVX2 flags may be set when compiling up
> SSE code, leading to compilation errors. Changing from "CC_AVX*_SUPPORT"
> to the compiler-defined "__AVX*__" macros fixes this issue.
> 
> Bugzilla ID: 788
> Fixes: 0604b1f2208f ("net/i40e: fix crash in AVX512")
> Cc: wenzhuo.lu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index f52ed98d62..65715ed1ce 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -268,7 +268,7 @@ i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>  #endif
>  }
>  
> -#ifdef CC_AVX2_SUPPORT
> +#ifdef __AVX2__
>  static __rte_always_inline void
>  i40e_rxq_rearm_common(struct i40e_rx_queue *rxq, __rte_unused bool avx512)
>  {

On a higher-level, I'd suggest we look to remove the use of these macros
and AVX code in general from this header file (and ice driver equivalent).
IIRC this file was originally meant to contain only the "common" code i.e.
the scalar code, to be shared among vector implementations. Having AVX code
in this file can lead to these sorts of bugs and just makes the file no
longer truely common. The code in question here, should probably go in a
"common_avx" header, which means that we can remove the AVX2 conditions
from it etc.

/Bruce
Qi Zhang Sept. 1, 2021, 6:23 a.m. UTC | #2 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Bruce Richardson
> Sent: Thursday, August 19, 2021 12:38 AM
> To: dev@dpdk.org
> Cc: brian90013@gmail.com; Richardson, Bruce <bruce.richardson@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; stable@dpdk.org; Xing, Beilei
> <beilei.xing@intel.com>
> Subject: [dpdk-dev] [PATCH 1/2] net/i40e: fix generic build on FreeBSD
> 
> The common header file for vectorization is included in multiple files, and so
> must use macros for the current compilation unit, rather than the
> compiler-capability flag set for the whole driver. With the current, incorrect,
> macro, the AVX512 or AVX2 flags may be set when compiling up SSE code,
> leading to compilation errors. Changing from "CC_AVX*_SUPPORT"
> to the compiler-defined "__AVX*__" macros fixes this issue.
> 
> Bugzilla ID: 788
> Fixes: 0604b1f2208f ("net/i40e: fix crash in AVX512")
> Cc: wenzhuo.lu@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index f52ed98d62..65715ed1ce 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -268,7 +268,7 @@
> i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
> #endif  }
> 
> -#ifdef CC_AVX2_SUPPORT
> +#ifdef __AVX2__
>  static __rte_always_inline void
>  i40e_rxq_rearm_common(struct i40e_rx_queue *rxq, __rte_unused bool
> avx512)  { @@ -329,7 +329,7 @@ i40e_rxq_rearm_common(struct
> i40e_rx_queue *rxq, __rte_unused bool avx512)
>  		_mm_store_si128((__m128i *)&rxdp++->read, dma_addr1);
>  	}
>  #else
> -#ifdef CC_AVX512_SUPPORT
> +#ifdef __AVX512VL__
>  	if (avx512) {
>  		struct rte_mbuf *mb0, *mb1, *mb2, *mb3;
>  		struct rte_mbuf *mb4, *mb5, *mb6, *mb7;
> --
> 2.30.2

Applied to dpdk-next-net-intel.

Thanks
Qi
Ferruh Yigit Sept. 3, 2021, 10 a.m. UTC | #3 
On 8/18/2021 5:42 PM, Bruce Richardson wrote:
> On Wed, Aug 18, 2021 at 05:38:15PM +0100, Bruce Richardson wrote:
>> The common header file for vectorization is included in multiple files,
>> and so must use macros for the current compilation unit, rather than the
>> compiler-capability flag set for the whole driver. With the current,
>> incorrect, macro, the AVX512 or AVX2 flags may be set when compiling up
>> SSE code, leading to compilation errors. Changing from "CC_AVX*_SUPPORT"
>> to the compiler-defined "__AVX*__" macros fixes this issue.
>>
>> Bugzilla ID: 788
>> Fixes: 0604b1f2208f ("net/i40e: fix crash in AVX512")
>> Cc: wenzhuo.lu@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>> ---
>>  drivers/net/i40e/i40e_rxtx_vec_common.h | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
>> index f52ed98d62..65715ed1ce 100644
>> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
>> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
>> @@ -268,7 +268,7 @@ i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
>>  #endif
>>  }
>>  
>> -#ifdef CC_AVX2_SUPPORT
>> +#ifdef __AVX2__
>>  static __rte_always_inline void
>>  i40e_rxq_rearm_common(struct i40e_rx_queue *rxq, __rte_unused bool avx512)
>>  {
> 
> On a higher-level, I'd suggest we look to remove the use of these macros
> and AVX code in general from this header file (and ice driver equivalent).
> IIRC this file was originally meant to contain only the "common" code i.e.
> the scalar code, to be shared among vector implementations. Having AVX code
> in this file can lead to these sorts of bugs and just makes the file no
> longer truely common. The code in question here, should probably go in a
> "common_avx" header, which means that we can remove the AVX2 conditions
> from it etc.
> 

Indeed I come to this thread to make exact same comment, I think better to move
avx specific code into a "common_avx" header.
diff mbox series

Patch

diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h b/drivers/net/i40e/i40e_rxtx_vec_common.h
index f52ed98d62..65715ed1ce 100644
--- a/drivers/net/i40e/i40e_rxtx_vec_common.h
+++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
@@ -268,7 +268,7 @@  i40e_rx_vec_dev_conf_condition_check_default(struct rte_eth_dev *dev)
 #endif
 }
 
-#ifdef CC_AVX2_SUPPORT
+#ifdef __AVX2__
 static __rte_always_inline void
 i40e_rxq_rearm_common(struct i40e_rx_queue *rxq, __rte_unused bool avx512)
 {
@@ -329,7 +329,7 @@  i40e_rxq_rearm_common(struct i40e_rx_queue *rxq, __rte_unused bool avx512)
 		_mm_store_si128((__m128i *)&rxdp++->read, dma_addr1);
 	}
 #else
-#ifdef CC_AVX512_SUPPORT
+#ifdef __AVX512VL__
 	if (avx512) {
 		struct rte_mbuf *mb0, *mb1, *mb2, *mb3;
 		struct rte_mbuf *mb4, *mb5, *mb6, *mb7;