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

["Gonzalez Monroy, Sergio" <sergio.gonzalez.monroy@intel.com>]

On 22/02/2015 23:37, Neil Horman wrote:
> On Fri, Feb 20, 2015 at 02:31:36PM +0000, Gonzalez Monroy, Sergio wrote:
>> On 13/02/2015 12:51, Neil Horman wrote:
>>> On Fri, Feb 13, 2015 at 11:08:02AM +0000, Gonzalez Monroy, Sergio wrote:
>>>> On 13/02/2015 10:14, Panu Matilainen wrote:
>>>>> On 02/12/2015 05:52 PM, Neil Horman wrote:
>>>>>> On Thu, Feb 12, 2015 at 04:07:50PM +0200, Panu Matilainen wrote:
>>>>>>> On 02/12/2015 02:23 PM, Neil Horman wrote:
>>>>> [...snip...]
>>>>>>>>>>> So I just realized that I was not having into account a possible
>>>>>>>>>>> scenario, where
>>>>>>>>>>> we have an app built with static dpdk libs then loading a dso
>>>>>>>>>>> with -d
>>>>>>>>>>> option.
>>>>>>>>>>>
>>>>>>>>>>> In such case, because the pmd would have DT_NEEDED entries,
>>>>>>>>>>> dlopen will
>>>>>>>>>>> fail.
>>>>>>>>>>> So to enable such scenario we would need to build PMDs without
>>>>>>>>>>> DT_NEEDED
>>>>>>>>>>> entries.
>>>>>>>>>> Hmm, for that to be a problem you'd need to have the PMD built
>>>>>>>>>> against
>>>>>>>>>> shared dpdk libs and while the application is built against
>>>>>>>>>> static dpdk
>>>>>>>>>> libs. I dont think that's a supportable scenario in any case.
>>>>>>>>>>
>>>>>>>>>> Or is there some other scenario that I'm not seeing?
>>>>>>>>>>
>>>>>>>>>>     - Panu -
>>>>>>>>>>
>>>>>>>>> I agree with you. I suppose it comes down to, do we want to
>>>>>>>>> support such
>>>>>>>>> scenario?
>>>>>>>>>
>>>>>>>>>  From what I can see, it seems that we do currently support such
>>>>>>>>> scenario by
>>>>>>>>> building dpdk apps against all static dpdk libs using
>>>>>>>>> --whole-archive (all
>>>>>>>>> libs and not only PMDs).
>>>>>>>>> http://dpdk.org/browse/dpdk/commit/?id=20afd76a504155e947c770783ef5023e87136ad8
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Am I misunderstanding this?
>>>>>>>>>
>>>>>>>> Shoot, you're right, I missed the static build aspect to this.  Yes,
>>>>>>>> if we do the following:
>>>>>>>>
>>>>>>>> 1) Build the DPDK as a static library
>>>>>>>> 2) Link an application against (1)
>>>>>>>> 3) Use the dlopen mechanism to load a PMD built as a DSO
>>>>>>>>
>>>>>>>> Then the DT_NEEDED entries in the DSO will go unsatisfied, because
>>>>>>>> the shared
>>>>>>>> objects on which it (the PMD) depends will not exist in the file
>>>>>>>> system.
>>>>>>> I think its even more twisty:
>>>>>>>
>>>>>>> 1) Build the DPDK as a static library
>>>>>>> 2) Link an application against (1)
>>>>>>> 3) Do another build of DPDK as a shared library
>>>>>>> 4) In app 2), use the dlopen mechanism to load a PMD built as a part
>>>>>>> of or
>>>>>>> against 3)
>>>>>>>
>>>>>>> Somehow I doubt this would work very well.
>>>>>>>
>>>>>> Ideally it should, presuming the ABI is preserved between (1) and (3),
>>>>>> though I
>>>>>> agree, up until recently, that was an assumption that was unreliable.
>>>>> Versioning is a big and important step towards reliability but there are
>>>>> more issues to solve. This of course getting pretty far from the original
>>>>> topic, but at least one such issue is that there are some cases where a
>>>>> config value affects what are apparently public structs (rte_mbuf wrt
>>>>> RTE_MBUF_REFCNT for example), which really is a no-go.
>>>>>
>>>> Agree, the RTE_MBUF_REFCNT is something that needs to be dealt with asap.
>>>> I'll look into it.
>>>>
>>>>>>>> I think the problem is a little bit orthogonal to the libdpdk_core
>>>>>>>> problem you
>>>>>>>> were initially addressing.  That is to say, this problem of
>>>>>>>> dlopen-ed PMD's
>>>>>>>> exists regardless of weather you build the DPDK as part of a static
>>>>>>>> or dynamic
>>>>>>>> library.  The problems just happen to intersect in their
>>>>>>>> manipulation of the
>>>>>>>> DT_NEEDED entries.
>>>>>>>>
>>>>>>>> Ok, so, given the above, I would say your approach is likely
>>>>>>>> correct, just
>>>>>>>> prevent DT_NEEDED entries from getting added to PMD's. Doing so will
>>>>>>>> sidestep
>>>>>>>> loading issue for libraries that may not exist in the filesystem,
>>>>>>>> but thats ok,
>>>>>>>> because by all rights, the symbols codified in those needed
>>>>>>>> libraries should
>>>>>>>> already be present in the running application (either made available
>>>>>>>> by the
>>>>>>>> application having statically linked them, or having the linker load
>>>>>>>> them from
>>>>>>>> the proper libraries at run time).
>>>>>>> My 5c is that I'd much rather see the common case (all static or all
>>>>>>> shared)
>>>>>>> be simple and reliable, which in case of DSOs includes no lying
>>>>>>> (whether by
>>>>>>> omission or otherwise) about DT_NEEDED, ever. That way the issue is
>>>>>>> dealt
>>>>>>> once where it belongs. If somebody wants to go down the rabbit hole of
>>>>>>> mixed
>>>>>>> shared + static linkage, let them dig the hole by themselves :)
>>>>>>>
>>>>>> This is a fair point.  Can DT_NEEDED sections be stripped via tools like
>>>>>> objcopy
>>>>>> after the build is complete?  If so, end users can hack this corner case
>>>>>> to work
>>>>>> as needed.
>>>>> Patchelf (http://nixos.org/patchelf.html) appears to support that, but
>>>>> given that source is available it'd be easier to just modify the makefiles
>>>>> if that's really needed.
>>>>>
>>>> I think we agree on the issue.
>>>>
>>>> So I'll be sending a patch to add DT_NEEDED entries to all libraries and
>>>> PMDs. The only exception would be librte_eal, which would not have proper
>>>> NEEDED entries.
>>>> Do we bother adding a linker script for librte_eal that would include
>>>> dependent libraries?
>>>>
>>> I say yes to the linker script, but will happily bow to an alternate consensus
>>> Neil
>>>
>> So the case we want to solve is the following circular dependencies:
>> eal             -> mempool, malloc
>> mempool -> eal , malloc, ring
>> malloc      -> eal
>> ring           -> eal, malloc
>>
>> We cannot write/create the proposed (below) linker script at least until we
>> have built mempool and malloc.
>> INPUT ( -lrte_eal.so -lrte_mempool -lrte_malloc )
>>
> Not sure I understand why you have a build time dependency on this.  Link time
> perhaps, but not build time.  Or am I reading too much into your use of the term
> 'built' above?
I meant 'built' as compiled + linked. Am I misusing the term?
>> Few ways I have thought about implementing this (not particularly fond of
>> any of them) :
>>   - Have the linker script file in the repo (scripts/ ?) in a fixed location
>> and just copy it to $(RTE_OUTPUT)/lib/ once all libs have finished building.
>>   - Generate the file on build time from a defined make variable once all
>> libs have finished
>>
> I'm still not sure I understand.  Why does this dependency exist at build time?
> The dependency between malloc and eal shouldn't be a problem during the build,
> as symbols from each other should just remain undefined, and get resolved at
> load time.
Is that not the way it is currently implemented?
I get the impression that we are talking about different goals (correct 
me if it is not the case)

I thought that the agreed solution was to:
1) NOT to create/generate a 'core' library
2) Add DT_NEEDED entries for all libraries (except eal which is the 
first library we link)
3) Use linker script for eal

Given the previously mentioned circular dependencies between eal, 
mempool, malloc and ring:
- eal would not be linked against other libraries (no NEEDED entries)
- malloc is linked against eal (previously built), so malloc would have 
a NEEDED entry for eal.

In that scenario, if the linker script is setup/created after we build 
eal, then when we try to link malloc
against eal, the linker will pull mempool and malloc too (because we 
included them in the linker script).
Therefore, the link fails as none of those libraries (malloc and 
mempool) have been built yet.

Was your suggestion to leave all of these libraries (eal, mempool, 
malloc, ring) without NEEDED entries?

Regards,
Sergio
> What is the error you are getting?
>
> Best
> Neil
>
>> Thoughts? any other approached is more than welcome!
>>
>> Sergio
>>
>> PS: Thinking again on the core library and the issue of having multiple
>> version.map files, we could have a core_version.map instead instead of
>> multiple files per core library (eal, mempool, etc)
>>
>>