diff mbox

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

Message ID 20170104110144.GA8258@localhost.localdomain (mailing list archive)
State Not Applicable, archived
Headers

Checks

Context Check Description
ci/Intel compilation success Compilation OK

Commit Message

Jerin Jacob Jan. 4, 2017, 11:01 a.m. UTC 
  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.

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.

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


[dpdk-master] $ git diff
 
> 
> > 
> > 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
> > 
>

Comments

Ferruh Yigit Jan. 4, 2017, 11:44 a.m. UTC | #1 
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
>>>
>>
Jerin Jacob Jan. 4, 2017, 12:59 p.m. UTC | #2 
On Wed, Jan 04, 2017 at 11:44:21AM +0000, Ferruh Yigit wrote:
> 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.
> > ----
> > 
> >>
> >> 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.

OK. I will post the v2 based on following patch then.

> 
> > 
> > 
> > [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
> >>>
> >>
>
diff mbox

Patch

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