From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hemant Agrawal Subject: Re: [PATCH v2] mempool/dpaa2: add DPAA2 hardware offloaded mempool Date: Tue, 11 Apr 2017 11:28:26 +0530 Message-ID: <9977cf0c-e1cf-b235-e199-c5242daa75c2@nxp.com> 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> <20170410215847.704092d2@neon> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Cc: , , , To: Olivier MATZ Return-path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0073.outbound.protection.outlook.com [104.47.34.73]) by dpdk.org (Postfix) with ESMTP id 0F521559A for ; Tue, 11 Apr 2017 07:58:36 +0200 (CEST) In-Reply-To: <20170410215847.704092d2@neon> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 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 > > [...] > > >> --- 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 > >