[RFC PATCH] i40: remove weak version of i40e_rx_vec_dev_conf_condition_check()

[RFC PATCH] i40: remove weak version of i40e_rx_vec_dev_conf_condition_check()

[Zoltan Kiss]

Using weak symbols have a few issues with static linking:

- normally the linker searches the .o files already linked, if your weak
  one is there, it won't check if there is a strong version
- unless the symbol is directly referred, but it's not the right thing to
  rely on
- or --whole-archive specified in the command line, which pulls in a lot
  of unwanted stuff
- whole-archive also makes libtool dropping the library from the command
  line, which is what should happen with dynamic linking, but not with
  static (observed on Ubuntu 14.04). This is an important bug if you
  build a static library depending on DPDK

This patch merges the two version and make the behaviour rely on the
config.

If we can agree in the concept, I can send a series to fix this for the
other weak functions.

Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
---
 drivers/net/i40e/i40e_rxtx.c     | 36 +++++++++++++++++++++++++++++++++++-
 drivers/net/i40e/i40e_rxtx_vec.c | 36 ------------------------------------
 2 files changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index d3cfb98..ad34d3a 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -3278,10 +3278,44 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
 }
 
 /* Stubs needed for linkage when CONFIG_RTE_I40E_INC_VECTOR is set to 'n' */
-int __attribute__((weak))
+int __attribute__((cold))
 i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev __rte_unused *dev)
 {
+#ifndef RTE_LIBRTE_I40E_INC_VECTOR
 	return -1;
+#else
+#ifndef RTE_LIBRTE_IEEE1588
+	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
+
+	/* need SSE4.1 support */
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_1))
+		return -1;
+
+#ifndef RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE
+	/* whithout rx ol_flags, no VP flag report */
+	if (rxmode->hw_vlan_strip != 0 ||
+	    rxmode->hw_vlan_extend != 0)
+		return -1;
+#endif /* RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE */
+
+	/* no fdir support */
+	if (fconf->mode != RTE_FDIR_MODE_NONE)
+		return -1;
+
+	 /* - no csum error report support
+	 * - no header split support
+	 */
+	if (rxmode->hw_ip_checksum == 1 ||
+	    rxmode->header_split == 1)
+		return -1;
+
+	return 0;
+#else
+	RTE_SET_USED(dev);
+	return -1;
+#endif /* RTE_LIBRTE_IEEE1588 */
+#endif /* RTE_LIBRTE_I40E_INC_VECTOR */
 }
 
 uint16_t __attribute__((weak))
diff --git a/drivers/net/i40e/i40e_rxtx_vec.c b/drivers/net/i40e/i40e_rxtx_vec.c
index 05cb415..983b2c0 100644
--- a/drivers/net/i40e/i40e_rxtx_vec.c
+++ b/drivers/net/i40e/i40e_rxtx_vec.c
@@ -723,39 +723,3 @@ i40e_txq_vec_setup(struct i40e_tx_queue __rte_unused *txq)
 {
 	return 0;
 }
-
-int __attribute__((cold))
-i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev *dev)
-{
-#ifndef RTE_LIBRTE_IEEE1588
-	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
-	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
-
-	/* need SSE4.1 support */
-	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_1))
-		return -1;
-
-#ifndef RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE
-	/* whithout rx ol_flags, no VP flag report */
-	if (rxmode->hw_vlan_strip != 0 ||
-	    rxmode->hw_vlan_extend != 0)
-		return -1;
-#endif
-
-	/* no fdir support */
-	if (fconf->mode != RTE_FDIR_MODE_NONE)
-		return -1;
-
-	 /* - no csum error report support
-	 * - no header split support
-	 */
-	if (rxmode->hw_ip_checksum == 1 ||
-	    rxmode->header_split == 1)
-		return -1;
-
-	return 0;
-#else
-	RTE_SET_USED(dev);
-	return -1;
-#endif
-}
-- 
1.9.1

[PATCH v2] net/i40e: remove weak symbols

[Zoltan Kiss]

Using weak symbols have a few issues with static linking:

- normally the linker searches the .o files already linked, if your weak
  one is there, it won't check if there is a strong version
- unless the symbol is directly referred, but it's not the right thing to
  rely on
- or --whole-archive specified in the command line, which pulls in a lot
  of unwanted stuff
