[dpdk-dev,1/8] app: link the whole rte_cfgfile library

Message ID 1506418805-12117-2-git-send-email-tdu@semihalf.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Tomasz Duszynski Sept. 26, 2017, 9:39 a.m. UTC
  Since MRVL NET PMD needs librte_cfgfile to parse QoS configuration file
link it as the whole library.

Signed-off-by: Jacek Siuda <jck@semihalf.com>
Signed-off-by: Tomasz Duszynski <tdu@semihalf.com>
---
 mk/rte.app.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Comments

Bruce Richardson Sept. 26, 2017, 2:31 p.m. UTC | #1
On Tue, Sep 26, 2017 at 11:39:58AM +0200, Tomasz Duszynski wrote:
> Since MRVL NET PMD needs librte_cfgfile to parse QoS configuration file
> link it as the whole library.
> 
> Signed-off-by: Jacek Siuda <jck@semihalf.com>
> Signed-off-by: Tomasz Duszynski <tdu@semihalf.com>
> ---

Can you clarify a bit more why this is needed? For a static build, the
cfgfile should be linked in to the after the PMDs, so the dependencies
in the driver should be satisfied in the link. For a dynamic build, the
PMD should depend upon the cfgfile directly, and use it at runtime
appropriately. 
Am I missing something?

/Bruce
  
Tomasz Duszynski Sept. 27, 2017, 7:36 a.m. UTC | #2
On Tue, Sep 26, 2017 at 03:31:18PM +0100, Bruce Richardson wrote:
> On Tue, Sep 26, 2017 at 11:39:58AM +0200, Tomasz Duszynski wrote:
> > Since MRVL NET PMD needs librte_cfgfile to parse QoS configuration file
> > link it as the whole library.
> >
> > Signed-off-by: Jacek Siuda <jck@semihalf.com>
> > Signed-off-by: Tomasz Duszynski <tdu@semihalf.com>
> > ---
>
> Can you clarify a bit more why this is needed? For a static build, the
> cfgfile should be linked in to the after the PMDs, so the dependencies
> in the driver should be satisfied in the link. For a dynamic build, the
> PMD should depend upon the cfgfile directly, and use it at runtime
> appropriately.
> Am I missing something?
>
> /Bruce
Hi Bruce,

You are correct, all dependencies in the driver will be satisfied.
The reason this change was introduced is that, librte_pmd_mrvl.a
contains undefined symbols from librte_cfgfile.a thus linking
applications under app/ directory will fail as librte_cfgfile.a comes
before librte_pmd_mrvl.a during the linking stage.

Linking librte_cfgfile.a with --whole-archive solves the issue.

--
- Tomasz Duszyński
  
Bruce Richardson Sept. 27, 2017, 12:01 p.m. UTC | #3
On Wed, Sep 27, 2017 at 09:36:03AM +0200, Tomasz Duszynski wrote:
> On Tue, Sep 26, 2017 at 03:31:18PM +0100, Bruce Richardson wrote:
> > On Tue, Sep 26, 2017 at 11:39:58AM +0200, Tomasz Duszynski wrote:
> > > Since MRVL NET PMD needs librte_cfgfile to parse QoS configuration file
> > > link it as the whole library.
> > >
> > > Signed-off-by: Jacek Siuda <jck@semihalf.com>
> > > Signed-off-by: Tomasz Duszynski <tdu@semihalf.com>
> > > ---
> >
> > Can you clarify a bit more why this is needed? For a static build, the
> > cfgfile should be linked in to the after the PMDs, so the dependencies
> > in the driver should be satisfied in the link. For a dynamic build, the
> > PMD should depend upon the cfgfile directly, and use it at runtime
> > appropriately.
> > Am I missing something?
> >
> > /Bruce
> Hi Bruce,
> 
> You are correct, all dependencies in the driver will be satisfied.
> The reason this change was introduced is that, librte_pmd_mrvl.a
> contains undefined symbols from librte_cfgfile.a thus linking
> applications under app/ directory will fail as librte_cfgfile.a comes
> before librte_pmd_mrvl.a during the linking stage.
> 
> Linking librte_cfgfile.a with --whole-archive solves the issue.
> 
> --
Thanks for the explanation.

I believe the proper fix in this case is to ensure that when linking
apps that cfgfile comes after librte_pmd_mrvl. The drivers should always
come first when linking, then the DPDK libs.

/Bruce
  

Patch

diff --git a/mk/rte.app.mk b/mk/rte.app.mk
index c25fdd9..94568a8 100644
--- a/mk/rte.app.mk
+++ b/mk/rte.app.mk
@@ -81,10 +81,10 @@  _LDLIBS-$(CONFIG_RTE_LIBRTE_POWER)          += -lrte_power
 
 _LDLIBS-$(CONFIG_RTE_LIBRTE_TIMER)          += -lrte_timer
 _LDLIBS-$(CONFIG_RTE_LIBRTE_EFD)            += -lrte_efd
-_LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE)        += -lrte_cfgfile
 
 _LDLIBS-y += --whole-archive
 
+_LDLIBS-$(CONFIG_RTE_LIBRTE_CFGFILE)        += -lrte_cfgfile
 _LDLIBS-$(CONFIG_RTE_LIBRTE_HASH)           += -lrte_hash
 _LDLIBS-$(CONFIG_RTE_LIBRTE_VHOST)          += -lrte_vhost
 _LDLIBS-$(CONFIG_RTE_LIBRTE_KVARGS)         += -lrte_kvargs