[dpdk-dev] [PATCHv6 16/33] drivers/pool/dpaa2: adding hw offloaded mempool

[Ferruh Yigit]

On 1/24/2017 9:12 AM, Shreyansh Jain wrote:
> On Monday 23 January 2017 11:04 PM, Ferruh Yigit wrote:
>> On 1/23/2017 11:59 AM, Hemant Agrawal wrote:
>>> Adding NXP DPAA2 architecture specific mempool support
>>> Each mempool instance is represented by a DPBP object
>>> from the FSL-MC bus.
>>>
>>> This patch also registers a dpaa2 type MEMPOOL OPS
>>>
>>> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>>> ---
>> <...>
>>
>>> diff --git a/drivers/common/Makefile b/drivers/common/Makefile
>>> index b52931c..0bb75b5 100644
>>> --- a/drivers/common/Makefile
>>> +++ b/drivers/common/Makefile
>>> @@ -35,7 +35,11 @@ ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_PMD),y)
>>>  CONFIG_RTE_LIBRTE_DPAA2_COMMON = $(CONFIG_RTE_LIBRTE_DPAA2_PMD)
>>>  endif
>>>
>>> -ifeq ($(CONFIG_RTE_LIBRTE_FSLMC_BUS),y)
>>> +ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_POOL),y)
>>> +CONFIG_RTE_LIBRTE_DPAA2_COMMON = $(CONFIG_RTE_LIBRTE_DPAA2_POOL)
>>> +endif
>>> +
>>> +ifneq ($(CONFIG_RTE_LIBRTE_FSLMC_BUS),y)
>>
>> I guess this is a typo, but this prevents DPAA2_COMMON to be compiled !!
> 
> It should be 'ifeq' rather than 'ifneq'. 

> And it will prevent COMMON
> compilation only if CONFIG_RTE_LIBRTE_FSLMC_BUS=n which is not the case
> right now.

It was the case for me for x86 config, but you are right it is not the
default case for arm.

> 
> We will fix it.
> 
>>
>>>  CONFIG_RTE_LIBRTE_DPAA2_COMMON = $(CONFIG_RTE_LIBRTE_FSLMC_BUS)
>>>  endif
>>>
>>
>> <...>
>>> +# library dependencies
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_DPAA2_POOL) += lib/librte_eal
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_DPAA2_POOL) += lib/librte_mempool
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_DPAA2_POOL) += lib/librte_common_dpaa2_qbman
>>
>> This dependeny doesn not looks correct, there is no folder like that.
> 
> This is something even I need to understand. From the DEPDIRS what I
> understood was that though it refers to a directory, it essentially
> links libraries in build/lib/*.
> 
> Further, somehow the development is deploying drivers/bus,
> drivers/common and drivers/pool in lib/* under the name specified as
> LIB in Makefile. My understanding was that it is expected behavior and
> not special because of drivers folder.
> 
> Thus, above line only links lib/librte_common_dpaa2_qbman generated by
> drivers/common/dpaa2/qbman code.
> 
> In fact, I think, this might also one of the issues why a parallel 
> shared build fails for DPAA2 PMD (added in Cover letter).
> The dependency graph cannot create a graph for drivers/common
> as dependency for drivers/net or drivers/bus and hence parallel build
> fails because of missing libraries which are being parallely compiled.

DEPDIRS-y is mainly to resolve dependencies for compilation order, and
should point to the folder,

Following line will cause "librte_eal" to be compiled before driver:
DEPDIRS-$(CONFIG_RTE_LIBRTE_DPAA2_POOL) += lib/librte_eal

So "lib/librte_common_dpaa2_qbman" does not makes more sense, since
there is no folder like that.


Somewhere in the history, with following commit, DEPDIRS-y gained a side
effect, it has been used to set dynamic linking dependencies, to fix
underlinking issue:
 bf5a46fa5972 ("mk: generate internal library dependencies")

I guess you are having that line to benefit from this side effect, but
this can be done with following more properly:
LDLIBS += lib/librte_common_dpaa2_qbman


To resolve the drivers/net to drivers/common dependency, following line
in this Makefile should work:
DEPDIRS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += drivers/common/dpaa2

This adds following, which will cause "drivers/common" compiled before
any "drivers/net":
LOCAL_DEPDIRS-drivers/net += drivers/common

> 
>>
>>> +DEPDIRS-$(CONFIG_RTE_LIBRTE_DPAA2_POOL) += lib/librte_bus_fslmc
>>> +
>>> +include $(RTE_SDK)/mk/rte.lib.mk
>>
>> <...>
>>
>