On Fri, Jan 30, 2015 at 01:39:28PM +0000, Gonzalez Monroy, Sergio wrote: > > From: Neil Horman [mailto:nhorman at tuxdriver.com] > > Sent: Thursday, January 29, 2015 7:46 PM > > To: Gonzalez Monroy, Sergio > > Cc: dev at 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 at tuxdriver.com] > > > > Sent: Thursday, January 29, 2015 4:39 PM > > > > To: Gonzalez Monroy, Sergio > > > > Cc: dev at 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 > > >
