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 109895586 for ; Mon, 13 Jun 2016 14:17:00 +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 1bCQpj-0007tF-Ie; Mon, 13 Jun 2016 14:19:12 +0200 To: David Hunt , 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> Cc: viktorin@rehivetech.com, jerin.jacob@caviumnetworks.com, shreyansh.jain@nxp.com From: Olivier Matz Message-ID: <575EA42D.2030003@6wind.com> Date: Mon, 13 Jun 2016 14:16:45 +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: <1465571806-22008-2-git-send-email-david.hunt@intel.com> Content-Type: text/plain; charset=windows-1252 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 12:17:00 -0000 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. > +/** 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. > +/** > + * 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. > +/* 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()). Regards, Olivier