Re: [dpdk-dev] [PATCH v2] net/i40e: Eliminate weak symbols in i40e_rxtx.c

[Ferruh Yigit <ferruh.yigit@intel.com>]

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>
>>>
>