[v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c
Checks
Commit Message
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(-)
Comments
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>
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>
>
> -----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>
> >
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.
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>
>>>
>
@@ -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
@@ -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 */
@@ -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,