From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jerin Jacob Subject: Re: [PATCH v5 1/3] mempool: support external handler Date: Mon, 30 May 2016 15:11:35 +0530 Message-ID: <20160530094129.GA7963@localhost.localdomain> References: <1460642270-8803-1-git-send-email-olivier.matz@6wind.com> <1463665501-18325-1-git-send-email-david.hunt@intel.com> <1463665501-18325-2-git-send-email-david.hunt@intel.com> <20160524153509.GA11249@localhost.localdomain> <574818EA.7010806@intel.com> <20160527103311.GA13577@localhost.localdomain> <57485D4F.9020302@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Cc: , , , To: "Hunt, David" Return-path: Received: from na01-bn1-obe.outbound.protection.outlook.com (mail-bn1on0061.outbound.protection.outlook.com [157.56.110.61]) by dpdk.org (Postfix) with ESMTP id 224115683 for ; Mon, 30 May 2016 11:42:00 +0200 (CEST) Content-Disposition: inline In-Reply-To: <57485D4F.9020302@intel.com> 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" On Fri, May 27, 2016 at 03:44:31PM +0100, Hunt, David wrote: > > Hi David, [snip] > That chunk of code above would be better moved all right. I'd suggest > moving it to the > rte_mempool_create function, as that's the one that needs the backward > compatibility. OK > > On the flags issue, each mempool handler can re-interpret the flags as > needed. Maybe we > could use the upper half of the bits for different handlers, changing the > meaning of the > bits depending on which handler is being set up. We can then keep the lower > half for bits that are common across all handlers? That way the user can Common lower half bit in flags looks good. > just set the bits they > are interested in for that handler. Also, the alloc function has access to > the flags, so maybe the > handler specific setup could be handled in the alloc function rather than > adding a new function pointer? Yes. I agree. > > Of course, that won't help if we need to pass in more data, in which case > we'd probably need an > opaque data pointer somewhere. It would probably be most useful to pass it > in with the > alloc, which may need the data. Any suggestions? But the top level rte_mempool_create() function needs to pass the data. Right? That would be an ABI change. IMO, we need to start thinking about passing a struct of config data to rte_mempool_create to create backward compatibility on new argument addition to rte_mempool_create() Other points in HW assisted pool manager perspective, 1) May be RING can be replaced with some other higher abstraction name for the internal MEMPOOL_F_RING_CREATED flag 2) IMO, It is better to change void *pool in struct rte_mempool to anonymous union type, something like below, so that mempool implementation can choose the best type. union { void *pool; uint64_t val; } 3) int32_t handler_idx creates 4 byte hole in struct rte_mempool in 64 bit system. IMO it better to rearrange.(as const struct rte_memzone *mz comes next) 4) IMO, It is better to change ext_alloc/ext_free to ext_create/ext_destroy as their is no allocation in HW assisted pool manager case, it will be mostly creating some HW initialization. > > Regards, > Dave. > > > > > > > > > > > > > > > > > >