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 C7EA69ACB for ; Wed, 8 Jun 2016 14:13:33 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1bAcOf-0000hb-DO; Wed, 08 Jun 2016 14:15:45 +0200 To: David Hunt , dev@dpdk.org References: <1464874043-67467-1-git-send-email-david.hunt@intel.com> <1464965906-108927-1-git-send-email-david.hunt@intel.com> <1464965906-108927-2-git-send-email-david.hunt@intel.com> Cc: viktorin@rehivetech.com, jerin.jacob@caviumnetworks.com From: Olivier Matz Message-ID: <57580BE2.9040204@6wind.com> Date: Wed, 8 Jun 2016 14:13:22 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <1464965906-108927-2-git-send-email-david.hunt@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jun 2016 12:13:33 -0000 Hi David, Please find some comments below. On 06/03/2016 04:58 PM, David Hunt wrote: > --- a/lib/librte_mempool/rte_mempool.h > +++ b/lib/librte_mempool/rte_mempool.h > +/** > + * Prototype for implementation specific data provisioning function. > + * > + * The function should provide the implementation specific memory for > + * for use by the other mempool ops functions in a given mempool ops struct. > + * E.g. the default ops provides an instance of the rte_ring for this purpose. > + * it will mostlikely point to a different type of data structure, and > + * will be transparent to the application programmer. > + */ > +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp); In http://dpdk.org/ml/archives/dev/2016-June/040233.html, I suggested to change the prototype to return an int (-errno) and directly set the set mp->pool_data (void *) or mp->pool_if (uint64_t). No cast would be required in this latter case. By the way, there is a typo in the comment: "mostlikely" -> "most likely" > --- /dev/null > +++ b/lib/librte_mempool/rte_mempool_default.c > +static void > +common_ring_free(struct rte_mempool *mp) > +{ > + rte_ring_free((struct rte_ring *)mp->pool_data); > +} I don't think the cast is needed here. (same in other functions) > --- /dev/null > +++ b/lib/librte_mempool/rte_mempool_ops.c > +/* add a new ops struct in rte_mempool_ops_table, return its index */ > +int > +rte_mempool_ops_register(const struct rte_mempool_ops *h) > +{ > + struct rte_mempool_ops *ops; > + int16_t ops_index; > + > + rte_spinlock_lock(&rte_mempool_ops_table.sl); > + > + if (rte_mempool_ops_table.num_ops >= > + RTE_MEMPOOL_MAX_OPS_IDX) { > + rte_spinlock_unlock(&rte_mempool_ops_table.sl); > + RTE_LOG(ERR, MEMPOOL, > + "Maximum number of mempool ops structs exceeded\n"); > + return -ENOSPC; > + } > + > + if (h->put == NULL || h->get == NULL || h->get_count == NULL) { > + rte_spinlock_unlock(&rte_mempool_ops_table.sl); > + RTE_LOG(ERR, MEMPOOL, > + "Missing callback while registering mempool ops\n"); > + return -EINVAL; > + } > + > + if (strlen(h->name) >= sizeof(ops->name) - 1) { > + RTE_LOG(DEBUG, EAL, "%s(): mempool_ops <%s>: name too long\n", > + __func__, h->name); > + rte_errno = EEXIST; > + return NULL; > + } This has already been noticed by Shreyansh, but in case of: rte_mempool_ops.c:75:10: error: return makes integer from pointer without a cast [-Werror=int-conversion] return NULL; ^ > +/* sets mempool ops previously registered by rte_mempool_ops_register */ > +int > +rte_mempool_set_ops_byname(struct rte_mempool *mp, const char *name) When I compile with shared libraries enabled, I get the following error: librte_reorder.so: undefined reference to `rte_mempool_ops_table' librte_mbuf.so: undefined reference to `rte_mempool_set_ops_byname' ... The new functions and global variables must be in rte_mempool_version.map. This was in v5 ( http://dpdk.org/ml/archives/dev/2016-May/039365.html ) but was dropped in v6. Regards, Olivier