* [dpdk-dev] [PATCH] net/i40e: Eliminate weak symbols in i40e_rxtx.c @ 2019-05-15 13:05 David Harton 2019-05-15 14:13 ` Bruce Richardson 2019-05-15 16:13 ` [dpdk-dev] [PATCH v2] " David Harton 0 siblings, 2 replies; 11+ messages in thread From: David Harton @ 2019-05-15 13:05 UTC (permalink / raw) To: dev, beilei.xing, qi.z.zhang, bruce.richardson; +Cc: David Harton Use of weak symbols can hide makefile errors especially when custom makefiles are used. Removing the use of weak symbols to avoid a stub function being linked in production code. Signed-off-by: David Harton <dharton@cisco.com> --- drivers/net/i40e/Makefile | 1 + drivers/net/i40e/i40e_rxtx.c | 52 +++++++++++++++++++----------------- drivers/net/i40e/meson.build | 2 ++ 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/drivers/net/i40e/Makefile b/drivers/net/i40e/Makefile index 3f869a8d6..cbcfce099 100644 --- a/drivers/net/i40e/Makefile +++ b/drivers/net/i40e/Makefile @@ -106,6 +106,7 @@ endif ifeq ($(CC_AVX2_SUPPORT), 1) SRCS-$(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR) += i40e_rxtx_vec_avx2.c + CFLAGS_i40e_rxtx.o += -DCC_AVX2_SUPPORT endif # install this header file diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index 1489552da..cbf31ec88 100644 --- a/drivers/net/i40e/i40e_rxtx.c +++ b/drivers/net/i40e/i40e_rxtx.c @@ -3181,14 +3181,15 @@ i40e_set_default_pctype_table(struct rte_eth_dev *dev) } } +#ifndef RTE_LIBRTE_I40E_INC_VECTOR /* Stubs needed for linkage when CONFIG_RTE_LIBRTE_I40E_INC_VECTOR is set to 'n' */ -__rte_weak int +int i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev __rte_unused *dev) { return -1; } -__rte_weak uint16_t +uint16_t i40e_recv_pkts_vec( void __rte_unused *rx_queue, struct rte_mbuf __rte_unused **rx_pkts, @@ -3197,7 +3198,7 @@ i40e_recv_pkts_vec( return 0; } -__rte_weak uint16_t +uint16_t i40e_recv_scattered_pkts_vec( void __rte_unused *rx_queue, struct rte_mbuf __rte_unused **rx_pkts, @@ -3206,52 +3207,55 @@ i40e_recv_scattered_pkts_vec( return 0; } -__rte_weak uint16_t -i40e_recv_pkts_vec_avx2(void __rte_unused *rx_queue, - struct rte_mbuf __rte_unused **rx_pkts, - uint16_t __rte_unused nb_pkts) -{ - return 0; -} - -__rte_weak uint16_t -i40e_recv_scattered_pkts_vec_avx2(void __rte_unused *rx_queue, - struct rte_mbuf __rte_unused **rx_pkts, - uint16_t __rte_unused nb_pkts) -{ - return 0; -} - -__rte_weak int +int i40e_rxq_vec_setup(struct i40e_rx_queue __rte_unused *rxq) { return -1; } -__rte_weak int +int i40e_txq_vec_setup(struct i40e_tx_queue __rte_unused *txq) { return -1; } -__rte_weak void +void i40e_rx_queue_release_mbufs_vec(struct i40e_rx_queue __rte_unused*rxq) { return; } -__rte_weak uint16_t +uint16_t i40e_xmit_fixed_burst_vec(void __rte_unused * tx_queue, struct rte_mbuf __rte_unused **tx_pkts, uint16_t __rte_unused nb_pkts) { return 0; } +#endif /* ifndef RTE_LIBRTE_I40E_INC_VECTOR */ + +#ifndef CC_AVX2_SUPPORT +uint16_t +i40e_recv_pkts_vec_avx2(void __rte_unused *rx_queue, + struct rte_mbuf __rte_unused **rx_pkts, + uint16_t __rte_unused nb_pkts) +{ + return 0; +} -__rte_weak uint16_t +uint16_t +i40e_recv_scattered_pkts_vec_avx2(void __rte_unused *rx_queue, + struct rte_mbuf __rte_unused **rx_pkts, + uint16_t __rte_unused nb_pkts) +{ + return 0; +} + +uint16_t i40e_xmit_pkts_vec_avx2(void __rte_unused * tx_queue, struct rte_mbuf __rte_unused **tx_pkts, uint16_t __rte_unused nb_pkts) { return 0; } +#endif /* ifndef CC_AVX2_SUPPORT */ diff --git a/drivers/net/i40e/meson.build b/drivers/net/i40e/meson.build index d783f3626..dcbca39bf 100644 --- a/drivers/net/i40e/meson.build +++ b/drivers/net/i40e/meson.build @@ -35,8 +35,10 @@ if arch_subdir == 'x86' # a. we have AVX supported in minimum instruction set baseline # b. it's not minimum instruction set, but supported by compiler if dpdk_conf.has('RTE_MACHINE_CPUFLAG_AVX2') + cflags += ['-DCC_AVX2_SUPPORT'] sources += files('i40e_rxtx_vec_avx2.c') elif cc.has_argument('-mavx2') + cflags += ['-DCC_AVX2_SUPPORT'] i40e_avx2_lib = static_library('i40e_avx2_lib', 'i40e_rxtx_vec_avx2.c', dependencies: [static_rte_ethdev, -- 2.19.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-05-15 13:05 [dpdk-dev] [PATCH] net/i40e: Eliminate weak symbols in i40e_rxtx.c David Harton @ 2019-05-15 14:13 ` Bruce Richardson 2019-05-15 16:13 ` [dpdk-dev] [PATCH v2] " David Harton 1 sibling, 0 replies; 11+ messages in thread From: Bruce Richardson @ 2019-05-15 14:13 UTC (permalink / raw) To: David Harton; +Cc: dev, beilei.xing, qi.z.zhang On Wed, May 15, 2019 at 09:05:12AM -0400, David Harton wrote: > Use of weak symbols can hide makefile errors especially when > custom makefiles are used. Removing the use of weak symbols > to avoid a stub function being linked in production code. > > Signed-off-by: David Harton <dharton@cisco.com> > --- Looks reasonable to me. Reviewed-by: Bruce Richardson <bruce.richardson@intel.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-05-15 13:05 [dpdk-dev] [PATCH] net/i40e: Eliminate weak symbols in i40e_rxtx.c David Harton 2019-05-15 14:13 ` Bruce Richardson @ 2019-05-15 16:13 ` David Harton 2019-05-16 14:08 ` Bruce Richardson 2019-05-16 18:28 ` [dpdk-dev] [PATCH v3] " David Harton 1 sibling, 2 replies; 11+ messages in thread From: David Harton @ 2019-05-15 16:13 UTC (permalink / raw) To: dev, beilei.xing, qi.z.zhang, bruce.richardson; +Cc: David Harton Use of weak symbols can hide makefile errors especially when custom makefiles are used. Removing the use of weak symbols to avoid a stub function being linked in production code. Signed-off-by: David Harton <dharton@cisco.com> --- v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors drivers/net/i40e/Makefile | 1 + drivers/net/i40e/i40e_rxtx.c | 60 +++++++++++++++++++----------------- drivers/net/i40e/meson.build | 2 ++ 3 files changed, 35 insertions(+), 28 deletions(-) diff --git a/drivers/net/i40e/Makefile b/drivers/net/i40e/Makefile index 3f869a8d6..cbcfce099 100644 --- a/drivers/net/i40e/Makefile +++ b/drivers/net/i40e/Makefile @@ -106,6 +106,7 @@ endif ifeq ($(CC_AVX2_SUPPORT), 1) SRCS-$(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR) += i40e_rxtx_vec_avx2.c + CFLAGS_i40e_rxtx.o += -DCC_AVX2_SUPPORT endif # install this header file diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index 1489552da..984d322cd 100644 --- a/drivers/net/i40e/i40e_rxtx.c +++ b/drivers/net/i40e/i40e_rxtx.c @@ -2919,7 +2919,7 @@ i40e_txq_info_get(struct rte_eth_dev *dev, uint16_t queue_id, static eth_rx_burst_t i40e_get_latest_rx_vec(bool scatter) { -#ifdef RTE_ARCH_X86 +#if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2)) return scatter ? i40e_recv_scattered_pkts_vec_avx2 : i40e_recv_pkts_vec_avx2; @@ -2931,7 +2931,7 @@ i40e_get_latest_rx_vec(bool scatter) static eth_rx_burst_t i40e_get_recommend_rx_vec(bool scatter) { -#ifdef RTE_ARCH_X86 +#if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) /* * since AVX frequency can be different to base frequency, limit * use of AVX2 version to later plaforms, not all those that could @@ -3048,7 +3048,7 @@ i40e_set_tx_function_flag(struct rte_eth_dev *dev, struct i40e_tx_queue *txq) static eth_tx_burst_t i40e_get_latest_tx_vec(void) { -#ifdef RTE_ARCH_X86 +#if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2)) return i40e_xmit_pkts_vec_avx2; #endif @@ -3058,7 +3058,7 @@ i40e_get_latest_tx_vec(void) static eth_tx_burst_t i40e_get_recommend_tx_vec(void) { -#ifdef RTE_ARCH_X86 +#if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) /* * since AVX frequency can be different to base frequency, limit * use of AVX2 version to later plaforms, not all those that could @@ -3181,14 +3181,15 @@ i40e_set_default_pctype_table(struct rte_eth_dev *dev) } } +#ifndef RTE_LIBRTE_I40E_INC_VECTOR /* Stubs needed for linkage when CONFIG_RTE_LIBRTE_I40E_INC_VECTOR is set to 'n' */ -__rte_weak int +int i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev __rte_unused *dev) { return -1; } -__rte_weak uint16_t +uint16_t i40e_recv_pkts_vec( void __rte_unused *rx_queue, struct rte_mbuf __rte_unused **rx_pkts, @@ -3197,7 +3198,7 @@ i40e_recv_pkts_vec( return 0; } -__rte_weak uint16_t +uint16_t i40e_recv_scattered_pkts_vec( void __rte_unused *rx_queue, struct rte_mbuf __rte_unused **rx_pkts, @@ -3206,52 +3207,55 @@ i40e_recv_scattered_pkts_vec( return 0; } -__rte_weak uint16_t -i40e_recv_pkts_vec_avx2(void __rte_unused *rx_queue, - struct rte_mbuf __rte_unused **rx_pkts, - uint16_t __rte_unused nb_pkts) -{ - return 0; -} - -__rte_weak uint16_t -i40e_recv_scattered_pkts_vec_avx2(void __rte_unused *rx_queue, - struct rte_mbuf __rte_unused **rx_pkts, - uint16_t __rte_unused nb_pkts) -{ - return 0; -} - -__rte_weak int +int i40e_rxq_vec_setup(struct i40e_rx_queue __rte_unused *rxq) { return -1; } -__rte_weak int +int i40e_txq_vec_setup(struct i40e_tx_queue __rte_unused *txq) { return -1; } -__rte_weak void +void i40e_rx_queue_release_mbufs_vec(struct i40e_rx_queue __rte_unused*rxq) { return; } -__rte_weak uint16_t +uint16_t i40e_xmit_fixed_burst_vec(void __rte_unused * tx_queue, struct rte_mbuf __rte_unused **tx_pkts, uint16_t __rte_unused nb_pkts) { return 0; } +#endif /* ifndef RTE_LIBRTE_I40E_INC_VECTOR */ + +#ifndef CC_AVX2_SUPPORT +uint16_t +i40e_recv_pkts_vec_avx2(void __rte_unused *rx_queue, + struct rte_mbuf __rte_unused **rx_pkts, + uint16_t __rte_unused nb_pkts) +{ + return 0; +} -__rte_weak uint16_t +uint16_t +i40e_recv_scattered_pkts_vec_avx2(void __rte_unused *rx_queue, + struct rte_mbuf __rte_unused **rx_pkts, + uint16_t __rte_unused nb_pkts) +{ + return 0; +} + +uint16_t i40e_xmit_pkts_vec_avx2(void __rte_unused * tx_queue, struct rte_mbuf __rte_unused **tx_pkts, uint16_t __rte_unused nb_pkts) { return 0; } +#endif /* ifndef CC_AVX2_SUPPORT */ diff --git a/drivers/net/i40e/meson.build b/drivers/net/i40e/meson.build index d783f3626..dcbca39bf 100644 --- a/drivers/net/i40e/meson.build +++ b/drivers/net/i40e/meson.build @@ -35,8 +35,10 @@ if arch_subdir == 'x86' # a. we have AVX supported in minimum instruction set baseline # b. it's not minimum instruction set, but supported by compiler if dpdk_conf.has('RTE_MACHINE_CPUFLAG_AVX2') + cflags += ['-DCC_AVX2_SUPPORT'] sources += files('i40e_rxtx_vec_avx2.c') elif cc.has_argument('-mavx2') + cflags += ['-DCC_AVX2_SUPPORT'] i40e_avx2_lib = static_library('i40e_avx2_lib', 'i40e_rxtx_vec_avx2.c', dependencies: [static_rte_ethdev, -- 2.19.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-05-15 16:13 ` [dpdk-dev] [PATCH v2] " David Harton @ 2019-05-16 14:08 ` Bruce Richardson 2019-06-04 15:59 ` Ferruh Yigit 2019-05-16 18:28 ` [dpdk-dev] [PATCH v3] " David Harton 1 sibling, 1 reply; 11+ messages in thread From: Bruce Richardson @ 2019-05-16 14:08 UTC (permalink / raw) To: David Harton; +Cc: dev, beilei.xing, qi.z.zhang On Wed, May 15, 2019 at 12:13:46PM -0400, David Harton wrote: > Use of weak symbols can hide makefile errors especially when > custom makefiles are used. Removing the use of weak symbols > to avoid a stub function being linked in production code. > > Signed-off-by: David Harton <dharton@cisco.com> > --- > > v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors > Testing a few compiles here, this breaks when vector mode is disabled, because it's possible that CC_AVX2_SUPPORT=1 when VECTOR=n. I'd suggest adding "ifeq ($(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR),y) ... endif" around the block in the makefile checking for AVX2 support, so that we never set AVX2 unless we have vector support. With this change, you can include my ack in v3. /Bruce Acked-by: Bruce Richardson <bruce.richardson@intel.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-05-16 14:08 ` Bruce Richardson @ 2019-06-04 15:59 ` Ferruh Yigit 2019-06-04 16:19 ` David Harton (dharton) 2019-06-04 16:25 ` Bruce Richardson 0 siblings, 2 replies; 11+ messages in thread From: Ferruh Yigit @ 2019-06-04 15:59 UTC (permalink / raw) To: Bruce Richardson, David Harton; +Cc: dev, beilei.xing, qi.z.zhang On 5/16/2019 3:08 PM, Bruce Richardson wrote: > On Wed, May 15, 2019 at 12:13:46PM -0400, David Harton wrote: >> Use of weak symbols can hide makefile errors especially when >> custom makefiles are used. Removing the use of weak symbols >> to avoid a stub function being linked in production code. >> >> Signed-off-by: David Harton <dharton@cisco.com> >> --- >> >> v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors >> > Testing a few compiles here, this breaks when vector mode is disabled, > because it's possible that CC_AVX2_SUPPORT=1 when VECTOR=n. I'd suggest > adding "ifeq ($(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR),y) ... endif" around the > block in the makefile checking for AVX2 support, so that we never set AVX2 > unless we have vector support. Concern is this is pushing vectorization support more to compile time configuration. Do we really have to select if to use vector PMD or not in compile time? Can't we get rid of the 'CONFIG_RTE_LIBRTE_I40E_INC_VECTOR' config option completely? As done in the ICE driver now. Isn't it better to compile vectorization support in as much as possible and do the vector or scalar path selection in runtime, this patch may prevent us to do that, weak functions enables us being more dynamic. > > With this change, you can include my ack in v3. > > /Bruce > > Acked-by: Bruce Richardson <bruce.richardson@intel.com> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-06-04 15:59 ` Ferruh Yigit @ 2019-06-04 16:19 ` David Harton (dharton) 2019-06-06 9:07 ` Ferruh Yigit 2019-06-04 16:25 ` Bruce Richardson 1 sibling, 1 reply; 11+ messages in thread From: David Harton (dharton) @ 2019-06-04 16:19 UTC (permalink / raw) To: Ferruh Yigit, Bruce Richardson; +Cc: dev, beilei.xing, qi.z.zhang > -----Original Message----- > From: Ferruh Yigit <ferruh.yigit@intel.com> > Sent: Tuesday, June 04, 2019 12:00 PM > To: Bruce Richardson <bruce.richardson@intel.com>; David Harton (dharton) > <dharton@cisco.com> > Cc: dev@dpdk.org; beilei.xing@intel.com; qi.z.zhang@intel.com > Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in > i40e_rxtx.c > > On 5/16/2019 3:08 PM, Bruce Richardson wrote: > > On Wed, May 15, 2019 at 12:13:46PM -0400, David Harton wrote: > >> Use of weak symbols can hide makefile errors especially when custom > >> makefiles are used. Removing the use of weak symbols to avoid a stub > >> function being linked in production code. > >> > >> Signed-off-by: David Harton <dharton@cisco.com> > >> --- > >> > >> v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors > >> > > Testing a few compiles here, this breaks when vector mode is disabled, > > because it's possible that CC_AVX2_SUPPORT=1 when VECTOR=n. I'd > > suggest adding "ifeq ($(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR),y) ... > > endif" around the block in the makefile checking for AVX2 support, so > > that we never set AVX2 unless we have vector support. > > Concern is this is pushing vectorization support more to compile time > configuration. Do we really have to select if to use vector PMD or not in > compile time? > > Can't we get rid of the 'CONFIG_RTE_LIBRTE_I40E_INC_VECTOR' config option > completely? As done in the ICE driver now. > > Isn't it better to compile vectorization support in as much as possible > and do the vector or scalar path selection in runtime, this patch may > prevent us to do that, weak functions enables us being more dynamic. Choosing a vector dynamically can be done without the use of weak symbols. IMHO, weak symbols should really be used for cross library support where a user wants to override a 3rd party function not within a lib. In fact the weak symbol usage may preclude supporting all the variants you might need/want to select. > > > > > With this change, you can include my ack in v3. > > > > /Bruce > > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-06-04 16:19 ` David Harton (dharton) @ 2019-06-06 9:07 ` Ferruh Yigit 0 siblings, 0 replies; 11+ messages in thread From: Ferruh Yigit @ 2019-06-06 9:07 UTC (permalink / raw) To: David Harton (dharton), Bruce Richardson; +Cc: dev, beilei.xing, qi.z.zhang On 6/4/2019 5:19 PM, David Harton (dharton) wrote: > >> -----Original Message----- >> From: Ferruh Yigit <ferruh.yigit@intel.com> >> Sent: Tuesday, June 04, 2019 12:00 PM >> To: Bruce Richardson <bruce.richardson@intel.com>; David Harton (dharton) >> <dharton@cisco.com> >> Cc: dev@dpdk.org; beilei.xing@intel.com; qi.z.zhang@intel.com >> Subject: Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in >> i40e_rxtx.c >> >> On 5/16/2019 3:08 PM, Bruce Richardson wrote: >>> On Wed, May 15, 2019 at 12:13:46PM -0400, David Harton wrote: >>>> Use of weak symbols can hide makefile errors especially when custom >>>> makefiles are used. Removing the use of weak symbols to avoid a stub >>>> function being linked in production code. >>>> >>>> Signed-off-by: David Harton <dharton@cisco.com> >>>> --- >>>> >>>> v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors >>>> >>> Testing a few compiles here, this breaks when vector mode is disabled, >>> because it's possible that CC_AVX2_SUPPORT=1 when VECTOR=n. I'd >>> suggest adding "ifeq ($(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR),y) ... >>> endif" around the block in the makefile checking for AVX2 support, so >>> that we never set AVX2 unless we have vector support. >> >> Concern is this is pushing vectorization support more to compile time >> configuration. Do we really have to select if to use vector PMD or not in >> compile time? >> >> Can't we get rid of the 'CONFIG_RTE_LIBRTE_I40E_INC_VECTOR' config option >> completely? As done in the ICE driver now. >> >> Isn't it better to compile vectorization support in as much as possible >> and do the vector or scalar path selection in runtime, this patch may >> prevent us to do that, weak functions enables us being more dynamic. > > Choosing a vector dynamically can be done without the use of weak symbols. No objection then. > > IMHO, weak symbols should really be used for cross library support where a user wants to override a 3rd party function not within a lib. In fact the weak symbol usage may preclude supporting all the variants you might need/want to select. > >> >>> >>> With this change, you can include my ack in v3. >>> >>> /Bruce >>> >>> Acked-by: Bruce Richardson <bruce.richardson@intel.com> >>> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-06-04 15:59 ` Ferruh Yigit 2019-06-04 16:19 ` David Harton (dharton) @ 2019-06-04 16:25 ` Bruce Richardson 1 sibling, 0 replies; 11+ messages in thread From: Bruce Richardson @ 2019-06-04 16:25 UTC (permalink / raw) To: Ferruh Yigit; +Cc: David Harton, dev, beilei.xing, qi.z.zhang On Tue, Jun 04, 2019 at 04:59:47PM +0100, Ferruh Yigit wrote: > On 5/16/2019 3:08 PM, Bruce Richardson wrote: > > On Wed, May 15, 2019 at 12:13:46PM -0400, David Harton wrote: > >> Use of weak symbols can hide makefile errors especially when > >> custom makefiles are used. Removing the use of weak symbols > >> to avoid a stub function being linked in production code. > >> > >> Signed-off-by: David Harton <dharton@cisco.com> > >> --- > >> > >> v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors > >> > > Testing a few compiles here, this breaks when vector mode is disabled, > > because it's possible that CC_AVX2_SUPPORT=1 when VECTOR=n. I'd suggest > > adding "ifeq ($(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR),y) ... endif" around the > > block in the makefile checking for AVX2 support, so that we never set AVX2 > > unless we have vector support. > > Concern is this is pushing vectorization support more to compile time > configuration. Do we really have to select if to use vector PMD or not in > compile time? > > Can't we get rid of the 'CONFIG_RTE_LIBRTE_I40E_INC_VECTOR' config option > completely? As done in the ICE driver now. > > Isn't it better to compile vectorization support in as much as possible and do > the vector or scalar path selection in runtime, this patch may prevent us to do > that, weak functions enables us being more dynamic. > Weak functions are not needed to do the runtime selection - they are needed for compilation only. They have the downside of potentially causing runtime problems due to a mis-configured compile, which is only seen later by the end user. By using real functions rather than weak functions it means that any mischosen compile paths will flag a compile error rather than silently succeeding and then accidentally using an incorrect function at runtime. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [dpdk-dev] [PATCH v3] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-05-15 16:13 ` [dpdk-dev] [PATCH v2] " David Harton 2019-05-16 14:08 ` Bruce Richardson @ 2019-05-16 18:28 ` David Harton 2019-05-17 10:21 ` Bruce Richardson 1 sibling, 1 reply; 11+ messages in thread From: David Harton @ 2019-05-16 18:28 UTC (permalink / raw) To: dev; +Cc: beilei.xing, qi.z.zhang, bruce.richardson, David Harton Use of weak symbols can hide makefile errors especially when custom makefiles are used. Removing the use of weak symbols to avoid a stub function being linked in production code. Signed-off-by: David Harton <dharton@cisco.com> --- v3 - added CONFIG_RTE_LIBRTE_I40E_INC_VECTOR to makefile v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors drivers/net/i40e/Makefile | 3 ++ drivers/net/i40e/i40e_rxtx.c | 60 +++++++++++++++++++----------------- drivers/net/i40e/meson.build | 2 ++ 3 files changed, 37 insertions(+), 28 deletions(-) diff --git a/drivers/net/i40e/Makefile b/drivers/net/i40e/Makefile index 3f869a8d6..1c3bbe329 100644 --- a/drivers/net/i40e/Makefile +++ b/drivers/net/i40e/Makefile @@ -89,6 +89,7 @@ SRCS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += rte_pmd_i40e.c SRCS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e_tm.c SRCS-$(CONFIG_RTE_LIBRTE_I40E_PMD) += i40e_vf_representor.c +ifeq ($(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR),y) ifeq ($(findstring RTE_MACHINE_CPUFLAG_AVX2,$(CFLAGS)),RTE_MACHINE_CPUFLAG_AVX2) CC_AVX2_SUPPORT=1 else @@ -103,9 +104,11 @@ else endif endif endif +endif ifeq ($(CC_AVX2_SUPPORT), 1) SRCS-$(CONFIG_RTE_LIBRTE_I40E_INC_VECTOR) += i40e_rxtx_vec_avx2.c + CFLAGS_i40e_rxtx.o += -DCC_AVX2_SUPPORT endif # install this header file diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c index 1489552da..984d322cd 100644 --- a/drivers/net/i40e/i40e_rxtx.c +++ b/drivers/net/i40e/i40e_rxtx.c @@ -2919,7 +2919,7 @@ i40e_txq_info_get(struct rte_eth_dev *dev, uint16_t queue_id, static eth_rx_burst_t i40e_get_latest_rx_vec(bool scatter) { -#ifdef RTE_ARCH_X86 +#if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2)) return scatter ? i40e_recv_scattered_pkts_vec_avx2 : i40e_recv_pkts_vec_avx2; @@ -2931,7 +2931,7 @@ i40e_get_latest_rx_vec(bool scatter) static eth_rx_burst_t i40e_get_recommend_rx_vec(bool scatter) { -#ifdef RTE_ARCH_X86 +#if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) /* * since AVX frequency can be different to base frequency, limit * use of AVX2 version to later plaforms, not all those that could @@ -3048,7 +3048,7 @@ i40e_set_tx_function_flag(struct rte_eth_dev *dev, struct i40e_tx_queue *txq) static eth_tx_burst_t i40e_get_latest_tx_vec(void) { -#ifdef RTE_ARCH_X86 +#if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_AVX2)) return i40e_xmit_pkts_vec_avx2; #endif @@ -3058,7 +3058,7 @@ i40e_get_latest_tx_vec(void) static eth_tx_burst_t i40e_get_recommend_tx_vec(void) { -#ifdef RTE_ARCH_X86 +#if defined(RTE_ARCH_X86) && defined(CC_AVX2_SUPPORT) /* * since AVX frequency can be different to base frequency, limit * use of AVX2 version to later plaforms, not all those that could @@ -3181,14 +3181,15 @@ i40e_set_default_pctype_table(struct rte_eth_dev *dev) } } +#ifndef RTE_LIBRTE_I40E_INC_VECTOR /* Stubs needed for linkage when CONFIG_RTE_LIBRTE_I40E_INC_VECTOR is set to 'n' */ -__rte_weak int +int i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev __rte_unused *dev) { return -1; } -__rte_weak uint16_t +uint16_t i40e_recv_pkts_vec( void __rte_unused *rx_queue, struct rte_mbuf __rte_unused **rx_pkts, @@ -3197,7 +3198,7 @@ i40e_recv_pkts_vec( return 0; } -__rte_weak uint16_t +uint16_t i40e_recv_scattered_pkts_vec( void __rte_unused *rx_queue, struct rte_mbuf __rte_unused **rx_pkts, @@ -3206,52 +3207,55 @@ i40e_recv_scattered_pkts_vec( return 0; } -__rte_weak uint16_t -i40e_recv_pkts_vec_avx2(void __rte_unused *rx_queue, - struct rte_mbuf __rte_unused **rx_pkts, - uint16_t __rte_unused nb_pkts) -{ - return 0; -} - -__rte_weak uint16_t -i40e_recv_scattered_pkts_vec_avx2(void __rte_unused *rx_queue, - struct rte_mbuf __rte_unused **rx_pkts, - uint16_t __rte_unused nb_pkts) -{ - return 0; -} - -__rte_weak int +int i40e_rxq_vec_setup(struct i40e_rx_queue __rte_unused *rxq) { return -1; } -__rte_weak int +int i40e_txq_vec_setup(struct i40e_tx_queue __rte_unused *txq) { return -1; } -__rte_weak void +void i40e_rx_queue_release_mbufs_vec(struct i40e_rx_queue __rte_unused*rxq) { return; } -__rte_weak uint16_t +uint16_t i40e_xmit_fixed_burst_vec(void __rte_unused * tx_queue, struct rte_mbuf __rte_unused **tx_pkts, uint16_t __rte_unused nb_pkts) { return 0; } +#endif /* ifndef RTE_LIBRTE_I40E_INC_VECTOR */ + +#ifndef CC_AVX2_SUPPORT +uint16_t +i40e_recv_pkts_vec_avx2(void __rte_unused *rx_queue, + struct rte_mbuf __rte_unused **rx_pkts, + uint16_t __rte_unused nb_pkts) +{ + return 0; +} -__rte_weak uint16_t +uint16_t +i40e_recv_scattered_pkts_vec_avx2(void __rte_unused *rx_queue, + struct rte_mbuf __rte_unused **rx_pkts, + uint16_t __rte_unused nb_pkts) +{ + return 0; +} + +uint16_t i40e_xmit_pkts_vec_avx2(void __rte_unused * tx_queue, struct rte_mbuf __rte_unused **tx_pkts, uint16_t __rte_unused nb_pkts) { return 0; } +#endif /* ifndef CC_AVX2_SUPPORT */ diff --git a/drivers/net/i40e/meson.build b/drivers/net/i40e/meson.build index d783f3626..dcbca39bf 100644 --- a/drivers/net/i40e/meson.build +++ b/drivers/net/i40e/meson.build @@ -35,8 +35,10 @@ if arch_subdir == 'x86' # a. we have AVX supported in minimum instruction set baseline # b. it's not minimum instruction set, but supported by compiler if dpdk_conf.has('RTE_MACHINE_CPUFLAG_AVX2') + cflags += ['-DCC_AVX2_SUPPORT'] sources += files('i40e_rxtx_vec_avx2.c') elif cc.has_argument('-mavx2') + cflags += ['-DCC_AVX2_SUPPORT'] i40e_avx2_lib = static_library('i40e_avx2_lib', 'i40e_rxtx_vec_avx2.c', dependencies: [static_rte_ethdev, -- 2.19.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-05-16 18:28 ` [dpdk-dev] [PATCH v3] " David Harton @ 2019-05-17 10:21 ` Bruce Richardson 2019-05-29 17:18 ` Zhang, Qi Z 0 siblings, 1 reply; 11+ messages in thread From: Bruce Richardson @ 2019-05-17 10:21 UTC (permalink / raw) To: David Harton; +Cc: dev, beilei.xing, qi.z.zhang On Thu, May 16, 2019 at 02:28:03PM -0400, David Harton wrote: > Use of weak symbols can hide makefile errors especially when > custom makefiles are used. Removing the use of weak symbols > to avoid a stub function being linked in production code. > > Signed-off-by: David Harton <dharton@cisco.com> > --- > > v3 - added CONFIG_RTE_LIBRTE_I40E_INC_VECTOR to makefile > v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors > Acked-by: Bruce Richardson <bruce.richardson@intel.com> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [dpdk-dev] [PATCH v3] net/i40e: Eliminate weak symbols in i40e_rxtx.c 2019-05-17 10:21 ` Bruce Richardson @ 2019-05-29 17:18 ` Zhang, Qi Z 0 siblings, 0 replies; 11+ messages in thread From: Zhang, Qi Z @ 2019-05-29 17:18 UTC (permalink / raw) To: Richardson, Bruce, David Harton; +Cc: dev, Xing, Beilei > -----Original Message----- > From: Richardson, Bruce > Sent: Friday, May 17, 2019 3:21 AM > To: David Harton <dharton@cisco.com> > Cc: dev@dpdk.org; Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z > <qi.z.zhang@intel.com> > Subject: Re: [PATCH v3] net/i40e: Eliminate weak symbols in i40e_rxtx.c > > On Thu, May 16, 2019 at 02:28:03PM -0400, David Harton wrote: > > Use of weak symbols can hide makefile errors especially when custom > > makefiles are used. Removing the use of weak symbols to avoid a stub > > function being linked in production code. > > > > Signed-off-by: David Harton <dharton@cisco.com> > > --- > > > > v3 - added CONFIG_RTE_LIBRTE_I40E_INC_VECTOR to makefile > > v2 - added CC_AVX2_SUPPORT check to code enabling avx2 vectors > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com> Applied to dpdk-next-net-intel. Thanks Qi ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-06-06 9:07 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-15 13:05 [dpdk-dev] [PATCH] net/i40e: Eliminate weak symbols in i40e_rxtx.c David Harton 2019-05-15 14:13 ` Bruce Richardson 2019-05-15 16:13 ` [dpdk-dev] [PATCH v2] " David Harton 2019-05-16 14:08 ` Bruce Richardson 2019-06-04 15:59 ` Ferruh Yigit 2019-06-04 16:19 ` David Harton (dharton) 2019-06-06 9:07 ` Ferruh Yigit 2019-06-04 16:25 ` Bruce Richardson 2019-05-16 18:28 ` [dpdk-dev] [PATCH v3] " David Harton 2019-05-17 10:21 ` Bruce Richardson 2019-05-29 17:18 ` Zhang, Qi Z
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.