From mboxrd@z Thu Jan  1 00:00:00 1970
From: Olivier MATZ <olivier.matz@6wind.com>
Subject: Re: [PATCH v2] mempool/dpaa2: add DPAA2 hardware
	offloaded mempool
Date: Mon, 10 Apr 2017 21:58:47 +0200
Message-ID: <20170410215847.704092d2@neon>
References: <1489754838-1455-2-git-send-email-hemant.agrawal@nxp.com>
 <1491724786-6468-1-git-send-email-hemant.agrawal@nxp.com>
 <1491724786-6468-2-git-send-email-hemant.agrawal@nxp.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: <dev@dpdk.org>, <thomas.monjalon@6wind.com>, <shreyansh.jain@nxp.com>,
 <ferruh.yigit@intel.com>
To: Hemant Agrawal <hemant.agrawal@nxp.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-wr0-f179.google.com (mail-wr0-f179.google.com
 [209.85.128.179]) by dpdk.org (Postfix) with ESMTP id EDFB92C1A
 for <dev@dpdk.org>; Mon, 10 Apr 2017 21:58:50 +0200 (CEST)
Received: by mail-wr0-f179.google.com with SMTP id o21so127635544wrb.2
 for <dev@dpdk.org>; Mon, 10 Apr 2017 12:58:50 -0700 (PDT)
In-Reply-To: <1491724786-6468-2-git-send-email-hemant.agrawal@nxp.com>
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>

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