From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 70812C3E6 for ; Sun, 19 Jun 2016 13:44:05 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP; 19 Jun 2016 04:44:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,493,1459839600"; d="scan'208";a="831161394" Received: from prekosz-mobl.ger.corp.intel.com (HELO [10.252.20.147]) ([10.252.20.147]) by orsmga003.jf.intel.com with ESMTP; 19 Jun 2016 04:44:01 -0700 To: Jan Viktorin 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> <20160617163551.4f9f4a77@pcviktorin.fit.vutbr.cz> Cc: dev@dpdk.org, olivier.matz@6wind.com, jerin.jacob@caviumnetworks.com, shreyansh.jain@nxp.com From: "Hunt, David" Message-ID: <57668580.30105@intel.com> Date: Sun, 19 Jun 2016 12:44:00 +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: <20160617163551.4f9f4a77@pcviktorin.fit.vutbr.cz> Content-Type: text/plain; charset=windows-1252; format=flowed 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: Sun, 19 Jun 2016 11:44:06 -0000 On 17/6/2016 3:35 PM, Jan Viktorin wrote: > 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? I'll change it, if you think it's weird. -rte_mempool_ops_get(int ops_index) +rte_mempool_get_ops(int ops_index) >> +{ >> + RTE_VERIFY(ops_index < RTE_MEMPOOL_MAX_OPS_IDX); > According to the doc comment: > > RTE_VERIFY(ops_index >= 0); Fixed. - RTE_VERIFY(ops_index < RTE_MEMPOOL_MAX_OPS_IDX); + RTE_VERIFY((ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)); >> + >> + return &rte_mempool_ops_table.ops[ops_index]; >> +} > [...] > >> + >> +/** >> + * @internal Wrapper for mempool_ops get callback. > This comment is obsolete "get callback" vs. dequeue_bulk. Done. - * @internal Wrapper for mempool_ops get callback. + * @internal Wrapper for mempool_ops dequeue 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 get. >> + * @return >> + * - 0: Success; got n objects. >> + * - <0: Error; code of get function. > "get function" seems to be obsolete, too... Done. - * - <0: Error; code of get function. + * - <0: Error; code of dequeue function. >> + */ >> +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" Done. - * @internal wrapper for mempool_ops put callback. + * @internal wrapper for mempool_ops enqueue 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" Done. - * - <0: Error; code of put function. + * - <0: Error; code of enqueue 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? Yes, well spotted. Fixed. + rte_spinlock_unlock(&rte_mempool_ops_table.sl); >> + 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. I've put in a check for NULL. >> + if (ops->free == NULL) >> + return; >> + return ops->free(mp); > This return statement here is redundant (void). Removed. >> +} >> + >> +/* wrapper to get available objects in an external mempool. */ >> +unsigned int >> +rte_mempool_ops_get_count(const struct rte_mempool *mp) > [...] > > Regards > Jan Regards, David.