From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id ABE2F558B for ; Mon, 13 Jun 2016 15:46:54 +0200 (CEST) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga104.fm.intel.com with ESMTP; 13 Jun 2016 06:46:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,466,1459839600"; d="scan'208";a="1000887377" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.221.50]) ([10.237.221.50]) by fmsmga002.fm.intel.com with ESMTP; 13 Jun 2016 06:46:52 -0700 To: Olivier Matz , dev@dpdk.org References: <1464965906-108927-1-git-send-email-david.hunt@intel.com> <1465571806-22008-1-git-send-email-david.hunt@intel.com> <1465571806-22008-2-git-send-email-david.hunt@intel.com> <575EA42D.2030003@6wind.com> Cc: viktorin@rehivetech.com, jerin.jacob@caviumnetworks.com, shreyansh.jain@nxp.com From: "Hunt, David" Message-ID: <575EB94B.8080006@intel.com> Date: Mon, 13 Jun 2016 14:46:51 +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: <575EA42D.2030003@6wind.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v9 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: Mon, 13 Jun 2016 13:46:55 -0000 Hi Olivier, On 13/6/2016 1:16 PM, Olivier Matz wrote: > Hi David, > > Some comments below. > > On 06/10/2016 05:16 PM, David Hunt wrote: >> Until now, the objects stored in a mempool were internally stored in a >> ring. This patch introduces the possibility to register external handlers >> replacing the ring. >> >> The default behavior remains unchanged, but calling the new function >> rte_mempool_set_handler() right after rte_mempool_create_empty() allows >> the user to change the handler that will be used when populating >> the mempool. >> >> This patch also adds a set of default ops (function callbacks) based >> on rte_ring. >> >> Signed-off-by: Olivier Matz >> Signed-off-by: David Hunt >> >> ... >> @@ -386,10 +352,14 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr, >> int ret; >> >> /* create the internal ring if not already done */ >> - if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) { >> - ret = rte_mempool_ring_create(mp); >> - if (ret < 0) >> - return ret; >> + if ((mp->flags & MEMPOOL_F_POOL_CREATED) == 0) { >> + rte_errno = 0; >> + ret = rte_mempool_ops_alloc(mp); >> + if (ret != 0) { >> + if (rte_errno == 0) >> + return -EINVAL; >> + return -rte_errno; >> + } >> } > The rte_errno should be removed. Just return the error code from > rte_mempool_ops_alloc() on failure. Done. >> +/** Structure defining mempool operations structure */ >> +struct rte_mempool_ops { >> + char name[RTE_MEMPOOL_OPS_NAMESIZE]; /**< Name of mempool ops struct */ >> + rte_mempool_alloc_t alloc; /**< Allocate private data */ >> + rte_mempool_free_t free; /**< Free the external pool. */ >> + rte_mempool_put_t put; /**< Put an object. */ >> + rte_mempool_get_t get; /**< Get an object. */ >> + rte_mempool_get_count get_count; /**< Get qty of available objs. */ >> +} __rte_cache_aligned; >> + > Sorry, I missed that in the previous reviews, but since the get/put > functions have been renamed in dequeue/enqueue, I think the same change > should also be done here. Done. I also changed the common_ring_[sc|mc]_put and common_ring_[sc|mc]_get. I didn't go as far as changing the rte_mempool_put or rte_mempool_put_bulk calls, as they are used in several drivers and apps. > >> +/** >> + * 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 most likely point to a different type of data structure, and >> + * will be transparent to the application programmer. >> + */ >> +typedef int (*rte_mempool_alloc_t)(struct rte_mempool *mp); > A comment saying that this function should set mp->pool_data > would be nice here, I think. Done >> +/* wrapper to allocate an external mempool's private (pool) data */ >> +int >> +rte_mempool_ops_alloc(struct rte_mempool *mp) >> +{ >> + struct rte_mempool_ops *ops; >> + >> + ops = rte_mempool_ops_get(mp->ops_index); >> + if (ops->alloc == NULL) >> + return -ENOMEM; >> + return ops->alloc(mp); >> +} > Now that we check that ops->alloc != NULL in the register function, > I wonder if we should keep this test or not. Yes, it doesn't hurt, > but for consistency with the other functions (get/put/get_count), > we may remove it. > > This would be a good thing because it would prevent any confusion > with rte_mempool_ops_get(), which returns a pointer to the ops struct > (and has nothing to do with ops->get()). Done. > > Regards, > Olivier Thanks, Dave.