- whole-archive also makes libtool dropping the library from the command
  line, which is what should happen with dynamic linking, but not with
  static (observed on Ubuntu 14.04). This is an important bug if you
  build a static library depending on DPDK

This patch merges the two version and make the behaviour rely on the
config.

If we can agree in the concept, I can send a series to fix this for the
other weak functions.

Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
---

Notes:
    v2: fix commit message

 drivers/net/i40e/i40e_rxtx.c     | 36 +++++++++++++++++++++++++++++++++++-
 drivers/net/i40e/i40e_rxtx_vec.c | 36 ------------------------------------
 2 files changed, 35 insertions(+), 37 deletions(-)

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index d3cfb98..ad34d3a 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -3278,10 +3278,44 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
 }
 
 /* Stubs needed for linkage when CONFIG_RTE_I40E_INC_VECTOR is set to 'n' */
-int __attribute__((weak))
+int __attribute__((cold))
 i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev __rte_unused *dev)
 {
+#ifndef RTE_LIBRTE_I40E_INC_VECTOR
 	return -1;
+#else
+#ifndef RTE_LIBRTE_IEEE1588
+	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
+	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
+
+	/* need SSE4.1 support */
+	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_1))
+		return -1;
+
+#ifndef RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE
+	/* whithout rx ol_flags, no VP flag report */
+	if (rxmode->hw_vlan_strip != 0 ||
+	    rxmode->hw_vlan_extend != 0)
+		return -1;
+#endif /* RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE */
+
+	/* no fdir support */
+	if (fconf->mode != RTE_FDIR_MODE_NONE)
+		return -1;
+
+	 /* - no csum error report support
+	 * - no header split support
+	 */
+	if (rxmode->hw_ip_checksum == 1 ||
+	    rxmode->header_split == 1)
+		return -1;
+
+	return 0;
+#else
+	RTE_SET_USED(dev);
+	return -1;
+#endif /* RTE_LIBRTE_IEEE1588 */
+#endif /* RTE_LIBRTE_I40E_INC_VECTOR */
 }
 
 uint16_t __attribute__((weak))
diff --git a/drivers/net/i40e/i40e_rxtx_vec.c b/drivers/net/i40e/i40e_rxtx_vec.c
index 05cb415..983b2c0 100644
--- a/drivers/net/i40e/i40e_rxtx_vec.c
+++ b/drivers/net/i40e/i40e_rxtx_vec.c
@@ -723,39 +723,3 @@ i40e_txq_vec_setup(struct i40e_tx_queue __rte_unused *txq)
 {
 	return 0;
 }
-
-int __attribute__((cold))
-i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev *dev)
-{
-#ifndef RTE_LIBRTE_IEEE1588
-	struct rte_eth_rxmode *rxmode = &dev->data->dev_conf.rxmode;
-	struct rte_fdir_conf *fconf = &dev->data->dev_conf.fdir_conf;
-
-	/* need SSE4.1 support */
-	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_SSE4_1))
-		return -1;
-
-#ifndef RTE_LIBRTE_I40E_RX_OLFLAGS_ENABLE
-	/* whithout rx ol_flags, no VP flag report */
-	if (rxmode->hw_vlan_strip != 0 ||
-	    rxmode->hw_vlan_extend != 0)
-		return -1;
-#endif
-
-	/* no fdir support */
-	if (fconf->mode != RTE_FDIR_MODE_NONE)
-		return -1;
-
-	 /* - no csum error report support
-	 * - no header split support
-	 */
-	if (rxmode->hw_ip_checksum == 1 ||
-	    rxmode->header_split == 1)
-		return -1;
-
-	return 0;
-#else
-	RTE_SET_USED(dev);
-	return -1;
-#endif
-}
-- 
1.9.1

Re: [PATCH v2] net/i40e: remove weak symbols

[bynes adam]

