Re: [dpdk-dev] [PATCH 0/8] Improve build process

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Jan 30, 2015 at 01:39:28PM +0000, Gonzalez Monroy, Sergio wrote:
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Thursday, January 29, 2015 7:46 PM
> > To: Gonzalez Monroy, Sergio
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 0/8] Improve build process
> > 
> > On Thu, Jan 29, 2015 at 05:04:20PM +0000, Gonzalez Monroy, Sergio wrote:
> > > > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > > Sent: Thursday, January 29, 2015 4:39 PM
> > > > To: Gonzalez Monroy, Sergio
> > > > Cc: dev@dpdk.org
> > > > Subject: Re: [dpdk-dev] [PATCH 0/8] Improve build process
> > > >
> > > > On Thu, Jan 29, 2015 at 03:20:03PM +0000, Sergio Gonzalez Monroy
> > wrote:
> > > > > This patch series improves the DPDK build system mostly for shared
> > > > > libraries (and a few nits for static libraries) with the following goals:
> > > > >  - Create a library containing core DPDK libraries (librte_eal,
> > > > >    librte_malloc, librte_mempool, librte_mbuf and librte_ring).
> > > > >    The idea of core libraries is to group those libraries that are
> > > > >    always required (and have interdependencies) for any DPDK
> > application.
> > > > >  - Remove config option to build a combined library.
> > > > >  - For shared libraries, explicitly link against dependant
> > > > >    libraries (adding entries to DT_NEEDED).
> > > > >  - Update app linking flags for static/shared DPDK libs.
> > > > >
> > > > > Sergio Gonzalez Monroy (8):
> > > > >   mk: remove combined library and related options
> > > > >   core: create new librte_core
> > > > >   mk: new corelib makefile
> > > > >   lib: update DEPDIRS variable
> > > > >   lib: set LDLIBS for each library
> > > > >   mk: use LDLIBS when linking shared libraries
> > > > >   mk: update LDLIBS for app building
> > > > >   mk: add -lpthread to linuxapp EXECENV_LDLIBS
> > > > >
> > > > >  config/common_bsdapp                        |   6 --
> > > > >  config/common_linuxapp                      |   6 --
> > > > >  config/defconfig_ppc_64-power8-linuxapp-gcc |   2 -
> > > > >  lib/Makefile                                |   1 -
> > > > >  lib/librte_acl/Makefile                     |   5 +-
> > > > >  lib/librte_cfgfile/Makefile                 |   4 +-
> > > > >  lib/librte_cmdline/Makefile                 |   6 +-
> > > > >  lib/librte_core/Makefile                    |  45 +++++++++++++
> > > > >  lib/librte_distributor/Makefile             |   5 +-
> > > > >  lib/librte_eal/bsdapp/eal/Makefile          |   3 +-
> > > > >  lib/librte_eal/linuxapp/eal/Makefile        |   3 +-
> > > > >  lib/librte_ether/Makefile                   |   4 +-
> > > > >  lib/librte_hash/Makefile                    |   4 +-
> > > > >  lib/librte_ip_frag/Makefile                 |   6 +-
> > > > >  lib/librte_ivshmem/Makefile                 |   4 +-
> > > > >  lib/librte_kni/Makefile                     |   6 +-
> > > > >  lib/librte_kvargs/Makefile                  |   6 +-
> > > > >  lib/librte_lpm/Makefile                     |   6 +-
> > > > >  lib/librte_malloc/Makefile                  |   2 +-
> > > > >  lib/librte_mbuf/Makefile                    |   2 +-
> > > > >  lib/librte_mempool/Makefile                 |   2 +-
> > > > >  lib/librte_meter/Makefile                   |   4 +-
> > > > >  lib/librte_pipeline/Makefile                |   3 +
> > > > >  lib/librte_pmd_af_packet/Makefile           |   5 +-
> > > > >  lib/librte_pmd_bond/Makefile                |   7 +-
> > > > >  lib/librte_pmd_e1000/Makefile               |   8 ++-
> > > > >  lib/librte_pmd_enic/Makefile                |   8 ++-
> > > > >  lib/librte_pmd_i40e/Makefile                |   8 ++-
> > > > >  lib/librte_pmd_ixgbe/Makefile               |   8 ++-
> > > > >  lib/librte_pmd_pcap/Makefile                |   5 +-
> > > > >  lib/librte_pmd_ring/Makefile                |   6 +-
> > > > >  lib/librte_pmd_virtio/Makefile              |   7 +-
> > > > >  lib/librte_pmd_vmxnet3/Makefile             |   8 ++-
> > > > >  lib/librte_pmd_xenvirt/Makefile             |   8 ++-
> > > > >  lib/librte_port/Makefile                    |   8 +--
> > > > >  lib/librte_power/Makefile                   |   4 +-
> > > > >  lib/librte_ring/Makefile                    |   2 +-
> > > > >  lib/librte_sched/Makefile                   |   7 +-
> > > > >  lib/librte_table/Makefile                   |   8 +--
> > > > >  lib/librte_timer/Makefile                   |   6 +-
> > > > >  lib/librte_vhost/Makefile                   |   9 +--
> > > > >  mk/exec-env/linuxapp/rte.vars.mk            |   2 +
> > > > >  mk/rte.app.mk                               |  53 ++++-----------
> > > > >  mk/rte.corelib.mk                           |  84 +++++++++++++++++++++++
> > > > >  mk/rte.lib.mk                               |  49 +++-----------
> > > > >  mk/rte.sdkbuild.mk                          |   3 -
> > > > >  mk/rte.sharelib.mk                          | 101 ----------------------------
> > > > >  mk/rte.vars.mk                              |   9 ---
> > > > >  48 files changed, 276 insertions(+), 282 deletions(-)  create
> > > > > mode
> > > > > 100644 lib/librte_core/Makefile  create mode 100644
> > > > > mk/rte.corelib.mk delete mode 100644 mk/rte.sharelib.mk
> > > > >
> > > > > --
> > > > > 1.9.3
> > > > >
> > > > >
> > > > Something occured to me thinking about this patch set.  I noticed
> > > > recently that different rules are used to build the shared combined
> > > > lib from the individual shared objects.  The implication here is
> > > > that linker options specified in individual make files (like the
> > > > LIBABIVER and EXPORT_MAP options in my ABI versioning script) get
> > > > ignored, which is bad.  Any other file specific linker options (like
> > > > <file>_LDFLAGS specified in individual library makefiles are getting
> > dropped for the combined lib.
> > > >
> > > > It seems like it would be better if the combined libs were
> > > > manufactured as linker scripts themselves (textfiles that used
> > > > linker directives to include individual libraries under the covers (see
> > /lib64/libc.so for an example).
> > > >
> > > > The disadvantage of such an approach are fairly minimal.  With such
> > > > a combined library, you still need to install individual libraries,
> > > > but for applications that wish to link and run against a single dpdk
> > > > library will still work just as they currently do, you can link to just a single
> > library.
> > > >
> > > > The advantage is clear however.  By following a linker script
> > > > aproach, objects build as separate libraries are built exactly the
> > > > same way, using the same rules with the same options.  It reduces
> > > > the dpdk build environment size and complexity, and reduces the
> > > > opportunity for bugs to creep in from forgetting to add build
> > > > options to multiple locations.  It also provides a more granular
> > > > approach for grouping files.  Creating a dpdk core library becomes a
> > > > matter of creating a one line linker script named libdpdk_core.so, rather
> > than re- arraning sections of the build system.
> > > >
> > > > Thoughts?
> > > > Neil
> > > >
> > > Hi  Neil,
> > >
> > > I think that is a very interesting approach.
> > > I have tried to do something similar in this patch by removing
> > > rte.sharelib.mk and just having rte.lib.mk to do the linking, leaving
> > > as you suggest a single file to modify anything related to building libs.
> > >
> > > I do think however that your proposal is an improvement over the current
> > patch.
> > >
> > > So basically we want:
> > > - get rid of rte.corelib.mk
> > > - generate librte_core.so linker script grouping core libs
> > > - we do not modify DEPDIR variables
> > > - when setting LDLIBS to each lib, we do specify -lrte_core, right?
> > >
> > Exactly, and librte_core.so is really just a text file containing the following line
> > :
> > INPUT(-lrte_malloc -lrte_mbuf -lrte_eal ....)
> > 
> > Adding in whatever libraries you want librte_core to consist of.  Truthfully,
> > you could almost get rid of the COMBINE_LIBS option entirely, and just
> > create this file statically if you wanted to (not sure thats the best approach,
> > but its definately do-able).
> > 
> Hi Neil,
> 
> Actually, the first patch series does get rid of COMBINE_LIBS entirely.
> 
Sorry, I didn't mean to imply your patch wasn't, just re-iterating that the
option is not needed using the alternate method we're discussing, but I really
wasn't very clear on that.

> So as I was looking into this, by using this approach we do not resolve the interdependencies issue of the core libraries.
> We would effectively leave all core libraries (or at least EAL) without proper DT_NEEDED entries.
> 
> Thoughts?
> 
You're correct, or at least what you assert is possible, depending on the
implementation.  Adding DT_NEEDED entries is something of an orthogonal problem
(though your current implementation I think handles it well).  You could specify
linker directives when building each library so that each DSO contains the
proper DT_NEEDED entries (using -l<lib> and --no-as-needed).  using a linker
script approach doesn't preclude you from doing that, though its not strictly
speaking necessecary.  When you write the linker script, you implicitly specify
the link dependencies by the order in whcih you list the inferior libraries in
the scripts INPUT line.  It doesn't give you the DT_NEEDED entries, but from an
application build/run standpoint, it won't matter, because the libraries will be
linked/loaded in the order specified.  You can still do the --no-as-needed
method though if you like for safety on the part of those using libraries
independently.
Neil

> Regards,
> Sergio
> 
> > Regards
> > Neil
> > 
>