Re: [dpdk-dev] [PATCH 2/2] app/testpmd: remove explicit ixgbe link request

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

On 1/4/2017 11:01 AM, Jerin Jacob wrote:
> On Tue, Jan 03, 2017 at 01:30:26PM +0000, Ferruh Yigit wrote:
>> On 12/27/2016 10:09 AM, Jerin Jacob wrote:
>>> Removed explicit ixgbe driver linkage request from
>>> app/testpmd makefile to mk/rte.app.mk to
>>> 1)Maintain the correct link ordering(from higher level libraries
>>> to lower level libraries)
>>> 2)In shared lib configuration, any application can use ixgbe
>>> exposed pmd specific APIs not just testpmd.
>>
>> In testpmd, "explicit ixgbe driver linkage request" added because
>> testpmd uses ixgbe PMD specific APIs.
>>
>> Overall, that line is for shared library, for static library result
>> should be same.
> 
> Unfortunately for the static library, the result is different.You can
> check the testpmd.map file for the effects linking rte_pmd_ixgbe first.

This was unintentional/unexpected, thanks for catching this.

> 
> I found this while debugging the a strange issue where including ixgbe
> build is dropping 300kpps/core on thunderx pmd.Finally, git bisected and saw
> the following check-in causes this
> 
> commit 425781ff5afe08b77c58ec5e4d5cf56b9ac19e02
> Author: Bernard Iremonger <bernard.iremonger@intel.com>
> Date:   Wed Oct 12 18:54:12 2016 +0100
> 
>     app/testpmd: add ixgbe VF management
> 
> Nothing wrong in this check-in wrt functionality, but moving rte_pmd_ixgbe
> to first is completely changing the symbol generation for the static build.

I can guess the reason.
Now "-Wl,-lrte_pmd_ixgbe" provided twice, and build system removes the
duplication by keeping only first occurrence.
This moves "-Wl,-lrte_pmd_ixgbe" out of "-Wl,--whole-archive" flag.
Since PMD API not directly called, but register themselves via
constructors, and initialized in runtime using registered callbacks,
without "--whole-archive" some APIs from that PMD may be missing in
final application.

> 
> ----
> gcc -o testpmd -m64 -pthread  -march=native -DRTE_MACHINE_CPUFLAG_SSE
> 
> [snip]
> 
> -Wcast-qual -Wformat-nonliteral -Wformat-security -Wundef
> -Wwrite-strings -Werror testpmd.o parameters.o cmdline.o cmdline_flow.o
> config.o iofwd.o macfwd.o macswap.o flowgen.o rxonly.o txonly.o
> csumonly.o icmpecho.o -Wl,-lrte_pmd_ixgbe
>                        ^^^^^^^^^^^^^^^^^^^^^^^^^
> -L/export/dpdk-master/build/lib -Wl,-lrte_kni -Wl,-lrte_pipeline
> -Wl,-lrte_table -Wl,-lrte_port -Wl,-lrte_pdump -Wl,-lrte_distributor
> -Wl,-lrte_reorder -Wl,-lrte_ip_frag -Wl,-lrte_meter -Wl,-lrte_sched
> -Wl,-lrte_lpm -Wl,--whole-archive -Wl,-lrte_acl -Wl,--no-whole-archive
> -Wl,-lrte_jobstats -Wl,-lrte_power -Wl,--whole-archive -Wl,-lrte_timer
> -Wl,-lrte_hash -Wl,-lrte_vhost -Wl,-lrte_kvargs -Wl,-lrte_mbuf
> -Wl,-lrte_net -Wl,-lrte_ethdev -Wl,-lrte_cryptodev -Wl,-lrte_eventdev
> -Wl,-lrte_mempool -Wl,-lrte_ring -Wl,-lrte_eal -Wl,-lrte_cmdline
> -Wl,-lrte_cfgfile -Wl,-lrte_pmd_bond -Wl,-lrte_pmd_af_packet
> -Wl,-lrte_pmd_bnxt -Wl,-lrte_pmd_cxgbe -Wl,-lrte_pmd_e1000
> -Wl,-lrte_pmd_ena -Wl,-lrte_pmd_enic -Wl,-lrte_pmd_fm10k
> -Wl,-lrte_pmd_i40e -Wl,-lrte_pmd_null -Wl,-lrte_pmd_qede
> -Wl,-lrte_pmd_ring -Wl,-lrte_pmd_virtio -Wl,-lrte_pmd_vhost
> -Wl,-lrte_pmd_vmxnet3_uio -Wl,-lrte_pmd_null_crypto
> -Wl,-lrte_pmd_skeleton_event -Wl,--no-whole-archive -Wl,-lrt -Wl,-lm
> -Wl,-ldl -Wl,-export-dynamic -Wl,-export-dynamic -Wl,-export-dynamic
> -L/export/dpdk-master/build/lib -Wl,--as-needed -Wl,-Map=testpmd.map
> -Wl,--cref 
> ----
> 
>>
>> I believe it is good to keep it in testpmd Makefile, updating rte.app.mk
>> to have it will:
>> - link library to the applications which does not use PMD specific APIs
>> and want to load PMD dynamically.
>> - link library to the application that won't use driver at all. This may
>> break the distributed binaries, since testpmd will now be dependent to a
>> specific PMD.
> 
> No strong opinion here as it is specific to ixgbe. But can we include
> ixgbe only for shared library in testpmd so that it won't effect any
> symbol generation in static build.