On Wed, Jul 20, 2016 at 06:11:16PM +0100, Zoltan Kiss wrote:
Hi, Kiss
> Using weak symbols have a few issues with static linking:
> 
> - normally the linker searches the .o files already linked, if your weak
>   one is there, it won't check if there is a strong version
> - unless the symbol is directly referred, but it's not the right thing to
>   rely on
> - or --whole-archive specified in the command line, which pulls in a lot
>   of unwanted stuff
--whole-archive on the other hand can ensure all the object files are linked,
and the strong symbol will take precedence over weak symbol. So we don't need to
take care of the sequence of the object files in the ar or between ar.
> - whole-archive also makes libtool dropping the library from the command
>   line, which is what should happen with dynamic linking, but not with
>   static (observed on Ubuntu 14.04). This is an important bug if you
>   build a static library depending on DPDK
you mean the libtool bug for --whole-archive?
if it does, you shouldn't using the libtool, 
BTW what's the circumstance you must use the libtool. using you own makefile for
the library or APP.
> 
> This patch merges the two version and make the behaviour rely on the
> config.
> 
> If we can agree in the concept, I can send a series to fix this for the
> other weak functions.
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
> ---
> 
> Notes:
>     v2: fix commit message
> 
>  drivers/net/i40e/i40e_rxtx.c     | 36 +++++++++++++++++++++++++++++++++++-
>  drivers/net/i40e/i40e_rxtx_vec.c | 36 ------------------------------------
>  2 files changed, 35 insertions(+), 37 deletions(-)
> 
>From the original design, we follow the rule, we don't want the Macro in the file
to seperate the different Rx/Tx path for disabled/enabled vector configuration.
Adam Bynes

Re: [PATCH v2] net/i40e: remove weak symbols

[Zoltan Kiss]

On 21/07/16 19:58, bynes adam wrote:
> On Wed, Jul 20, 2016 at 06:11:16PM +0100, Zoltan Kiss wrote:
> Hi, Kiss
>> Using weak symbols have a few issues with static linking:
>>
>> - normally the linker searches the .o files already linked, if your weak
>>   one is there, it won't check if there is a strong version
>> - unless the symbol is directly referred, but it's not the right thing to
>>   rely on
>> - or --whole-archive specified in the command line, which pulls in a lot
>>   of unwanted stuff
> --whole-archive on the other hand can ensure all the object files are linked,
> and the strong symbol will take precedence over weak symbol. So we don't need to
> take care of the sequence of the object files in the ar or between ar.

--whole-archive is primarily for shared libraries, just as weak symbols. 
When people do static linking, their reason is often to avoid carrying 
around a big library while they only use a fraction of it.

>> - whole-archive also makes libtool dropping the library from the command
>>   line, which is what should happen with dynamic linking, but not with
>>   static (observed on Ubuntu 14.04). This is an important bug if you
>>   build a static library depending on DPDK
> you mean the libtool bug for --whole-archive?
> if it does, you shouldn't using the libtool,

GNU libtool is a quite common tool for building libraries, it's a bit 
painful sometimes, but it works. What else do you recommend? I mean, 
something which is proven to be better?

> BTW what's the circumstance you must use the libtool. using you own makefile for
> the library or APP.

Building libraries which depend on DPDK

>>
>> This patch merges the two version and make the behaviour rely on the
>> config.
>>
>> If we can agree in the concept, I can send a series to fix this for the
>> other weak functions.
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
>> ---
>>
>> Notes:
>>     v2: fix commit message
>>
>>  drivers/net/i40e/i40e_rxtx.c     | 36 +++++++++++++++++++++++++++++++++++-
>>  drivers/net/i40e/i40e_rxtx_vec.c | 36 ------------------------------------
>>  2 files changed, 35 insertions(+), 37 deletions(-)
>>
> From the original design, we follow the rule, we don't want the Macro in the file
> to seperate the different Rx/Tx path for disabled/enabled vector configuration.

And why is it better to compile two versions of the same function when 
you already know at the time of compilation which one do you want to use?

> Adam Bynes
>

Re: [PATCH v2] net/i40e: remove weak symbols

[Zoltan Kiss]

Hi,

I forgot to add the i40 maintainers as well. Would anyone like to weigh in?

Regards,

Zoltan

