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 2BAB91B319 for ; Fri, 19 Jan 2018 11:01:50 +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 1ecTUf-0001Fq-Mh; Fri, 19 Jan 2018 11:01:55 +0100 Received: by droids-corp.org (sSMTP sendmail emulation); Fri, 19 Jan 2018 11:01:47 +0100 Date: Fri, 19 Jan 2018 11:01:47 +0100 From: Olivier Matz To: Hemant Agrawal Cc: dev@dpdk.org, jerin.jacob@caviumnetworks.com, santosh.shukla@caviumnetworks.com Message-ID: <20180119100147.wbhddhtgcsbdac5w@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1516281992-6873-4-git-send-email-hemant.agrawal@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 10:01:50 -0000 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. 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. > + 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. I suggest to simply do a strdup() instead. > + 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? > + 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 :)