From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hemant Agrawal Subject: Re: [PATCH v3 3/7] mbuf: add pool ops name selection API helpers Date: Fri, 19 Jan 2018 18:11:47 +0530 Message-ID: <89a76d94-b4a1-7bf4-dd64-16096f8b6bbe@nxp.com> References: <1515996674-26338-1-git-send-email-hemant.agrawal@nxp.com> <1516281992-6873-1-git-send-email-hemant.agrawal@nxp.com> <1516281992-6873-4-git-send-email-hemant.agrawal@nxp.com> <20180119100147.wbhddhtgcsbdac5w@platinum> 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-by2nam01on0041.outbound.protection.outlook.com [104.47.34.41]) by dpdk.org (Postfix) with ESMTP id A1BBC1AEF1 for ; Fri, 19 Jan 2018 13:41:52 +0100 (CET) In-Reply-To: <20180119100147.wbhddhtgcsbdac5w@platinum> 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 1/19/2018 3:31 PM, Olivier Matz wrote: > On Thu, Jan 18, 2018 at 06:56:28PM +0530, Hemant Agrawal wrote: >> This patch add support for various mempool ops config helper APIs. >> >> 1.User defined mempool ops >> 2.Platform detected HW mempool ops (active). >> 3.Best selection of mempool ops by looking into user defined, >> platform registered and compile time configured. >> >> Signed-off-by: Hemant Agrawal >> --- > > ... > >> --- /dev/null >> +++ b/lib/librte_mbuf/rte_mbuf_pool_ops.c >> @@ -0,0 +1,68 @@ >> +/* SPDX-License-Identifier: BSD-3-Clause >> + * Copyright 2018 NXP >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +static char *plat_mbuf_pool_ops_name; > > I have some doubts about secondary processes. > > Maybe it's ok if the loaded driver and eal arguments are exactly the > same in the secondary process. It would be safer to use a named memzone > for that. > Typically a secondary process should not set the platform mempool name. I can also add a check to know if secondary process is trying to do it. Yes. I can change it to a named memzone. > It would be even safer to not use secondary processes ;) > > >> + >> +int >> +rte_mbuf_register_platform_mempool_ops(const char *ops_name) >> +{ > > We have "register" for platform and "set" for user. > I think "set" should be used everywhere. > ok >> + if (plat_mbuf_pool_ops_name == NULL) { >> + plat_mbuf_pool_ops_name = >> + rte_malloc(NULL, RTE_MEMPOOL_OPS_NAMESIZE, 0); >> + if (plat_mbuf_pool_ops_name == NULL) >> + return -ENOMEM; >> + strcpy((char *)plat_mbuf_pool_ops_name, ops_name); > > If strlen(ops_name) >= RTE_MEMPOOL_OPS_NAMESIZE, this may lead to > bad behavior. > That should not happen, we can check that. > I suggest to simply do a strdup() instead. Well, strdup based string will not be readable/accessible from the secondary process? > > >> + return 0; >> + } else if (strcmp(plat_mbuf_pool_ops_name, ops_name) == 0) { >> + return 0; >> + } >> + >> + RTE_LOG(ERR, MBUF, >> + "%s is already registered as platform mbuf pool ops\n", >> + plat_mbuf_pool_ops_name); > > So, this log means that a we should try to never have 2 drivers registering > different platform drivers on the same machine, right? > > So this API is kind of reserved for network processors and should not be > used in usual PCI PMDs? > No, PCI PMDs can also use it. But only one registration allowed for now. As I mentioned in the cover letter: ~~~~~ This logic can be further extended with addition for following patch, which is still under discussion. The ethdev PMD capability exposed through existing rte_eth_dev_pool_ops_supported() to select the update the mempool ops with some "weight" based algorithm like: http://dpdk.org/dev/patchwork/patch/32245/ ~~~~~~ > >> + return -EEXIST; >> +} >> + >> +const char * >> +rte_mbuf_platform_mempool_ops(void) >> +{ >> + return (const char *)plat_mbuf_pool_ops_name; > > cast is not required > >> +} >> + >> +void >> +rte_mbuf_set_user_mempool_ops(const char *ops_name) >> +{ >> + rte_eal_set_mbuf_user_mempool_ops(ops_name); >> +} > > Instead of calling the EAL API, we can set a static variable as > for platform ops. > >> + >> +const char * >> +rte_mbuf_user_mempool_ops(void) >> +{ >> + return rte_eal_mbuf_default_mempool_ops(); >> +} > > And here, I suggest instead: > > rte_mbuf_user_mempool_ops(void) > { > if (user_mbuf_pool_ops_name != NULL) > return user_mbuf_pool_ops_name; > return rte_eal_mbuf_default_mempool_ops(); > } > > i.e. rte_eal_mbuf_default_mempool_ops() remains the ops passed as > command line arguments. > > >> + >> +/* Return mbuf pool ops name */ >> +const char * >> +rte_mbuf_best_mempool_ops(void) >> +{ >> + /* User defined mempool ops takes the priority */ >> + const char *best_ops = rte_mbuf_user_mempool_ops(); >> + if (best_ops) >> + return best_ops; >> + >> + /* Next choice is platform configured mempool ops */ >> + best_ops = rte_mbuf_platform_mempool_ops(); >> + if (best_ops) >> + return best_ops; >> + >> + /* Last choice is to use the compile time config pool */ >> + return RTE_MBUF_DEFAULT_MEMPOOL_OPS; >> +} > > I like this function, this is much clearer than what we have today :) >