On 22/07/16 12:34, Zoltan Kiss wrote:
>
>
> On 21/07/16 19:58, bynes adam wrote:
>> On Wed, Jul 20, 2016 at 06:11:16PM +0100, Zoltan Kiss wrote:
>> Hi, Kiss
>>> Using weak symbols have a few issues with static linking:
>>>
>>> - normally the linker searches the .o files already linked, if your weak
>>>   one is there, it won't check if there is a strong version
>>> - unless the symbol is directly referred, but it's not the right
>>> thing to
>>>   rely on
>>> - or --whole-archive specified in the command line, which pulls in a lot
>>>   of unwanted stuff
>> --whole-archive on the other hand can ensure all the object files are
>> linked,
>> and the strong symbol will take precedence over weak symbol. So we
>> don't need to
>> take care of the sequence of the object files in the ar or between ar.
>
> --whole-archive is primarily for shared libraries, just as weak symbols.
> When people do static linking, their reason is often to avoid carrying
> around a big library while they only use a fraction of it.
>
>>> - whole-archive also makes libtool dropping the library from the command
>>>   line, which is what should happen with dynamic linking, but not with
>>>   static (observed on Ubuntu 14.04). This is an important bug if you
>>>   build a static library depending on DPDK
>> you mean the libtool bug for --whole-archive?
>> if it does, you shouldn't using the libtool,
>
> GNU libtool is a quite common tool for building libraries, it's a bit
> painful sometimes, but it works. What else do you recommend? I mean,
> something which is proven to be better?
>
>> BTW what's the circumstance you must use the libtool. using you own
>> makefile for
>> the library or APP.
>
> Building libraries which depend on DPDK
>
>>>
>>> This patch merges the two version and make the behaviour rely on the
>>> config.
>>>
>>> If we can agree in the concept, I can send a series to fix this for the
>>> other weak functions.
>>>
>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
>>> ---
>>>
>>> Notes:
>>>     v2: fix commit message
>>>
>>>  drivers/net/i40e/i40e_rxtx.c     | 36
>>> +++++++++++++++++++++++++++++++++++-
>>>  drivers/net/i40e/i40e_rxtx_vec.c | 36
>>> ------------------------------------
>>>  2 files changed, 35 insertions(+), 37 deletions(-)
>>>
>> From the original design, we follow the rule, we don't want the Macro
>> in the file
>> to seperate the different Rx/Tx path for disabled/enabled vector
>> configuration.
>
> And why is it better to compile two versions of the same function when
> you already know at the time of compilation which one do you want to use?
>
>> Adam Bynes
>>

Re: [PATCH v2] net/i40e: remove weak symbols

[Ferruh Yigit]

On 7/20/2016 6:11 PM, Zoltan Kiss wrote:
> Using weak symbols have a few issues with static linking:
> 
> - normally the linker searches the .o files already linked, if your weak
>   one is there, it won't check if there is a strong version
> - unless the symbol is directly referred, but it's not the right thing to
>   rely on
> - or --whole-archive specified in the command line, which pulls in a lot
>   of unwanted stuff
> - whole-archive also makes libtool dropping the library from the command
>   line, which is what should happen with dynamic linking, but not with
>   static (observed on Ubuntu 14.04). This is an important bug if you
>   build a static library depending on DPDK
> 
> This patch merges the two version and make the behaviour rely on the
> config.
> 
> If we can agree in the concept, I can send a series to fix this for the
> other weak functions.
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
> ---

+ i40e maintainers

I personally prefer macros against weak symbols, because dpdk can be
compiled as static and dynamic libraries, and weak symbols are not
working fine when compiled as static.

