From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Hunt, David" Subject: Re: [PATCH v10 1/3] mempool: support external mempool operations Date: Tue, 14 Jun 2016 14:20:52 +0100 Message-ID: <576004B4.7070708@intel.com> References: <1465571806-22008-1-git-send-email-david.hunt@intel.com> <1465897575-37638-1-git-send-email-david.hunt@intel.com> <1465897575-37638-2-git-send-email-david.hunt@intel.com> <1713942.6DB6T3RQsY@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, olivier.matz@6wind.com, viktorin@rehivetech.com, jerin.jacob@caviumnetworks.com, shreyansh.jain@nxp.com To: Thomas Monjalon Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 714A295D8 for ; Tue, 14 Jun 2016 15:21:02 +0200 (CEST) In-Reply-To: <1713942.6DB6T3RQsY@xps13> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Thomas, On 14/6/2016 1:55 PM, Thomas Monjalon wrote: > Hi David, > > 2016-06-14 10:46, David Hunt: >> 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 > Glad to see we are close to have this feature integrated. > > I've just looked into few details before pushing. > One of them are the comments. In mempool they were all ended by a dot. > Please check the new comments. Do you mean the rte_mempool struct definition, or all comments? Shall I leave the old comments the way they were before the change, or will I clean up? If I clean up, I'd suggest I add a separate patch for that. > The doc/guides/rel_notes/deprecation.rst must be updated to remove > the deprecation notice in this patch. Will do. As a separate patch in the set? > Isn't there some explanations to add in > doc/guides/prog_guide/mempool_lib.rst? Yes, I'll adapt some of the cover letter, and add as a separate patch. > Isn't there a better name than "default" for the default implementation? > I don't think the filename rte_mempool_default.c is meaningful. I could call it rte_mempool_ring.c? Since the default handler is ring based? >> +/** >> + * Register mempool operations >> + * >> + * @param h >> + * Pointer to and ops structure to register > The parameter name and its description are not correct. Will fix. >> + * @return >> + * - >=0: Success; return the index of the ops struct in the table. >> + * - -EINVAL - some missing callbacks while registering ops struct >> + * - -ENOSPC - the maximum number of ops structs has been reached >> + */ >> +int rte_mempool_ops_register(const struct rte_mempool_ops *ops); > You can check the doc with doxygen: > make doc-api-html Will do. >> --- a/lib/librte_mempool/rte_mempool_version.map >> +++ b/lib/librte_mempool/rte_mempool_version.map >> @@ -19,6 +19,8 @@ DPDK_2.0 { >> DPDK_16.7 { >> global: >> >> + rte_mempool_ops_table; >> + > Why this empty line? No particular reason. I will remove. >> rte_mempool_check_cookies; >> rte_mempool_obj_iter; >> rte_mempool_mem_iter; >> @@ -29,6 +31,8 @@ DPDK_16.7 { >> rte_mempool_populate_default; >> rte_mempool_populate_anon; >> rte_mempool_free; >> + rte_mempool_set_ops_byname; >> + rte_mempool_ops_register; > Please keep it in alphabetical order. > It seems the order was not respected before in mempool. I will fix this also. Regards, Dave.