From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 141B81B1D8 for ; Fri, 19 Jan 2018 14:10:59 +0100 (CET) Received: from lfbn-lil-1-110-231.w90-45.abo.wanadoo.fr ([90.45.197.231] helo=droids-corp.org) by mail.droids-corp.org with esmtpsa (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1ecWRf-0001WD-34; Fri, 19 Jan 2018 14:11:04 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Fri, 19 Jan 2018 14:10:52 +0100 Date: Fri, 19 Jan 2018 14:10:52 +0100 From: Olivier Matz To: Hemant Agrawal Cc: dev@dpdk.org, jerin.jacob@caviumnetworks.com, santosh.shukla@caviumnetworks.com Message-ID: <20180119131052.war36l7jxfc67rd5@platinum> 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> <89a76d94-b4a1-7bf4-dd64-16096f8b6bbe@nxp.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <89a76d94-b4a1-7bf4-dd64-16096f8b6bbe@nxp.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v3 3/7] mbuf: add pool ops name selection API helpers X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 19 Jan 2018 13:10:59 -0000 Hi Hemant, On Fri, Jan 19, 2018 at 06:11:47PM +0530, Hemant Agrawal wrote: > 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. With a memzone, maybe there is even no need for a static variable. Something like this? set(): mz = rte_memzone_lookup("mbuf_platform_pool_ops"); if (mz == NULL) { mz = rte_memzone_reszerve("mbuf_platform_pool_ops", LEN); if (mz == NULL) return -rte_errno; /* << this even protects against a set() in a 2nd process */ } if (strlen(name) >= LEN) return -ENAMETOOLONG; strncpy(mz->addr, name, LEN); get(): mz = rte_memzone_lookup("mbuf_platform_pool_ops"); if (mz == NULL) return NULL; return mz->addr; > > > > 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? Correct. something to check the length should be added though.