I think this is better, I am OK with below patch, thanks.

> 
> 
> [dpdk-master] $ git diff
> diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
> index 5988c3e..050663a 100644
> --- a/app/test-pmd/Makefile
> +++ b/app/test-pmd/Makefile
> @@ -59,7 +59,9 @@ SRCS-y += csumonly.c
>  SRCS-y += icmpecho.c
>  SRCS-$(CONFIG_RTE_LIBRTE_IEEE1588) += ieee1588fwd.c
>  
> +ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),y)
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
> +endif
>  
>  CFLAGS_cmdline.o := -D_GNU_SOURCE
> 
>  
>>
>>>
>>> Signed-off-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
>>> ---
>>>  app/test-pmd/Makefile | 2 --
>>>  mk/rte.app.mk         | 2 +-
>>>  2 files changed, 1 insertion(+), 3 deletions(-)
>>>
>>> diff --git a/app/test-pmd/Makefile b/app/test-pmd/Makefile
>>> index 5988c3e..96e0c67 100644
>>> --- a/app/test-pmd/Makefile
>>> +++ b/app/test-pmd/Makefile
>>> @@ -59,8 +59,6 @@ SRCS-y += csumonly.c
>>>  SRCS-y += icmpecho.c
>>>  SRCS-$(CONFIG_RTE_LIBRTE_IEEE1588) += ieee1588fwd.c
>>>  
>>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD) += -lrte_pmd_ixgbe
>>> -
>>>  CFLAGS_cmdline.o := -D_GNU_SOURCE
>>>  
>>>  # this application needs libraries first
>>> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
>>> index f75f0e2..aee235c 100644
>>> --- a/mk/rte.app.mk
>>> +++ b/mk/rte.app.mk
>>> @@ -101,6 +101,7 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE)        += -lrte_cfgfile
>>>  
>>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_BOND)       += -lrte_pmd_bond
>>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT)    += -lrte_pmd_xenvirt -lxenstore
>>> +_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD)      += -lrte_pmd_ixgbe
>>>  
>>>  ifeq ($(CONFIG_RTE_BUILD_SHARED_LIB),n)
>>>  # plugins (link only if static libraries)
>>> @@ -114,7 +115,6 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_ENA_PMD)        += -lrte_pmd_ena
>>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_ENIC_PMD)       += -lrte_pmd_enic
>>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_FM10K_PMD)      += -lrte_pmd_fm10k
>>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_I40E_PMD)       += -lrte_pmd_i40e
>>> -_LDLIBS-$(CONFIG_RTE_LIBRTE_IXGBE_PMD)      += -lrte_pmd_ixgbe
>>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX4_PMD)       += -lrte_pmd_mlx4 -libverbs
>>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_MLX5_PMD)       += -lrte_pmd_mlx5 -libverbs
>>>  _LDLIBS-$(CONFIG_RTE_LIBRTE_MPIPE_PMD)      += -lrte_pmd_mpipe -lgxio
>>>
>>