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

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Jan 30, 2015 at 05:38:49PM +0000, Gonzalez Monroy, Sergio wrote:
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Friday, January 30, 2015 2:05 PM
> > To: Gonzalez Monroy, Sergio
> > Cc: Thomas Monjalon; dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 0/8] Improve build process
> > 
> > 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.
> 
> So would it be reasonable to add DT_NEEDED entries to all DPDK libraries but EAL?
> If I understood what you were saying right, we could enforce the 'dependency' in the 
> linker script with something like this:
> $ cat  librte_eal.so
> INPUT( librte_eal.so.1 -lrte_mempool -lrte_malloc)
> We could have such linker script for librte_eal.so instead of the soft link once
> versioning is in place.
> 
Correct.

> Things that would be missing versus the proposed patch:
>  - As I have mention in previous post, ldd info for EAL library would not reflect
>    its dependency to other DPDK libs.
librte_eal.so would no show those dependencies, as far as I know (though I
haven't explicitly checked).  The subordunate libraries included in the input
line, may or may not show dependencies among themselves, depending on your build
setup (and the use of --no-as-needed and -l when linking the individual .so
libraries.

>  - I was enforcing resolving all references when building the libraries (-z defs), so
>    we either remove it altogether or skip eal.
I think thats correct, yes.

>  - All apps would show DT_NEEDED entries for a set of DPDK libraries that
>    in most cases are required (eal, mempool, malloc, mbuf, ring VS dpdk_core)
> 
I think apps linked to libdpdk_core would have DT_NEEDED entries for
libdpdk_core, not the subordonate libraries (though check me on that to be
sure).

> I think that the linker script approach is reasonable if we prefer to go that way
> instead of creating a core library.
> 
I think it would make sense from a build environment point of view, in that it
allows library specific flags to be incorporated properly.  I think the only
downside is that the individual libraries still need to be carried around
(though they can be ignored from an application build/run standpoint).  You're
question should probably be asked of people using COMBINED_LIBS currently to
make sure that meets their needs, though I think it will.

Neil

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