[dpdk-dev] [PATCH v2] mempool/dpaa2: add DPAA2 hardware offloaded mempool

[Hemant Agrawal]

Hi Olivier,

On 4/11/2017 1:28 AM, Olivier MATZ wrote:
> Hi Hemant,
>
> On Sun, 9 Apr 2017 13:29:46 +0530
> Hemant Agrawal <hemant.agrawal at nxp.com> wrote:
>
>> DPAA2 Hardware Mempool handlers allow enqueue/dequeue from NXP's
>> QBMAN hardware block.
>> CONFIG_RTE_MBUF_DEFAULT_MEMPOOL_OPS is set to 'dpaa2', if the pool
>> is enabled.
>>
>> This memory pool currently supports packet mbuf type blocks only.
>>
>> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>
>
> [...]
>
>
>> --- a/drivers/bus/Makefile
>> +++ b/drivers/bus/Makefile
>> @@ -33,6 +33,10 @@ include $(RTE_SDK)/mk/rte.vars.mk
>>
>>  core-libs := librte_eal librte_mbuf librte_mempool librte_ring librte_ether
>>
>> +ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_MEMPOOL),y)
>> +CONFIG_RTE_LIBRTE_FSLMC_BUS = $(CONFIG_RTE_LIBRTE_DPAA2_MEMPOOL)
>> +endif
>> +
>>  DIRS-$(CONFIG_RTE_LIBRTE_FSLMC_BUS) += fslmc
>>  DEPDIRS-fslmc = ${core-libs}
>>
>
> What's the purpose of this?
> Not sure we are allowed to modify the configs in the Makefiles.

DPAA2_MEMPOOL will not work without the DPAA2 mempool hw instance 
detected on FSLMC_BUS.
So, it is required that if you are enabling DPAA2_MEMPOOL, FSLMC_BUS is 
to be enabled.

Currently the config structure do not provide such dependency definitions.

This was done based on the suggestions on the initial patches from 
Ferruh and Jerin.




>> +	ret = dpbp_get_attributes(&avail_dpbp->dpbp, CMD_PRI_LOW,
>> +				  avail_dpbp->token, &dpbp_attr);
>> +	if (ret != 0) {
>> +		PMD_INIT_LOG(ERR, "Resource read failure with"
>> +			     " err code: %d\n", ret);
>> +		p_ret = ret;
>> +		ret = dpbp_disable(&avail_dpbp->dpbp, CMD_PRI_LOW,
>> +				   avail_dpbp->token);
>> +		return p_ret;
>> +	}
>> +
>> +	/* Allocate the bp_list which will be added into global_bp_list */
>> +	bp_list = (struct dpaa2_bp_list *)malloc(sizeof(struct dpaa2_bp_list));
>> +	if (!bp_list) {
>> +		PMD_INIT_LOG(ERR, "No heap memory available");
>> +		return -ENOMEM;
>> +	}
>> +
>
> I think the cast is not needed.
> Are you sure you want to use malloc() and not rte_malloc()? It would be in
> hugepages.
>

Yes! you are right, cast is not required and rte_malloc will be better 
than malloc.

> [...]
>
>>
>>
>
>
> I still have some concerns about the fact that the mempool handler assumes that
> the objects are necessarily mbufs. I guess for this reason it does not pass
> mempool autotests?
>

Based on some of your previous suggestion, I have some thoughts to 
address it, but it will be longer term.

1. add some kind of capability APIs in mempool
2. Or, indicate to mempool to differentiate between mbuf pool vs 
non-mbuf pool.

I will initiate discussions on these topics.

> We should probably move forward and let it go in 17.05, but this is something
> that should be enhanced in my opinion.
>

Thanks for your valued suggestions and reviews. Yes! Once the basic 
stuff in in the 17.05, we will start working on making it more generic.


> Regards,
> Olivier
>
>