From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so1.wedos.net (wes1-so1.wedos.net [46.28.106.15]) by dpdk.org (Postfix) with ESMTP id A1C415686 for ; Thu, 2 Jun 2016 15:48:47 +0200 (CEST) Received: from pcviktorin.fit.vutbr.cz (pcviktorin.fit.vutbr.cz [147.229.13.147]) by wes1-so1.wedos.net (Postfix) with ESMTPSA id 3rL7qM2XMlzBxV; Thu, 2 Jun 2016 15:48:47 +0200 (CEST) Date: Thu, 2 Jun 2016 15:43:31 +0200 From: Jan Viktorin To: "Hunt, David" Cc: dev@dpdk.org, olivier.matz@6wind.com, jerin.jacob@caviumnetworks.com Message-ID: <20160602154331.28dee852@pcviktorin.fit.vutbr.cz> In-Reply-To: <5750173D.5080404@intel.com> References: <1463665501-18325-1-git-send-email-david.hunt@intel.com> <1464797998-76690-1-git-send-email-david.hunt@intel.com> <1464797998-76690-2-git-send-email-david.hunt@intel.com> <20160601195456.47ac2a7c@pcviktorin.fit.vutbr.cz> <5750173D.5080404@intel.com> Organization: RehiveTech MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 1/5] mempool: support external handler 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, 02 Jun 2016 13:48:47 -0000 On Thu, 2 Jun 2016 12:23:41 +0100 "Hunt, David" wrote: > On 6/1/2016 6:54 PM, Jan Viktorin wrote: > > > > mp->populated_size--; > > @@ -383,13 +349,16 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr, > > unsigned i = 0; > > size_t off; > > struct rte_mempool_memhdr *memhdr; > > - 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; > > + rte_errno = 0; > > + mp->pool = rte_mempool_ops_alloc(mp); > > + if (mp->pool == NULL) { > > + if (rte_errno == 0) > > + return -EINVAL; > > + return -rte_errno; > > Are you sure the rte_errno is a positive value? > > If the user returns EINVAL, or similar, we want to return negative, as > per the rest of DPDK. Oh, yes... you're right. > > >> @@ -204,9 +205,13 @@ struct rte_mempool_memhdr { > >> struct rte_mempool { > >> char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */ > >> struct rte_ring *ring; /**< Ring to store objects. */ > >> + union { > >> + void *pool; /**< Ring or pool to store objects */ > > What about calling this pdata or priv? I think, it can improve some doc comments. > > Describing a "pool" may refer to both the rte_mempool itself or to the mp->pool > > pointer. The "priv" would help to understand the code a bit. > > > > Then the rte_mempool_alloc_t can be called rte_mempool_priv_alloc_t. Or something > > similar. It's than clear enough, what the function should do... > > I'd lean towards pdata or maybe pool_data. Not sure about the function > name change though, > the function does return a pool_data pointer, which we put into > mp->pool_data. Yes. But from the name of the function, it is difficult to understand what is its purpose. > > >> + uint64_t *pool_id; /**< External mempool identifier */ > > Why is pool_id a pointer? Is it a typo? I've understood it should be 64b wide > > from the discussion (Olivier, Jerin, David): > [...] > > >> +typedef void (*rte_mempool_free_t)(void *p); > >> + > >> +/** > >> + * Put an object in the external pool. > >> + * The *p pointer is the opaque data for a given mempool handler (ring, > >> + * array, linked list, etc) > > The obj_table is not documented. Is it really a table? I'd called an array instead. > > You're probably right, but it's always been called obj_table, and I'm > not sure > this patchset is the place to change it. Maybe a follow up patch? In that case, it's OK. > > >> + */ > >> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n); > > unsigned int > Done > > > >> + > >> +/** Get an object from the external pool. */ > >> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n); > > unsigned int > Done > > > >> + > >> +/** Return the number of available objects in the external pool. */ > > Is the number of available objects or the total number of all objects > > (so probably a constant value)? > > It's intended to be the umber of available objects OK, forget it. > > > > >> +typedef unsigned (*rte_mempool_get_count)(void *p); > > Should it be const void *p? > > Yes, it could be. Changed. > > > [...] > > >> + * @return > >> + * - 0: Sucess; the new handler is configured. > >> + * - EINVAL - Invalid handler name provided > >> + * - EEXIST - mempool already has a handler assigned > > They are returned as -EINVAL and -EEXIST. > > > > IMHO, using "-" here is counter-intuitive: > > > > - EINVAL > > > > does it mean a positive with "-" or negative value? > > EINVAL is positive, so it's returning negative. Common usage in DPDK, > afaics. Yes, of course. But it is not so clear from the doc. I've already wrote a code checking the positive error codes. That was because of no "minus sign" in the doc. So, my code was wrong and it took me a while to find out the reason... I supposed, the positive value was intentional. Finally, I had to lookup the source code (the calling path) to verify... > > > >> + */ > >> +int > >> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name); > > rte_mempool_set_ops > > > > What about rte_mempool_set_ops_byname? Not a big deal... > > I agree. rte_mempool_set_ops_byname > > >> + > >> +/** > >> + * Register an external pool handler. > > Register mempool operations > > Done > > >> + * > >> + * @param h > >> + * Pointer to the external pool handler > >> + * @return > >> + * - >=0: Sucess; return the index of the handler in the table. > >> + * - EINVAL - missing callbacks while registering handler > >> + * - ENOSPC - the maximum number of handlers has been reached > > - -EINVAL > > - -ENOSPC > > :) Similar as above... If it's a DPDK standard to write it like this, then I am OK with that. > [...] -- Jan Viktorin E-mail: Viktorin@RehiveTech.com System Architect Web: www.RehiveTech.com RehiveTech Brno, Czech Republic