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 CC1FBCEC9 for ; Fri, 17 Jun 2016 16:40:55 +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 3rWNGb3ghvz8fq; Fri, 17 Jun 2016 16:40:55 +0200 (CEST) Date: Fri, 17 Jun 2016 16:35:51 +0200 From: Jan Viktorin To: David Hunt Cc: dev@dpdk.org, olivier.matz@6wind.com, jerin.jacob@caviumnetworks.com, shreyansh.jain@nxp.com Message-ID: <20160617163551.4f9f4a77@pcviktorin.fit.vutbr.cz> In-Reply-To: <1466171618-27358-2-git-send-email-david.hunt@intel.com> References: <1466080236-112618-1-git-send-email-david.hunt@intel.com> <1466171618-27358-1-git-send-email-david.hunt@intel.com> <1466171618-27358-2-git-send-email-david.hunt@intel.com> Organization: RehiveTech MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v14 1/3] mempool: support mempool handler 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: Fri, 17 Jun 2016 14:40:56 -0000 Hi David, still few nits... Do you like the upstreaming process? :) I hope finish this patchset soon. The major issues seem to be OK. [...] > + > +/** > + * @internal Get the mempool ops struct from its index. > + * > + * @param ops_index > + * The index of the ops struct in the ops struct table. It must be a valid > + * index: (0 <= idx < num_ops). > + * @return > + * The pointer to the ops struct in the table. > + */ > +static inline struct rte_mempool_ops * > +rte_mempool_ops_get(int ops_index) What about to rename the ops_get to get_ops to not confuse with the ops wrappers? The thread on this topic has not been finished. I think, we can live with this name, it's just a bit weird... Olivier? Thomas? > +{ > + RTE_VERIFY(ops_index < RTE_MEMPOOL_MAX_OPS_IDX); According to the doc comment: RTE_VERIFY(ops_index >= 0); > + > + return &rte_mempool_ops_table.ops[ops_index]; > +} [...] > + > +/** > + * @internal Wrapper for mempool_ops get callback. This comment is obsolete "get callback" vs. dequeue_bulk. > + * > + * @param mp > + * Pointer to the memory pool. > + * @param obj_table > + * Pointer to a table of void * pointers (objects). > + * @param n > + * Number of objects to get. > + * @return > + * - 0: Success; got n objects. > + * - <0: Error; code of get function. "get function" seems to be obsolete, too... > + */ > +static inline int > +rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp, > + void **obj_table, unsigned n) > +{ > + struct rte_mempool_ops *ops; > + > + ops = rte_mempool_ops_get(mp->ops_index); > + return ops->dequeue(mp, obj_table, n); > +} > + > +/** > + * @internal wrapper for mempool_ops put callback. similar: "put callback" > + * > + * @param mp > + * Pointer to the memory pool. > + * @param obj_table > + * Pointer to a table of void * pointers (objects). > + * @param n > + * Number of objects to put. > + * @return > + * - 0: Success; n objects supplied. > + * - <0: Error; code of put function. similar: "put function" > + */ > +static inline int > +rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table, > + unsigned n) > +{ > + struct rte_mempool_ops *ops; > + > + ops = rte_mempool_ops_get(mp->ops_index); > + return ops->enqueue(mp, obj_table, n); > +} > + [...] > + > +/* 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->alloc == NULL || h->enqueue == NULL || > + h->dequeue == 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); The unlock is missing here, isn't? > + rte_errno = EEXIST; > + return -EEXIST; > + } > + > + ops_index = rte_mempool_ops_table.num_ops++; > + ops = &rte_mempool_ops_table.ops[ops_index]; > + snprintf(ops->name, sizeof(ops->name), "%s", h->name); > + ops->alloc = h->alloc; > + ops->enqueue = h->enqueue; > + ops->dequeue = h->dequeue; > + ops->get_count = h->get_count; > + > + rte_spinlock_unlock(&rte_mempool_ops_table.sl); > + > + return ops_index; > +} > + > +/* 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); > + return ops->alloc(mp); > +} > + > +/* wrapper to free an external pool ops. */ > +void > +rte_mempool_ops_free(struct rte_mempool *mp) > +{ > + struct rte_mempool_ops *ops; > + > + ops = rte_mempool_ops_get(mp->ops_index); I assume there would never be an rte_mempool_ops_unregister. Otherwise this function can return NULL and may lead to a NULL pointer exception. > + if (ops->free == NULL) > + return; > + return ops->free(mp); This return statement here is redundant (void). > +} > + > +/* wrapper to get available objects in an external mempool. */ > +unsigned int > +rte_mempool_ops_get_count(const struct rte_mempool *mp) [...] Regards Jan