From mboxrd@z Thu Jan 1 00:00:00 1970 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) Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga104.fm.intel.com with ESMTP; 14 Jun 2016 06:21:01 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,470,1459839600"; d="scan'208";a="121637097" Received: from dhunt5-mobl.ger.corp.intel.com (HELO [10.237.221.7]) ([10.237.221.7]) by fmsmga004.fm.intel.com with ESMTP; 14 Jun 2016 06:20:52 -0700 To: Thomas Monjalon 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> Cc: dev@dpdk.org, olivier.matz@6wind.com, viktorin@rehivetech.com, jerin.jacob@caviumnetworks.com, shreyansh.jain@nxp.com From: "Hunt, David" Message-ID: <576004B4.7070708@intel.com> Date: Tue, 14 Jun 2016 14:20:52 +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: <1713942.6DB6T3RQsY@xps13> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v10 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: Tue, 14 Jun 2016 13:21:02 -0000 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.