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

On 14/09/16 15:42, Ferruh Yigit wrote:

On 7/20/2016 6:11 PM, Zoltan Kiss wrote:

quoted

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

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

quoted

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