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

[Olivier MATZ <olivier.matz@6wind.com>]

Hi Hemant,

On Sun, 9 Apr 2017 13:29:46 +0530
Hemant Agrawal <hemant.agrawal@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@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.

> +	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.

[...]

>
>


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?

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

Regards,
Olivier