> 
> Notes:
>     v2: fix commit message
> 
>  drivers/net/i40e/i40e_rxtx.c     | 36 +++++++++++++++++++++++++++++++++++-
>  drivers/net/i40e/i40e_rxtx_vec.c | 36 ------------------------------------
>  2 files changed, 35 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index d3cfb98..ad34d3a 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -3278,10 +3278,44 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
>  }
> 
>  /* Stubs needed for linkage when CONFIG_RTE_I40E_INC_VECTOR is set to 'n' */
> -int __attribute__((weak))
> +int __attribute__((cold))
>  i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev __rte_unused *dev)
>  {
> +#ifndef RTE_LIBRTE_I40E_INC_VECTOR
>         return -1;
> +#else

This moves all vector implementation into non-vector file, this is bad
for maintaining. Why not:

in i40e_rxtx.c:
#ifndef RTE_LIBRTE_I40E_INC_VECTOR
i40e_rx_vec_dev_conf_condition_check()
{
	return -1;
}
#endif

in i40e_rxtx_vec.c:
No change, keep i40e_rx_vec_dev_conf_condition_check() as it is.

Thanks,
ferruh

Re: [PATCH v2] net/i40e: remove weak symbols

[Zoltan Kiss]

On 14/09/16 15:42, Ferruh Yigit wrote:
> On 7/20/2016 6:11 PM, Zoltan Kiss wrote:
>> Using weak symbols have a few issues with static linking:
>>
>> - normally the linker searches the .o files already linked, if your weak
>>    one is there, it won't check if there is a strong version
>> - unless the symbol is directly referred, but it's not the right thing to
>>    rely on
>> - or --whole-archive specified in the command line, which pulls in a lot
>>    of unwanted stuff
>> - whole-archive also makes libtool dropping the library from the command
>>    line, which is what should happen with dynamic linking, but not with
>>    static (observed on Ubuntu 14.04). This is an important bug if you
>>    build a static library depending on DPDK
>>
>> This patch merges the two version and make the behaviour rely on the
>> config.
>>
>> If we can agree in the concept, I can send a series to fix this for the
>> other weak functions.
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
>> ---
> + i40e maintainers
>
> I personally prefer macros against weak symbols, because dpdk can be
> compiled as static and dynamic libraries, and weak symbols are not
> working fine when compiled as static.
>
>> Notes:
>>      v2: fix commit message
>>
>>   drivers/net/i40e/i40e_rxtx.c     | 36 +++++++++++++++++++++++++++++++++++-
>>   drivers/net/i40e/i40e_rxtx_vec.c | 36 ------------------------------------
>>   2 files changed, 35 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
>> index d3cfb98..ad34d3a 100644
>> --- a/drivers/net/i40e/i40e_rxtx.c
>> +++ b/drivers/net/i40e/i40e_rxtx.c
>> @@ -3278,10 +3278,44 @@ i40e_set_tx_function(struct rte_eth_dev *dev)
>>   }
>>
>>   /* Stubs needed for linkage when CONFIG_RTE_I40E_INC_VECTOR is set to 'n' */
>> -int __attribute__((weak))
>> +int __attribute__((cold))
>>   i40e_rx_vec_dev_conf_condition_check(struct rte_eth_dev __rte_unused *dev)
>>   {
>> +#ifndef RTE_LIBRTE_I40E_INC_VECTOR
>>          return -1;
>> +#else
> This moves all vector implementation into non-vector file, this is bad
> for maintaining. Why not:
>
> in i40e_rxtx.c:
> #ifndef RTE_LIBRTE_I40E_INC_VECTOR
> i40e_rx_vec_dev_conf_condition_check()
> {
> 	return -1;
> }
> #endif
>
> in i40e_rxtx_vec.c:
> No change, keep i40e_rx_vec_dev_conf_condition_check() as it is.

Sounds good. But before I send a next version, I would like to hear from 
the maintainers if they are happy with the goal of this patch.

>
> Thanks,
> ferruh

Re: [PATCH v2] net/i40e: remove weak symbols

[Wu, Jingjing]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Zoltan Kiss
> Sent: Thursday, July 21, 2016 1:11 AM
> To: dev@dpdk.org
> Cc: Matias Elo <matias.elo@nokia-bell-labs.com>; Gonzalez Monroy, Sergio
> <sergio.gonzalez.monroy@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> damarion@cisco.com; thomas.monjalon@6wind.com
> Subject: [dpdk-dev] [PATCH v2] net/i40e: remove weak symbols
> 
> Using weak symbols have a few issues with static linking:
> 
> - normally the linker searches the .o files already linked, if your weak
>   one is there, it won't check if there is a strong version
> - unless the symbol is directly referred, but it's not the right thing to
>   rely on
> - or --whole-archive specified in the command line, which pulls in a lot
>   of unwanted stuff
> - whole-archive also makes libtool dropping the library from the command
>   line, which is what should happen with dynamic linking, but not with
>   static (observed on Ubuntu 14.04). This is an important bug if you
>   build a static library depending on DPDK
> 
> This patch merges the two version and make the behaviour rely on the
> config.
> 
> If we can agree in the concept, I can send a series to fix this for the
> other weak functions.
> 
> Signed-off-by: Zoltan Kiss <zoltan.kiss@schaman.hu>
> ---

Looks good to remove weak symbols.
Just one concern is vector PMD is one specific part to different Micro-Architecture.
It's better to keep independent from normal tx/rx files. Think about ixgbe_rxtx_vec_neon.c and ixgbe_rxtx_vec_sse.c


Thanks
Jingjing