From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 03E66C38C for ; Thu, 9 Jun 2016 12:33:35 +0200 (CEST) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga102.jf.intel.com with ESMTP; 09 Jun 2016 03:33:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,444,1459839600"; d="scan'208";a="118862935" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.220.91]) ([10.237.220.91]) by fmsmga004.fm.intel.com with ESMTP; 09 Jun 2016 03:33:33 -0700 To: Olivier Matz , 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> <57580BE2.9040204@6wind.com> Cc: viktorin@rehivetech.com, jerin.jacob@caviumnetworks.com From: "Hunt, David" Message-ID: <575945FD.709@intel.com> Date: Thu, 9 Jun 2016 11:33:33 +0100 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-Version: 1.0 In-Reply-To: <57580BE2.9040204@6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Thu, 09 Jun 2016 10:33:36 -0000 Hi Olivier, On 8/6/2016 1:13 PM, Olivier Matz wrote: > 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. Done. > By the way, there is a typo in the comment: > "mostlikely" -> "most likely" Fixed. >> --- /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) Removed. > >> --- /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; > ^ Changed to return -EEXIST > >> +/* 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. OK, Added. > > Regards, > Olivier Thanks, David.